GNOME Bugzilla – Bug 609767
RTL tags in status icon bubble (includes patch)
Last modified: 2010-02-14 10:25:44 UTC
Created attachment 153647 [details] [review] Patch to achieve proposed behaviour Please refer to the screenshot that shows both the problem and the proposed solution. When the track's tags have a different direction than the locale's versions of "by" and "from", the notification bubble is unsightly: 1. It uses different justification for the title and other data; 2. It switches text directions and character sets mid-sentence; 3. Neutral characters like numbers and punctuation might appear at the wrong position. All these problems are seen in the attached screenshot. Attached is a possible solution. It identifies when the directions of the tags and the locale's "by" and "from" are different, and if so drops "by" and replaces "from" with a neutral "/" (see screenshot). If the directions are the same, like Russian tags in an English locale or Hebrew tags in an Arabic locale, the behaviour is not changed from the current one.
Created attachment 153648 [details] Screenshot showing current and proposed displays
A similar solution might be appropriate for bug 518540 and bug 508796, that both stem from tags with a different direction than the locale.
Review of attachment 153647 [details] [review]: Thanks for looking into this - I don't know much about how to deal with RTL text, so attention here is always welcome. There are a few code style nits, and since this is an area I don't think many people are familiar with, I'd like there to be comments explaining what's going on, but otherwise I think this looks good. ::: plugins/status-icon/rb-status-icon-plugin.c @@ +647,3 @@ static void +get_artist_album_templates( + const char *artist, const char *album, please follow the formatting of the surrounding code. This should be: static void get_artist_album_templates (const char *artist, const char *album, @@ +648,3 @@ +get_artist_album_templates( + const char *artist, const char *album, + char **artist_template, char **album_template) artist_template and album_template should be const, since the caller can't free the returned strings @@ +660,3 @@ + if (artist != NULL && artist[0] != '\0') { + tag_dir = pango_find_base_dir(artist, -1); + template_dir = pango_find_base_dir(*artist_template, -1); missing spaces between function names and arguments @@ +670,3 @@ + if ( + ((tag_dir == PANGO_DIRECTION_LTR) && (template_dir == PANGO_DIRECTION_RTL)) || + ((tag_dir == PANGO_DIRECTION_RTL) && (template_dir == PANGO_DIRECTION_LTR))) { the condition should start on the same line as 'if ('.. @@ +672,3 @@ + ((tag_dir == PANGO_DIRECTION_RTL) && (template_dir == PANGO_DIRECTION_LTR))) { + *artist_template = "<i>%s</i>"; + *album_template = "/ <i>%s</i>"; Should the album template be marked for translation? I'm not sure how this could differ between languages, but maybe it does. @@ +723,3 @@ } + get_artist_album_templates(artist, album, &artist_template, &album_template); space between function and arguments again
Created attachment 153724 [details] [review] Patch to achieve proposed behaviour (take 2) Fixed coding style.
(In reply to comment #3) > @@ +672,3 @@ > + ((tag_dir == PANGO_DIRECTION_RTL) && (template_dir == > PANGO_DIRECTION_LTR))) { > + *artist_template = "<i>%s</i>"; > + *album_template = "/ <i>%s</i>"; > > Should the album template be marked for translation? I'm not sure how this > could differ between languages, but maybe it does. No, it shouldn't be translated, because it is used specifically when we can't use the current locale's values. Example: say the user has an Italian locale and plays tracks with Hebrew tags. We don't want to use the Italian "da %s" because da is LTR and the tags are RTL. You can see the result in the screenshot. The two things that make sense are: 1. Using a Hebrew template; 2. Using a totally neutral template, like "/ %s". Doing (1) means detecting the language of the tags which is certainly an overkill. Doing (2) is easy, and there's no need for translation, as there is no "Italian way" for separating Hebrew words.
The question is more whether there's an Italian way of separating somewhat related strings. I don't think we can really settle it, so let's leave it alone. pushed as e478d49.
(In reply to comment #6) > The question is more whether there's an Italian way of separating somewhat > related strings. I don't think we can really settle it, so let's leave it > alone. I see what you mean, and I agree it's not for us to decide. > pushed as e478d49. Thanks for spending time on this.