GNOME Bugzilla – Bug 649771
Implement "attributes-changed" and "relation-changed" signals
Last modified: 2021-06-10 11:26:39 UTC
Whenever Orca receives an event, it calls getAttributeSet in order to check the toolkit associated with the sender. AT-SPI cannot cache the return value since it has no way of knowing when it may change, so it needs to make a round-trip D-Bus call every time the attribute set is requested. Adding an event that could be listened to may be useful. The situation is similar with relation sets, so an event would be useful if they can change. A
As I think about it, bug 640949 might at least partly subsume the need for this, depending on what we decide. If Orca can ask applications to send the event source's attribute set as a data item with the event, then AT-SPI will not need to call GetAttributes in order to fetch it.
ATK Hackfest 2011 Conclusion: * In general seems a good idea. * Was discussed if it makes sense to emit those signals from the Set itself, but in the end was concluded that would be easier to emit them from AtkObject, and also more coherent with current AtkObject:state-changed * The signal detail would include the specific attribute/relation modified
When I worked on this, I found AtkRelation is an AtkObject so it source file includes atkobject.h. Thus we cannot include atkrelation.h in atkobject.h to create AtkRelation related signals.
I wonder if we can emit the signal with the relation_set instead, which is basically a list and do not have such problem.
Created attachment 188866 [details] [review] Draft patch Draft patch for this bug. Comments welcomed.
(In reply to comment #3) > When I worked on this, I found AtkRelation is an AtkObject so it source file > includes atkobject.h. I don't undertand it, looking at the code: struct _AtkRelation { GObject parent; GPtrArray *target; AtkRelationType relationship; }; struct _AtkRelationClass { GObjectClass parent; }; So AtkRelation is as GObject. Did you mean that atk-relation source file is using atkobject? > Thus we cannot include atkrelation.h in atkobject.h to create AtkRelation related signals. I guess that there are any kind of workaround using typedef, and so on. But anyway, I think that there is not a problem. Because I guess that here you are worried about the signal handler, as you wanted to define something like this: void (* relation_change) (AtkObject *accessible, AtkRelation *relation); Two things: 1.) These days people tend to avoid to create signal handlers on the class for all the signals, mostly if they are "action signals". Right now AtkObject only has two pads, so it would be good to not spend them if not necessary. More reasons here [1]. So you could implement it if you want. 2.) Not sure if it makes sense to emit this signal with the relation. After all, what AtkObject returns is the relation set. And what would mean that a relation has changed? Normally the AtkObject has the relation set, and then you just add/remove relations on that set. And reading again comment 0, it seems that the problem is about caching the attribute set and the relation set, so this also applies to the attribute. The case of state-change is different, as we already have a list of possible states that could change. In this case a relation is always something new (as it is a relation between two objects, created on runtime. Mike, as you were the one that created the bug, what makes more sense? a "relation-changed", "attributes-changed" pair, or a "relation-set-changed", "attributes-set-changed" pair? [1] http://bugzilla.clutter-project.org/show_bug.cgi?id=1894#c2
(In reply to comment #5) > Created an attachment (id=188866) [details] [review] > Draft patch > > Draft patch for this bug. Comments welcomed. It seems ok, but as I said on comment 6, I think that it is not required to consume pads for the signal handlers (as normally you will just connect to that function, and not redefine it), and we still need to conclude if we want those signals for the relation/attribute or for the relation-set/attribute-set
At least with the current GAIL implementation, an object's relation_set gets updated when ref_relation_set is called. It is also theoretically possible to override ref_relation_set so that an object is returned independently of the relation_set item in the struct, although I don't know if any atk implementors actually do this. Additionally, we have BGO#647351 to allow querying one particular type of relation given an AtkObject. So I don't think it makes sense to pass a relation set when emitting the event. Instead, I would include an enum for the relation type and probably a GPtrArray of targets as signal details. atkobject.h already includes atkrelationtype.h, so this should not introduce header-tangling issues. An atk implementor would perhaps have the option of omitting one or both detail items (passing ATK_RELATION_NULL or a NULL target array) if calculating the relation set would be slow/difficult, as with gail not emitting the accessible when a child is added to the tree view.
(In reply to comment #6) > (In reply to comment #3) > > When I worked on this, I found AtkRelation is an AtkObject so it source file > > includes atkobject.h. > > I don't undertand it, looking at the code: > > struct _AtkRelation > { > GObject parent; > > GPtrArray *target; > AtkRelationType relationship; > }; > > struct _AtkRelationClass > { > GObjectClass parent; > }; > > So AtkRelation is as GObject. Did you mean that atk-relation source file is > using atkobject? > > > Thus we cannot include atkrelation.h in atkobject.h to create AtkRelation related signals. > Yes. I don't know why I said that. > Two things: > > 1.) These days people tend to avoid to create signal handlers on the class for > all the signals, mostly if they are "action signals". Right now AtkObject only > has two pads, so it would be good to not spend them if not necessary. More > reasons here [1]. So you could implement it if you want. Good suggestion. (In reply to comment #8) > At least with the current GAIL implementation, an object's relation_set gets > updated when ref_relation_set is called. It is also theoretically possible to > override ref_relation_set so that an object is returned independently of the > relation_set item in the struct, although I don't know if any atk implementors > actually do this. Additionally, we have BGO#647351 to allow querying one > particular type of relation given an AtkObject. So I don't think it makes > sense to pass a relation set when emitting the event. Instead, I would include > an enum for the relation type and probably a GPtrArray of targets as signal > details. atkobject.h already includes atkrelationtype.h, so this should not > introduce header-tangling issues. An atk implementor would perhaps have the > option of omitting one or both detail items (passing ATK_RELATION_NULL or a > NULL target array) if calculating the relation set would be slow/difficult, as > with gail not emitting the accessible when a child is added to the tree view. The new patch would take this way.
Created attachment 189528 [details] [review] draft patch2
Review of attachment 189528 [details] [review]: One question, I'm not sure about this change and how it could work so ... instead of committing it directly to atk and then implement it on gail and use it on at-spi2, could we test it first? I mean that it would be good to check that this is the proper signature, and that we don't find problems when we start to implement it. So I propose to try to implement it on gail (using a atk branch with this signals) and use it on at-spi2 before applying it to the master. ::: atk/atkobject.c @@ +1155,3 @@ + * @accessible: an #AtkObject + * @relation_type: the type of the changed relations + * @relations: the changed relations From Mike comment: "Instead, I would include an enum for the relation type and probably a GPtrArray of targets as signal details" After all the relation is already included on relation_type. I guess that Mike was asking for a GPtrArray with the target like the one we have returned by this: GPtrArray * atk_relation_get_target (AtkRelation *relation); So I think that this should be something like @targets, instead of relations
(In reply to comment #11) > Review of attachment 189528 [details] [review]: > > One question, I'm not sure about this change and how it could work so ... > instead of committing it directly to atk and then implement it on gail and use > it on at-spi2, could we test it first? > > I mean that it would be good to check that this is the proper signature, and > that we don't find problems when we start to implement it. So I propose to try > to implement it on gail (using a atk branch with this signals) and use it on > at-spi2 before applying it to the master. > Maybe we should create a branch for the next version of atk, because sooner or later we will break api I think. And we can test the code in that branch by patching gail and at-spi2. After that we put the patch back to master if it doesn't break API.
[Mass-reassigning open atk bug reports for better trackability as requested in https://bugzilla.gnome.org/show_bug.cgi?id=653179 . PLEASE NOTE: If you have watched the previous assignee of this bug report as a workaround for actually getting notified of changes in atk bugs, you yourself will now have to add atk-maint@gnome.bugs to your watchlist at the bottom of https://bugzilla.gnome.org/userprefs.cgi?tab=email to keep watching atk bug reports in GNOME Bugzilla. Sorry for the noise: Feel free to filter for this comment in order to mass-delete the triggered bugmail.]
Created attachment 191431 [details] [review] patch New patch based on comments. I am creating a new branch called atk-3, and committing the patch into it.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of atk, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/atk/-/issues/ Thank you for your understanding and your help.