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 649771 - Implement "attributes-changed" and "relation-changed" signals
Implement "attributes-changed" and "relation-changed" signals
Status: RESOLVED OBSOLETE
Product: atk
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: ATK maintainer(s)
ATK maintainer(s)
Depends on:
Blocks: 638537
 
 
Reported: 2011-05-09 04:22 UTC by Mike Gorse
Modified: 2021-06-10 11:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Draft patch (5.85 KB, patch)
2011-05-30 09:49 UTC, Li Yuan
none Details | Review
draft patch2 (5.63 KB, patch)
2011-06-09 06:23 UTC, Li Yuan
none Details | Review
patch (5.64 KB, patch)
2011-07-07 04:34 UTC, Li Yuan
none Details | Review

Description Mike Gorse 2011-05-09 04:22:30 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
Comment 1 Mike Gorse 2011-05-09 11:31:50 UTC
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.
Comment 2 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-11 09:35:01 UTC
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
Comment 3 Li Yuan 2011-05-30 07:31:18 UTC
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.
Comment 4 Li Yuan 2011-05-30 07:59:14 UTC
I wonder if we can emit the signal with the relation_set instead, which is basically a list and do not have such problem.
Comment 5 Li Yuan 2011-05-30 09:49:18 UTC
Created attachment 188866 [details] [review]
Draft patch

Draft patch for this bug. Comments welcomed.
Comment 6 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-30 14:16:46 UTC
(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
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-30 14:18:29 UTC
(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
Comment 8 Mike Gorse 2011-06-04 19:52:41 UTC
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.
Comment 9 Li Yuan 2011-06-09 06:10:52 UTC
(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.
Comment 10 Li Yuan 2011-06-09 06:23:17 UTC
Created attachment 189528 [details] [review]
draft patch2
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-06-09 13:51:02 UTC
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
Comment 12 Li Yuan 2011-06-13 03:32:52 UTC
(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.
Comment 13 André Klapper 2011-06-23 22:05:14 UTC
[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.]
Comment 14 Li Yuan 2011-07-07 04:34:00 UTC
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.
Comment 15 André Klapper 2021-06-10 11:26:39 UTC
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.