GNOME Bugzilla – Bug 779081
GtkTextView: expose API to get Pango line direction
Last modified: 2017-03-29 15:37:05 UTC
There is this recently fixed bug #136059 of GtkTextView that solves a 13 years old problem of ctrl+arrow keys movement in RTL text. However, most people had reported that bug against gedit which actualy uses GtkSourceView which is a heavily modified subclass of GtkSourceView. In order to fix this problem it is necessary to expose the functionality to get the Pango direction of a line of text. Nelson Benitez has already made a patch that exposes that functionality and also fixes the problem in GtkSourceView (bug #778928). Here is his API patch: https://bugzilla.gnome.org/attachment.cgi?id=346460&action=diff Here is hist GtkSourceView patch: https://bugzilla.gnome.org/attachment.cgi?id=346461&action=diff
Mehdi: Thank you for creating the new bug. Let's let Bug 136059 rest at long last. Nelson: To enable review in place, please attach your updated GTK+ patch here, and your GtkSourceView one at Bug 778928.
sorry, forgot to CC: (In reply to Daniel Boles from comment #1) > Nelson: To enable review in place, please attach your updated GTK+ patch > here, and your GtkSourceView one at Bug 778928.
Isn't it possible to get the text direction at a GtkTextIter with the gtk_text_iter_get_attributes() function. The GtkTextAttributes struct has a GtkTextDirection field.
?
(In reply to Sébastien Wilmet from comment #3) > Isn't it possible to get the text direction at a GtkTextIter with the > gtk_text_iter_get_attributes() function. The GtkTextAttributes struct has a > GtkTextDirection field. Maybe, though that dives into _gtk_text_attributes_fill_from_tags(), which is a bit of a beast. That's almost certainly better performance wise than the current trip through gtk_text_layout_get_line_display(), but the proposed gtk_text_layout_find_line_base_dir() still looks leanest for the current purposes.
Performance should not be a problem for the intended use-case, which is to move by words with ctrl+left/right. If it's possible to implement in GtkSourceView what we need with the current API, there is no need to add a new API.
Ah, GtkTextAttributes is now private in GTK+ 4… But GtkSourceView still uses GTK+ 3.
Just a note that a fix might also be required that occurred in the other widgets (haven't tried it) for the following: * Select some text * Press left/right, or Ctrl + left/right * Cursor moves to wrong, opposite end of selection than pressed
(In reply to Daniel Boles from comment #8) > Just a note that a fix might also be required that occurred in the other > widgets (haven't tried it) for the following: > * Select some text > * Press left/right, or Ctrl + left/right > * Cursor moves to wrong, opposite end of selection than pressed I've tried Nelson's patch and I didn't experience this problem.
Created attachment 346485 [details] [review] New patch that adds GtkTextView public API to be used by eg. GtkSourceView
Thank you Daniel and Mehdi for creating new bugzilla bugs. (In reply to Sébastien Wilmet from comment #3) > Isn't it possible to get the text direction at a GtkTextIter with the > gtk_text_iter_get_attributes() function. The GtkTextAttributes struct has a > GtkTextDirection field. Hi sebastian, interesting suggestion! I went right to it to tried it, but it's proved not working in my testing, the key to that is in _gtk_text_attributes_fill_from_tags () function, which if you inspect you'll see look for some tags to set some values, in the case of ->direction field it doesn't look for any tag so it just blindly set the default value already existent on the passed values struct. I will post the patch I used to test in case you want to try it.
Created attachment 346490 [details] [review] Diff of debug test for gtk_text_iter_get_attributes()
Created attachment 346492 [details] [review] GtkSourceView part of debug test for gtk_text_iter_get_attributes()
Forgot to put convenience link to the function I mentioned: https://git.gnome.org/browse/gtk+/tree/gtk/gtktextattributes.c#n245 That get's called from gtk_text_iter_get_attributes (). So, I think the patches I posted are still relevant.
I just went through many duplicates of the original bug, seven of them are reported directly against gedit. Moreover, I realized that there are other non-GNOME tools such as Zim wiki which are using gtksourceview via pygtk. These are not fixed yet. After 13 years old we identified the problem and fixed the original bug in Gtktextview. However, all the other duplicates which are opened against gedit are still waiting for some love. We have identified the problem and we have a working patch, what is blocking the progress?
Partly Emmanuele's point on one of the other bugs, that this requires new API, which isn't something that just gets added to stable releases like GTK+ 3.22 without proper consideration. And on that note - did you try to find a patch for SourceView that does not require new API, like we managed for TextView and Entry? Since you want the fix a lot, it might make sense to put that in place now, even if it's not maximally efficient (like the TextView one), so that the bug can be fixed. Then we can see what happens regarding the new API to do it in a less roundabout way.
(In reply to Daniel Boles from comment #16) > Partly Emmanuele's point on one of the other bugs, that this requires new > API, which isn't something that just gets added to stable releases like GTK+ > 3.22 without proper consideration. True. That's also my point, we have worked a lot and I want to see us make progress and not just forget and let the bug die for many more years. > And on that note - did you try to find a patch for SourceView that does not > require new API, like we managed for TextView and Entry? Since you want the > fix a lot, it might make sense to put that in place now, even if it's not > maximally efficient (like the TextView one), so that the bug can be fixed. > Then we can see what happens regarding the new API to do it in a less > roundabout way. Yes, I tried and failed miserably. Once before Nelson's patch and once after your comment! The problem is that GtkTextLayout and GtkTextLine and their private counterparts are not available to Gtksourceview. I didn't find a way that not involves those types, if there is one, give me a hint and I'll work on it. Any idea is much appreciated.
Created attachment 348285 [details] [review] Partially working patch This patch depends on current API, however it is yet incomplete.
Created attachment 348724 [details] [review] This patch fixes the problem without any new API This patch fixes the problem using already available functions. The approach is simple. Read the surrounding lines and ask pango to find out its direction. This patch makes my previous incomplete patch obsolete. I appreciate your feedback on the patch and will be very happy to see it in 3.22.
Review of attachment 348724 [details] [review]: ::: gtksourceview/gtksourceview.c @@ +2141,3 @@ + /* Select a reasonable piece of text for RTL analysis */ + gtk_text_iter_backward_line(&start); + gtk_text_iter_forward_line(&end); Doesn't this return the span between the start of the previous line and the end of the current line (really, start of next line)? Could that break cases where the previous line has a different direction? Maybe a slightly different approach is needed. For example, gtk_text_iter_set_line_offset (&start, 0); @@ +2145,3 @@ + + /* Swap direction for RTL */ + gtk_text_iter_backward_line(&start); Notwithstanding the above, I wonder if it might be better for TextView to use this kind of approach there too, instead of messing with Layouts and LineDisplays like it currently does.
(In reply to Daniel Boles from comment #20) > Review of attachment 348724 [details] [review] [review]: > > ::: gtksourceview/gtksourceview.c > @@ +2141,3 @@ > + /* Select a reasonable piece of text for RTL analysis */ > + gtk_text_iter_backward_line(&start); > + gtk_text_iter_forward_line(&end); > > Doesn't this return the span between the start of the previous line and the > end of the current line (really, start of next line)? Could that break cases > where the previous line has a different direction? Maybe a slightly > different approach is needed. For example, gtk_text_iter_set_line_offset > (&start, 0); Good catch! Yes, it breaks in that case. I made a new patch applying your suggestion and it works fine considering the current line. > @@ +2145,3 @@ > + > + /* Swap direction for RTL */ > + gtk_text_iter_backward_line(&start); > > Notwithstanding the above, I wonder if it might be better for TextView to > use this kind of approach there too, instead of messing with Layouts and > LineDisplays like it currently does. I think whichever is faster should be considered (I don't know which one).
Created attachment 348735 [details] [review] Use current line for RTL direction analysis This patch fixes the problem mentioned by Daniel. Namely, we have to consider the current line not previous one for guessing the direction, otherwise it will break cases when previous line has another direction.
(In reply to Mehdi Sadeghi from comment #21) > Good catch! Yes, it breaks in that case. I made a new patch applying your > suggestion and it works fine considering the current line. Great! > I think whichever is faster should be considered (I don't know which one). Actually, I wasn't thinking much before. In fact, I think what you do here is better, and is a candidate for replacing what is currently done in gtktextview.c as it would avoid how that must construct a LineDisplay just to incidentally get the direction out of it. So if the Pango method used here covers all cases, then we should think about applying it to TextView as well.
I can't test right now and forgot to ask: Does this also catch the case where there is a selection, and the user presses left/right to move to its start/end? i.e. does that take them to the right side of the selection when in RTL mode? (In reply to Daniel Boles from comment #23) > Actually, I wasn't thinking much before. um, I was, because I said the same thing in both cases :)
(In reply to Mehdi Sadeghi from comment #22) > Created attachment 348735 [details] [review] [review] > Use current line for RTL direction analysis > > This patch fixes the problem mentioned by Daniel. Namely, we have to > consider the current line not previous one for guessing the direction, > otherwise it will break cases when previous line has another direction. Good job Mehdi!! very nice patch, btw I think you forgot to free 'str' in your patch. I agree with Daniel, this looks slimmer than what was added to gtktextview.c
(In reply to Nelson Benitez from comment #25) > (In reply to Mehdi Sadeghi from comment #22) > Good job Mehdi!! very nice patch, btw I think you forgot to free 'str' in > your patch. Thanks Nelson! And yes, I made another patch to free the 'str'. Just a free time hacker C hacker!
Created attachment 348738 [details] [review] Use current line for RTL direction analysis (mem leak fixed) This is the same as previous patch except the memory leak fix pointed out by Nelson and better naming for the str variable.
(In reply to Daniel Boles from comment #24) > I can't test right now and forgot to ask: Does this also catch the case > where there is a selection, and the user presses left/right to move to its > start/end? i.e. does that take them to the right side of the selection when > in RTL mode? Pressing arrow keys with or without Ctrl key moves the cursor in visual order to the expected end. When we have a selection pressing ctrl and right arrow moves the cursor one word out of the right bound of the selection and vice versa. If we move selection inward, the cursor moves one word inside the select. In both cases the selection cancels correctly. I tried to explain it clearly!
Review of attachment 348738 [details] [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.
(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. Perhaps the GtkTextIters should get more specific names to clarify their scope, e.g. line_start and line_end - which would make line_text a good name for the string. But I don't want to take this too far into endless naming philosophy. :)
Review of attachment 348738 [details] [review]: ::: gtksourceview/gtksourceview.c @@ +2128,3 @@ GtkTextIter newplace; + GtkTextIter start; + GtkTextIter end; See ponderings in above comment. @@ +2129,3 @@ + GtkTextIter start; + GtkTextIter end; + gchar *rtl_specimen; Probably plain char should be preferred in new code. The g* typedefs for primitive types long ago outlived any tenuous usefulness they might've once had.
D'oh, I just realised we are on the wrong thread. Please can you post your next version to the bug# for GtkSourceView and keep discussion there?
(last OT comment!) (In reply to Nelson Benitez from comment #25) > I agree with Daniel, this looks slimmer than what was added to gtktextview.c I have pushed a patch for GtkTextView that adapts Mehdi's use of GtkTextIter. Thanks! The only thing I changed was to use gtk_text_iter_get_visible_text (start, end) instead of gtk_text_buffer_get_text (priv->buffer, start, end, FALSE). I could not see any reason to involve GtkTextBuffer, and this way works just as well. So unless I missed something, maybe the next version of the patch for GtkSourceView can use that too... on the right thread :)
Comment on attachment 348738 [details] [review] Use current line for RTL direction analysis (mem leak fixed) Updated at correct Bug 778928
Do you think this bug is still required? Your method of getting the direction from any GtkTextIter is short and works well, so maybe there is no need to add new API that would require maintenance just to save a couple of lines. :)
(In reply to Daniel Boles from comment #35) > Do you think this bug is still required? Your method of getting the > direction from any GtkTextIter is short and works well, so maybe there is no > need to add new API that would require maintenance just to save a couple of > lines. :) Since we found a way to fix gtksourceview without this API, I think at the moment it is unnecessary, unless future circumstances prove otherwise. BTW, I can't wait to see the other patch in bug 778928 accepted and get the update on my archlinux eventually! :)
(In reply to Mehdi Sadeghi from comment #36) > Since we found a way to fix gtksourceview without this API, I think at the > moment it is unnecessary, unless future circumstances prove otherwise. I agree, now that you (and, slightly, I) figured out the other solution. So I'll close this for the moment. Thanks!
Cool, thanks for working this out!