GNOME Bugzilla – Bug 509165
Support "spacer" value in /apps/metacity/general/button_layout
Last modified: 2008-02-29 20:44:20 UTC
Could be interesting support a "separator" value for /apps/metacity/general/button_layout GConf key to add more spacing between buttons. This should be useful to space out a bit Maximize and Close. Just like in current KDE4 Kwin setup.
Created attachment 102746 [details] The current KDE4 window buttons setup Maybe too much space.. We could add about 3/4 of button width.
Following the post on metacity's blog I stump across this bug and I've done some work on it. (Just for fun since I don't know the window manager internals at all ;) ) My first approach was trying to define a new button type (I've the patch for it), but soon I realized that doing so breaks every theme. So I've added the functionality directly to metacity. It's not perfect and I haven't tested it with rtl layout, so feel free to suggest improvements etc... (some "code style" decisions were made because I was trying to minimize the patch impact on existing codebase).
Created attachment 104272 [details] [review] Separators "attribute" patch
Created attachment 104273 [details] Metacity with separator patch applied
Sterling work! I have a few thoughts on the patch just from looking over it: 1) left_separators and right_separators are not separators (as, say, right_buttons are actually button functions), they are counts of separators, so the name is probably a bit misleading. 2) g_debug causes extra crap printed everywhere; try the meta_topic macro instead (or just leave it out) 3) "separator" is a workable name for these, but is there a hard-and-fast reason for calling them that or would "spacer" be just as good and shorter? 4) it seems that you can't put multiple separators in a row (although why you'd want to I don't know). I note that you can put them at the end, though. 5) left_separators and right_separators are ints, but the value can never be negative, so use guint Thanks again!
Hi Thomas, > Sterling work! I have a few thoughts on the patch just from looking over it: thank you. > 1) left_separators and right_separators are not separators (as, say, > right_buttons are actually button functions), they are counts of separators, so > the name is probably a bit misleading. Yes they can be an array of counters, but I think they should be just an array of bools (I can think a better name for this). They represent "if there is a separator" just after the button. So if you look at the prefs.c code the word "separator" has an effect only after a button function definition otherwise it's ignored, the same for multiple consecutive separators (but this can be easly changed). > 2) g_debug causes extra crap printed everywhere; try the meta_topic macro > instead (or just leave it out) I missed it when I cleaned the patch :) > 3) "separator" is a workable name for these, but is there a hard-and-fast > reason for calling them that or would "spacer" be just as good and shorter? I haven't any specific reason for the word "separator", just taken from the bug title. I will change to "spacer" if you like it better. > 4) it seems that you can't put multiple separators in a row (although why you'd > want to I don't know). I note that you can put them at the end, though. My patch allows this configuration (tested): menu:minimize,separator,maximize,separator,close and *can* allow (but I haven't implemented because I see no reason to do it) this config too: menu:minimize,separator,separator,maximize,separator,close (see comment to point 1) > 5) left_separators and right_separators are ints, but the value can never be > negative, so use guint yes I agree, but I think that it's better to change to an array of bools as I've stated in point 1 > Thanks again! > Just "giving back" :) P.S. I will update the patch this evening (6 hours from now) because I've to go to work :(
1) I think I would call them has_separator instead, then. (I was confused as to their purpose by their type, after all. But that makes point 4 easier to understand.) 3) I don't really care; it was just a suggestion. Does kwin use any particular keyword, do you know? 4) I doubt anyone wants it, and if they do they can come and ask for it. :) I still have to test in rtl locales.
(In reply to comment #7) 1) We have to distingue left area from right, may be a good name is left_has_spacer, right_has_spacer or better left/right_buttons_has_spacer (gboolean) 3) I don't know what kwin use and a quick search with google didn't bring any interesting result. But I think that spacer is more appropriate since we are just adding some space... 4) I agree :) If you give me some pointers, I can test rtl myself.
Pick any language which runs right-to-left (useful candidates might be ar (Arabic), he (Hebrew)... I'm rather surprised we don't have an Urdu translation, come to think of it.) I don't know what distro you use, but on Ubuntu you can install a locale thus: /usr/share/locales/install-language-pack ar dpkg-reconfigure locales (as root) Then you can launch Metacity using LANG=ar src/metacity --replace&
As promised a new revision of the patch. I think it address everything we have talked about, but it's still a work in progress. 1) I choose right/left_buttons_has_spacer for variable names, but feel free to change or suggest a better name :). The type now is gboolean. 2) cleaned the g_debug 4) tested ltr & rtl and it worked Why is it still a wip? 1) svn rev. 3551 didn't compile without the compositor enabled, so I've changed with some #ifdef the composite_at_least_version function in core/compositor.c (see the beginning of the patch) 2) In theme.c (see the TODO comment) I want to use the meta_ui_get_direction from ui.c but ld complains that is missing at linking time. If I add ui/ui.c to libmetacity_private_la_SOURCES (src/Makefile.am) I've to put in also every other dependency. What can I do? Move the function to utils.c? For now I just call gtk_widget_get_default_direction. Help needed :)
Created attachment 104443 [details] [review] Spacer config implementation
Created attachment 104444 [details] Right to left test screenshot
(In reply to comment #10) > 1) svn rev. 3551 didn't compile without the compositor enabled, so I've changed > with some #ifdef the composite_at_least_version function in core/compositor.c > (see the beginning of the patch) Oh, thanks for noticing that: I raised bug 514453 accordingly, now closed. > 2) In theme.c (see the TODO comment) I want to use the meta_ui_get_direction > from ui.c but ld complains that is missing at linking time. > If I add ui/ui.c to libmetacity_private_la_SOURCES (src/Makefile.am) I've to > put in also every other dependency. What can I do? Move the function to > utils.c? > For now I just call gtk_widget_get_default_direction. Help needed :) I will try to figure out the visibility rules for this case when I'm feeling a little less sleepy, probably tomorrow.
OT - about RTL/LTR I've just filed bug #514479 against GTK+: Provide commandline option or environment variable to choose RTL/LTR
Update OT - bug #514479 has now a working patch, just need to apply, rebuild your gtk+ and run metacity with: GTK_FORCE_DIRECTION=rtl metacity --replace
I read on the metacity blog that visibility rules are a complex beast, and since nobody stepped in, I tried to solve the problem. You know, I'm not the right person to do it, but I quickly realize (after some reading in doc/* and digging through the code for 2 hours) that it's no a good idea include ui.h in theme.c and after that I give up. In think that a possible approach is to include a direction attribute in some Meta* class like Style, Frame, Display or directly in ButtonLayout that it's were I need at the moment. But I don't really know if it makes sense. So I try to totally avoid the problem and I find a solution in prefs.c where the buttons are already swapped if the direction is rtl. Basically "a standard" metacity do something like this ltr layout: [L1] [R1][R2][R3] and at the end of update_button_layout in core/prefs.c checks if the direction is rtl and swaps the buttons: rtl layout: [R3][R2][R1] [L1] With my patch in we can have (S=spacer): ltr layout: [L1] [R1][R2S][R3] So i changed the rtl swap routine. Now the Spacer is swapped and then *shifted* logically to the previous button if present or to the last one if not, obtaning: rtl layout: [R1S][R2][R3] [L1] I've tested it and everything seems right, so no more "visibility rule problem". Hope this help and that I explain it well ;) P.S. In the new patch I update the key schema description too. Since I'm not a native english speaker (as you may have guessed ;) ), can you check my spelling and grammar? Thanks.
Created attachment 106069 [details] [review] Spacer config implementation 2
Created attachment 106072 [details] Screenshot of metacity with the new patch applied (ltr+rtl)
Created attachment 106130 [details] [review] Version of Andrea's second patch, cleaned up slightly It's all good: the code's fine, the English is mostly good (the only error is "adiacent" for "adjacent"), it works, and as you say there are no visibility problems. I pronounce this a Good Patch. Attached is a version with the "adjacent" fixed and a few spacing problems cleaned up. So, do we put this in? I think perhaps we should. Thoughts from others?
Seems reasonable to me.
The commit script does its magic: http://svn.gnome.org/viewvc/metacity?rev=3615&view=rev