GNOME Bugzilla – Bug 705779
The text in the search-entry isn't aligned to right in RTL text
Last modified: 2014-03-04 12:18:40 UTC
Created attachment 251280 [details] Screenshot - English ('Type to search...') The text in the search-entry isn't aligned to right in RTL text: Text LTR should to aligned to Left, and text RTL (e.g. Hebrew string) should to aligned to Right. Text RTL * isn't * aligned to right in RTL text. See screenshots.
Created attachment 251281 [details] Screenshot - Hebrew 'Type to search...' in Hebrew is 'יש להקליד כדי לחפש...', and this need to be alignment to right
I guess the icon should go to the other side as well ?
(In reply to comment #2) > I guess the icon should go to the other side as well ? Yes, similar to GtkSearchEntry.
Created attachment 253194 [details] [review] Make the search entry behave better in RTL locales It is expected that the primary and secondary icons in entries change places in RTL locales. When doing so, the edit-clear icon must be replaced by an rtl variant too. http://bugzilla.gnome.org/show_bug.cgi?id=705779
This is not quite sufficient - it looks to me as if ClutterText is not setting the base direction of the PangoLayout properly. Ie the equivalent of this code snipplet: if (gtk_entry_get_display_mode (entry) == DISPLAY_NORMAL) pango_dir = pango_find_base_dir (display, n_bytes); else pango_dir = PANGO_DIRECTION_NEUTRAL; if (pango_dir == PANGO_DIRECTION_NEUTRAL) { if (gtk_widget_has_focus (widget)) { GdkDisplay *display = gtk_widget_get_display (widget); GdkKeymap *keymap = gdk_keymap_get_for_display (display); if (gdk_keymap_get_direction (keymap) == PANGO_DIRECTION_RTL) pango_dir = PANGO_DIRECTION_RTL; else pango_dir = PANGO_DIRECTION_LTR; } else { if (gtk_widget_get_direction (widget) == GTK_TEXT_DIR_RTL) pango_dir = PANGO_DIRECTION_RTL; else pango_dir = PANGO_DIRECTION_LTR; } } pango_context_set_base_dir (gtk_widget_get_pango_context (widget), pango_dir);
ping ? some review or reaction would be nice here.
Created attachment 254424 [details] Screencast - GNOME Shell with the patch
Review of attachment 253194 [details] [review]: ::: js/ui/viewSelector.js @@ +86,3 @@ this._entry.set_primary_icon(new St.Icon({ style_class: 'search-entry-icon', icon_name: 'edit-find-symbolic' })); + if (this._entry.get_text_direction() == ClutterTextDirection.RTL) { Style nit: no braces ::: src/st/st-entry.c @@ +442,3 @@ { + clutter_actor_get_preferred_width (left_icon, -1, NULL, &icon_w); + clutter_actor_get_preferred_height (left_icon, -1, NULL, &icon_h); As you are touching this anyway, maybe use clutter_actor_get_preferred_size() now?
(In reply to comment #7) > Created an attachment (id=254424) [details] > Screencast - GNOME Shell with the patch Note: gnome-shell freez in the end of the video. I recorded from other tty...
Review of attachment 253194 [details] [review]: Sorry I didn't catch this before. Also the text itself should be right-aligned as well (priv->entry in allocate) ::: js/ui/viewSelector.js @@ +86,3 @@ this._entry.set_primary_icon(new St.Icon({ style_class: 'search-entry-icon', icon_name: 'edit-find-symbolic' })); + if (this._entry.get_text_direction() == ClutterTextDirection.RTL) { It's Clutter.TextDirection, not ClutterTextDirection
> Also the text itself should be right-aligned as well (priv->entry in allocate) That is what I was talking about in comment 5: set the base direction of the PangoLayout. I fear that has to be done inside clutter. I would appreciate if you could try to finish this up, I'm not that much of shell / clutter developer, I merely wanted to push this along...
Created attachment 254624 [details] [review] Make the search entry behave better in RTL locales It is expected that the primary and secondary icons in entries change places in RTL locales. When doing so, the edit-clear icon must be replaced by an rtl variant too. http://bugzilla.gnome.org/show_bug.cgi?id=705779
Created attachment 254625 [details] [review] st: Fix spacing on right icon in entry
Created attachment 254626 [details] [review] text: Consider text direction when computing layout offsets Currently this is only the case when the actor's x-expand/x-align flags have been set and clutter_text_compute_layout_offsets() is used.
Yosef, could you give these patches a try ?
Created attachment 254725 [details] Screenshot - it very good
Review of attachment 254625 [details] [review]: OK.
Review of attachment 254624 [details] [review]: OK.
Comment on attachment 254624 [details] [review] Make the search entry behave better in RTL locales Attachment 254624 [details] pushed as 1b6090f - Make the search entry behave better in RTL locales
Comment on attachment 254625 [details] [review] st: Fix spacing on right icon in entry Attachment 254625 [details] pushed as 954d262 - st: Fix spacing on right icon in entry
I see now more problem, in the search entry with the patches. Just string which start in RTL character should to be align to right (e.g. string start with word in Hebrew), string which start with LTR character (e.g. string start with word in English), should to be align to left. Now also string start with LTR character is align to right, and is wrong.
Created attachment 254729 [details] Screenshot - RTL character
Created attachment 254730 [details] Screenshot - LTR character
(In reply to comment #21) > Now also string start with LTR character is align to right, and is wrong. Yes, it's based on the base direction, not the direction of the content. I don't see any API in Pango to change this in a non-intrusive way, so this is likely out of scope for 3.10; in any case, the remaining problem is a Clutter issue, so reassigning.
Review of attachment 254626 [details] [review]: looks okay to me.
It doesn't address comment #21 in any way though ...
(In reply to comment #26) > It doesn't address comment #21 in any way though ... I know, but half-right is better than wholly wrong.
(In reply to comment #27) > (In reply to comment #26) > > It doesn't address comment #21 in any way though ... > > I know, but half-right is better than wholly wrong. I agree. Now, for example, in 'Type command' (Alt+F2) the command is always align to right, and it very bad. I think need to look how it implemented in GtkEntry or in GtkTextView.
Comment on attachment 254626 [details] [review] text: Consider text direction when computing layout offsets Attachment 254626 [details] pushed as 986e46d - text: Consider text direction when computi ng layout offsets Yeah. Leaving bug open for the missing bits ...
Note: before the patches (exactly - the patch in clutter), the text is was always align to left, so I think not need to reverse him :-)
moving off the blocker list
Created attachment 260039 [details] [review] text: Consider text direction according to the content of the text
Review of attachment 260039 [details] [review]: I don't think this is enough. GtkEntry checks the mode of the entry, i.e. whether the entry is in password or in normal mode, to check if the PangoDirection should be neutral. in case the direction is neutral, it also uses whether the entry has focus, and the direction of the keymap. all of this is done when creating the PangoLayout, not when drawing it.
I try to set the direction in clutter_text_create_layout_no_cache() like you sad in IRC (some times ago), and this look like there isn't any affect on the real direction of the text. This look like just what I do in clutter_text_paint() is affect on the direction of the text. Any idea? I really want to see this fix for the next stable version, and also if this not complete, like my patch, but if it better than what we have now.
(In reply to comment #34) > I try to set the direction in clutter_text_create_layout_no_cache() > like you sad in IRC (some times ago), and this look like there isn't > any affect on the real direction of the text. > > This look like just what I do in clutter_text_paint() is affect on the > direction of the text. do you have a patch that I can test or review? otherwise I'll have to write my own. > Any idea? > I really want to see this fix for the next stable version, and also > if this not complete, like my patch, but if it better than what we > have now. I'd rather have a working and complete approach. we still have plenty of time before the stable release.
(In reply to comment #35) > (In reply to comment #34) > > I try to set the direction in clutter_text_create_layout_no_cache() > > like you sad in IRC (some times ago), and this look like there isn't > > any affect on the real direction of the text. > > > > This look like just what I do in clutter_text_paint() is affect on the > > direction of the text. > > do you have a patch that I can test or review? otherwise I'll have to write my > own. Now already not, what I had deleted after updated clutter from git (I never remember to hack in a different folder than the one I build for my system, this not the first code I lose :-(). > > > Any idea? > > I really want to see this fix for the next stable version, and also > > if this not complete, like my patch, but if it better than what we > > have now. > > I'd rather have a working and complete approach. we still have plenty of time > before the stable release. Me too. What I tried myself to solve all the cases I think.
Created attachment 270826 [details] [review] text: Discover the direction of the contents We should set the direction on the PangoContext when creating a PangoLayout based on a best effort between the contents of the text itself and the text direction of the widget, in case that fails.
Created attachment 270838 [details] [review] text: Use the resolved text direction Now that we compute the effective text direction when creating the Pango layout, we should also use it when painting it.
Created attachment 270839 [details] test-text-field with CLUTTER_TEXT_DIRECTION=rtl result after applying both attachment 270826 [details] [review] and attachment 270838 [details] [review]
Created attachment 270852 [details] [review] backend: Add private accessor for the keymap direction We need to ask the backend (wherever possible) for the direction of the current keymap.
Created attachment 270853 [details] [review] x11: Add keymap direction query We should use the Xkb API to query the direction of the key map, depending on the group. To get a valid result we need to go over the Unicode equivalents of the key symbols for each group, so we should cache the result. The code used to query and cache the key map direction is taken from GDK.
Created attachment 270854 [details] [review] text: Use the keymap direction when focused If the ClutterText actor has key focus then we should ask for the direction of the key map, instead of the direction of the actor.
pushed and released as part of Clutter 1.17.6; feel free to re-open if there are further issues. before 1.18 I'll make the same changes to the GDK and evdev/libinput backends.
Thanks Emmanuele! I see just more one thing: When there isn't text and the entry is focused, the position of the cursor is always on the left size, also if the keyword layout is Hebrew (which need to show the cursor on the right side). But generally it amazing, thanks!
we do bail out early if there's no text; it'll need another fix so we align the cursor with the right text direction. I'll open a new bug, this one has become fairly unwieldy.
filed bug 725650 - will work on it ASAP.