After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 405774 - Implement the Matchrule interface required for Collection
Implement the Matchrule interface required for Collection
Status: RESOLVED FIXED
Product: at-spi
Classification: Platform
Component: api
1.7.x
Other All
: Normal enhancement
: ---
Assigned To: Ariel Rios
bill.haneman
Depends on:
Blocks: 326516
 
 
Reported: 2007-02-08 15:56 UTC by Ariel Rios
Modified: 2007-04-10 15:35 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Starting point for Matchrule implementation (12.57 KB, patch)
2007-02-08 16:48 UTC, Ariel Rios
none Details | Review
Include Bill comments (18.03 KB, patch)
2007-02-13 14:46 UTC, Ariel Rios
reviewed Details | Review
Matchrule implementation with cspi bindings (26.83 KB, patch)
2007-02-14 22:41 UTC, Ariel Rios
accepted-commit_now Details | Review
Add spi_collection.c (33.57 KB, patch)
2007-04-02 15:45 UTC, Ariel Rios
committed Details | Review

Description Ariel Rios 2007-02-08 15:56:08 UTC
Opening this bug for keeping track and discussion on the development of 
the Matchrule interface. The first step in implementing collection:

http://bugzilla.gnome.org/show_bug.cgi?id=326516
http://www.gnome.org/~billh/at-spi-new-idl/html/html/classAccessibility_1_1MatchRule.html
Comment 1 Ariel Rios 2007-02-08 16:48:54 UTC
Created attachment 82163 [details] [review]
Starting point for Matchrule implementation

Bill,

I have a early patch just to get some feedback from you. Currently the patch implements a basic Matchrule and Collection::createMatchrule.

I create a MatchRule objct that include states, attributes, roles and their matchtypes. Since the Matchrule is supposed to be an opaque object I do not have those attributes on the idl. They are setted inside the SpiMatchRule object that is created by createMatchRule. Currently I have put the CORBA types but it might be better to put glib lists (instead of the string sequences).
Comment 2 bill.haneman 2007-02-12 16:30:18 UTC
Hi Ariel:

Initial patch looks like a good start.  

A couple of questions - one, why include the MatchType/etc. stuff in SpiCollection?  If a Collection instance remembers/stores its 'matchrule' info at all, it seems to me it should just persist the MatchRule that was used to obtain it, rather than the finer-grained info.

Not sure if the members of MatchRule are actually 'opaque', the type is aliased, but I forget, does this effectively 'hide' the members from the public struct?  Or could a client still access (_SpiMatchRule *)foo->roles ?  We don't want to allow that, do we?

Lastly, we should be careful about memory management decisions; it looks as though the current createMatchRule implementation doesn't dup its args, which means that the caller has to keep the arguments alive until the MatchRule is destroyed?  Shouldn't MatchRule dup the args which it adds to its internals (i.e. matchrule->attributes, etc.) and then free them when it is destroyed?

Just wondering, I could be misunderstanding the above points.

Thanks again for working on this!

Bill
Comment 3 Ariel Rios 2007-02-12 17:09:58 UTC
Bill, 

You mentioned 3 points.
1- Including Matchtype/etc on collection.h was a nit. Those should not be there at all.
2- Opaque MR members. You are right. I need to use
g_type_class_add_private ()
to make them private.

3- Memory management. Agreed.

I will do your suggested changes and provide a new patch

ariel
Comment 4 Ariel Rios 2007-02-13 14:46:19 UTC
Created attachment 82467 [details] [review]
Include Bill comments

New patch with suggested changes from comment #2.
Comment 5 bill.haneman 2007-02-13 16:43:17 UTC
The new patch looks good, thanks Ariel!  I haven't tested but it seems correct to me.
Comment 6 Ariel Rios 2007-02-13 16:51:13 UTC
Bill,

I know that you have to "silently" deprecate cspi bindings. Do we need to have cspi collection and matchrule?

ariel
Comment 7 bill.haneman 2007-02-13 17:38:16 UTC
So far, we've always added cspi bindings for new AT-SPI API.  Since these are just small wrappers, I think it's still something we should do for the time being.
Comment 8 Ariel Rios 2007-02-14 22:41:24 UTC
Created attachment 82573 [details] [review]
Matchrule implementation with cspi bindings

The cspi bindings have been added. I included the RoleSet def that was missing.
Do you want me to provide a separate patch for it. Is really just a struct.

ariel
Comment 9 Harry Lu 2007-03-22 06:28:51 UTC
Ariel, as Bill has OKed your patch, please commit it into HEAD.
Thanks for your work!
Comment 10 Harry Lu 2007-03-29 14:35:57 UTC
When I tried to build with the patch, got a error:

make[1]: Entering directory `/export/home/evo2a/work/at-spi/cspi'
make[1]: *** No rule to make target `spi_collection.c', needed by `spi_collection.lo'.  Stop.


It looks like you missed the spi_collection.c file in the patch. Could you please update your patch to include it? Thanks.
Comment 11 Ariel Rios 2007-04-02 15:45:37 UTC
Created attachment 85707 [details] [review]
Add spi_collection.c

Add spi_collection.c
Comment 12 Harry Lu 2007-04-02 16:05:07 UTC
committed into HEAD.