GNOME Bugzilla – Bug 705713
Deprecate getText{At|Before|After}Offset()
Last modified: 2015-02-27 16:41:04 UTC
Following the discussion in [1] and work currently in progress and tracked along with bug 697969 and bug 705580, we would need to update as well pyatspi2 to deprecate getText{At|Before|After}Offset() functions in favour of the new getTextForOffset() function. This bug is for that [1]https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00001.html
Created attachment 251230 [details] [review] Patch proposal This should do the work
Review of attachment 251230 [details] [review]: Note: I am pretty new to the atspi interface so you should NOT consider this a code review, at all. In fact I am just a GSoC student working on an a11y project for gnome-shell and so that why I am keen to understand the significance of the changes you are making. Apologies if it is the wrong place to be asking questions and feel free to ignore me if you have not got the time to answer the queries I wrote below. ::: pyatspi/text.py @@ +494,3 @@ return textRangeToList(ret) + def getTextForOffset(self, offset, type): Is the text really 'for' the offset? This seems a bit strange. Why does the offset want it? @@ +500,3 @@ + or paragraph granularity as specified by type. The starting and ending + offsets of the resulting substring are returned in startOffset + and endOffset. Are you talking about the arguments or the return value? @@ +501,3 @@ + offsets of the resulting substring are returned in startOffset + and endOffset. + @param : offset It might be useful if you say what kind of param this really is (integer for example). i.e. there you seem to call it an 'offset' and then describe it as one. @@ +512,3 @@ + @param : startOffset + back-filled with the starting offset of the resulting substring, + if one exists. What happens if one does not exist? What is 'one' anyway? Is it the starting offset or the resulting substring?
Thanks for this "informal review", Magdalene. It is very much appreciated! (In reply to comment #2) > [...] > Note: I am pretty new to the atspi interface so you should NOT consider this a > code review, at all. In fact I am just a GSoC student working on an a11y > project for gnome-shell and so that why I am keen to understand the > significance of the changes you are making. Yeah, I know who you are from the dinner the day of the a11y BoF in Brno, and you probably know who I am because I live in the lovely town of Staines, like Ali G :) > Apologies if it is the wrong place > to be asking questions and feel free to ignore me if you have not got the time > to answer the queries I wrote below. No apologies needed. Thanks for your comments. > ::: pyatspi/text.py > @@ +494,3 @@ > return textRangeToList(ret) > > + def getTextForOffset(self, offset, type): > > Is the text really 'for' the offset? This seems a bit strange. Why does the > offset want it? We were looking for a new name different than getTextAtOffset() to have a provide a new API without breaking the old one, and getTextForOffset() is the best that Pineiro and me could come up that was generic enough not to specify whether the text to be retrieved would be taking from the position at, after or before the offset. In our non-native English speaking minds it sounded good, but if you think it's a weird name we would probably need to aim for something different. Any specific suggestion? Please note that this name has not been proposed only for pyatspi, but also for ATK (bug 705580), at-spit2-core (bug 697969) and the at-spi2-atk bridge (bug 705581), so it's quite important I'd say to agree on a good name since it will be propagated across four different projects. > @@ +500,3 @@ > + or paragraph granularity as specified by type. The starting > and ending > + offsets of the resulting substring are returned in startOffset > + and endOffset. > > Are you talking about the arguments or the return value? Both. I'm talking about startOffset and endOffset (out) arguments, which are the recipients used to return those values, once found. > @@ +501,3 @@ > + offsets of the resulting substring are returned in startOffset > + and endOffset. > + @param : offset > > It might be useful if you say what kind of param this really is (integer for > example). i.e. there you seem to call it an 'offset' and then describe it as > one. I was actually just "basing" that documentation in the current one for the other methods, but I agree with you is a poor comment and I will try to extend it in a further patch. > @@ +512,3 @@ > + @param : startOffset > + back-filled with the starting offset of the resulting > substring, > + if one exists. > > What happens if one does not exist? What is 'one' anyway? Is it the starting > offset or the resulting substring? Again, taken from the current documentation of the other methods, yet agreeing that we should be more explicit mentioning, for instance, that a -1 would be returned if an error has happened and an offset could not be found (e.g. the exposed object does not implement the AtkText interface, the passed (in) offset is out of range...) Again, thanks for your comments. I will upload a new patch with the proposed changes soon, but first let's agree on the naming thing since that's the most important point under discussion here.
(In reply to comment #2) > Review of attachment 251230 [details] [review]: > > Note: I am pretty new to the atspi interface so you should NOT consider this a > code review, at all. In fact I am just a GSoC student working on an a11y > project for gnome-shell and so that why I am keen to understand the > significance of the changes you are making. Apologies if it is the wrong place > to be asking questions and feel free to ignore me if you have not got the time > to answer the queries I wrote below. > > ::: pyatspi/text.py > @@ +494,3 @@ > return textRangeToList(ret) > > + def getTextForOffset(self, offset, type): > > Is the text really 'for' the offset? This seems a bit strange. Why does the > offset want it? The bug (and all the related, like bug 705580) is about deprecating getTextAtOffset for a new method with a different API. So as it is a new method, it also need a new name, with a equivalent meaning. Mario suggested getTextForOffset, and that sounded well enough for me. Anyway, using our card of "non-native english speaker" again, probably I was wrong. Just talking with Joanmarie on IRC, she mentioned that using for_offset was "fine", but at was still better. She also suggested (for ATK), atk_text_get_string_at_offset. And I see some advantages on this name: * get text is more ambiguous (that also happened with the previous methods) * text is usually used as the text object, but we are using this method to get a text, that is in fact a string * The documentation for the (ATK) method is already hinting that: "Returns: a newly allocated *string* containing the text for the @offset bounded by the specified @granularity." > @@ +500,3 @@ > + or paragraph granularity as specified by type. The starting > and ending > + offsets of the resulting substring are returned in startOffset > + and endOffset. > > Are you talking about the arguments or the return value? > > @@ +501,3 @@ > + offsets of the resulting substring are returned in startOffset > + and endOffset. > + @param : offset > > It might be useful if you say what kind of param this really is (integer for > example). i.e. there you seem to call it an 'offset' and then describe it as > one. > > @@ +512,3 @@ > + @param : startOffset > + back-filled with the starting offset of the resulting > substring, > + if one exists. > > What happens if one does not exist? What is 'one' anyway? Is it the starting > offset or the resulting substring? Mario took all this documentation from the already existing getTextAtOffset.
(In reply to comment #4) > [...] > > Is the text really 'for' the offset? This seems a bit strange. Why does the > > offset want it? > > The bug (and all the related, like bug 705580) is about deprecating > getTextAtOffset for a new method with a different API. So as it is a new > method, it also need a new name, with a equivalent meaning. Mario suggested > getTextForOffset, and that sounded well enough for me. Anyway, using our card > of "non-native english speaker" again, probably I was wrong. > > Just talking with Joanmarie on IRC, she mentioned that using for_offset was > "fine", but at was still better. She also suggested (for ATK), > atk_text_get_string_at_offset. And I see some advantages on this name: > > * get text is more ambiguous (that also happened with the previous methods) > * text is usually used as the text object, but we are using this method to > get a text, that is in fact a string > * The documentation for the (ATK) method is already hinting that: > "Returns: a newly allocated *string* containing the text for the @offset > bounded by the specified @granularity." > FWIW, I'm perfectly fine with atk_text_get_string_at_offset() as a name too. Magdalen, what do you think?
Created attachment 251482 [details] [review] Patch proposal New patch to fix this issue after agreeing on the get_string_at_offset new name
[Mass-resetting default assignee, see bug 705890. Please reclaim this bug report by setting the assignee to yourself if you still plan to work on this. Thanks!]
Comment on attachment 251230 [details] [review] Patch proposal Committed as b41c8f.
[Moving at-spi/pyatspi2 bugs to separate product. See bug 740075]