GNOME Bugzilla – Bug 592503
Add a flexible version mechanism
Last modified: 2010-04-13 17:41:08 UTC
The current mechanism of metacity-theme-1.xml and metacity-theme-2.xml is not flexible for allowing small-scale additions. With this patch we bump the major version version once more to metacity-theme-3.xml and add a single feature: Any element in the DTD can have an attribute: version="[<|<=|=>|>] MAJOR.MINOR" And it will be ignored unless the predicate is met. (< and > should be to be entity escaped as < and >) This always having alternate sections of the theme file for older and newer version. While this is workable, I'm not completely satisfied that it was the best way to do it. An alternate approach would be to have the idea of 'extensions', and instead of: version=">= 3.1" and version="< 3.1" for alternatives elements have: requires="frame_center" and unless="frame_center" (You could do require="frame_center;title_ellipsize" to require multiple things.) (Considering that Metacity is apparently moving toward an entirely new theme format, I'm not sure if finding a perfect solution matters.) I talked to Thomas Thurman and he didn't care much one way or the other whether we went to metacity-theme-3.xml - because he wants to move to CSS themes, it's not useful to him, but it doesn't step on his toes.
Created attachment 141284 [details] [review] Clean up code to find themes Simplify code to find the right theme to load and loading it by moving all the loading code into a load_theme() helper function, and making meta_load_theme() use that as it searches through the directories. Look for old-version themes even when loading relative to the working in debug mode. Don't unnecessarily duplicate and then free info->theme_file and info->theme_dir.
Created attachment 141285 [details] [review] metacity-theme-3.xml: Add a flexible version mechanism The current mechanism of metacity-theme-1.xml and metacity-theme-2.xml is not flexible for allowing small-scale additions. With this patch we bump the major version version once more to metacity-theme-3.xml and add a single feature: Any element in the DTD can have an attribute: version="[<|<=|=>|>] MAJOR.MINOR" And it will be ignored unless the predicate is met. (< and > should be to be entity escaped as < and >) This always having alternate sections of the theme file for older and newer version. * Required GLib version is bumped to 2.14 so we can parse versions with a regular expression. * We switch internal version numbers to be "1000 * major + minor" * We keep a stack of the maximum required version for the current portion the XML tree so that the "cannot use versions you don't require" stricture of the old code can be made local to a subpart of the tree. * A version on the top metacity_theme element causes the entire file to be ignored; this allows having one metacity-theme-3.xml for version 3.2 and newer (say) and a metacity-1.xml for everything old. Actual new features will be added starting with 3.1 - 3.0 is just the version="" feature.
Both these patches look fine. Would it be more useful to sync the minor version to the release version? This will give us a good way to organize which theme properties we are supporting in a fairly organized manor.
Comment on attachment 141284 [details] [review] Clean up code to find themes Looks good.
Comment on attachment 141285 [details] [review] metacity-theme-3.xml: Add a flexible version mechanism I like the overall approach; the patch looks good.
Comment on attachment 141284 [details] [review] Clean up code to find themes Pushed the cleanup part of this patch (with one small fix revealed in testing where part of the flexible version mechanism patch leaked into the cleanup.) Attachment 141284 [details] pushed as 0ac4631 - Clean up code to find themes
Review of attachment 141285 [details] [review]: Noting down some problems that I saw when rereading the patch again. I'm not completely sure still whether it's better to go this way (with a linearly increasing version), or to have a set of "named features" that you can require, but I'm going to commit it this way for a number of reasons: * Probably the current metacity theme format is a bit of a dead end, so I don't think we really need to spend a lot of time worrying about whether gtk-window-decorator and metacity will implement some new features but not other new features. * This way is seems simplest if we need to add a number of ad-hoc features when working on the GNOME 3.0 look - I don't really want to spend a lot of time designing nicely orthogonal named features. * It's already implemented and tested this way. Beyond what's noted below I need to add docs to docs/theme-format.txt ::: src/ui/theme-parser.c @@ +97,3 @@ { GSList *states; + GSList *required_versions; Wouldn't be bad to add a comment for what these two lists are @@ +3320,3 @@ + +static gboolean +check_version (GMarkupParseContext *context, Function could use a brief comment as to the return value and other out values @@ +3349,3 @@ + + version = 1000 * atoi(major_str); + if (minor_str[0]) While g_match _info_fetch() is supposed to work like this - unmatched matches are "", there's actually a bug there currently when I tested with a short test program (already filed as 588217), so this needs to be (minor_str && minor_str[0]) @@ +3350,3 @@ + version = 1000 * atoi(major_str); + if (minor_str[0]) + version += atoi(minor_str + 1); Missing ' ' before ( on the atoi calls. @@ +3409,3 @@ + guint element_required; + + if (required_version < 3000) This could use a comment (that the "version" attribute can't be used in metacity-theme-[12].xm;) @@ +3412,3 @@ + { + set_error (error, context, G_MARKUP_ERROR, G_MARKUP_ERROR_PARSE, + ATTRIBUTE_NOT_FOUND, "version", element_name); ATTRIBUTE_NOT_FOUND is wrong here, it should be along the lines of _("Attribute \"%s\" is invalid on <%s> element in this context"), or probably just more specifically that version can only be used in version 3 or newer theme files. @@ +3419,3 @@ + return; + + if (peek_state (info) == STATE_START) Needs a comment here for why we are doing stuff differently for the toplevel (is explained in the commit message)
Pushed with the noted things fixed. Attachment 141285 [details] pushed as 020aea0 - metacity-theme-3.xml: Add a flexible version mechanism