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 644747 - Debate if AtkObject "property-change" signal should be deprecated and use the current GObject "notify"
Debate if AtkObject "property-change" signal should be deprecated and use th...
Status: RESOLVED FIXED
Product: atk
Classification: Platform
Component: atk
unspecified
Other Linux
: Normal normal
: ---
Assigned To: ATK maintainer(s)
ATK maintainer(s)
Depends on:
Blocks: 638537
 
 
Reported: 2011-03-14 18:03 UTC by Alejandro Piñeiro Iglesias (IRC: infapi00)
Modified: 2013-12-11 11:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Improve documentation for AtkObject::property-change (3.29 KB, patch)
2013-12-10 16:04 UTC, Alejandro Piñeiro Iglesias (IRC: infapi00)
reviewed Details | Review

Description Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-03-14 18:03:50 UTC
AtkObject includes this signal:

http://library.gnome.org/devel/atk/stable/AtkObject.html#AtkObject-property-change

"The signal "property-change" is emitted when an object's property value changes. The detail identifies the name of the property whose value has changed. "

But right now GObject has the signal "notify":

http://library.gnome.org/devel/gobject/stable/gobject-The-Base-Object-Type.html#GObject-notify

"The notify signal is emitted on an object when one of its properties has been changed. Note that getting this signal doesn't guarantee that the value of the property has actually changed, it may also be emitted when the setter for the property is called to reinstate the previous value. "

Whose purpose overlaps. After think a little if having property-change has any advantage, I only can think on:

   * Having property-change allow to have a better control about when emit the signal, as "notify" docs explicitly say that a emission of that signal doesn't means always a change on the property.
   * Right now property-change is only used on a subset of all the AtkObject properties. So having this signal adds a extra flexibility here. But not sure if this is a advantage, as you can connect to specific details on the "notify" signal.

So, should we deprecate AtkObject "property-change" and use GObject "notify" instead?

(It would be good if some of the original ATK designer could answer this question ... )
Comment 1 Mario Sánchez Prada 2011-03-22 18:20:40 UTC
There's another difference afaik, and it's that with the "property-change" signal you can report both the old and new values for the property that has changed (through the AtkPropertyValues struct), although documentation says that is barely used, only with detail "accessible-value":

http://library.gnome.org/devel/atk/unstable/AtkObject.html#AtkPropertyValues

So, I'm not sure about deprecating this one, although I agree it's at least confusing to have both the "notify" and the "property-change" signals.
Comment 2 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-03-23 16:42:29 UTC
(In reply to comment #1)
> There's another difference afaik, and it's that with the "property-change"
> signal you can report both the old and new values for the property that has
> changed (through the AtkPropertyValues struct), although documentation says
> that is barely used, only with detail "accessible-value":
> 
> http://library.gnome.org/devel/atk/unstable/AtkObject.html#AtkPropertyValues

Anyway, as we already discussed in person, this old_value is not really used at this moment, as most of the objects on gail basically cached old values.

> So, I'm not sure about deprecating this one, although I agree it's at least
> confusing to have both the "notify" and the "property-change" signals.

This is the reason I opened the bug ;)

If finally this signal is not deprecated, it is clear that the documentation should be clear about:
  * Why it is required
  * When it should be used.
Comment 3 Mario Sánchez Prada 2011-05-10 10:46:06 UTC
After some discussion in the ATK/AT-SPI Hackfest [1], we kind of agree that the "property-change" and the "notify" signals are definitely different in the sense they provide a different potential, even though we're not sure whether such a potential is currently being properly used since:

 - Sometimes implementors seem not to be filling the old_value in the AtkPropertyValues struct passed along with the "property-change" signal, even when they seem to be able to do it so.

 - Mike Gorse commented that the old values are being plainly ignored by at-spi2 because it just gets the new value from the struct and send it alone through D-Bus, so even though implementors had set the old_value, it would get lost at that point.

 - It looks like it makes sense to _always_ send something of type AtkPropertyValues as the arg1 parameter for the 'property-change' signal, so we wonder why the callback defines a gpointer instead of forcing an AtkPropertyValues type. Perhaps there are some cases where it could make sense to send something else.

So, conclusions so far would basically be:

  - Investigate a little bit more about the origins and reasons behind this 'property-change' signal, and check whether the arg1 parameter could be changed from a gpointer type to a AtkPropertyValues type.

  - Check if implementors that could be filling the old value in that struct are or ignoring it, and if that's the case, fill bugs for those.

  - It would be great to fix at-spi2 so it doesn't keep ignoring the old value when re-emitting the signal through D-Bus

[1] https://live.gnome.org/Hackfests/ATK2011
Comment 4 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-05-10 11:03:25 UTC
Additional info:

GObject::notify is being redefined, and it uses atk_object_notify.

In summary it emits the signal "property-change", filling the new_value field and not taking care of the old value.

So in the end it seems that this signals was added in order to extend the notify signal, and emit the property change with an old and a new value.

Anyway, Mario also pointed that it would be difficult to emit this "property-change" with the new and old value without triggering the atk_object_notify, that would implement this.

Now I think that it wIt is clear that we need to sort out how this signal is supposed to be used, and improve the documentation of that signal.
Comment 5 André Klapper 2011-06-23 22:06:26 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 6 Alejandro Piñeiro Iglesias (IRC: infapi00) 2011-12-14 23:37:15 UTC
More about this: bug 666210
Comment 7 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-12-10 12:09:58 UTC
I have been reviewing this during webkitgtk2013 hackfest.

(In reply to comment #0)

> Whose purpose overlaps. After think a little if having property-change has any
> advantage, I only can think on:
>    * Having property-change allow to have a better control about when emit the
> signal, as "notify" docs explicitly say that a emission of that signal doesn't
> means always a change on the property.
>    * Right now property-change is only used on a subset of all the AtkObject
> properties. So having this signal adds a extra flexibility here. But not sure
> if this is a advantage, as you can connect to specific details on the "notify"
> signal.

As I say on comment 4, property-change is basically a GObject:notify forward. But I found the reason of this forward. GObject:notify doesn't allow hooks (is defined as NO_HOOK [1]). That means that you can't listen to this signal using atk_add_global_event_listener, that is the procedure used to forward the event via DBUS.

> So, should we deprecate AtkObject "property-change" and use GObject "notify"
> instead?

We can't deprecate "property-change", as right now we depend on global event listening.


> (In reply to comment #1)

> If finally this signal is not deprecated, it is clear that the documentation
> should be clear about:
>   * Why it is required

Now we know why.

>   * When it should be used.

Implementors shouldn't use it. It is just an AtkObject (and subclasses) notify event forwarding procedure. That needs to be documented.


(In reply to comment #3)
> After some discussion in the ATK/AT-SPI Hackfest [1], we kind of agree that the
> "property-change" and the "notify" signals are definitely different in the
> sense they provide a different potential, even though we're not sure whether
> such a potential is currently being properly used since:
> 
>  - Sometimes implementors seem not to be filling the old_value in the
> AtkPropertyValues struct passed along with the "property-change" signal, even
> when they seem to be able to do it so.
> 
>  - Mike Gorse commented that the old values are being plainly ignored by
> at-spi2 because it just gets the new value from the struct and send it alone
> through D-Bus, so even though implementors had set the old_value, it would get
> lost at that point.

old value seems to be just not used at all: lets document that.

>  - It looks like it makes sense to _always_ send something of type
> AtkPropertyValues as the arg1 parameter for the 'property-change' signal, so we
> wonder why the callback defines a gpointer instead of forcing an
> AtkPropertyValues type. Perhaps there are some cases where it could make sense
> to send something else.

Using something that is not an AtkPropertyValues is a bad idea. at-spi2-atk assumes an AtkPropertyValues. We will document that an AtkPropertyValues is supposed.

> So, conclusions so far would basically be:

This signal is needed. Need to improve documentation.

[1] https://developer.gnome.org/gobject/unstable/gobject-Signals.html#GSignalFlags
Comment 8 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-12-10 16:04:58 UTC
Created attachment 263929 [details] [review]
Improve documentation for AtkObject::property-change

This patch improves the documentation, and clarifies why it is needed, how it should be used, and who should be used. I think that it is enough to close the bug.

Review of the patch, specifically the wording is welcome.
Comment 9 Mario Sánchez Prada 2013-12-11 09:56:31 UTC
Review of attachment 263929 [details] [review]:

I think the documentation makes now clear why that signal is there and why it should be normally ignored, so looks good enough to me :)
Comment 10 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-12-11 11:51:39 UTC
Comment on attachment 263929 [details] [review]
Improve documentation for AtkObject::property-change

Reviewed by Joanmarie Diggs on IRC
Comment 11 Alejandro Piñeiro Iglesias (IRC: infapi00) 2013-12-11 11:52:37 UTC
Two reviews. Committed after some changes proposed by Joanmarie Diggs. Closing bug.