GNOME Bugzilla – Bug 779099
TextLayout: weird and probably wrong 'special case' in get_line_display()
Last modified: 2018-02-19 10:01:03 UTC
https://git.gnome.org/browse/gtk+/tree/gtk/gtktextlayout.c#n2282 If the line is invisible, this exits early after setting the layout based on display->direction. But the latter is not set yet, so because the LineDisplay is new0’d, the direction is always GTK_TEXT_DIR_NONE. This hits the else case and means that we always create an LTR layout. display->direction is only assigned to by the function set_para_values(). What is the real fix? (A) move this to under the final call to set_para_values(), so the right direction is available and right layout created? (B) delete the futile check and just always return the LTR layout, so the same behaviour as current is kept, without weird code?
Setting the layout, and hence the read of the not-yet-set display->direction, was added in commit 86e4f7d1f305574a1d7e412efc56db8d02f23eb2
Moving it down would defeat the purpose - it is meant to be an early exist, avoiding all the extra work. The point being that with invisible lines, you can fit an infinite amount of text onto the screen...
OK, so is it OK to always return an LTR layout, but patched to do it explicitly, rather than using code that does it 'accidentally'? or are there both (A) an important reason to return a contextual direction of layout for invisible lines and (B) a quicker/earlier way to determine what that direction should be? put simply: what, if any, are the drawbacks of not returning an RTL layout for an RTL-but-invisible line? this is probably a simplistic question, but I know next to nothing about Pango and the GtkText* classes, and even less so about invisible lines therein
That change was written 12 years ago... I am hard pressed to remember anything about patches that are 2 years old. Given that the change has been in place for 12 years, I think it is safe to say that whatever the side-effect, it can't be terribly serious, since nobody has complained about it.
Created attachment 346996 [details] [review] TextLayout: Clarify implementation of special case TextLayout: Clarify implementation of special case This exists to exit early for invisible lines. It attempts to use the LineDisplay’s direction to create a corresponding PangoLayout. However, the dir is not yet set by this point, & the display was new0()d, so its dir is always 0 == TEXT_DIR_NONE. Thus, we always create an LTR layout. Whatever the original intent, this outcome seems to be OK, so let’s make the code say what it means, rather than using a misleading conditional.
Review of attachment 346996 [details] [review]: ok, lets do this
Thanks for pushing to master. Shall we cherry-pick to 3.22 for the sake of keeping them similar?