GNOME Bugzilla – Bug 740066
The documentation does not make it clear that state-change notifications are expected
Last modified: 2014-11-14 12:12:15 UTC
Created attachment 290639 [details] [review] Proposed patch An implementor once expressed surprise when I suggested that state-change notifications are usually expected when the state of an object changes. While I was surprised by his surprise (is an AT supposed to just magically know??), our documentation admittedly does not contain anything to inform implementors of this expectation (is an implementor supposed to just magically know?? ;) ) The attached patch adds an explicit statement that this notification is generally expected. It also adds references in several places so that implementors will be able to find this expectation. Lastly, while checking the resulting documentation changes, I noticed that clicking on "AtkStateSet" on the AtkObject doc page, jumps you down to Types and Values, but there's nothing about AtkStateSet there. See [1]. So I fixed that too. [1] https://developer.gnome.org/atk/unstable/AtkObject.html#AtkStateSet
Created attachment 290642 [details] [review] Some extra notes The previous patch looks good, but probably it would be good to even add more notes. This patch includes a comment that I think that should be explained somewhere. Feel free to change the place and mix with your patch. FWIW, the conversation I had with Joanmarie via IRC reminded me this old bug: https://bugzilla.gnome.org/show_bug.cgi?id=635807 That I opened due the doubts I had when I started to look to AtkStateSet. Instead of just closing the bug, we should had updated the documentation.
Created attachment 290647 [details] [review] Proposed patch Thanks for the review and notes. This version: 1. Changes the documentation I wrote because what I wrote was based on my misunderstanding and/or forgetting that the add and remove methods for state sets don't modify existing state sets. So my documentation implied things that aren't true. Oops. 2. Points out what Piñeiro stated in his extra notes patch -- and does so in multiple places so that we all know and remember that state sets are read-only.
Created attachment 290648 [details] [review] Proposed patch Take 2.1: Typo correction. Sorry for the noise.
Review of attachment 290648 [details] [review]: Looks good. Thanks for the patch.
Comment on attachment 290648 [details] [review] Proposed patch https://git.gnome.org/browse/atk/commit/?id=be5805852 Thanks!