After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753359 - Don't handle RTL icons manually for GTK+ >= 3.12
Don't handle RTL icons manually for GTK+ >= 3.12
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: User Interface
unspecified
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-07 15:43 UTC by Mario Sánchez Prada
Modified: 2015-09-06 10:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch proposal (3.86 KB, patch)
2015-08-07 15:48 UTC, Mario Sánchez Prada
none Details | Review
Patch proposal (4.33 KB, patch)
2015-08-07 15:50 UTC, Mario Sánchez Prada
none Details | Review
Patch proposal (bumping GTK+ version up to 3.12) (4.17 KB, patch)
2015-08-14 16:47 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review

Description Mario Sánchez Prada 2015-08-07 15:43:50 UTC
DESCRIPTION:

In a similar fashion to what it has been done for other components after GTK+ 3.12 (see [1] for what gnome-music did), we need to update the code not to explicitly request icons named like <icon name>-rtl-symbolic.svg if GTK+ >= 3.12 is being used, because since that version GTK+ handles that automatically.

Also, GTK+ >= 3.12 expects icon files to be named like <icon name>-symbolic-rtl.svg ('-rtl' after 'symbolic, not before), meaning that without fixing this Rhythmbox will look broken in RTL languages, as it won't be able to find the right icon anymore if something like 'media-playback-start-rtl-symbolic', for instance, is explicitly requested.

STEPS TO REPRODUCE:

  1. With a version of rhythmbox built against GTK >= 3.12, launch it
     using an RTL language (e.g. LANG=ar_AE.utf-8 rhythmbox)
  
  2. Look at the playback icons at the top right side of the main window


EXPECTED OUTCOME:

Playback icons should look fine both in LTR and RTL languages


ACTUAL OUTCOME:

Playback icons are not rendered due to rhythmbox trying to load the wrong files.


[1] https://bugzilla.gnome.org/show_bug.cgi?id=735890
Comment 1 Mario Sánchez Prada 2015-08-07 15:48:00 UTC
Created attachment 308906 [details] [review]
Patch proposal

This patch fixes the problem without bumping a newer required version for GTK+, so that it works as expected both for old and new versions.

Please review, thanks!
Comment 2 Mario Sánchez Prada 2015-08-07 15:50:00 UTC
Created attachment 308907 [details] [review]
Patch proposal
Comment 3 Mario Sánchez Prada 2015-08-11 16:49:32 UTC
Ping?
Comment 4 Jonathan Matthew 2015-08-13 10:02:28 UTC
I'm not aware of any strong reason not to bump the gtk+ requirement to 3.12, so how about we do that and drop the rtl icon handling entirely?
Comment 5 Mario Sánchez Prada 2015-08-14 16:25:09 UTC
I'd be perfectly fine with that too, just considered that in the current patch because I personally don't know whether rhythmbox would want to bump the version of GTK+ dependency to 3.12 or not, so I played safe.

But I can certainly propose a different patch, which will be much smaller, assuming that we have 3.12 (and bumping that version too). I will prepare it so that it will be a matter of pick one or the other.
Comment 6 Mario Sánchez Prada 2015-08-14 16:47:42 UTC
Created attachment 309290 [details] [review]
Patch proposal (bumping GTK+ version up to 3.12)

Here you have the alternative version, without the code needed to GTK+ versions previous to 3.12.
Comment 7 Mario Sánchez Prada 2015-09-01 08:27:15 UTC
Ping?
Comment 8 Jonathan Matthew 2015-09-05 13:16:56 UTC
Review of attachment 309290 [details] [review]:

still can't see a reason not to require gtk 3.12, so let's do it this way.
Comment 9 Mario Sánchez Prada 2015-09-06 10:56:32 UTC
Thanks for the feedback, I've already applied it upstream:
https://git.gnome.org/browse/rhythmbox/commit/?id=bb170987b1c6169d650b38041628e9364554e0a1