GNOME Bugzilla – Bug 647351
Implement atk_object_get_relation_by_type()
Last modified: 2021-06-10 11:25:19 UTC
It seems like an at would often only want one relation for an accessible, in that case it might be good to be able to only get that one relation. So we'd have a function something like atkRelation *atkObj->GetRelation(atkRelationType type); This would certainly help the applications performance, I'm not sure how useful it will be to an AT though because of the round trip costs of method calls in IPC.
Fwiw, GetRelationSet (the DBus method) transfers all of the relations in the relation set. This means that, for AT-SPI2, if an accessible's relation set changes, then an AT will not know about it unless it calls GetRelationSet again. Adding a GetRelationByType to ATK and AT-SPI wouldn't be hard, so perhaps it would be worth doing if ATs are often interested only in one particular relation type. ATK should have a default implementation that calls ref_relation_set and atk_relation_set_get_relation_by_type, but implementors could override this if it would be significantly more performant for them to avoid needing to possibly construct the whole relation set.
(In reply to comment #1) > Fwiw, GetRelationSet (the DBus method) transfers all of the relations in the > relation set. This means that, for AT-SPI2, if an accessible's relation set > changes, then an AT will not know about it unless it calls GetRelationSet > again. yeah, I assumed that was the case and at-spi2 and pyatspi would have to change to allow an at to get only one relation for this to make sense. > Adding a GetRelationByType to ATK and AT-SPI wouldn't be hard, so perhaps it > would be worth doing if ATs are often interested only in one particular > relation type. ATK should have a default implementation that calls > ref_relation_set and atk_relation_set_get_relation_by_type, but implementors > could override this if it would be significantly more performant for them to > avoid needing to possibly construct the whole relation set. I would expect that an AT often only wants one relation but I don't really know. I would personally do this the reverse way, provide a default refRelationSet() that just iterates over the types getting the relation for each and adds it to the set. That's mostly because I would expect that the time getting the set is almost always the sum of the time to get each relation. So if the default is for the ap to implement getRelationByType() then both it and refRelationSet() will in the common case be as performant as possible. On the other hand I don't expect its very often faster to get the set of relations than to just iterator over the types getting each, so few apps will have a need to override this default. However I could live with either way really.
A similar thing is proposed as the first item on this IA2 proposal: https://wiki.mozilla.org/Accessibility/IA2_1.3
Group consensus seems to be to implement this.
[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 196905 [details] [review] draft patch The draft patch. Note instead of ref-ing relationset, I ref-ed relation. Because the caller will not get the relationset and has no chance to un-ref it.
Review of attachment 196905 [details] [review]: ::: atk/atkobject.c @@ +864,3 @@ + * Gets the #AtkRelation in the given type associated with the object. + * + * Returns: (transfer full): an #AtkRelation in the given type of the object. Probably it would be good to mention on the documentation that the caller will receive a new ref of the relation (so he will require to unref it). Something similar to what we have on atk_object_ref_accessible_child: "* Gets a reference to the specified accessible child of the object." I know that this is inherent to the gobject-introspection annotation "transfer full", but not all the people are required to be aware of this. ::: atk/atkobject.h @@ +533,3 @@ + /* + * Gets the relation for the object in the given type. + * Since ATK 2.1.5 Hint: we need to update this Since, to the real number ::: atk/atkrelation.h @@ -43,3 @@ #define ATK_RELATION_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), ATK_TYPE_RELATION, AtkRelationClass)) -typedef struct _AtkRelation AtkRelation; Any reason for this removal?
(In reply to comment #6) > Created an attachment (id=196905) [details] [review] > draft patch > > The draft patch. Note instead of ref-ing relationset, I ref-ed relation. > Because the caller will not get the relationset and has no chance to un-ref it. why not just include this in the list of API breaks, and add a get_relation_by_type() vfunc to atkobject, and then consider depricating refRelationSet(). Getting all of the relations for an object and then dropping all but one on the floor will probably mean wasting a good amount of time on needless computations.
(In reply to comment #8) > (In reply to comment #6) > > Created an attachment (id=196905) [details] [review] [details] [review] > > draft patch > > > > The draft patch. Note instead of ref-ing relationset, I ref-ed relation. > > Because the caller will not get the relationset and has no chance to un-ref it. > > why not just include this in the list of API breaks, and add a > get_relation_by_type() vfunc to atkobject, and then consider depricating > refRelationSet(). Getting all of the relations for an object and then dropping > all but one on the floor will probably mean wasting a good amount of time on > needless computations. If you take a look to the patch, you can see that this method is already being implemented on atk as a new virtual method. atkobject have still two pads, so adding a virtual method is still possible without an API breakage. Anyway, not sure about deprecating refRelationSet. IMHO, having both methods is good. If you just need a specific relationship you could use get_relation_by_type. If you need all ref_relation_set, and in this case, you have a performance gain. Just to adding more info here, Joanie, when Orca ask for the relation set, is Orca always interested in just one relation?
(In reply to comment #9) > Just to adding more info here, Joanie, when Orca ask for the relation set, is > Orca always interested in just one relation? Sorry just saw this. 9 times out of 10, yes just one relation. 1 time out of 10, more than one relation, but the relations are related e.g. flows to and flows from; labelled by, label for. Having said that, consider, something like: Duration: <spin button> seconds In that case, the spin button is labelled by two labels: 'Duration:' and 'seconds' and the flow is from 'Duration:' to the spin button to 'seconds'. In order to locate the labels and present them in the correct order with respect to the thing being labelled, more than one relation is needed from the relation set.
(In reply to comment #10) > In that case, the spin button is labelled by two labels: 'Duration:' and > 'seconds' and the flow is from 'Duration:' to the spin button to 'seconds'. In > order to locate the labels and present them in the correct order with respect > to the thing being labelled, more than one relation is needed from the relation > set. Well, but remember that a relation can have more that one target. So in this case, calling atk_object_get_relation_by_type (spin_button, LABELLED_BY) will still return just one relation, and that relation will have several targets. Taking that into account, is that 9 of 10 still valid?
(In reply to comment #11) > (In reply to comment #10) > > > In that case, the spin button is labelled by two labels: 'Duration:' and > > 'seconds' and the flow is from 'Duration:' to the spin button to 'seconds'. In > > order to locate the labels and present them in the correct order with respect > > to the thing being labelled, more than one relation is needed from the relation > > set. > > Well, but remember that a relation can have more that one target. Of course. > So in this > case, calling atk_object_get_relation_by_type (spin_button, LABELLED_BY) will > still return just one relation, and that relation will have several targets. I didn't think that was the question; I thought instead the question was "is Orca always interested in just one relation type from the relation set?" And in the above example, Orca would hypothetically be interested in two relation types: 1. Labelled by 2a. Flows from (to present its label to the left) 2b. Flows to (to present its label to the right) Or am I totally failing to see your point? > Taking that into account, is that 9 of 10 still valid? I believe so....
(In reply to comment #12) > > 1. Labelled by > 2a. Flows from (to present its label to the left) > 2b. Flows to (to present its label to the right) Ok, sorry I forgot the flows relationships. > Or am I totally failing to see your point? > > > Taking that into account, is that 9 of 10 still valid? > > I believe so.... Taking that into account, I think that makes sense to maintain get_relation_set. I will try to take another look to the patch on the soon to start ATK/AT-SPI2 hackfest. Hopefully we will have get_relation_by_type soon.
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.