GNOME Bugzilla – Bug 405774
Implement the Matchrule interface required for Collection
Last modified: 2007-04-10 15:35:30 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
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).
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
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
Created attachment 82467 [details] [review] Include Bill comments New patch with suggested changes from comment #2.
The new patch looks good, thanks Ariel! I haven't tested but it seems correct to me.
Bill, I know that you have to "silently" deprecate cspi bindings. Do we need to have cspi collection and matchrule? ariel
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.
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
Ariel, as Bill has OKed your patch, please commit it into HEAD. Thanks for your work!
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.
Created attachment 85707 [details] [review] Add spi_collection.c Add spi_collection.c
committed into HEAD.