GNOME Bugzilla – Bug 499575
Color Schemes selection for metathemes
Last modified: 2011-10-22 02:47:38 UTC
Hello, I coded a patch for gnome-appearance-properties to implement the color scheme selection for each metatheme by a combo box in the colors selection window. Patch: http://zmax.netsons.org/colorscheme.patch Regards, Giuseppe
Created attachment 99624 [details] [review] the patch for color schemes selection
Could you remove the whitespace changes and split the patch into two seperate files (one for the glade xml and one for the code)?
Even better, keep it in one patch, but try to isolate the really necessary glade changes (shouldn't be more than 20-30 lines).
Oh, I am sorry! Here is a cleaned patch... Giuseppe
Created attachment 99635 [details] [review] color schemes selection patch - cleaned
Created attachment 99647 [details] [review] a new patch - I fixed a little problem There was a little problem with the first patch I sent, I fixed in this new one. There are some other fix's I would like to do improve the quality of my code in general.
First off, it would have been helpful if you had described how you envision color schemes to work in general. I first thought it could be extracted from the patch pretty easily, but some things made me wonder whether it's just the implementation or the general concept that needs a bit more thought. (and even if it could be easily extracted, not having to do so would have earned you a bonus point ;-)). E.g. what *are* color schemes, what are they good for, what kinds of themes do they work with, who provides them, what do they look like, etc. I'll provide some comments on your patch below, and I assume that some of the above questions will be answered by simply addressing the comments below, but others won't. Two general remarks in advance: Please make sure your patches don't have additional whitespace at line endings. This is going to mess up later commits for people who take care to strip trailing whitespace from their changes. And please generate patches with the -p option to diff (yes, it's bad that svn diff doesn't support this by default...). +++ gnome-control-center_mod/capplets/appearance/appearance-style.c 2007-11-26 11:15:13.000000000 +0100 +static gboolean +color_scheme_key_file_load (AppearanceData *data) +{ + gchar *theme_name = NULL; + gchar *themes_dir = NULL; + gchar *schemes_path = NULL; + gboolean result = FALSE; Indentation is a bit off here, please adhere to the coding style in the file. Additionally, initializing variables where it's not necessary is often considered bad style, since it helps obscur what you're doing (or not doing) and sometimes even hides real mistakes. This applies to other places in your patch as well. + themes_dir = gtk_rc_get_theme_dir (); + theme_name = gconf_client_get_string (data->client, METACITY_THEME_KEY, NULL); You need to be able to gracefully handle NULL return values here. Also, why the metacity theme? + schemes_path = g_build_filename (themes_dir, theme_name, "colorschemes.ini", NULL); It is generally frowned upon in Unix circles to call config files ".ini" ;-) It looks like the scheme files are supposed to be per theme, but you have one global instance in the AppearanceData struct. That looks very wrong. If they are per theme, the correct thing to do would (probably) be to add that information when loading the theme (in common/gnome-theme-info.c), or possibly to just pass the GKeyFile struct around locally where needed. + result = g_key_file_load_from_file (data->color_schemes_key_file, schemes_path, + G_KEY_FILE_NONE, NULL); + + g_free (theme_name); + g_free (themes_dir); + g_free (schemes_path); + + return result; +} + +static GString * Any specific reason why you return a GString instead of a simple gchar? You don't seem to reference the result as a GString anywhere. +color_scheme_key_file_get_scheme (guint index, AppearanceData *data) +{ + guint i, j; + gchar *colorstr = NULL; + gchar **scheme_groups = NULL; + gchar **color_keys = NULL; + GString *scheme = g_string_new (NULL); + + scheme_groups = g_key_file_get_groups (data->color_schemes_key_file, NULL); + color_keys = g_key_file_get_keys (data->color_schemes_key_file, scheme_groups[index], + NULL, NULL); Why don't you just use g_key_file_get_value here instead of iterating over all available keys? Saves you the inner loop. + /* generate a valid gconf color scheme */ + for (i = 0; i < NUM_SYMBOLIC_COLORS; ++i) + { + for (j = 0; color_keys[j]; ++j) + { + if (strcmp (color_keys[j], symbolic_names[i]) == 0) + { + colorstr = g_key_file_get_string (data->color_schemes_key_file, + scheme_groups[index], color_keys[j], + NULL); + g_string_append_printf (scheme, "%s:%s\n", color_keys[j], colorstr); + g_free (colorstr); + + break; /* disregard color redefinitions */ + } + } + } + + g_strfreev (color_keys); + g_strfreev (scheme_groups); + + if (scheme->str) I think checking for scheme->len would be both more independent from the actual implementation and robust here (even though gtk promises not to change behaviour, we've seen it before). + g_string_truncate (scheme, scheme->len - 1); + + return scheme; +} + +static void +color_scheme_combo_update (GtkSettings *settings, AppearanceData *data) +{ + guint i; + gboolean found = FALSE; + gchar **scheme_groups = NULL; + gchar *scheme_name = NULL; + gchar *cur_scheme = NULL; + gchar *def_scheme = NULL; + gchar *theme_name = NULL; + GtkComboBox *combo = NULL; + GString *scheme = g_string_new (NULL); + + combo = GTK_COMBO_BOX (glade_xml_get_widget (data->xml, "color_scheme_combo")); + gtk_list_store_clear (GTK_LIST_STORE (gtk_combo_box_get_model (combo))); + + gtk_combo_box_append_text (combo, "Default"); The label needs to be translatable. + theme_name = gconf_client_get_string (data->client, METACITY_THEME_KEY, NULL); Why the metacity theme? + def_scheme = gtkrc_get_color_scheme_for_theme (theme_name); + g_object_get (settings, "gtk-color-scheme", &cur_scheme, NULL); + + if (theme_name && def_scheme && cur_scheme) + { + /* default theme selection */ + if (gnome_theme_color_scheme_equal (def_scheme, cur_scheme)) { + gtk_combo_box_set_active (combo, 0); + found = TRUE; + } + + /* update combo box with color scheme names from the file */ + if (data->color_schemes_key_file) + { + scheme_groups = g_key_file_get_groups (data->color_schemes_key_file, NULL); + + for (i = 0; scheme_groups[i]; ++i) + { + scheme_name = g_key_file_get_locale_string (data->color_schemes_key_file, + scheme_groups[i], "Name", NULL, NULL); + if (scheme_name) + gtk_combo_box_append_text (combo, scheme_name); If you don't get a name, you won't add it to the combo, but you might try to activate it regardless. Bad. + /* autoselect the current color scheme */ + if (!found) { + scheme = color_scheme_key_file_get_scheme (i, data); + + if (gnome_theme_color_scheme_equal (cur_scheme, scheme->str)) { + gtk_combo_box_set_active (combo, i + 1); + found = TRUE; + } + + g_string_free (scheme, TRUE); + } + + g_free (scheme_name); + } + } + } I think you'll find out pretty quickly that people want the list sorted alphabetically. + g_free (def_scheme); + g_free (cur_scheme); + g_free (theme_name); + g_strfreev (scheme_groups); +} +++ gnome-control-center_mod/capplets/appearance/data/appearance.glade 2007-11-25 21:36:25.000000000 +0100 @@ -2209,6 +2209,39 @@ </packing> </child> <child> + <widget class="GtkTable" id="color_scheme_selection_table"> + <property name="visible">True</property> + <property name="events">GDK_POINTER_MOTION_MASK | GDK_POINTER_MOTION_HINT_MASK | GDK_BUTTON_PRESS_MASK | GDK_BUTTON_RELEASE_MASK</property> + <property name="n_rows">1</property> + <property name="n_columns">2</property> + <child> + <widget class="GtkComboBox" id="color_scheme_combo"> + <property name="visible">True</property> + <property name="events">GDK_POINTER_MOTION_MASK | GDK_POINTER_MOTION_HINT_MASK | GDK_BUTTON_PRESS_MASK | GDK_BUTTON_RELEASE_MASK</property> + <property name="items" translatable="yes"></property> Don't mark this as translatable. + </widget> Finally, my appearance window came up with an empty combobox (ie. not even a Default entry in it). So much for now. I might have a few more bits, but that depends on the answers to the questions.
First of all, thanks very very much for considering my work, that certainly must be improved a lot. I tried to implement this first patch for GNOME because I would like to take part in the development process and possibly learn a lot from it. I am ready to learn of the necessary to do a good work and your reply, so rich of details, certainly can help me in that direction. Now, let me explain what I did where I hadn't make a mistake. :) > First off, it would have been helpful if you had described how you envision > color schemes to work in general. I first thought it could be extracted from > the patch pretty easily, but some things made me wonder whether it's just the > implementation or the general concept that needs a bit more thought. (and even > if it could be easily extracted, not having to do so would have earned you a > bonus point ;-)). Yes, first of starting coding I asked for specification for color schemes, but the answer was that there aren't specs yet and that actually a color scheme is a line into the index.theme file that defines a set of color for some graphical elements of the desktop. Andrea Cimitan asked me to implement a new functionality to select the preferred color scheme from a combo box loading them from another file for each metatheme. It was a good idea in my opinion, because for each theme it would be interesting to define a set of variants of the default color scheme, sharable with other themes (this implies a separate file). So, I accepted the challange. > + themes_dir = gtk_rc_get_theme_dir (); > + theme_name = gconf_client_get_string (data->client, METACITY_THEME_KEY, > NULL); > > You need to be able to gracefully handle NULL return values here. > Also, why the metacity theme? Ok for handling errors in general; Here I used METACITY_THEME_KEY as defined in the theme-util.h file, to reach the information about the selected theme in gconf. Is it generally wrong? If yes, why? > It looks like the scheme files are supposed to be per theme, but you have one > global instance in the AppearanceData struct. That looks very wrong. If they > are per theme, the correct thing to do would (probably) be to add that > information when loading the theme (in common/gnome-theme-info.c), or possibly > to just pass the GKeyFile struct around locally where needed. Yes, each theme have a scheme file. Here, I initially passed a pointer to GKeyFile through the functions when needed, but I finally decided to create a global pointer in the AppearanceData structure, even with some doubts... So, why is it generally "very wrong"? > +static GString * > > Any specific reason why you return a GString instead of a simple gchar? You > don't seem to reference the result as a GString anywhere. No specific reason... only to manipulate the string within the function, I could return a gchar as well. > Why don't you just use g_key_file_get_value here instead of iterating over all > available keys? Saves you the inner loop. > > + /* generate a valid gconf color scheme */ > + for (i = 0; i < NUM_SYMBOLIC_COLORS; ++i) > + { > + for (j = 0; color_keys[j]; ++j) > + { > [...] I'll use g_key_file_get_value(). > + theme_name = gconf_client_get_string (data->client, METACITY_THEME_KEY, > NULL); > > Why the metacity theme? As above, to reach information of a selected theme in gconf. > + if (scheme_name) > + gtk_combo_box_append_text (combo, scheme_name); > > If you don't get a name, you won't add it to the combo, but you might try to > activate it regardless. Bad. > > + /* autoselect the current color scheme */ > + if (!found) { > + scheme = color_scheme_key_file_get_scheme (i, data); > + > + if (gnome_theme_color_scheme_equal (cur_scheme, scheme->str)) { > + gtk_combo_box_set_active (combo, i + 1); > + found = TRUE; > + } > + > + g_string_free (scheme, TRUE); > + } > + > + g_free (scheme_name); > + } > + } > + } > > I think you'll find out pretty quickly that people want the list sorted > alphabetically. > > + g_free (def_scheme); > + g_free (cur_scheme); > + g_free (theme_name); > + g_strfreev (scheme_groups); > +} You're right at all. I can try to re-think this algorithm. > Finally, my appearance window came up with an empty combobox (ie. not even a > Default entry in it). > > So much for now. I might have a few more bits, but that depends on the answers > to the questions Yes, the combobox was empty because of a mistake in the second patch I sent. But in the third the combobox fills correctly. So, thanks a lot for all, obviously I have to improve more my code and study harder gtk+ and the project itself. I'd like to know if, in your opinion, it is a good idea to modify the patch so that it could be included in the next stable version or not. However, I'd like to take part at the project... Thanks for reply, Giuseppe.
(In reply to comment #8) > I tried to implement this first patch for GNOME because > I would like to take part in the development process and possibly learn a lot > from it. I am ready to learn of the necessary to do a good work and your reply, > so rich of details, certainly can help me in that direction. Fresh blood is certainly welcome, and we can always use some additional hands and minds. ;-) > Andrea Cimitan asked me to implement a new functionality to select the > preferred color scheme from a combo box loading them from another file for each > metatheme. It was a good idea in my opinion, because for each theme it would be > interesting to define a set of variants of the default color scheme, sharable > with other themes (this implies a separate file). So, how do you expect this scheme sharing to work? Your current patch simply assumes a separate file for each theme. I don't see anything wrong with having colour schemes defined per theme, but I'd rather have them included in the existing fileset (index.theme most likely). I don't see how putting them in a separate file makes it (much) easier to share those definitions. It seems to me that it's just unnecessarily complicating the metatheme format. > > + theme_name = gconf_client_get_string (data->client, METACITY_THEME_KEY, > > NULL); > > > > You need to be able to gracefully handle NULL return values here. > > Also, why the metacity theme? > > Ok for handling errors in general; Here I used METACITY_THEME_KEY as defined in > the theme-util.h file, to reach the information about the selected theme in > gconf. > Is it generally wrong? If yes, why? Two problems here: 1) gconf_client_get_* can return NULL if the key is unset, and the code following this line is not prepared to handle that case. 2) What bearing does the metacity theme have on the colour scheme? Maybe you've become confused by metacity theme vs. metatheme here? > Yes, each theme have a scheme file. > Here, I initially passed a pointer to GKeyFile through the functions when > needed, but I finally decided to create a global pointer in the AppearanceData > structure, even with some doubts... So, why is it generally "very wrong"? I'll take that back. It's not wrong (as I thought at first glance). It's just not very nice because the necessary resource handling is not symmetrical, so it's rather easy to mess up. Unless you make alternate colour schemes a property of the metatheme struct, this is probably the easiest way to solve it, though. > > +static GString * > > > > Any specific reason why you return a GString instead of a simple gchar? You > > don't seem to reference the result as a GString anywhere. > > No specific reason... only to manipulate the string within the function, I > could return a gchar as well. Since all callers of the function use GString->str anyway, that would be better. > So, thanks a lot for all, obviously I have to improve more my code and study > harder gtk+ and the project itself. I'd like to know if, in your opinion, it is > a good idea to modify the patch so that it could be included in the next stable > version or not. I certainly think that colour schemes are a useful concept, and your patch is a nice first attempt at supporting them. I have two basic reservations about the current approach, though. The first is the aspect of sharing colour schemes between themes that you mentioned at the beginning. I'm not sure how that's supposed to work. The second is I don't like that with the schemes combobox, we now have two competing ways to basically do the same thing: set theme colours. We might need to rethink that part of the UI a bit more (Thomas said he had some ideas about the colours tab). Once these more fundamental questions are answered, I'm sure your patch can be made fit and included in the repository. Thanks for working on this!
Hello Jens, > Fresh blood is certainly welcome, and we can always use some additional hands > and minds. ;-) > Sure, I have two objectives: first, I want to learn more about how GNOME works and the system in general; and second, I'd like to give an hand to you all. :-) > So, how do you expect this scheme sharing to work? Your current patch simply > assumes a separate file for each theme. I don't see anything wrong with having > colour schemes defined per theme, but I'd rather have them included in the > existing fileset (index.theme most likely). I don't see how putting them in a > separate file makes it (much) easier to share those definitions. It seems to me > that it's just unnecessarily complicating the metatheme format. I thought this and talked with Andrea and others. In a first time, it seemed to me that a separate file could make more immediate sharing between users a certain number of schemes; so, we have at least two possibilities for sharing which don't exclude each other: 1) Share the color schemes between the /users/: thinking more, a separate file makes possible the share even a /big/ number of schemes at a time simply "copy & paste" the file. I don't know if in the future this feature could be useful or interesting for the users, but if we don't consider that case, I think that editing and pasting into the index.theme a large quantity of schemes is quite dangerous and invasive... 2) Share the color schemes between the /themes/: in this case, we could think of a single, global, file or one file for each theme plus the possibility to select at what theme a selected scheme belongs. We certainly can consider both the possibilities, or simply leave at the end user the responsibility of the color schemes sharing and management saving them in the index.theme. In this case, we could simplify the code and avoid to define a new file format and documentation for schemes maybe giving up at further developments such as schemes descriptions (localized) like gedit does. The problem here is to supporting or not the user in that direction. >> Ok for handling errors in general; Here I used METACITY_THEME_KEY as defined in >> the theme-util.h file, to reach the information about the selected theme in >> gconf. >> Is it generally wrong? If yes, why? > > Two problems here: > 1) gconf_client_get_* can return NULL if the key is unset, and the code > following this line is not prepared to handle that case. > > 2) What bearing does the metacity theme have on the colour scheme? Maybe you've > become confused by metacity theme vs. metatheme here? 1) Yes, I understand it very well. I have to validate in general all the user input such as text inputs, gconf or configuration files as well. 2) I am sorry for that mistake, I intended there GTK_THEME_KEY instead of METACITY_THEME_KEY. Sincerely, the reason is that I am dealing with GTK+ programming from a relative short time and all my efforts are targeted to increase my knowledge of GNOME coding day by day. So, I have to adhere to your style and knowledge asap. >> Yes, each theme have a scheme file. >> Here, I initially passed a pointer to GKeyFile through the functions when >> needed, but I finally decided to create a global pointer in the AppearanceData >> structure, even with some doubts... So, why is it generally "very wrong"? > > I'll take that back. It's not wrong (as I thought at first glance). It's just > not very nice because the necessary resource handling is not symmetrical, so > it's rather easy to mess up. Unless you make alternate colour schemes a > property of the metatheme struct, this is probably the easiest way to solve it, > though. In fact, the color scheme key file is a metatheme property, not an "appearance" property itself. So I have to review also this concept. > I certainly think that colour schemes are a useful concept, and your patch is a > nice first attempt at supporting them. I have two basic reservations about the > current approach, though. The first is the aspect of sharing colour schemes > between themes that you mentioned at the beginning. I'm not sure how that's > supposed to work. The second is I don't like that with the schemes combobox, we > now have two competing ways to basically do the same thing: set theme colours. > We might need to rethink that part of the UI a bit more (Thomas said he had > some ideas about the colours tab). Once these more fundamental questions are > answered, I'm sure your patch can be made fit and included in the repository. I will ask to Thomas Wood more information about his ideas, so that we can answer to this basic questions. > Thanks for working on this! > Thank you for consider it! :-)
(In reply to comment #10) > 1) Share the color schemes between the /users/ ... > 2) Share the color schemes between the /themes/ I think neither makes a lot of sense if if colour schemes are supposed to come from the themes themselves. Of course, people can create their own, but they'll soon have to merge with others at which point they'll start hacking the files anyway. I'd say we should either 1) make colour schemes per theme, let the themes define some that make sense/look good, and include them in index.theme or 2) make colour schemes global (ie. per user) and give each user one file that contains all his colour schemes. Personally, I prefer 1). > 2) I am sorry for that mistake, I intended there GTK_THEME_KEY instead of > METACITY_THEME_KEY. Sincerely, the reason is that I am dealing with GTK+ > programming from a relative short time and all my efforts are targeted to > increase my knowledge of GNOME coding day by day. So, I have to adhere to your > style and knowledge asap. I predict that's going to be very difficult. ;-) Anyway, making mistakes is part of the development process (I do it all the time ;-)), so no need to apologize here.
I'm absolutely _AGAINST_ adding colorschemes inside the index.theme. Maybe we can change the name but adding to the index.theme is just _useless_ and _illogic_ (or maybe you want 200Kb of index.theme :D)
The final decision will be to add colorschemes into a separate file, inside the root of the theme (we have discussed for an entire afternoon in #gnome-art with other developer), but I let you chose the name if you don't like colorscheme.ini :) 1) Simpler to add in the future inheritance between themes (imagine to use an index.theme for the Industrial theme and specify the colorscheme of another one, like in ColorScheme=Clearlooks so we have ONE index.theme, industrial, and ONE colorscheme, Clearlooks. a lot better than having TWO index.theme, Industrial and Clearlooks that may give mistakes between) 2) Users may share in websites like art.gnome.org or gnomelook.org just the "Colorscheme.ini" files, and NOT the INDEX.THEME, SO NO PROBLEMS IN TRANSLATIONS AND VERSIONS (if someone in the svn do update the index.theme then the guy who uploaded the index.theme MUST reupdate it... just useless) 3) Faster to add to a new theme (imagine I create a new theme and I want to copy the colorschemes I just drag'n'drop a file without open gedit and copying single lines without errors) and for share between two themes that share same colorschemes. I don't have to get the index.theme, edit it, bla bla... 4) A separate file is better to code (easier for us, themers/developer) and more stable (we wont make mistakes by editing an idnex.theme). 5) It is more logical (gtkrc for gtk, metacity for metacity, colorscheme for colorscheme, index for the index of the theme) and intuitive. 6) the last but not the least I like it more :D
Hi Jens and Thomas, > 1) make colour schemes per theme, let the themes define some that make > sense/look good, and include them in index.theme > > or > > 2) make colour schemes global (ie. per user) and give each user one file that > contains all his colour schemes. > > Personally, I prefer 1). I also don't like adding colour schemes into the index.theme file. The idea of a separtate file is born also to think at that scheme definitions as an override of the "default one" in the gtkrc file, so that if one would define only one field for a scheme (i.e. "base_color"), the others would remain the same. Also, it was a good idea because of use of GKeyFile support for localization of their names. We could also think to move the colour schemes into the gtkrc file, but again we'll have at least to introduce a new field for colour schemes such as a "Name" field, possibly localized and also an identifier to distinguish uniquely the schemes from each other. All problems we can easily solve by using a GKeyFile object with a new configuration file. On the other hand we could again give up at localization of names (or even descriptions) adding one or two new fields to the existing colour scheme format into the gtkrc, which in my opinion isn't very natural because of GtkSettings properties. So, here we are... I prefer a separate file according to Andrea Cimitan.
(In reply to comment #12) > I'm absolutely _AGAINST_ adding colorschemes inside the index.theme. And I am absolutely against adding the colour schemes feature as currently proposed. It's simply not a sustainable format, _especially_ if you want to start "officially" distributing colour schemes via a.g.o or something. Sooner or later you (someone) will want to have tool support for downloading them and applying them to your themes, and the proposed format (colour schemes are part of the theme, but not) makes that a nightmare to support. Think about how you want to download additional colurs for a system theme. Now what? It doesn't work. (Unless you admit that the sharing argument is a strawman and pretty much irrelevant.) We're painting ourselves into a corner here.
The problem Jens, that we discussed for hours in #gnome-art and that was the format chosen. And there are reasonable reasons for a separate file and zero reasons for index.theme, which is just a problem. If you're still interested in changing the format, what you can do is discuss freely with me, benzea, sarts, thos, and giuseppe. As we did.
I am not arguing for putting the schemes in index.them. Some of the reasons you outlined are valid reasons for not doing that. I am, however, arguing against making colour schemes a property of the metatheme AND making them freely user-configurable/shareable at the same time, since that doesn't align properly with the metatheme format.
and so what you propose?
I think this discussion needs to be taken onto the gnomecc-list mailing list where everyone can contribute.
I can see three ways of approaching this that can be made to work: 1) Make colour schemes part of the theme, and basically forget about the "I can share colour schemes with the intarweb" part. 2) Completely decouple colour schemes from the themes. Each user has one schemes file containing all his favourites which he can apply to themes at will (and share, too). Those schemes don't have any relation to actual metathemes, though. 3) A combination of 1) and 2). Make themes ship (immutable) defaults, and give users a global file where they can add other schemes they like. I certainly don't claim the list to be complete, but those are the sane solutions I can see.
hi! from Jens answer in the bug 537997 discussion, i'd ask here too a suggestion about this mockup http://img135.imageshack.us/img135/7162/customizemockupmc3.png - as well how gnome-color-chooser working process may help 'Colorize Theme' from 'Appearance Preferences'. gnome-color-chooser is a very complete tool for colour schemes i really reccomend you all to experiment, and getting some interesting ideas from there as well.
Mass move to gnome-tweak-tool, for theme handling bugs.
In general the trend is for metacity/mutter to pick up gtk theme colors *** This bug has been marked as a duplicate of bug 654864 ***