To implement the versioned theme file, some general ideas:

1. in meta_theme_load(), first try the newer theme filename, then try 
   each older theme version filename. 

2. depending on which file we load, set info.format_version = 2 or 
   info.format_version = 1 or whatever

3. for each modification to the theme format, we need to conditionally 
   do the right thing. If something was an error in version 1, it 
   must remain an error when loaded from a file with version 1.

So for example, when changing line width from a simple integer to an
expression, 
  if (info.format_version > 1)
    /* handle it as an expression */ ;
  else 
    /* validate with parse_positive_integer that it is not 
       an expression, then convert to an expression */ ;

4. for each change to the format, we should make a note in
doc/theme-format.txt along with the theme format version 
where the change was made.

Created attachment 22963 [details] [review]
proposed patch

Rather than multiple files I would suggest that we simply add a
theme_version="..." attribute to the metacity_theme tag.  If the
attribute is missing we assume that it's a version 1 theme.  We then
further specify that metacity should try to load the oldest version
newer than or equal to the current theme version, and ignore anything
that it doesn't understand in the theme file.  Theme authors could
then specify multiple metacity_theme blocks in the XML file with
different versions should they so desire.

This may not work though, since I'm not sure if metacity would
currently barf it if saw an attribute that it doesn't know about or
what it would do if it saw multiple metacity_theme blocks.  I suspect
that it would work and just use the first metacity_theme tag though.

Remember that we can't change already-released metacity versions,
themes have to work with old metacity without changing old metacity.

One point is that the multiple files thing seems simple for theme
authors to understand.

that's why I said it might not work -- the question is really how
metacity currently deals with slightly invalid theme format files.

Frankly, the code sort of implied it was a done deal and I didn't
question Havoc's design :-)
Now that I think of it I can see a number of reasons to have separate
file:

- It's faster. You don't have to open the file to know which versions
of the theme it contains.
- It's simpler. You can have tricky failsafe, like all the way down
from broken user theme v2 to a system theme v1. I realize you can do
that too with one file but imagine the code... Here you have it for free.
- It's consistent wih gtk1/2 scheme.
- If you break the xml formatting by editing the theme file or the
file gets corrupted, you don't break all versions at once.
- and last but not least it's not the same as gtk1 vs. gtk2. It's not
like you want to support older versions forever. Upgrading will be a
no-brainer and I know that as a theme author I will probably not even
provide a v1 file for new themes.

Hope that convinced you :-)

there are 2 bugs in this patch, one of them a crasher.
updated patch follow.

Created attachment 23030 [details] [review]
updated patch

#121603 should be added as a dependency

Sorry to "ping" you like that but can you please make a call on this
patch ?
Can I continue to work in this direction ?
I have pending updates (namely a "get_theme_version" function, needed
for #86040 and probably all the other dependencies down there).

We're feature-frozen for Gnome 2.6, so it'll have to wait until Gnome
2.8, but I don't think that there's any obstacle to this going in.

I do need to review this patch before it goes in, I certainly hope to
be able to do so before GNOME 2.8.

If you want to keep your work in CVS it's totally OK to create a
branch called something like metacity-theme-changes-branch and work
there; to create a branch typically you would first make a tag:
 METACITY_THEME_CHANGES_BRANCHPOINT
then use cvs rtag to create a branch based on that tag. You can then
recover your entire patch by diffing the branch vs. the tag. See cvs
manual for details.

Of course this is only worthwhile if you're doing a fairly large set
of changes.

In any case, continue to ping us periodically, and I'll also go
through bugs with PATCH keyword. I hope to be back hacking on metacity
more regularly pretty soon.

I think it would be nice to target fixing some of the dependencies of this bug
for 2.10. I don't think it's realistic to fix all the dependencies at once, but
we should try to do more than one of them to make bumping the theme format
version worthwhile.

Also, we should add a section to "theme-format.txt" that's "new in theme format
version 2"

Comment on attachment 23030 [details] [review]
updated patch

I just looked at this bug again and was reminded of this patch. Some comments.

- no real point using "char" for format_version, due to alignment it will use
up 32 bits anyhow. int is better.

- The current format does
"metacity-1/metacity-theme-1.xml"
I think the "-1" in the directory name was a mistake, and will end up never
changing. So we shold try versions by scanning for metacity-theme-2,
metacity-theme-1 all under metacity-1.

- if (text == NULL) at top of a for loop that has text == NULL as condition,
redundant I think

- this patch will read a 
file with a new version number, so we can't add it until we have code to parse
the new file format.

bug #151261 has a patch showing how to get basic infrastructure for a new format
version in place, though I don't know if we should apply that until we have a
batch of format changes ready.

updating per comments

Remove old target milestones on old bugs; sorry for the spam.

So before any changes to the theme format get checked in, we need to create a branch in CVS they should live in? And then eventually we merge them back to the main branch.

That's one approach, but I wouldn't do a CVS branch unless someone was actively planning to go through some of these theme format bugs and work on them, then do the new format version in the next release. If the theme format will only be versioned at some vague time in the future, it's probably easier to just keep the patches in bugzilla.

Created attachment 61879 [details] [review]
Rewritten patch to work against current CVS and taking suggestions into account

Attachment 23030 [details] had gone stale by now; I've rewritten it to work cleanly against current CVS. I also took Havoc's suggestions into account.

> this patch will read a 
> file with a new version number, so we can't add it until we have code to parse
> the new file format.

I think we should add this file in HEAD now, before we add the code to parse the new file format. After all, any such code would need to rely on the value of the format_version field, which is added by this patch. And I suppose it's not really such a problem if the format's not well defined for a few weeks: it's not like people are going to be distributing version 2 themes until we make a release which supports them.

Perhaps we could write

#ifdef ALLOW_NEW_THEME_FORMAT
#define THEME_VERSION 2
#else
#define THEME_VERSION 1
#endif

so it wouldn't even be possible to use version 2 files unless we deliberately asked for it during development.

(Whether we do or not, I'd like to go through a bunch of theme v2 bugs and work on them for the next release.)

It looks like I'd elaborated on this patch a bit in the patch on #151261, some of the ideas in there look worthwhile, at least I think the documentation in that other patch is important. (Modified to match whatever the final patch is, of course.) theme-format.txt definitely needs to explain the deal here, and comments in the code need to make very clear the guidelines in comment #1
(again, modified to match the actual patch, obviously)

Created attachment 61954 [details] [review]
As before, with some extra commenting and documentation

Okay, I've added a section like that to the theme documentation, and an explanatory comment block to explain what's going on in the code. The section in the documentation doesn't mention what's new in version 2 yet, but we can fill in that section as we go clearing up the other metacity-theme-2 bugs.

Suggest establishing a convention for theme version tests, since you may test the same thing in multiple places. i.e. say you add alpha blending, I think in my other patch I had:
 if (theme->version_features & ALPHA_BLENDING)
or something like that. A simpler/better approach might be:
 #define META_THEME_VERSION_WITH_ALPHA_BLENDING 2
and then use "if (theme->version >= META_THEME_VERSION_WITH_ALPHA_BLENDING)"
around the code that parses this.

Anyway, no need to add that to this patch (the patch looks OK) but something to copy from the other patch is to think through a way to handle this. The multi-version handling will never get tested so the only hope is to be disciplined about docs and conditionalization when adding each new feature to the format.

That sounds a good idea.

I want to put in a bunch of work on getting metacity-theme-2 ready. Should I branch in CVS, or shall I just work in HEAD?

I'd suggest the branch approach; I used it for doing the constraints rewrite last fall.  It allowed me to try out lots of crazy stuff and not have to wait for review, until I had finally gotten everything working.  And it let me make totally destabilizing changes, though I don't know if that's as applicable for you or not.  One more good reason may be that I personally don't care much for theme stuff (and thus don't know much about it), while Havoc has an awful lot of things on his plate these days, so you probably don't want to wait for lots of intermediate reviews.  ;-)

Okay, branched with name metacity-theme-2, and this patch checked into the branch. This should be fun!

It certainly was!

I believe I have implemented everything necessary now. How should I proceed? Shall I produce an enormous diff between HEAD and metacity-theme-2, attach it to this bug, and ask whether anyone can comment on it?

you'll want to produce a diff between metacity-theme-2 and the start of metacity-theme-2, and then try to apply that diff to head.  It'll probably take some conjoling.  At that point, you'll have a patch against HEAD.

If you diff it between head and metacity-theme-2, then you'd revert all the changes in head that aren't on your branch.  This is the sort of thing that's a lot easier with subversion, but oh well.

Thank you.  Well, I hear we're due for subversion soon.

So when I have this patch-against-HEAD, I should post it here and ask for someone to approve or not with all the changes? It'll be pretty huge.

Do you have a design document or something for all changes? Especially something that describes how the new theme format should be parsed would be good.

I'll post it with the diff.

Created attachment 68095 [details] [review]
metacity-theme-2: first pass

Here's the diff against HEAD...

Created attachment 68096 [details]
metacity-theme-2: first pass: changelog

...and the changelog...

Created attachment 68097 [details]
metacity-theme-2 version of Bright

I haven't yet produced formal documentation of the element-by-element changes to the XML; I hope to have this finished by tomorrow. Meanwhile, here is a working version of the Bright theme...

Created attachment 68098 [details]
metacity-theme-2 version of Crux

...and here's a metacity-theme-2 version of the Crux theme.

I was afraid this huge patch would be scarier - it looks manageable, barely. ;-)
On a quick scan-through nothing looks egregiously wrong to me.

Docs in the theme format file would be pretty helpful for reviewing it, and of course to anyone trying to make a multiversion theme.

Created attachment 68139 [details] [review]
Changes to documentation

Here's a new section to theme-format.txt, partly based on text Havoc wrote ages ago, explaining in detail all the changes to the XML format.

Sorry for having been slow to help with the review; since it's after feature freeze, we won't be able to get this in until after 2.16.  But let's make sure to get it in right after 2.16.0.  :)

A couple weeks ago, I started a patch review ignoring the fact that I don't know how themes work at all and just concentrating on the parts I did know.  I haven't yet gotten back to it, but let me at least just cut and paste the comments I wrote down at the time (in strict order going through your patch) so I don't lose them.  Overall it looked okay, I mostly just have nitpicks.


starting in frames.c:

+#include "window.h"

As per the technical gotchas in the HACKING file, this cannot be
included here.


-  if (!(fgeom.top_left_corner_rounded ||
-        fgeom.top_right_corner_rounded ||
-        fgeom.bottom_left_corner_rounded ||
-        fgeom.bottom_right_corner_rounded ||
+  if (!(fgeom.top_left_corner_rounded_radius!=0 ||
+        fgeom.top_right_corner_rounded_radius!=0 ||
+        fgeom.bottom_left_corner_rounded_radius!=0 ||
+        fgeom.bottom_right_corner_rounded_radius!=0 ||

spacing missing here.

 
-      xrect.y = 3;
-      xrect.width = 1;
-      xrect.height = 2;
-      XUnionRectWithRegion (&xrect, corners_xregion, corners_xregion);
+      for (i=0; i<radius; i++)
+        {
+          const int width = 1 + (radius - sqrt(radius*radius - (radius-i)*(radius-i)));

shouldn't this be round(sqrt(...))?  Otherwise, it's not going to be
clear what the code is doing to those who can't remember how C
implicitly converts floats to ints.  Same thing goes for other 3 cases.

   
+    case META_FRAME_CONTROL_STICK:
+      tiptext = _("Stick Window On All Workspaces");
+      break;

Not a correct explanation of the feature anymore (created nasty corner
case bugs that would have needed an EWMH change of dubious real value
to fix things to really work that way).  Note that the right click
window menu refers to this as "Always on Visible Workspace"


+        case META_GRAB_OP_CLICKING_ABOVE:
+          if (point_in_control (frames, frame,
+                                META_FRAME_CONTROL_ABOVE,
+                                event->x, event->y))
+            meta_window_make_above (meta_display_lookup_x_window (

meta_window_make_above() should not be called here (part of that
gotcha where window.h can't be included; you have to make another pair
of functions in core.[ch] which forwards the call to window.c like
other function calls in the surrounding code here)


+            meta_window_unmake_above (meta_display_lookup_x_window (

Same here.


-  meta_verbose ("Updating prelit control from %u to %u\n",
-                frame->prelit_control, control);

What's wrong with this debugging message?


-  /* South resize always has priority over north resize,
+   /* South resize always has priority over north resize,
    * in case of overlap.
    */

The old spacing was correct, not the new one.  ;)

 
--- src/iconcache.c	20 Jan 2006 22:03:55 -0000	1.8
+++ src/iconcache.c	28 Jun 2006 02:14:35 -0000
@@ -23,6 +23,8 @@
 #include "iconcache.h"
 #include "ui.h"
 #include "errors.h"
+#include "theme.h"
+#include "window.h"

Given that theme.h includes some gtk/gdk headers, iconcache.c should
not include both it and window.h.


+      if (theme->fallback_icon==NULL || theme->fallback_mini_icon==NULL)
+        {
+          get_fallback_icons (screen,
+                              iconp,
+                              ideal_width,
+                              ideal_height,
+                              mini_iconp,
+                              ideal_mini_width,
+                              ideal_mini_height);
+        }
+
+      if (theme->fallback_icon!=NULL) *iconp = theme->fallback_icon;
+      if (theme->fallback_mini_icon!=NULL) *mini_iconp = theme->fallback_mini_icon;

Style issues: spaces needed around '==' and '!=', then clause on
separate line from if.


+  for (displays = meta_displays_list (); displays != NULL; displays = displays->next) {
+
+    for (windows = meta_display_list_windows (displays->data); windows != NULL; windows = windows->next) {

Would be nice to split these long lines.  Also, braces should be on
separate line and indented over from other code (a similar braces problem was
also found in lines following this snippet).


   long l;
+  int j;
<snip>
+      l = j;

Seems odd to have l and j have different types.  Is there a reason for this?


+  if (META_THEME_ALLOWS(theme, META_THEME_COLOR_CONSTANTS) &&

spacing between META_THEME_ALLOWS and the opening parenthesis.  ;-)


-      if (strchr (value, '.'))
+      if (strchr(value, '.') && parse_double (value, &dval, context, error))

spacing between function name and opening parenthesis.  ;-)

+          info->style->window_background_color = meta_color_spec_new_from_string(background, error);

Again.  ;-)

+               if (!success) return;

if & then clauses should be on separate lines.

Created attachment 71068 [details] [review]
Another try at the metacity-theme-2 patch

Thanks. Here's another try, addressing the issues you found. Sorry it took so long-- I had to hunt down another bug in it.

I've started looking at the patches; doing a bit each day.  I'll try to finish up in a week or so and hopefully we can get the patches in soon.

Since I don't know a thing about how the theme code works, though, it'd be great if Havoc could look over it too.

Comment on attachment 71068 [details] [review]
Another try at the metacity-theme-2 patch

I didn't check theme/theme-parser to closely since I'm not familiar with those parts of the code, but overall the patch looks pretty good to me.  I couldn't find too many issues, just a couple comments:

>+void
>+meta_invalidate_default_icons (void)
>+{
>+  GSList *displays, *windows;
>+
>+  for (displays = meta_displays_list ();
>+      displays != NULL;
>+      displays = displays->next)

unimportant nitpick: poor alignment here; I'd prefer the 'displays' to line up (much how if's & else if's are handled throughout the code).  Same thing in a few other places...

>+          const int width = 1 + (radius - rint(sqrt(radius*radius - (radius-i)*(radius-i))));

I was unfamiliar with rint(), so I looked it up.  I saw a bunch of stuff about implementationally dependent being either being like floor() or ceil() or even rounding to the nearest even integer, but didn't see anything about rounding to the nearest integer.  Do we really want this instead of e.g. round()?

>@@ -1563,6 +1576,60 @@ meta_frames_button_release_event    (Gtk
>           meta_core_end_grab_op (gdk_display, event->time);
>           break;
> 
>+        case META_GRAB_OP_CLICKING_SHADE:
>+          if (control == META_FRAME_CONTROL_SHADE)
>+            meta_core_shade (gdk_display, frame->xwindow);
>+          
>+          redraw_control (frames, frame, META_FRAME_CONTROL_SHADE);

If redraw_control() is called here, shouldn't this be META_FRAME_CONTROL_UNSHADE?  However, I don't think redraw_control() should be needed here since it isn't used for example with maximize/unmaximize (note that the reason it isn't needed there is because of another difference in get_control() which I'll cover later).  Same thing goes for ABOVE/UNABOVE and STICK/UNSTICK in this same area of the patch.

>-  meta_verbose ("Updating prelit control from %u to %u\n",
>-                frame->prelit_control, control);
>-  

What do you have against this debugging message?  ;-)

>@@ -2332,6 +2521,36 @@ get_control (MetaFrames *frames,
>         return META_FRAME_CONTROL_MAXIMIZE;
>     }
>       
>+  if (POINT_IN_RECT (x, y, fgeom.shade_rect.clickable))
>+    {
>+      return META_FRAME_CONTROL_SHADE;
>+    }
>+
>+  if (POINT_IN_RECT (x, y, fgeom.unshade_rect.clickable))
>+    {
>+      return META_FRAME_CONTROL_UNSHADE;
>+    }

This isn't consistent with how MAXIMIZE/UNMAXIMIZE is done; I'd prefer this were changed so that the code in meta_frames_button_release_event() can also be made consistent with MAXIMIZE/UNMAXIMIZE.  Same thing goes for the ABOVE/UNABOVE and STICK/UNSTICK parts of this area of the patch.

Elijah: Before this patch, we've only had one button which toggled,
i.e. maximise/restore. To metacity, these appear to be the same button,
and the only reason that they show up with different images is that
every <frame_style_set> in the theme defines every <frame> twice:
once for non-maximised (with the image on the button being the ordinary
maximise image) and once for maximised (with the image being the
restore image). I didn't implement the three new toggle buttons the
same way, because even if doing two copies of everything isn't an
unreasonable burden on theme authors, writing out each different
<frame> 2**4==16 times with every possible combination of the buttons
might be. So instead I defined the toggle buttons as pairs of
independent buttons which just happen never to be seen at the same time.

(In reply to comment #43)
> Elijah: Before this patch, we've only had one button which toggled...
> So instead I defined the toggle buttons as pairs of
> independent buttons which just happen never to be seen at the same time.

So, does that imply that fgeom.shade_rect.clickable == fgeom.unshade_rect.clickable, always?  If so, I don't see how get_control() could ever return META_FRAME_CONTROL_UNSHADE.  If not, I'm a bit confused by what those rects do contain.

Also, I don't see the need for redraw_control() in meta_frames_button_release_event() at all now.

Yes, it does imply that, and you're right about get_control-- that was a thinko on my part.

I seem to remember adding the calls to redraw_control() to force the button to be redrawn so you could see the toggled state, but I don't remember now: it was a while ago. I'll experiment with and without.

Okay, three results of my experimentation:

1) I used rint() because round() is C99. Someone suggested floor() instead, which is a very good idea.

2) Actually fgeom.shade_rect.clickable gets reset (to some sort of "n/a" value, I'm not sure what) when the shade button isn't visible, and so get_control does need to be able to return META_FRAME_CONTROL_UNSHADE and kin. If you disable this, the button becomes unclickable when it's turned to "unshade".

3) I have tried with and without redraw_control() and it makes no difference. It doesn't solve the problem I was trying to address, which is that if you click the button and don't move the mouse at all, the image doesn't update until you do move it (but the action gets carried out). This problem does still exist, but it's probably trivial enough we can address it separately.

Created attachment 74189 [details] [review]
State of the union

Here's the current state of play as it exists on my hard disk. Is this good to go?

Go go go!  Let's watch them theme bugs drop like flies!!!

Thanks for all your awesome work.

Hurrah! Thank you!

All checked in!