GNOME Bugzilla – Bug 705581
Implement GetTextForOffset in the text adaptor in the ATK bridge
Last modified: 2013-09-02 12:57:50 UTC
Implement this function in the ATK bridge as discussed in [1], so the work already started through bug 697969 and bug 705580 are finally complete (and so the ATs can actually use the new API in AtkText) [1]https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00001.html
Created attachment 250994 [details] [review] Patch proposal
Review of attachment 250994 [details] [review]: Informal review (as I'm not the maintainer of the bug), but just wanted to mention something. ::: atk-adaptor/adaptors/text-adaptor.c @@ +321,3 @@ + } + +#if ATK_CHECK_VERSION (2, 9, 4) See my explanation at the devel mailing list: https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00006.html But in short: that macro only provides you the information about the atk in use with at-spi-atk, not the implementor, so that check doesn't provide the information we wanted. In my humble opinion, it would be better to replace this for a atk dependency bump on configure.ac, and keeping the fallback. FWIW, fallback that eventually should disappear. Probably we could mention that the fallback will be there for 3.10, but 3.12 would expect all the atk implementors to have done their job.
Created attachment 251235 [details] [review] Patch proposal (In reply to comment #2) > Review of attachment 250994 [details] [review]: > > Informal review (as I'm not the maintainer of the bug), but just wanted to > mention something. Thanks. I'm now uploading a new patch taking into consideration your comments here. > ::: atk-adaptor/adaptors/text-adaptor.c > @@ +321,3 @@ > + } > + > +#if ATK_CHECK_VERSION (2, 9, 4) > > See my explanation at the devel mailing list: > https://mail.gnome.org/archives/gnome-accessibility-devel/2013-August/msg00006.html > > But in short: that macro only provides you the information about the atk in use > with at-spi-atk, not the implementor, so that check doesn't provide the > information we wanted. In my humble opinion, it would be better to replace this > for a atk dependency bump on configure.ac, and keeping the fallback. I agree. That is probably better in the end. Patch updated. > FWIW, fallback that eventually should disappear. Probably we could mention that > the fallback will be there for 3.10, but 3.12 would expect all the atk > implementors to have done their job. Yes, I personally will take care of doing it in WebKitGTK+, unless someone else is faster :)
Created attachment 251480 [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 251480 [details] [review] Patch proposal I'm sorry for not looking at this until now. I would have put the fallback code in atk (ie, added an atk_text_real_get_string_at_offset) rather than at-spi2-atk, but either is fine really. One knit: you have a reference to get_text_for_text() in a comment; should be get_string_at_offset. It looks fine other than that, though, so I'd suggest updating the comment and committing.
(In reply to comment #6) > (From update of attachment 251480 [details] [review]) > I'm sorry for not looking at this until now. > > I would have put the fallback code in atk (ie, added an > atk_text_real_get_string_at_offset) rather than at-spi2-atk, but either is fine > really. Mario and me debated about that during GUADEC. The main reason to not implement the fallback at ATK was avoid to use ATK deprecated code at ATK itself. I know that this is debatable, as atk_text_real_get_bounded_ranges is still using the deprecated method, but in any case, thanks for allowing the fallback at at-spi2-atk.
Comment on attachment 251480 [details] [review] Patch proposal Committed as 2fc87b, with the comment fixed. Also, if you submitted a patch for at-spi2-core, then I couldn't find it, so I ended up writing my own, since I needed to make the release. Thanks for all of your work.
(In reply to comment #8) > (From update of attachment 251480 [details] [review]) > Committed as 2fc87b, with the comment fixed. Thanks for taking care of this, Mike. I just arrived today from holidays and could not check it before, sorry about that. > Also, if you submitted a patch for at-spi2-core, then I couldn't find it, so I > ended up writing my own, since I needed to make the release. The patch for at-spi2-core was attached for bug 697969, but it's basically the same thing that you committed upstream already now (I checked it now). The only difference is that you are putting atspi_text_get_text_at_offset() only behind the ATSPI_DISABLE_DEPRECATED guards, instead of doing it for the three old methods (before, at and after), but everything else it's identical. For that detail, I will just provide a new patch for bug 697969 so we can have everything completely finished. > Thanks for all of your work. Thanks