GNOME Bugzilla – Bug 584662
RTL locales
Last modified: 2011-03-22 18:39:51 UTC
How should gnome-shell look in RTL locales? Assuming we want the standard "flip everything horizontally", we're going to need to patch BigBox to support that, and fix up a lot of other manual actor placement, eg in the overlay.
Ok, I have something (mostly) working locally (by hardcoding RTL), it seems it's not very difficult, I have however some questions : about patching BigBox, should the "flip everything horizontally for rtl locale" logic be in it, or just an inverted horizontal (and vertical) orientation laying components right-to-left (and bottom-to-top). Although it would be less work to do, I don't think that BigBox should be locale-aware (let alone locale-dependent). about manual placement, the only problem remaining is the _paneContainer, as it doesn't have a fixed width, and connecting to notify::width doesn't seem to work. (I've added a function to calculate the x coordinate based on (x, width))
(In reply to comment #1) > about patching BigBox, should the "flip everything horizontally for rtl locale" > logic be in it, or just an inverted horizontal (and vertical) orientation > laying components right-to-left (and bottom-to-top). Although it would be less > work to do, I don't think that BigBox should be locale-aware (let alone > locale-dependent). The way this works in Gtk is that a widget can call gtk_widget_get_direction() on itself, which returns GTK_TEXT_DIR_LTR or GTK_TEXT_DIR_RTL (based on the locale), and then the widget can adjust the way it lays things out accordingly. We probably want to do something similar. > about manual placement, the only problem remaining is the _paneContainer, as it > doesn't have a fixed width, and connecting to notify::width doesn't seem to > work. > (I've added a function to calculate the x coordinate based on (x, width)) Well, a lot of the existing non-StWidget-based layout will eventually be redone using StBoxLayout, StTable, etc. (eg, bug 598463). So I'm not sure how much time we should spend thinking about that stuff now. Adding something like gtk_widget_get_direction() to StWidget, and making StBoxLayout and StTable use it would definitely be useful though.
(In reply to comment #2) > Well, a lot of the existing non-StWidget-based layout will eventually be redone > using StBoxLayout, StTable, etc. (eg, bug 598463). So In'm not sure how much > time we should spend thinking about that stuff now. > > Adding something like gtk_widget_get_direction() to StWidget, and making > StBoxLayout and StTable use it would definitely be useful though. Done :-) well, mostly, what's missing is the logic for get_direction, gtk has a per-widget direction (set_direction/get_direction) and a global direction (set_default_direction/get_default_direction) for widgets that don't have a direction assigned. It is set to either RTL or LTR on startup based on a gettext translation. so the questions are: * should st have as much flexibility as gtk or something like get_default_direction is enough? (if it's specific to gnome-shell, I guess the latter) * how to determine if we want RTL? st doesn't use gettext for now, and hardcoding ar, fa and he will get bug reports as soon as shell is widely used. the solution I'm leaning towards now is to have a set_direction/get_direction and using the same translation trick as gtk but in the shell (and call set_direction from there). thoughts? (and btw, is anyone working on replacing the manual placement in the shell with layout based on StTable, or should I try to tackle it once this problem is sorted out?)
(In reply to comment #3) > * should st have as much flexibility as gtk or something like > get_default_direction is enough? Well... hm... I think looking glass (the debugging console) would want to have LTR directionality even in an RTL locale, since you use it to type javascript, which would be (at least mostly) ASCII. So I guess we have a use case for per-widget directionality override. > * how to determine if we want RTL? st doesn't use gettext for now, and > hardcoding ar, fa and he will get bug reports as soon as shell is widely used. > the solution I'm leaning towards now is to have a set_direction/get_direction > and using the same translation trick as gtk but in the shell (and call > set_direction from there). Well, we'll probably need gettext in St at some point, but maybe not for a while, so that sounds good. > (and btw, is anyone working on replacing the manual placement in the shell with > layout based on StTable, or should I try to tackle it once this problem is > sorted out?) It's being done in bits and pieces. If you want to tackle a piece, first make sure there isn't a bug open about it already, and then file a bug of your own to "claim" it so we don't duplicate effort.
Note there's a lot of development relating to boxes going on in Clutter 1.2; we may want to consider a rebase to that branch some point relatively soon.
(In reply to comment #5) > Note there's a lot of development relating to boxes going on in Clutter 1.2; we > may want to consider a rebase to that branch some point relatively soon. There's a balancing job here between: - Testing out the new Clutter functionality and makes sure it works for us - Having GNOME Shell versions that can be built for and packaged with current operating systems My feeling right now is that we probably want to wait at least another month before doing such a cut-over.
(In reply to comment #3) > (In reply to comment #2) > > Well, a lot of the existing non-StWidget-based layout will eventually be redone > > using StBoxLayout, StTable, etc. (eg, bug 598463). So In'm not sure how much > > time we should spend thinking about that stuff now. > > > > Adding something like gtk_widget_get_direction() to StWidget, and making > > StBoxLayout and StTable use it would definitely be useful though. > > Done :-) > well, mostly, what's missing is the logic for get_direction, gtk has a > per-widget direction (set_direction/get_direction) and a global direction > (set_default_direction/get_default_direction) for widgets that don't have a > direction assigned. It is set to either RTL or LTR on startup based on a > gettext translation. > > so the questions are: > * should st have as much flexibility as gtk or something like > get_default_direction is enough? (if it's specific to gnome-shell, I guess the > latter) The canonical case where per-widget directions is useful if you have a box with a arrows to move left and move right [ <= ] [ => ]. If you swap the layout in the box it doesn't make any sense. My guess is that some things like this will come up for the shell > * how to determine if we want RTL? st doesn't use gettext for now, and > hardcoding ar, fa and he will get bug reports as soon as shell is widely used. > the solution I'm leaning towards now is to have a set_direction/get_direction > and using the same translation trick as gtk but in the shell (and call > set_direction from there). That sounds fine to me - you can even use the GTK+ translation domain and avoid adding a shell-specific string to translate - see how calendar:MY is handled in calender.js.
(In reply to comment #4) > Well... hm... I think looking glass (the debugging console) would want to have > LTR directionality even in an RTL locale, since you use it to type javascript, > which would be (at least mostly) ASCII. So I guess we have a use case for > per-widget directionality override. right now, I've only converted BoxLayout and Table, and it looks fine in the looking glass (with the "introspect" button to the right). I'm not sure if we want to do the same for Entry. > > > (and btw, is anyone working on replacing the manual placement in the shell with > > layout based on StTable, or should I try to tackle it once this problem is > > sorted out?) > > It's being done in bits and pieces. If you want to tackle a piece, first make > sure there isn't a bug open about it already, and then file a bug of your own > to "claim" it so we don't duplicate effort. I was talking about the overlay (which is the most visible thing). I'll file a bug once I finish this. (In reply to comment #7) > The canonical case where per-widget directions is useful if you have a box with > a arrows to move left and move right [ <= ] [ => ]. If you swap the layout in > the box it doesn't make any sense. My guess is that some things like this will > come up for the shell I guess we can swap the arrows and have them mean previous and next instead (I think this would also reduce special casing in the code). Any other argument? ;-p (I can do it anyway if you think it's useful to have)
(In reply to comment #8) > (In reply to comment #7) > > The canonical case where per-widget directions is useful if you have a box with > > a arrows to move left and move right [ <= ] [ => ]. If you swap the layout in > > the box it doesn't make any sense. My guess is that some things like this will > > come up for the shell > I guess we can swap the arrows and have them mean previous and next instead (I > think this would also reduce special casing in the code). > > Any other argument? ;-p > (I can do it anyway if you think it's useful to have) I'm talking about arrows that mean "move left" and "move right" in this case, not arrows that mean "next" and "previous". The functionality is needed to be complete, we could wait until we hit such a case, but since its only a couple of functions, I'd be favor of just going ahead and adding it now, and making things work just like GTK+ (except for the direction-changed signal, which is a complexity we don't need right now.)
Created attachment 146435 [details] [review] [StWidget] add API support for right-to-left UI
Created attachment 146436 [details] [review] [StBoxLayout] now RTL aware
Created attachment 146437 [details] [review] [StTable] now RTL aware
Created attachment 146438 [details] [review] environment.js: set default direction for St widgets
Comment on attachment 146435 [details] [review] [StWidget] add API support for right-to-left UI >+ g_return_if_fail(dir != ST_DIRECTION_NONE); need a space before the "(". Likewise in many other places in all 4 patches >+typedef enum { >+ ST_DIRECTION_NONE, >+ ST_DIRECTION_LTR, >+ ST_DIRECTION_RTL >+} StDirection; I think I'd prefer "StTextDirection", like with Gtk. You can keep the method names the same though.
Comment on attachment 146436 [details] [review] [StBoxLayout] now RTL aware >+ else if (flip) >+ { >+ child_box.x1 = (int)(0.5 + next_position); >+ child_box.x2 = (int)(0.5 + position); Those two lines are the only difference between the flip and non-flip horizontal cases. It would be better to have just those lines inside the if(flip)/else, and everything else shared between them.
Comment on attachment 146437 [details] [review] [StTable] now RTL aware looks good, other than some space-before-( issues
Comment on attachment 146438 [details] [review] environment.js: set default direction for St widgets likewise
Created attachment 147816 [details] [review] [StWidget] add API support for right-to-left UI
Created attachment 147817 [details] [review] [StBoxLayout] now RTL aware
Created attachment 147818 [details] [review] [StTable] now RTL aware
Created attachment 147819 [details] [review] environment.js: set default direction for St widgets
Looks good. Thanks for the patches. Attachment 147816 [details] pushed as a5edc78 - [StWidget] add API support for right-to-left UI Attachment 147817 [details] pushed as 4e8206d - [StBoxLayout] now RTL aware Attachment 147818 [details] pushed as 3529b8c - [StTable] now RTL aware Attachment 147819 [details] pushed as 9ba5ca0 - environment.js: set default direction for St widgets
Created attachment 150416 [details] [review] calendar.js: switch buttons direction in RTL mode also don't hang if translation of 'calendar:week_start:0' is incorrect
Created attachment 150417 [details] [review] statusmenu.js: fix menu position for RTL locales
Created attachment 150418 [details] [review] overview.js: fix manual placement for RTL locales
Created attachment 150419 [details] [review] NOT WORKING: fix hot corner position for RTL locales
(In reply to comment #26) > Created an attachment (id=150419) [details] [review] > NOT WORKING: fix hot corner position for RTL locales This is where I was able to do, I've left the debugging things: the problem is that here the hot corner is no longer "hot" as soon as it's less than 4 pixels away from the end of screen (so either it's not hot or not a corner)
Reopening as requested on IRC; in general, I'd suggest not extending bugs once they are closed - just open a new one.
*** Bug 605373 has been marked as a duplicate of this bug. ***
Comment on attachment 150416 [details] [review] calendar.js: switch buttons direction in RTL mode Attachment 150416 [details] pushed as 7bb14bd - calendar.js: switch buttons direction in RTL mode
when I run "LANG=ar_SA.UTF8 ./src/gnome-shell", the panel is still in LTR order ("Activities" button on the left, but with the word translated into Arabic), and user menu on the right. Applying the statusmenu and overview patches makes it so that when I click on my username on the right side of the screen, the status menu pops up on the left side, and when I click on "الأنشطة" on the left side of the screen, it opens the overview with the dashboard on the right. Maybe you've got another patch locally that you forgot to attach here?
My fault, the last patch (with a big NOT WORKING warning) needs to be split in two.
Created attachment 151992 [details] [review] panel.js: fix right and left boxes for RTL locales
Created attachment 151994 [details] [review] NOT WORKING: fix hot corner position for RTL locales
(In reply to comment #32) > My fault, the last patch (with a big NOT WORKING warning) needs to be split in > two. oh, i see. Of course, it makes more sense as a single patch once it's working. (In reply to comment #27) > > NOT WORKING: fix hot corner position for RTL locales > > This is where I was able to do, I've left the debugging things: the problem is > that here the hot corner is no longer "hot" as soon as it's less than 4 pixels > away from the end of screen (so either it's not hot or not a corner) It looks like the problem is in the CSS (data/theme/gnome-shell.css): #panelLeft { padding-right: 4px; } #panelRight { padding-left: 4px; } Those end up putting the padding on the wrong side when we flip RTL. (If you change #panelLeft to have padding-left instead, then the hotspot works fine on the corner.) Should the CSS properties flip automatically? Does HTML set any precedent here?
Created attachment 152385 [details] [review] [statusmenu] fix menu position for RTL locales I'm sending the latest version of the patches, everything works ok now. (I still need to fix other things though, e.g. the new workspace view)
Created attachment 152386 [details] [review] [overview] fix manual placement for RTL locales
Created attachment 152387 [details] [review] [panel] fix positions for RTL locales the [leftBox, rightBox] variables could have a better name, but I couldn't find one. leading/trailing ? activities/status ?
Created attachment 153454 [details] [review] [panel] fix app icon fading for RTL locales I'm not sure this is the right way to do it, comments welcome
Review of attachment 152387 [details] [review]: ::: js/ui/panel.js @@ +339,3 @@ + [leftBox, rightBox] = [rightBox, leftBox]; + if (St.Widget.get_default_direction () == St.TextDirection.RTL) { + let [leftBox, rightBox] = [this._leftBox, this._rightBox]; It's confusing to me that this._leftBox and this._rightBox aren't swapped, but we introduce new variables leftBox and rightBox. Why not just swap this._leftBox and this._rightBox?
Review of attachment 152385 [details] [review]: Looks OK
Review of attachment 152386 [details] [review]: This looks OK.
Review of attachment 153454 [details] [review]: ::: src/shell-drawing.c @@ +97,3 @@ + pixel = &pixels[(j + 1) * rowstride - (i + 1) * 4]; + if (rtl) + guchar *pixel; This will be overwriting random memory. You want to change fade_start and fade_range (to be 0 and width / 2 respectively)
(In reply to comment #40) > Review of attachment 152387 [details] [review]: > > ::: js/ui/panel.js > @@ +339,3 @@ > + [leftBox, rightBox] = [rightBox, leftBox]; > + if (St.Widget.get_default_direction () == St.TextDirection.RTL) { > + let [leftBox, rightBox] = [this._leftBox, this._rightBox]; > > It's confusing to me that this._leftBox and this._rightBox aren't swapped, but > we introduce new variables leftBox and rightBox. > > Why not just swap this._leftBox and this._rightBox? This may be a solution, I'll try. (In reply to comment #43) > Review of attachment 153454 [details] [review]: > > ::: src/shell-drawing.c > @@ +97,3 @@ > + pixel = &pixels[(j + 1) * rowstride - (i + 1) * 4]; > + if (rtl) > + guchar *pixel; > > This will be overwriting random memory. You want to change fade_start and > fade_range (to be 0 and width / 2 respectively) I beleive it won't overwrite random memory (my first try did in fact). I beleive I need to go from width/2 down to 0, since the fading isn't uniform (we want to have the edge to be darker than the center). I find this solution is better.
Created attachment 153632 [details] [review] [windowManager] fix minimize animation for RTL locales
Created attachment 153633 [details] [review] [overview, StBin] fix workspace controls for RTL locales
(In reply to comment #40) > Why not just swap this._leftBox and this._rightBox? Or really, we should port the panel to use StBoxLayout (bug 598463), and then we don't need to do any special-casing
(In reply to comment #47) > Or really, we should port the panel to use StBoxLayout (bug 598463), and then > we don't need to do any special-casing The patch there seems to be already applied, and StBoxLayout cannot do the same as the custom container afaics. (the custom container ensures the calendar is in the absolute center).
Created attachment 154966 [details] [review] [overview] fix workspace controls for RTL locales
Created attachment 154967 [details] [review] [appDisplay] fix AppIconMenu position in RTL locales Ideas on a better way of handling this are welcome, the problem is that padding isn't included in width
Review of attachment 154966 [details] [review]: Looks good.
Created attachment 154968 [details] [review] [StScrollView] put vertical scrollbar to the left for RTL locales
Created attachment 154969 [details] [review] [panel] WIP: fix ripple animation for RTL locales What's still missing is the flipped icon. (We need to flip a couple other icons as well) btw, what do you think is the best approach fo this? (websites generally use an alternative stylesheet for rtl, but I'm afraid we will end up with LTR only themes)
Procedural note: Can we limit this bug report to the patches that are already attached, and file anything additional as new separate bugs? There's always going to be new shell features that need new RTL work, and I don't want to have an infinitely extending bug report.
(In reply to comment #53) > Created an attachment (id=154969) [details] [review] > [panel] WIP: fix ripple animation for RTL locales > > What's still missing is the flipped icon. (We need to flip a couple other icons > as well) > btw, what do you think is the best approach fo this? (websites generally use an > alternative stylesheet for rtl, but I'm afraid we will end up with LTR only > themes) I'd imagine that if we went with a separate stylesheet, a theme would consist of two stylesheets: shell-default.css shell-default-rtl.css Where the -rtl stylesheet was *only* overrides for RTL. A theme can't be a single file anyways since it needs to contain the images it is referencing. But you are right, there would a tendency for people to copy both files into their theme, and then just ignore the -rtl one and then the theme would be completely broken in RTL until someone fixed it. The other approach would be to have st_theme_node_set_direction() and then make the :ltr :rtl pseudo-selectors automatically work in the theme code based on the direction of the theme node. So you would add to the stylesheet: .ripple-box:rtl { background-image: url("upper-right-corner-ripple.png"); } The second approach is more elegant, I think. Less like CSS on the web, but on the other hand, our RTL approach is already not like CSS on the web. Does it make sense to you?
Review of attachment 154967 [details] [review]: ::: js/ui/appDisplay.js @@ +616,3 @@ let x, y; + if (this._windowContainerBox.get_direction() == St.TextDirection.RTL) + x = Math.floor(stageX - this.actor.width - this._windowContainerBox.get_theme_node().get_padding(St.Side.LEFT)); I haven't tried this out, but I don't understand the reasoning here. As I read the code, windowContainerBox is inside this.actor and has equal padding on left and right. So, I'd expect just stageX - this.actor.width should give equal spacing between the menu contents and this._source - why is subtracting the padding necessary?
Review of attachment 154968 [details] [review]: This looks right to me, but will be need to be rebased against the extensive StScrollView changes in bug 611740, which I believe change the same lines of code (in a fairly minor way)
Review of attachment 154969 [details] [review]: I think this isn't going to result in the ripple being fully at the right of the screen, since: let [x, y] = this._hotCorner.get_transformed_position(); Will be the *left* edge of the hot corner actor. ::: js/ui/panel.js @@ +678,3 @@ + ripple.opacity = 255 * Math.sqrt(ripple._opacity); + if (rtl) + ripple.x = x - ripple.width * ripple.scale_x; Hmm, a little ugly, but I can't think of a better way to do it. Unfortunately, the anchor point support support in Clutter isn't quite good enough to do this.
(In reply to comment #54) > Procedural note: Can we limit this bug report to the patches that are already > attached, and file anything additional as new separate bugs? There's always > going to be new shell features that need new RTL work, and I don't want to have > an infinitely extending bug report. Seems reasonable, but I think it's good to keep a tracker bug that depends on all such bugs though. (In reply to comment #55) > I'd imagine that if we went with a separate stylesheet, a theme would consist > of two stylesheets: > > shell-default.css > shell-default-rtl.css > > Where the -rtl stylesheet was *only* overrides for RTL. A theme can't be a > single file anyways since it needs to contain the images it is referencing. But > you are right, there would a tendency for people to copy both files into their > theme, and then just ignore the -rtl one and then the theme would be completely > broken in RTL until someone fixed it. Right. > > The other approach would be to have st_theme_node_set_direction() and then make > the :ltr :rtl pseudo-selectors automatically work in the theme code based on > the direction of the theme node. So you would add to the stylesheet: > > .ripple-box:rtl { > background-image: url("upper-right-corner-ripple.png"); > } > > The second approach is more elegant, I think. Less like CSS on the web, but on > the other hand, our RTL approach is already not like CSS on the web. Does it > make sense to you? Yes, it does. I'll try to implement this. (In reply to comment #56) > I haven't tried this out, but I don't understand the reasoning here. As I read > the code, windowContainerBox is inside this.actor and has equal padding on left > and right. So, I'd expect just > > stageX - this.actor.width > > should give equal spacing between the menu contents and this._source - why is > subtracting the padding necessary? As strange as this may sound, this.actor.width contains only the width and not the padding. I don't understand either, it's just that it works like this. (In reply to comment #57) > Review of attachment 154968 [details] [review]: > > This looks right to me, but will be need to be rebased against the extensive > StScrollView changes in bug 611740, which I believe change the same lines of > code (in a fairly minor way) I guess it's fine, a git pull --rebase didn't complain. btw, I still think attachment 153454 [details] [review] is fine, and I'm afraid attachment 153632 [details] [review] was overlooked
Review of attachment 152387 [details] [review]: Marking needs-work based on previous comment
Review of attachment 153454 [details] [review]: ::: src/shell-drawing.c @@ +97,3 @@ + pixel = &pixels[(j + 1) * rowstride - (i + 1) * 4]; + else + pixel = &pixels[j * rowstride + i * 4]; Iterating backwards through the pixels seems like an OK approach to me (within the constraints of this already being a pretty inefficient loop). I don't like the style of this expression - althogh rowstride is always 4 * width, we shouldn't be assuming it here. This would be better as: &pixels[j * rowstride + ((width - 1) - i) * 4] ::: src/shell-drawing.h @@ +24,3 @@ int minute); +ClutterTexture * shell_fade_app_icon (ClutterTexture *source, gboolean rtl); This needs to be broken onto two lines in the same style as the other declarations in this header.
Review of attachment 153632 [details] [review]: Looks good to me (in general, you really should be trying to provide a commit message body for each commit. Even if it doesn't seem to need it - so this one might be Fix minimize animation for RLT locales When minimizing a window in RTL corner, animate it moving to the upper right corner, not the upper left corner.
Review of attachment 154968 [details] [review]: Did apply cleanly, but looking through it, found some more issues: - needs to adjust the horizontal position of the horizontal scrollbar and shadow as well (suggest using separate variables for the allocation of the main child widget, and the ClutterAllocationBox used to allocate the other children. Then you can just line up the horizontal scrollbar and shadow with the child's horizontal bounds) - Needs to follow the GNU identation style we use in C code: - Spaces before opening '(' - '{' on separate line, indented half way between the two block levels
Review of attachment 154967 [details] [review]: ::: js/ui/appDisplay.js @@ +616,3 @@ let x, y; + if (this._windowContainerBox.get_direction() == St.TextDirection.RTL) + x = Math.floor(stageX - this.actor.width - this._windowContainerBox.get_theme_node().get_padding(St.Side.LEFT)); The solution to the mystery can be found in AppIconMenu._allocate() childBox.x1 = 0; childBox.x2 = this._arrowSize; childBox.y1 = Math.floor((height / 2) - (this._arrowSize / 2)); childBox.y2 = childBox.y1 + this._arrowSize; this._arrow.allocate(childBox, flags); (and so forth.) It's offsetting the children as allocated by an "arrow size" (apparently we are currently drawing space instead of an arrow.) So you need to make this part of the code RTL aware as well. What you were doing is compensating for this offset. Also, in your existing code, fix: if (this._windowContainerBox.get_direction() == St.TextDirection.RTL) to be: if (this.actor.get_direction() == St.TextDirection.RTL) when removing the offset.
After being unable to use the shell for some time because of a clutter bug, I'm slowly starting to follow again on this bug. I don't have much time to patch this now, but here are the bugs I noticed when I tried it (after the overview relayout was merged). 1. workspaces seem to be in inverted order in the overview and using ctrl-alt-arrow for moving moves in the opposite direction of the arrow (outside of the overview, the animations looks correct). (This was filed just now as bug 636083) 2. the ripple animation doesn't show (discussed above). (btw, what's the state of "anchor point support in Clutter"? can it be used instead of this ugly code?). So what's remaining here is only to implement the direction pseudo-selectors. 3. message tray's icons and "hot-corner" are in the lower right corner (should be lower left). (this should probably go in a new bug) 4. menus that show when right-clicking an application in the overview are on the right (and so go out of the screen). btw, there is a problem with icons here, so I can't confirm whether the app icon fading code got fixed or not. I'll try to look closer at what needs to be done (and rebase all those needs-work) later (unless someone does it instead :-p).
(In reply to comment #65) > 1. workspaces seem to be in inverted order in the overview and using > ctrl-alt-arrow for moving moves in the opposite direction of the arrow (outside > of the overview, the animations looks correct). (This was filed just now as bug > 636083) The patch in that bug has landed. There are still some remaining issues, which are addressed in bug 634691. > 4. menus that show when right-clicking an application in the overview are on > the right (and so go out of the screen). I attached a patch as part of bug 635645. > btw, there is a problem with icons here, so I can't confirm whether the app > icon fading code got fixed or not. Not as far as I can tell ...
Comment on attachment 152387 [details] [review] [panel] fix positions for RTL locales fixed in master
Comment on attachment 154967 [details] [review] [appDisplay] fix AppIconMenu position in RTL locales this is now bug 635645
Created attachment 175564 [details] [review] st: add rtl and ltr pseudo class selectors (right-to-left and left-to-right respectively). This allows theme-specific images/padding/border size to be mirrored for RTL locales I've tried not to touch much code, and so didn't touch StThemeNode as suggested above, is this correct?
Created attachment 175565 [details] [review] [panel] fix ripple animation for RTL locales the animation was invisible because it was going out of screen, also add a mirrored version of the image to the theme. (the code is a bit ugly, but I couldn't find a better way)
ahem: comment #54: > Procedural note: Can we limit this bug report to the patches that are already > attached, and file anything additional as new separate bugs? There's always > going to be new shell features that need new RTL work, and I don't want to have > an infinitely extending bug report.
Created attachment 175574 [details] [review] [panel] fix ripple animation for RTL locales the animation was invisible because it was going out of screen, also add a mirrored version of the image to the theme. (the code is a bit ugly, but I couldn't find a better way) (forgot to add the new image to Makefile.am)
(In reply to comment #71) > ahem: comment #54: > > Procedural note: Can we limit this bug report to the patches that are already > > attached, and file anything additional as new separate bugs? There's always > > going to be new shell features that need new RTL work, and I don't want to have > > an infinitely extending bug report. As I understand it, a new version of a needs-work patch is "already attached". I'm going to file other things as new bugs (if you prefer filing everything as new bugs, I don't mind)
Comment on attachment 153454 [details] [review] [panel] fix app icon fading for RTL locales this is now in bug 636201
(In reply to comment #73) > As I understand it, a new version of a needs-work patch is "already attached". oh, yeah, sorry, I didn't remember there being a ripple patch before
Review of attachment 175564 [details] [review]: While the patch is small and simple and working well in practice, it is missing some corner cases. In fact, by implementing this in StWidget instead of StThemeNode, widgets will _never_ pick up a changed default direction. On the other hand, I'm not sure that adding the functionality to StThemeNode wouldn't miss other cases, so given that in practice I'd expect the default direction to be set early (e.g. before any widgets are created) and only once, having this in StWidget is acceptable in my opinion - the limitation should be documented in a comment though. ::: src/st/st-widget.c @@ +1305,3 @@ + st_widget_add_style_pseudo_class (actor, "rtl"); + else + st_widget_add_style_pseudo_class (actor, "ltr"); This is a bit ugly, as st_widget_style_changed() will be called before any "normal" style classes are set. On the other hand, the widget is not yet mapped, so the actual style computation is deferred. The main problem though is that widgets will miss the rtl/ltr magic if the pseudo_class property is set explicitly (either via g_object_set() or st_widget_set_style_pseudo_class()) - I think overwriting all style automagic is correct in that case *except* when set in the contructor, e.g. let foo = St.Bar({ pseudo_class: 'baz' }); I would expect automatically assigned pseudo classes to work in this case (and existing ones like :hover, :first-child, :last-child do). You should get the correct behavior if you implement GObjectClass->constructor and set the pseudo class there. @@ +1421,3 @@ { g_return_if_fail (ST_IS_WIDGET (self)); + StTextDirection old_dir = st_widget_get_direction (self); Variables must be declared at the start of a code block, so this should be: StTextDirection old_dir; g_return_if_fail (ST_IS_WIDGET (self)); old_dir = st_widget_get_direction (self);
Review of attachment 175574 [details] [review]: The patch works fine, but I think that using ClutterGravity looks nicer :-) ::: data/Makefile.am @@ +25,3 @@ theme/close-window.svg \ theme/close.svg \ theme/corner-ripple.png \ It might be a good idea to rename the file to corner-ripple-ltr.png or use something like corner-ripple-topleft.png/corner-ripple-topright.png. ::: js/ui/panel.js @@ +925,3 @@ y: y }); ripple._opacity = startOpacity; + let rtl = ripple.get_direction() == St.TextDirection.RTL; I think it is more elegant to use if (St.Widget.get_default_direction() == St.TextDirection.RTL) ripple.set_anchor_point_from_gravity(Clutter.Gravity.NORTH_EAST); to do the positioning - you don't need to modify onUpdate then.
Comment on attachment 154968 [details] [review] [StScrollView] put vertical scrollbar to the left for RTL locales My apologies, but I only noticed the patch in attachment 154968 [details] [review] after opening bug 643156.
Comment on attachment 175564 [details] [review] st: add rtl and ltr pseudo class selectors It would be really helpful to have the :ltr :rtl selectors in 3.0, so I filed an alternative patch in bug 643835. Sorry for hijacking this, but the patch has not been updated for quite some time and the release date is approaching rather quickly.
Updated attachment 175574 [details] [review] according to my review and pushed as 12fad6f - Fix ripple animation for RTL locales