GNOME Bugzilla – Bug 702631
Set button arrow icons according to locale's text direction
Last modified: 2013-09-18 12:58:35 UTC
Created attachment 247247 [details] [review] Set button arrow icons according to locale's text direction See patch.
Created attachment 247255 [details] [review] Set button arrow icons according to locale's text direction
Depending on how we go with the GtkMainToolbar widget, either we'll replace the code you're modifying with GtkMainToolbar, or I'll commit your patch. See bug 700918.
Review of attachment 247255 [details] [review]: Rest of the patch looks good. I'll commit it if we don't go the GtkMainToolbar way. ::: src/plugins/grilo/totem-grilo.c @@ +1279,3 @@ AtkObject *accessible; GtkAdjustment *adj; + gboolean rtl; indentation.
Created attachment 247310 [details] [review] Set button arrow icons according to locale's text direction
Created attachment 247323 [details] [review] Set button arrow icons according to locale's text direction It fixes all the problems with RTL.
(In reply to comment #5) > Created an attachment (id=247323) [details] [review] > Set button arrow icons according to locale's text direction > > It fixes all the problems with RTL. I'd need all the changes to be made in separate patches, as I don't think we'll require the portions related to the toolbars.
(In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=247323) [details] [review] [details] [review] > > Set button arrow icons according to locale's text direction > > > > It fixes all the problems with RTL. > > I'd need all the changes to be made in separate patches, as I don't think we'll > require the portions related to the toolbars. Why not put all one patch?
Because it makes it impossible to bisect for example, and that patches to plugins shouldn't be mixed with patches to the core program. This isn't CVS where committing a patch needs to connect to a super-slow network. Patches should be as small as they can be whilst staying meaningful.
Created attachment 247447 [details] [review] main: Set button arrow icons according to locale's text direction
Created attachment 247449 [details] [review] browser-plugin: Set button arrow icons according to locale's text direction
Created attachment 247450 [details] [review] backend: Set button arrow icons according to locale's text direction
Created attachment 247451 [details] [review] totem-grilo: Set button arrow icons according to locale's text direction
You push the patches?
I pushed the patches.
Why do you think you can push patches without review? Your commits conflict with work that I'm doing in Totem and GTK+.
I'm sorry. No one responded to my patches. I thought it was fine. Usually, when there is a problem, someone says it. mark 'Should work'. Again, I'm sorry.
Is there a problem with the patches?
Why 3.12? and why you created branch gnome-3-10?
Read desktop-devel-list
Created attachment 250339 [details] Screenshot - before
Created attachment 250340 [details] Screenshot - after
I read. Why not to commit the patches? the patches them already exits.
Review of attachment 247447 [details] [review]: Please use the totem_get_rtl_icon_name() helper I just added to src/gst/totem-rtl-helpers.h instead.
Review of attachment 247449 [details] [review]: Needs to use the helpers as well.
Review of attachment 247450 [details] [review]: I'm not interested in taking a patch for a test app that I'm probably the only one using.
Review of attachment 247451 [details] [review]: Needs to use the helper as well.
Created attachment 250493 [details] [review] main: Set button arrow icons according to locale's text direction
Created attachment 250494 [details] [review] browser-plugin: Set button arrow icons according to locale's text direction
Created attachment 250495 [details] [review] totem-grilo: Set button arrow icons according to locale's text direction
Review of attachment 250493 [details] [review]: ::: src/totem-object.c @@ +65,3 @@ #include "totem-preferences.h" #include "totem-session.h" +#include "gst/totem-rtl-helpers.h" Remove the "gst/" prefix, it's in the includes files search path. ::: src/totem-playlist.c @@ +37,3 @@ #include "totem-interface.h" #include "video-utils.h" +#include "gst/totem-rtl-helpers.h" Ditto ::: src/totem.c @@ +51,3 @@ #include "totem-session.h" #include "video-utils.h" +#include "gst/totem-rtl-helpers.h" Remove gst/ prefix again. @@ +77,3 @@ GtkSettings *gtk_settings; char *sidebar_pageid; + GtkAction *play, *next_chapter, *previous_chapter; Add a single "*action" variable. @@ +93,3 @@ totem_object_action_exit (NULL); + play = GTK_ACTION (gtk_builder_get_object (totem->xml, "play")); action = GTK_ACTION(...); gtk_action_set_icon_name (action, totem_get_rtl_action_name (...));
Review of attachment 250494 [details] [review]: Looks good.
Review of attachment 250495 [details] [review]: Looks good.
Created attachment 250545 [details] [review] main: Set button arrow icons according to locale's text direction
Review of attachment 250545 [details] [review]: looks good
Review of attachment 250545 [details] [review]: pushed as 804d8f77d54e5cfe171dd42486e143060bd8df3f - Set button arrow icons according to locale's text direction
Review of attachment 250494 [details] [review]: pushed as db8024fa28edfa2b21c9bfd157fc1751941e1ccf - browser-plugin: Set button arrow icons according to locale's text direction
Review of attachment 250495 [details] [review]: pushed as 55d7d204544d82933f604f8dafcf1b26a9af75a8 - totem-grilo: Set button arrow icons according to locale's text direction
Created attachment 250568 [details] [review] main: Set button arrow icons according to locale's text direction The patch for gnome-3-8 and branch gnome-3-10 branch. btw, need patch for build ("fatal error: totem-rtl-helpers.h: No such file or directory").
Created attachment 250569 [details] [review] browser-plugin: Set button arrow icons according to locale's text direction The patch for gnome-3-8 and branch gnome-3-10 branch.
Not need patch for totem-grilo for gnome-3-8 and branch gnome-3-10 branch.
Review of attachment 250568 [details] [review]: Rest looks fine. ::: src/totem.c @@ +94,3 @@ + action = GTK_ACTION (gtk_builder_get_object (totem->xml, "play")); + gtk_action_set_icon_name (action, totem_get_rtl_icon_name ("media-playback-start")); You're doing 5 times the same thing (getting a widget, setting an icon), please split this into a function.
Review of attachment 250569 [details] [review]: Looks good.
Created attachment 255164 [details] [review] browser-plugin: Set button arrow icons according to locale's text direction media-playback-start-symbolic in LTR, media-playback-start-rtl-symbolic in RTL.
Created attachment 255165 [details] [review] main: Set button arrow icons according to locale's text direction media-playback-start-symbolic in LTR, media-playback-start-rtl-symbolic in RTL. media-seek-forward-symbolic in LTR, media-seek-forward-rtl-symbolic in RTL. media-seek-backward-symbolic in LTR, media-seek-backward-rtl-symbolic in RTL. media-skip-forward-symbolic in LTR, media-skip-forward-rtl-symbolic in RTL. media-skip-backward-symbolic in LTR, media-skip-backward-rtl-symbolic in RTL.
Created attachment 255166 [details] [review] browser-plugin: Set button arrow icons according to locale's text direction media-playback-start-symbolic in LTR, media-playback-start-rtl-symbolic in RTL.
Created attachment 255167 [details] [review] browser-plugin: Don't disable deprecated functions We still rely on the stock API.
Created attachment 255168 [details] [review] main: Add helper function to set RTL icons
Created attachment 255169 [details] [review] main: Add a few missing header include paths https://bugzilla.gnome.org/show_bug.cgi?id=702631 Conflicts: src/Makefile.am
These all look safe to me.
All pushed to gnome-3-10 as appropriate, freeze break requests approved by walters and fredp. Attachment 255165 [details] pushed as 3ff23cf - main: Set button arrow icons according to locale's text direction Attachment 255166 [details] pushed as fe628f8 - browser-plugin: Set button arrow icons according to locale's text direction Attachment 255167 [details] pushed as 3cc4d3a - browser-plugin: Don't disable deprecated functions Attachment 255168 [details] pushed as 185c7aa - main: Add helper function to set RTL icons Attachment 255169 [details] pushed as dd86c29 - main: Add a few missing header include paths