GNOME Bugzilla – Bug 705580
Deprecate atk_text_get_text_at_offset()
Last modified: 2013-12-16 11:18:16 UTC
Deprecate this function by adding the new API New API atk_text_get_text_for_offset() as discussed in [1], so we can move away from the at/before/after methods and _END boundaries once and for all. [1]https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00001.html
Created attachment 250992 [details] [review] Patch proposal Sorry for the so many lines changes due to alignment :/
Created attachment 250993 [details] [review] Patch proposal This is the right one, sorry for the mess
Review of attachment 250993 [details] [review]: Looks good, in any case there are some issues to take into account. ::: atk/atktext.c @@ +467,3 @@ + * returned substring + * + * Gets the specified text. This documentation is not enough. I would adapt the current documetation from the deprecated atk_text_get_text_at_offset. @@ +470,3 @@ + * + * Returns: a newly allocated string containing the text for the @offset bounded + * by the specified @granularity. Use g_free() to free the returned string. Taking into account that we are returning NULL in some cases (error+not-implementation) I would mention that there. Taking into account the current discussion at the mailing list: https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00007.html I think that eventually we would return a -1 also on the offsets. @@ +479,3 @@ +{ + AtkTextIface *iface; + gint local_start_offset, local_end_offset; We don't have still a conclusion, but probably we will conclude that the default offsets for this would be -1 @@ +1295,3 @@ + line = atk_text_get_text_for_offset (text, bounds_max_offset, + ATK_TEXT_GRANULARITY_LINE, + &max_line_start, &max_line_end); Hmm, right now the idea is adding a wrapping to the old method on the bridge, until all the current implementors move to implement the new method. But that solution will not work here. And both options I see: * Keep using the deprecated method * Implement the same wrap here (that in any case would mean keep using the deprecated method) Seems somewhat hacky to me. So I foresee that this method will be failing until the implementors implements the new method. Any suggestion are welcome. ::: atk/atktext.h @@ +132,3 @@ + * Text boundary types used for specifying boundaries for regions of text. + * This enumerationis deprecated since 2.9.6 and should not be used. Use + * AtkTextGranularity with #atk_text_get_text_for_offset instead. Could be 2.9.4 instead? @@ +232,3 @@ * is deprecated and it should not be overridden. + * @get_text_at_offset: Gets specified text. This virtual function + * is deprecated and it should not be overridden. As you are adding the virtual get_text_for_offset, could you add the documentation for that one? (Ideally, we should have the documentation for all of them, but writing for the others is unrelated with this bug). @@ +320,3 @@ + AtkTextClipType y_clip_type); + + /* Place additional API past this point not to break the ABI */ This is general to any glib interface, making the comment useless. ::: docs/tmpl/atktext.sgml @@ +289,3 @@ +@Returns: + + Sigh, some day bug 684665 should be solved and the sgml files removed from the repository. In any case, not related at all with this bug. </rant_over>
Review of attachment 250993 [details] [review]: Thanks for the review, I'll address your comments soon (hopefully today) ::: atk/atktext.c @@ +467,3 @@ + * returned substring + * + * @granularity: An #AtkTextGranularity Sorry about that, you are right. I might have removed what I had by mistake when creating the patch. Will add it back soon @@ +470,3 @@ + * + * Returns: a newly allocated string containing the text for the @offset bounded + * @end_offset: (out): the offset of the first character after the Sure, I will add that @@ +479,3 @@ +{ + AtkTextIface *iface; + * Sounds good. I will change the patch to reflect that, and update documentation accordingly. @@ +1295,3 @@ + line = atk_text_get_text_for_offset (text, bounds_max_offset, + ATK_TEXT_GRANULARITY_LINE, + &max_line_start, &max_line_end); Good point. It's probably better to leave the old implementation here then, so I'll remove this change from the patch ::: atk/atktext.h @@ +132,3 @@ + * Text boundary types used for specifying boundaries for regions of text. + * This enumerationis deprecated since 2.9.6 and should not be used. Use + * AtkTextGranularity with #atk_text_get_text_for_offset instead. Sure @@ +232,3 @@ * is deprecated and it should not be overridden. + * @get_text_at_offset: Gets specified text. This virtual function + * is deprecated and it should not be overridden. Sure, I'll add that one @@ +320,3 @@ + AtkTextClipType y_clip_type); + + void (* get_range_extents) (AtkText *text, ok
Created attachment 251234 [details] [review] Patch proposal New version addressing API's comments
Review of attachment 251234 [details] [review]: Looks good, except for two nitpicks (sorry for not realizing about that before). ::: atk/atktext.c @@ +501,3 @@ + * Returns: a newly allocated string containing the text for the @offset bounded + * by the specified @granularity. Use g_free() to free the returned string. + * Returns %NULL if the offset is invalid or no implementation is available. Missing "Since: 2.9.4" ::: atk/atktext.h @@ +297,3 @@ + gint end_offset); + gboolean (* set_caret_offset) (AtkText *text, + gint offset); There are a hundred of lines modified just for the sake of improving the indentation. Something that it is not related at all with this bug. @@ +328,3 @@ + AtkTextGranularity granularity, + gint *start_offset, + gint *end_offset); Ditto. @@ +419,3 @@ +AtkTextAttribute atk_text_attribute_for_name (const gchar *name); +const gchar* atk_text_attribute_get_value (AtkTextAttribute attr, + gint index_); Ditto.
Created attachment 251251 [details] [review] Patch proposal (In reply to comment #6) > Review of attachment 251234 [details] [review]: > > Looks good, except for two nitpicks (sorry for not realizing about that > before). You're absolutely right. Now attaching a new patch...
(In reply to comment #7) > Created an attachment (id=251251) [details] [review] > Patch proposal > > (In reply to comment #6) > > Review of attachment 251234 [details] [review] [details]: > > > > Looks good, except for two nitpicks (sorry for not realizing about that > > before). > > You're absolutely right. Now attaching a new patch... And now waiting a little for the final name, see bug 705713 comment 4. Although FWIW, atk_get_string_at_offset sounds good to me.
(In reply to comment #8) > (In reply to comment #7) > > Created an attachment (id=251251) [details] [review] [details] [review] > > Patch proposal > > > > (In reply to comment #6) > > > Review of attachment 251234 [details] [review] [details] [details]: > > > > > > Looks good, except for two nitpicks (sorry for not realizing about that > > > before). > > > > You're absolutely right. Now attaching a new patch... > > And now waiting a little for the final name, see bug 705713 comment 4. Although > FWIW, atk_get_string_at_offset sounds good to me. After one day thinking on that, right now I like more the option of atk_get_string_at_offset. So unless you really really want the blessing from more native english speakers, please go on with the patch update.
Created attachment 251477 [details] [review] Patch proposal (In reply to comment #9) > [...] > After one day thinking on that, right now I like more the option of > atk_get_string_at_offset. So unless you really really want the blessing from > more native english speakers, please go on with the patch update. Looks good to me. Here you have the new patch.
Review of attachment 251477 [details] [review]: Looks good. I found two nitpicks, but you can apply the patch solving those without submitting a new patch. Thanks for the patch. ::: atk/atktext.c @@ +565,3 @@ + * if an error has occurred (e.g. invalid offset, not implemented) + * @end_offset: (out): the offset of the first character after the returned string, + * or -1 if an error has occurred (e.g. invalid offset, not implemented) Not implemented should be handled by the atk_text_get_string_at_offset implementation at ATK (so the code below), and there isn't anything filling the -1 value. I think that the easier would be set *start_offset/*end_offset to -1 as default value at the "if (start_offset)" if ::: atk/atktext.h @@ +235,3 @@ * @get_text_before_offset: Gets specified text. This virtual function * is deprecated and it should not be overridden. + * @get_string_at_offset: * Gets a portion of the text exposed through Nitpick: that '*' at "* Gets" is not needed.
(In reply to comment #11) > Review of attachment 251477 [details] [review]: > > Looks good. I found two nitpicks, but you can apply the patch solving those > without submitting a new patch. > Thanks for the review. Patch committed with those last minute fixes: https://git.gnome.org/browse/atk/commit/?id=3261a8a792f6688b5575512b8828eea262164713
*** Bug 668119 has been marked as a duplicate of this bug. ***
*** Bug 700183 has been marked as a duplicate of this bug. ***