GNOME Bugzilla – Bug 610753
Fix mixed-direction text in header widget (includes patch)
Last modified: 2010-03-07 07:26:29 UTC
Created attachment 154464 [details] [review] Patch to fix mixed-direction text display This patch attempts to do two things: 1. Solve subtle directionality problems, like this: https://bugzilla.gnome.org/show_bug.cgi?id=508796#c6 2. Change the way for displaying tags which have non-native direction (like English tags in an Arabic locale). It's easiest to look at the attached screenshot. "Current" is now. "Fixed" is like Current but correct. "Proposed" is how I think it should look. The patch gives Proposed, and can give Fixed by changing one line of code. Discussion follows. 1. Directionality problems The code that builds the header starts with 5 distinct textual units: "TITLE by ARTIST from ALBUM". Each unit may have its own direction. Then it strings all of them together into a single string, and the Unicode parser has to reconstruct the 5 separate units and give each a direction, which it can't do perfectly. The Right Way (TM) to do this in Unicode is to insert special Unicode markup characters that tell the parser where each distinct unit starts and ends and its direction. Once this is done, all the problems go away, as can be seen in the screenshot. 2. Proposal for change I think the "Proposed" versions in the screenshot are much more readable and aesthetic. I used a small dot between the tags which looks good to me, but any neutral characters will do. While I believe most people would prefer Proposed, I can see why some people might prefer Fixed, so maybe it should be a configurable option. I included a mixed-direction example in the screenshot (one before last, album is in Arabic). Since the tags don't have a common direction, the reasonable behaviour is to stick to native locale order. I marked the logic for Proposed with "TODO" so it's easy to spot. It's that single "if" statement. I'll fix the comment in the final patch. If the contents of this patch make sense, I suggest that I move the Unicode code to a new file (under widgets/ ?).
Created attachment 154465 [details] Screenshot with examples
I'm not doing a full review of this yet, just adding a few notes. Things like bidi_cat and common_direction (along with the unicode marker definitions) probably belong in a new file, lib/rb-text-helpers.c common_direction and bidi_cat don't work like most other vararg string functions where you pass in NULL to indicate the end of the arguments, so they look at bit odd. Maybe they'd work better if you passed in a GList instead, which the caller would selectively build up from the components it wanted to display? Another alternative would be to ensure they treat the empty string the same as they currently do NULL. Then the caller would have to ensure all strings were non-NULL, then they could use NULL as a sentinel (like most other string functions) rather than having to pass in a count.
Created attachment 154987 [details] [review] Patch v2 Moved text functions to lib/rb-text-helpers, changed API to accept NULL terminated argument lists.
Review of attachment 154987 [details] [review]: This generally looks good. Thanks especially for adding the API documentation. ::: widgets/rb-header.c @@ +434,3 @@ + tags_dir = rb_text_common_direction (title, artist, album, stream, NULL); + + /* TODO: proposed behaviour */ could you explain this in a bit more detail? ::: lib/rb-text-helpers.c @@ +166,3 @@ + * Return value: a new #GString containing the result. + */ +GString * Is there a particular reason it returns a GString, rather than just a char *? It's a bit unusual. @@ +179,3 @@ + + result = g_string_sized_new (100); + if (!result) return NULL; g_string_sized_new can never fail (it aborts if it can't allocate memory), so there's no need for this check @@ +202,3 @@ + + if (!first) { + g_string_append (result, " "); This could be a bit simpler: if (result->len > 0) { g_string_append (result, " "); }
(In reply to comment #4) Some quick answers, fixed patch will come later. > + tags_dir = rb_text_common_direction (title, artist, album, stream, NULL); > > could you explain this in a bit more detail? The idea is to display the track details in their natural direction, so: 1. If the natural direction is same as the user's locale, use verbal separators: "TITLE by ARTIST from ALBUM". 2. If the natural direction is opposite to the user's locale, use neutral separators: "TITLE - ARTIST - ALBUM" or "MUBLA - TSITRA - ELTIT" (I like a middle dot as separator). This has two benefits: the tags are displayed in correct reading order, and there is no mixture of scripts. I'll give an example in a minute. 3. If the tags have mixed direction, treat as option 1 (not sure about that, maybe 2 would still look better, but there is no defined "reading order"). This is actually quite rare so the solution for this is not that important. Let's take the first example in the screenshot ("What's up"), comparing the middle and right column. The existing behaviour (middle) has two problems: 1. The song's name is read left-to-right but the artist and album are placed to its left. It's very hard to read, you have to carefully figure out where each English part starts and ends. 2. The mixture of Latin and Arabic scripts looks awkward. It's OK to have a right-to-left sentence written in Arabic with an embedded English name, but here almost all the text is English besides two words, but the logical reading direction is still right-to-left. This is confusing and unsightly. The right column puts the details in reading order and without mixing scripts. It's much easier to read and looks better. Second example is exact same case as first. Third is same thing with directions reversed - Hebrew tags in an English locale. > + * Return value: a new #GString containing the result. > > Is there a particular reason it returns a GString, rather than just a char *? > It's a bit unusual. I guessed in GLib-land it's the norm to return GString, like in C++ you'd return std::string. I don't have any preference, I'll change it to char *.
(In reply to comment #5) > (In reply to comment #4) > > Some quick answers, fixed patch will come later. > > > + tags_dir = rb_text_common_direction (title, artist, album, stream, NULL); > > > > could you explain this in a bit more detail? Sorry, I actually meant to ask for the comment "TODO: proposed behaviour" to be explained a little, as it probably only makes sense to you.
> Sorry, I actually meant to ask for the comment "TODO: proposed behaviour" to be > explained a little, as it probably only makes sense to you. Oh. Right. :) It's actually a TODO for you, to decide if you accept this change in display. It's there just to mark the place for you, as I mentioned at the end of the bug's description. There's nothing missing in the code itself, if you're fine with the "proposed behaviour" the TODO comment should just go.
Created attachment 155071 [details] [review] Patch v3 Fixed points mentioned in comment 4.
Pushed as commit 05050be. Is there anything more to do here, or can we close this bug now?
(In reply to comment #9) > Pushed as commit 05050be. Thanks. > Is there anything more to do here, or can we close this bug now? That's it. You can also close bug 508796.
OK. Thanks again for working on this.
*** Bug 508796 has been marked as a duplicate of this bug. ***