Batch adding GNOME2 keyword to Metacity bugs.  Sorry for the spam.

Kenneth -- what exactly do you mean by the borders not being reversed?
 Could you supply a screenshot if that would be easier to understand?

I guess he meant that the window buttons (close, maximize, minimize etc) should
be reversed.

Just make it so that when you parse /apps/metacity/general/button_layout, the
computed value is the reverse of the inputted value. Though I'm not sure if it's
such a good idea...

Can we find whether we have consensus on whether reversing button order is a good idea for RTL languages before we implement anything?

Not only the buttons.  The entire window header (or whatever it's called).  App icon should be on the right, and buttons on the left...  Just, you know, invert it.

Pinged quite a few maintainers/translators from various teams and it seems they are all for reversing it. A gimped sample of what it should look like reversed will be attached.

Created attachment 84264 [details]
RTL border layout.

Created attachment 84267 [details]
RTL png image...

Ah really sorry, that is an image, not a patch. You can ignore me for the rest of week now.

While doing the reverse border work - 
There is a need to add a generic text direction support: e.g
metacity_get_direction().

This kind of API can be used by other RTL bugs in metacity, for example:
http://bugzilla.gnome.org/show_bug.cgi?id=387893

I believe the fix should be to move the default button layout to be translated and translate to "close,maximize,minimize:menu" in RTL .po files.

This is a really small fix in the schema files + translators work.

However, this will only work for new users. We also need a solution to change the gconf button_layout value for existing users.

NO.  No reason for .po hacks.

Created attachment 86580 [details] [review]
automatically reverse window frame's buttons order

This patch fix both gconf and no-gconf modes.

Also fix the way the theme engine reads the button's functions. In its previous implementation the no-gconf mode wasn't working correctly.

Instead of going over all the position in the buttons array - stop when the LAST_FUNCTION value is reached (that's why it was called LAST_FUNCTION).
This also eliminates the hack to fill the buttons array with LAST_FUNCTION values.

And to think that it only took 5 years to create a patch :)

Regarding the schemas thought, it's not good because then the inversion won't be automatic.

Almost forgot. This patch requires infrastructure work from:
http://bugzilla.gnome.org/attachment.cgi?id=86429&action=diff

Ping

It looks generally good; I was worried that left_buttons and right_buttons would be partially uninitialised, but you caught that in theme.c. I need to review your patch to bug 387893 before I can properly test it, of course, so I'll go do that now.

So, the things I'm testing for:
  * In LTR, left goes to the left workspace and right to the right; in RTL vice versa
  * In LTR, popping up the window menu with the keyboard puts it on the LHS of the window; in RTL the RHS
  * In LTR, the window menu is flush with the screen to the left; in RHS to the right
  * Tooltips are right-justified in RTL and left-justified in LTR. 

Oops, wrong bug. My apologies.

I've had the chance to look over the patch now. These are the problems I've found:

1) (trivial) It doesn't, in fact, handle the case when GConf is in use at all. init_button_layout(); needs to be called when HAVE_GCONF is defined as well as at present when it isn't, or (better) it needs to be called outside the #ifdef entirely. This is trivially fixed.

2) (more serious) You determine whether we're in LTR or RTL mode using meta_ui_get_direction(), a function added in the fix to bug 387893, which uses  gtk_widget_get_default_direction() to determine its result.  gtk_widget_get_default_direction() gets its data from an internal variable, which defaults to LTR when a program first runs. In RTL locales, it is set by a call to gtk_widget_set_default_direction(), which is called from gtk_get_option_group(), which is called from gtk_parse_args(), which is called from gtk_init_check(), which is called from meta_ui_init(). So any attempt to check the RTL/LTR status before meta_ui_init() is called is hopeless. But meanwhile, you're attempting to make use of it in init_button_layout(), which you call from meta_prefs_init(). But meta_prefs_init() gets called six lines *before* meta_ui_init() in main(). So I'm not sure how this patch could ever have worked!

(Obviously we could try switching round meta_ui_init() and meta_prefs_init() in main(), though that might have other subtle side-effects.)

I will attach a fixed patch asap. hopefully not more than a week.

Isn't gtk_widget_get_default_direction() initialized from the locale (using translations).

Behdad: yes, but's as thomas mentioned - that's triggered in meta_ui_init()

thomas: 1) you are incorrect. when gconf is enabled the button layout is taken from the conf entry. an initial value is meaningless, it is ignored.

2) i guess that it is not possible to call meta_ui_init() before meta_prefs_init(). However, i don't see any problem taking out gtk_init() from meta_ui_init() and putting it before meta_prefs_init().

Another possibility (which requires more work) is to make no changes when loading the buttons layout. instead do the reverting upon drawing the buttons.

It's your call, i can handle both ways.

As for how it worked, maybe there update conf calls that made miss this problem.
Or maybe my test environment was evil :)

1) Okay, but then how does this handle the problem raised in comment 10 about users who are using an RTL locale, who upgrade from a previous version of Metacity and who have the buttons in the "wrong" order?

2) I think moving the gtk_init() to main() before meta_prefs_init() sounds a good plan. Anyone else want to comment on the pros and cons of this?

Created attachment 89730 [details] [review]
automatically reverse window frame's buttons order

Now meta_ui_init() and meta_errors_init() are called before meta_prefs_init()

Fixed patch is available.

1) I don't see any problem raised in comment 10 . If the buttons are read from 
gconf, then it will be automatically reversed. If metacity was compiled with no gconf (have this ever happend?) then the rtl defaults will be used.

2) I've tool a deeper look and there was no problem with moving the whole meta_ui_init() befire neta_prefs_init().

This time i tested with no environment hacks.

Thanks for your patch!

1) I apologise: I meant to say comment 11. I think you do in fact handle this case (from looking at the patch; I haven't tested it yet because I'm away from my development environment at the moment).

In fact metacity is designed to work without gconf because of use in embedded systems. I don't have any information to hand as to how many people actually use it this way.

2) Thanks for looking into this.

When I get my hands on my development environment again (probably after work tonight) I'll test it and let you know whether it's okay to commit.

Any progress with the testing?

Totally fell off the radar (new job, etc); thanks for the nudge. I will try to fix this tonight.

Commited.

2007-07-22  Yair Hershkovitz  <yairhr@gmail.com>

        Reverse window buttons and align them to the left for RTL locales.
        Fixed #92212.

        * src/prefs.c (button_layout, init_button_layout, update_button_layout):
        Support reversing and left-aligning of buttons for both Gconf and
        NO-Gconf modes. 
        * src/main.c (main): Call meta_ui_init() before meta_prefs_init().
        meta_prefs_init() check for RTL locales which is initialized in
        meta_ui_init().
        * src/theme.c (meta_frame_layout_calc_geometry): Fixed access to
        button_layout to stop iterating when getting to a
        META_BUTTON_FUNCTION_LAST value.

Thanks.

Thomas, please close this bug. I don't have sufficient permissions to do it myself.

Sure, sorry. Done.

[Excersizing role as bugsquadder...] Yair: You can edit bugs now.  :-)