GNOME Bugzilla – Bug 778928
Ctrl-navigation works in opposite direction in right-to-left text
Last modified: 2017-03-29 14:26:37 UTC
Closely related bug: https://bugzilla.gnome.org/show_bug.cgi?id=136059 While editing RTL text in gtksourceview, holding down Ctrl and pressing left/right arrow keys move the cursor in opposite direction. Arabic, Hebrew and Persian languages use right-to-left scripts and hence are affected by this bug. For RTL users this is totally counter intuitive and confusing. This problem has been thoroughly discussed in the above mentioned GTK bug from 2004. Just recently, that bug is fixed. Even though gtksourceview is based on gtktextview (subject of the above GTK bug), it modifies it in such a way that cursor movement is done in gtksourceview and not its parent. Therefore a similar patch as in bug #136059 is necessary.
*** Bug 778878 has been marked as a duplicate of this bug. ***
c.f. https://bugzilla.gnome.org/show_bug.cgi?id=136059#c68
Please search the bugzilla before filing a new bug, this bug was already reported. But this one has more information (related GTK+ bug fixed recently). From bug #778878: Steps to reproduce: Just paste the following text to the window: שרה שרה שיר שמח And jump using Ctrl.
Setting this bug as depend on bug 136059 as it needs public api from it.
(In reply to Nelson Benitez from comment #4) > Setting this bug as depend on bug 136059 as it needs public api from it. Please, no. The 13-year-old issue for which that bug was opened has (in terms of the classes used by GEdit at the time it was opened) been resolved (as was another one, into the bargain). Your efforts to improve the performance of said resolution are much appreciated, but because they end up requiring new enhancements/API, they become a separate issue.
Created attachment 346488 [details] [review] Patch to fix GtkSourceView handling of RTL 'CTRL + cursor' movement Patch depending on patch posted on bug 779081
Created attachment 348747 [details] [review] Use current line to find out text direction using existing API This patch fixes this bug with API from GtkTextIter and Pango. The approach is reading the current line and ask pango to find out its direction. This was mistakenly posted to the Bug 779081 initially. (In reply to Daniel Boles from comment #29) > Review of attachment 348738 [details] [review] [review]: > > Mostly nitpicks on comments and coding style: > > ::: gtksourceview/gtksourceview.c > @@ +2129,3 @@ > + GtkTextIter start; > + GtkTextIter end; > + gchar *rtl_specimen; > > Personally I'm not convinced this name is better. I'd prefer something like > line_text, text, or the original str, but that's just my opinion. > > You can check whether there is any convention used elsewhere in the same > file for temporary strings, and follow that. > > @@ +2139,3 @@ > + start = end = newplace = insert; > + > + /* Select a reasonable piece of text for RTL analysis */ > > Since this just gets the text of the line containing the insert/newplace > iterator, this comment should just say that. > > @@ +2141,3 @@ > + /* Select a reasonable piece of text for RTL analysis */ > + gtk_text_iter_set_line_offset (&start, 0); > + gtk_text_iter_forward_line(&end); > > Add the missing space between function name and argument list. > > @@ +2146,3 @@ > + /* Swap direction for RTL */ > + if (pango_find_base_dir(rtl_specimen, -1) == PANGO_DIRECTION_RTL) > + count *= -1; > > Add the missing space between function name and argument list, and indent by > 2 spaces instead of tab. > > @@ +2149,3 @@ > + > + /* Doe with the selection string */ > + g_free(rtl_specimen); > > There's a typo in the comment, but I don't think it's necessary anyway: the > code explains itself. And add another missing space in the function call. Thanks for your quick review! I have applied all your changes except the comment. In that case I think mentioning RTL analysis is describing the purpose of those lines, otherwise the function call by itself is clear, but meaningless.
Review of attachment 348747 [details] [review]: Thanks for the updates! ::: gtksourceview/gtksourceview.c @@ +2139,3 @@ + line_start = line_end = newplace = insert; + + /* Select a reasonable piece of text for RTL analysis */ I didn't actually mean that you should remove this comment. I meant that instead of "Select a reasonable piece of text", it should say something like "Get the text of the current line". There is no question whether the selection is reasonable: it is, as it reflects the current line, and it's that line whose direction we want to ascertain. So the comment can be as precise as the code! @@ +2146,3 @@ + /* Swap direction for RTL */ + if (pango_find_base_dir (line_text, -1) == PANGO_DIRECTION_RTL) + count *= -1; Please excuse my previous silly comment about the indentation. I was still thinking in GTK+ mode... Clearly, GtkSourceView uses tabs for indentation, and your original patch was perfectly correct here. Do change it back! Sébastien can probably offer a better review than me anyway, so it might be worth waiting.
Created attachment 348748 [details] [review] Use current line for RTL direction analysis (minor comment/style changes) The is the same as previous patch, except for comment and style changes. Thanks for the quick reviews Daniel!
Review of attachment 348748 [details] [review]: And thanks a lot for the work and the fast updates! This looks A-OK to me now. My one last comment (like Columbo) is about the commit message: although much less important than the fix to the code, it would still be nice if it explained what the problem was, not just how the solution works. For what it's worth, here is what I would write. It's very possible you can come up with a better summary! Using Ctrl + Left or Right to jump by words was not accounting for the possibility that the text was right-to-left. So, the observed direction of movement was opposite to the direction of the arrow key pressed. This was unintuitive and inconvenient for users editing RTL text. This patch gets the Pango direction of the text in the current line when Ctrl + an arrow key is pressed, and inverts the direction of movement if it is RTL, thus ensuring that the pressed and observed direction match. Anyway, of course, I'll let Sébastien take it from here.
(In reply to Daniel Boles from comment #10) > Review of attachment 348748 [details] [review] [review]: > > And thanks a lot for the work and the fast updates! This looks A-OK to me > now. > > My one last comment (like Columbo) is about the commit message: although > much less important than the fix to the code, it would still be nice if it > explained what the problem was, not just how the solution works. > > For what it's worth, here is what I would write. It's very possible you can > come up with a better summary! > > Using Ctrl + Left or Right to jump by words was not accounting for the > possibility that the text was right-to-left. So, the observed direction > of movement was opposite to the direction of the arrow key pressed. This > was unintuitive and inconvenient for users editing RTL text. > > This patch gets the Pango direction of the text in the current line when > Ctrl + an arrow key is pressed, and inverts the direction of movement if > it is RTL, thus ensuring that the pressed and observed direction match. > > Anyway, of course, I'll let Sébastien take it from here. Your explanation looks perfectly OK to me! I'll try to explain the problem along with the solution in my future patches.
If a line contains mixed LTR and RTL text, does it work as intended?
(In reply to Sébastien Wilmet from comment #12) > If a line contains mixed LTR and RTL text, does it work as intended? I have tried various ways that I might write BiDi text (single or multi line) and as a user that all the time mixes LTR and RTL, it looks good. However, the nature of BiDi is complex. If you start a sentence with a single Latin alphabet, and write the rest using Arabic, our current approach with Pango considers the base direction to be LTR. These edge cases should be optimized inside Pango.
pango_find_base_dir() returns based on the first character with a "strong direction", so the behaviour should be determined by the order of the respective LTR and LTR words/characters in the line. I guess this is not necessarily correct for every cursor position. I did wonder whether it might be better to work on the basis of the current word instead, or something. As I'm not a regular RTL user, other people would need to evaluate this option. (It seems to me that we then have to consider a lot of different ways to delimit words, etc.)
Review of attachment 348748 [details] [review]: Small nitpicks, the code seems correct. Thanks for the patch! ::: gtksourceview/gtksourceview.c @@ +2144,3 @@ + line_text = gtk_text_iter_get_visible_text (&line_start, &line_end); + + /* Swap direction for RTL */ If prefer if the comment also explains *why* the direction is swapped. @@ +2146,3 @@ + /* Swap direction for RTL */ + if (pango_find_base_dir (line_text, -1) == PANGO_DIRECTION_RTL) + count *= -1; In GtkSourceView we add curly braces around 1-line blocks too, to avoid the need to remove/add them all the time.
Review of attachment 348748 [details] [review]: In the commit message the bugzilla URL has not been updated.
(In reply to Daniel Boles from comment #14) > I did wonder whether it might be better to work on the basis of the current > word instead, or something. As I'm not a regular RTL user, other people > would need to evaluate this option. (It seems to me that we then have to > consider a lot of different ways to delimit words, etc.) I have done that in my first try. It is very inaccurate. The behavior is much more reliable considering the current line (our current approach). This is a significant improvement over the current state of gtksourceview. BTW, starting a line with a Latin letter is uncommon. Latin numbers work fine, however.
Created attachment 348932 [details] [review] Use current line for RTL direction analysis (minor style changes) I applied the style conventions of Gtksourceview. Moreover, I added the problem description written by Daniel to the commit message.
Created attachment 348934 [details] [review] Use current line for RTL direction analysis (update the comment) Update the code comment section with Daniel's description (you described it better than myself, thanks!) and make it look like other multi-line comments.
Created attachment 348936 [details] [review] Use current line for RTL direction analysis (update bugzilla link) Update bugzilla URL.
Review of attachment 348936 [details] [review]: ::: gtksourceview/gtksourceview.c @@ +2156,3 @@ + + * See https://bugzilla.gnome.org/show_bug.cgi?id=778928 for more details. + */ I think this is too much. Personally I would keep it to one line of summary like before. Remember that "git blame" lets people find the commit and from there get all the gory details, like the commit message and BZ link, if they really want it. In the code, though, a comment should just succinctly summarise why something is done, if it's not already clear from the code, without going into full depth
Pushed slightly modified commit 494c76dd359351b8a703b6aec328126b0f84828b and cherry-picked on gnome-3-24. Thanks!
(In reply to Daniel Boles from comment #21) > Review of attachment 348936 [details] [review] [review]: > > ::: gtksourceview/gtksourceview.c > @@ +2156,3 @@ > + > + * See https://bugzilla.gnome.org/show_bug.cgi?id=778928 for more details. > + */ > > I think this is too much. Personally I would keep it to one line of summary > like before. Remember that "git blame" lets people find the commit and from > there get all the gory details, like the commit message and BZ link, if they > really want it. In the code, though, a comment should just succinctly > summarise why something is done, if it's not already clear from the code, > without going into full depth Hmm. While applying Sébastien's comment I looked how other comment blocks are formatted, so I saw another one with Bugzilla URL in the same file and I did the same! So, should I proceed and remove the link? (OK, when I was submitting this I got a 'collision on the fly!' error. The patch is merged.) Thank you Daniel and Sébastien!
(In reply to Mehdi Sadeghi from comment #23) > Hmm. While applying Sébastien's comment I looked how other comment blocks > are formatted, so I saw another one with Bugzilla URL in the same file and I > did the same! So, should I proceed and remove the link? The number of context lines added to the review comment makes it look like I was talking about the link, but I was actually referring to the preceding message. As I said, I think a one line summary, and/or the BZ link, is fine - but pasting in the entire narrative was excessive IMO. Anyway, it's pushed and looks great now! Thanks again for all your investigation and work on landing this improvement.