Ariel and Bill to collaborate on the implementation of Collection for Document, and possibly Collection for Frame (to help debug).  Suggest implementing in the following order: createMatchRule, freeMatchRule, getChildren, getNextChildre, getPreviousChildren.  Get consensus on isAncestorOf and getActiveDescendant before implementing.

I believe we reached agreement/conclusion that getActiveDescendant should live in Collection, but only apply to objects with the MANAGES_DESCENDANTS property.  Thus, a non-NIL implementation of getActiveDescendant will have to depend on AtkCollection, since we can't practicably implement Collection on behalf of MANAGES_DESCENDANTS objects in the bridge.

isAncestorOf seems OK, it basically asks "is a given object part of this Collection container" ?  _possibly_ it should be placed in Accessible instead, along with getAncestors, but we've run out of bincompat/unimplemented "slots" there, and thus we might need something like an AccessibleContainer interface instead.

I suggest we leave getActiveDescendant out of the 'first rev' of Collection, and add it later when we have AtkCollection as well.  This would also allow us the alternate approach of AccessibleContainer (which would, I suppose, derive from Accessible) instead.

Bumping to Gnome 2.18, to allow ample time for implementation, testing, and review before the API gets frozen.

Created attachment 81770 [details]
Collection IDL

BIll,

I'm starting again on Collection. I am using http://www.gnome.org/~billh/at-spi-new-idl/html/html/Accessibility__Collection_8idl-source.html
and fix a small typo (s/inOut/inout) but I'm sure you have a good one with docuemntation included.

ariel

Li,

I was trying to add bugs 405774, 407600 to the block fields of this bug so we can have a nice dependency graph for references purposes but couldn't do so because I don't have enough permissioes. Could you please assign this bug to me?

ariel

Created attachment 95739 [details]
First draft for collection work

This patch implements getMatches, getMatchesFrom and getMatchesTo using SORT_CANONICAL order. A brief sample script looks like this:

def run(acc):
    col = acc.queryInterface('IDL:Accessibility/Collection:1.0')	
    if col is None:						
       raise NotImplementedError('%s does not implement collection' % acc.name)

    roles = [Accessibility.ROLE_PUSH_BUTTON,Accessibility.ROLE_FRAME]  
    rule = col.createMatchRule(None, col.MATCH_ANY ,
	                       "", col.MATCH_ALL,
			       "", col.MATCH_ANY,
                               "", col.MATCH_ALL,
                               False)
  

    ls = col.getMatches (rule, col.SORT_ORDER_CANONICAL, True, 0)

I will be adding a bug to contain more usable scripts to test collection.

Created attachment 97489 [details] [review]
More work on Collection

Cleanup work, memory improvements et al.

I have reviewed part of your patch but not finished yet. It looks good,
thanks for your hard working.

I have some questions about the patch:

1. I see you use G_TYPE_INSTANCE_GET_PRIVATE() to get the private
structure for SpiCollection. From
http://library.gnome.org/devel/gobject/unstable/gobject-Type-Information.html#G-TYPE-INSTANCE-GET-PRIVATE:CAPS : "This macro should only be used in type implementations." So I guess we need to find a replacement.

2. SpiCollection's private structure is named MatchRulePrivate which I
think SpiCollectionPrivate is better. Was MatchRulePrivate designed as
MatchRule's private structure originally? If so, why do we change it to
SpiCollection?

3. After apply the patch, in impl_createMatchRule, do we create a 
SpiMatchrule by spi_matchrule_interface_new, don't do anything, and
return it (by CORBA_Object_duplicate)? Because after apply the patch, we
get mrp from SpiCollection. Is there a reason for this?

4. How about there are 2 or more ATs running and all of them call a
particular object's createMatchRule (assume it implements collection
interface)? From my imagination, buffers created by impl_createMatchRule
may be leaked. example: 
1) AT1 calls createMatchRule
2) AT2 calls createMatchRule
3) AT1 calls freeMatchRule
4) AT2 calls freeMatchRule

I think after step 2, we will lose the buffers we created at step 1 and
cannot free them. What do you think?

Created attachment 98964 [details]
Remove MATCH_RULE_PRIVATE.

Removes G_TYPE_INSTANCE_GET_PRIVATE().

Created attachment 99144 [details] [review]
Prevents memory leak when creating new mathrule.

(In reply to comment #10)
> Created an attachment (id=99144) [edit]
> Prevents memory leak when creating new mathrule.
> 

Li - does this patch look good to you?  If so, we should try to get it in for the 2.21.3 release coming up next week (tarballs due 03-Dec).

I still have some questions.

From the patch, we free the previous rule when another createMatchRule call comes in. Means if the first AT creates a rule, and the second creates another, and the first one is lost. Also an AT cannot create more than 2 rules at the same time. Is this the design? 

So far I always had in mind that you can only have one matchrule being executed at a certain moment in time. Usually, you would need to create a new match rule to look on the results from a query. This could be changed as a future enhancement or  I can work on it now. Whatever you prefer. I would prefer to have it commited so people can start working and get feedback from AT implementors. In any case I will start looking into this.

ariel

OK. 

Since we store MatchRulePrivate in collection now, what's your plan about SpiMatchrule? 

I found the answer from your mail before. Any update for this issue? (remove
Matchrule or make MatchRulePrivate to be bound withing MatchRule)

Is there any reason makes us wanna add collection interface to SpiApplication?

I am working on removing MatchRulePrivate and bounding it to MatchRule. The change will be internal and the API is not going to be changed but we will be able to have several MatchRule objects declared. Could it be possible to include what we have in the next GNOME release(2.21.3) and work on this improvements for the next release?

THe first reason of adding collection to SpiApplication was to have a easy way to get collection although it might not be a good place and might only be needed or useful in things like frames. I added another bug for discussion of this point. I really have no point against or in favour.

ariel

If we don't add it to SpiApplication, the collection should still work right? If so, I suggest we don't add it to SpiApplication.

Some changes might need to be done to the scripts but the rest should work just fine.

ariel

Created attachment 99896 [details] [review]
Some small nits 

This one removes the bridge part the added the collection interface to Application and removes copyright notices as we are unsure what to do.

Created attachment 100094 [details] [review]
the patch will be committed

Ariel, we have a problem here. The patch can be compiled well on Linux, but it causes errors on Solaris.
I think it is because SUN Studio's CC treat "restrict" as a keyword, which seems gcc do not by default. We may need to change it to another word.

You can make the diff from the svn trunk now because I have committed your patch. And please submit the new diff today to catch up with today's release. Thanks.

Created attachment 100157 [details] [review]
patch to avoid build error

(In reply to comment #10)
> Created an attachment (id=99144) [edit]
> Prevents memory leak when creating new mathrule.
> 

But in freeMatchRule, spimatchrule->_mrp is not setting to NULL.
So it will crash, if AT does freeMatchRule before re-creating matchrule.

Created attachment 105307 [details] [review]
patch to avoid crash

forgot to mention the bug report in mozilla bugzilla
https://bugzilla.mozilla.org/show_bug.cgi?id=415776

Comment on attachment 105307 [details] [review]
patch to avoid crash

This patch was taken and committed by Bug 517250.

[Resetting QA Contact to newly introduced "at-spi-maint@gnome.bugs". 
Reason: So far it was impossible to watch changes in at-spi bug reports without following all the specific persons (Li Yuan, Bill Haneman, Jeff Wai, ...) and also their activity outside of at-spi reports.

IMPORTANT: Anyone interested in following all bug activity (including all maintainers) must watch the "at-spi-maint@gnome.bugs" dummy user by adding it to the 'Users to watch' list under Preferences->Email preferences. This is also the default procedure nowadays in GNOME when setting up new products.]

[Mass-resetting default assignee, see bug 705890. Please reclaim this bug report by setting the assignee to yourself if you still plan to work on this. Thanks!]

I think this bug should be closed by now.