Created attachment 143394 [details] [review]
Use accessor functions instead direct access

GTK+ 2.17.10 is now the required version

I've used all the GTK+ 2.17.11 api available, still missing:

GTK_WIDGET_SET_FLAGS (WIDGET(), GTK_MAPPED);
GTK_WIDGET_REALIZED ()
GTK_WINDOW ()->type;
GTK_MENU_SHELL ()->have_xgrab

Thanks for the patch, it's going to have to wait for a bit, since we're not going to want a GTK+-2.18 dependency at least until it's been released for a couple of months.

Owen, could you have a new look at it ?  this is important as it makes building the proposed modules impossible with the full jhbuild stack.  Thanks.

Created attachment 158438 [details] [review]
Use accessor functions instead direct access.v2

Updated patch against current master.
The patch compiles but is not fully tested.

The GTK+ required version is now 2.20.
Only missing:

GTK_MENU_SHELL ()->have_xgrab

Also, I'm not very sure about this change in src/ui/frames.c:

@@ -592,9 +594,7 @@ meta_frames_attach_style (MetaFrames  *frames,
   if (frame->style != NULL)
     gtk_style_detach (frame->style);
 
-  /* Weirdly, gtk_style_attach() steals a reference count from the style passed in */
-  g_object_ref (GTK_WIDGET (frames)->style);
-  frame->style = gtk_style_attach (GTK_WIDGET (frames)->style, frame->window);
+  gtk_widget_style_attach (GTK_WIDGET (frame));

Please, could you verify that this is correct?

Review of attachment 158438 [details] [review]:

As far as I can tell the more mechanical parts of the changes are OK, but there are some problems.

In terms of the dependency on 2.20, I'd really rather avoid bringing in a 2.20 dependency until Ubuntu 10.04 and F-13 are released (Schedule April 29 and May 18), but if compiling against GTK+-2.90 is an important goal for the release team, then perhaps we can fudge that. Though I really like the fact that it takes just a few minutes to build the shell, we do have a bigger version of our jhbuild that builds the GTK+ stack as well.

::: src/ui/frames.c
@@ -594,4 +596,2 @@
 
-  /* Weirdly, gtk_style_attach() steals a reference count from the style passed in */
-  g_object_ref (GTK_WIDGET (frames)->style);
-  frame->style = gtk_style_attach (GTK_WIDGET (frames)->style, frame->window);
+  gtk_widget_style_attach (GTK_WIDGET (frame));

A quick examination of the original code and of the GTK+ function would show very clearly that this is not the same thing. Looking at 'git blame' output would provide further background for why the code is doing what it does.

The original code should just be changed to use gtk_widget_get_style() and otherwise left as is, but in general please don't just write code that compiles but you have no idea if it is right or not and submit it - I do appreciate that you pointed out that you had no idea if it was right or not, but that doesn't make up for not doing your homework.

::: src/ui/metaaccellabel.c
@@ +269,3 @@
       int ac_width;
 
+      gtk_widget_size_request (widget, &requisition);

size_request() [or get_child_requisition(), which would be more appropriate here] takes gtk_widget_set_size_request() into account, which is not appropriate for the logic here - we want to know how much space is taken up by the main part of the label.

For the purposes of Metacity, this change is probably OK, but needs a comment along the lines of 

 /* This would break if the application called set_size_request(), since
    we want the raw request size. Not a problem for our usage */

I would switch to using get_child_requisition().

Seems like GTK+ does need gtk_widget_get_requisition() to go along with get_child_requisition(). [Or maybe it should be called gtk_widget_get_raw_requisition(), but I think I  like the version without the _raw better.]

@@ -280,2 +288,2 @@
             }
-	  widget->allocation.width -= ac_width;
+	  allocation.width -= ac_width;

No. It's changing the widget's allocation in order to affect the position of the label when chaining up to the parent class. Changing a copy of the allocation is not the same thing.

You can't replace this with gtk_widget_size_allocate() since that invalidates part of the window which is a really bad thing to do in an expose-event - infinite loop city.

What you probably need to do is to paint gtk_label_get_layout() directly instead of chaining up; you can simplify from what gtk_label_expose() does because we don't care about selectable labels or links. [You'll need to use gtk_label_get_layout_offsets() as well]

::: src/ui/tabpopup.c
@@ +712,3 @@
+
+      gtk_widget_get_allocation (widget, &allocation);
+      gtk_widget_size_request (widget, &requisition);

As in the other place, widget->requisition cannot be replaced by size_request() in a widget implementation. If a widget is referring to widget->requisition() internally, it means the size *it requested*. If I have a centered image, and I call gtk_widget_set_size_request(image, 50, 50); the image should be centered - it shouldn't center the 50x50 artificial size I've put onto the image.

Again, this is probably OK with a comment, *in the context* of Mutter/Metacity.

::: src/ui/themewidget.c
@@ +131,3 @@
+      gtk_widget_get_allocation (widget, &allocation);
+      gtk_misc_get_padding (misc, &xpad, &ypad);
+      gtk_widget_size_request (widget, &requisition);

Same comment once again.

Created attachment 158514 [details] [review]
Use accessor functions instead direct access.v3

Hello Owen,

Thank you for all your informative comments.

I've used the recently added gtk_widget_get_requisition() and gtk_widget_set_allocation() functions to fix the previous issues.

Also, replaced the wrong src/ui/frames.c too:

-  /* Weirdly, gtk_style_attach() steals a reference count from the style passed in */
-  g_object_ref (GTK_WIDGET (frames)->style);
-  frame->style = gtk_style_attach (GTK_WIDGET (frames)->style, frame->window);
+  style = gtk_widget_get_style (widget);
+  gtk_widget_set_style (widget,
+                        gtk_style_attach (style, frame->window));

I think the patch is correct now, but please, tell me if something is still wrong.

(In reply to comment #6)
 
> -  /* Weirdly, gtk_style_attach() steals a reference count from the style
> passed in */
> -  g_object_ref (GTK_WIDGET (frames)->style);
> -  frame->style = gtk_style_attach (GTK_WIDGET (frames)->style, frame->window);
> +  style = gtk_widget_get_style (widget);
> +  gtk_widget_set_style (widget,
> +                        gtk_style_attach (style, frame->window));
> 
> I think the patch is correct now, but please, tell me if something is still
> wrong.

No, this is not not right. Please read the above again carefully.

Created attachment 158864 [details] [review]
Use accessor functions instead direct access.v4

OK, to move things forward, here a new patch with all the changes except:

GTK_MENU_SHELL ()->have_xgrab

and 

g_object_ref (GTK_WIDGET (frames)->style);
frame->style = gtk_style_attach (GTK_WIDGET (frames)->style, frame->window);
gtk_widget_style_attach (GTK_WIDGET (frame));

So a more experienced developer can fix this properly

Created attachment 160784 [details] [review]
Use accessor functions instead direct access.

Javier's patch rebased to master.

Created attachment 160785 [details] [review]
Replace left-over accesses of struct members

When replacing direct accesses with accessor functions, two snippets
were left out. Mutter now builds with GSEAL_ENABLE.

Created attachment 160786 [details] [review]
Add compatibility with GTK+ 2.18

To replace all calls to deprecated code, GTK+ 2.20 is required - add
some basic compatibility code, so that it is still possible to build
mutter with GTK+ 2.18 when not using -DGSEAL_ENABLE.

Review of attachment 160784 [details] [review]:

Looks good - some minor observations below, though the patch already is good to go as-is. Note that we do not want to depend on gtk 2.20 yet, so the actual commit should wait until the compatibility patch is reviewed and good to go as well.

::: src/ui/draw-workspace.c
@@ -146,3 @@
-  else
-    color = &widget->style->fg[state];
-  if (is_active)

Boy, I'd like to know the original intend of that  one...

::: src/ui/metaaccellabel.c
@@ +291,3 @@
 	  if (GTK_WIDGET_CLASS (parent_class)->expose_event)
 	    GTK_WIDGET_CLASS (parent_class)->expose_event (widget, event);
 

Do we expect expose_event() to change the widget's allocation? If not, allocation is still valid and needn't be copied again with gtk_widget_get_allocation() - but then, copying a struct with four ints is cheap, so erring on the safe side is probably a good idea.

::: src/ui/themewidget.c
@@ +135,2 @@
       if (gtk_widget_get_direction (widget) == GTK_TEXT_DIR_LTR)
+	temp_xalign = xalign;

You could rewrite this as

if (gtk_widget_get_direction (widget) == GTK_TEXT_DIR_RTL)
  xalign = 1.0 - xalign;

and get rid of temp_xalign, right?

Review of attachment 160784 [details] [review]:

OK, there's a bug which escaped my first review:

::: src/ui/frames.c
@@ +206,3 @@
+                NULL);
+                "type", GTK_WINDOW_POPUP,
+  g_object_set (frames,

GtkWindow->type is contruct-only, so setting it from the object init method does not work. Probably the safest place to put it is a custom constructor - you may want to have a look at bug 608802 ...

Created attachment 160800 [details] [review]
Use accessor functions instead direct access.v2

Thanks for the review, here a updated patch with your comments.

I added a custom constructor like the pointed bug.

Review of attachment 160784 [details] [review]:

::: src/ui/draw-workspace.c
@@ -146,3 @@
-    color = &widget->style->fg[state];
-  else
-    color = &widget->style->fg[state];

Code comes originally from the libwnck patch in:

 https://bugzilla.gnome.org/show_bug.cgi?id=339293

Benjamin Otte wrote there:

 "Then I worked on the colors for windows and background, trying to get nice
  prelight and selection display done There's bug 161542 about this. It's only
  subtle changes, but makes a difference."

I think leaving the is_active there without doing anything was meant to represent "this could conceptually depend on active or not, but I chose to have it be the same for both".

::: src/ui/metaaccellabel.c
@@ +292,2 @@
 	  if (GTK_WIDGET_CLASS (parent_class)->expose_event)
 	    GTK_WIDGET_CLASS (parent_class)->expose_event (widget, event);

There's no reason to get the allocation again here - and getting it again obscures the fact that we are saving the allocation, setting it to something temporary, and restoring it.

Review of attachment 160786 [details] [review]:

A couple of trivial style point about macro definitions, otherwise good.

::: src/gtk-compat.h
@@ +15,3 @@
+#define gtk_widget_get_requisition(w,r) (*r = GTK_WIDGET (w)->requisition)
+#define gtk_widget_set_mapped(w,m) \
+  do { \

Should use 

 G_STMT_START {
   <code>
 } G_STMT_END

@@ +17,3 @@
+  do { \
+    if (m) GTK_WIDGET_SET_FLAGS (w, GTK_MAPPED); \
+    else GTK_WIDGET_UNSET_FLAGS (w, GTK_MAPPED); \

Generally, it's clearest to just indent stuff in a macro like you would otherwise. (though this is just a temporary hack...)

Review of attachment 160785 [details] [review]:

Looks good. Suggestion for a small improvement:

::: src/ui/menu.c
@@ -523,3 +523,3 @@
                   timestamp);
 
-  if (!GTK_MENU_SHELL (menu->menu)->have_xgrab)
+  if (!gdk_pointer_is_grabbed ())

This is a pretty reasonable way to check if the popup succeeded, but there is one case in which it won't work - if the pointer was already grabbed by the Mutter process with a timestamp newer than the timestamp we are using there, then the XGrabPointer() in the popup will fail so, gdk_pointer_is_grabbed() is true (since the pointer was already grabbed by the process), but the menu didn't succeed in grabbing itself.

A slightly better thing to do is:

 if (!gtk_widget_get_visible (menu->menu))

Since the menu will have been set visible if and only if the pointer grab succeeded.

Created attachment 160842 [details] [review]
Replace left-over accesses of struct members

(In reply to comment #16)
> Generally, it's clearest to just indent stuff in a macro like you would
> otherwise. (though this is just a temporary hack...)

Yeah, it's just a little inconvenient when using a keyboard layout where typing backslash feels like using Emacs ;)

Created attachment 160843 [details] [review]
Add compatibility with GTK+ 2.18

Review of attachment 160842 [details] [review]:

Looks good

Review of attachment 160843 [details] [review]:

Looks good

Review of attachment 160800 [details] [review]:

Thanks for the quick update - unfortunately, you didn't get the constructor part right ("not right" as in "you broke the build" ;)

See comments below.

::: src/ui/draw-workspace.c
@@ -146,3 @@
-  else
-    color = &widget->style->fg[state];
-  if (is_active)

Quoting Owen:
I think leaving the is_active there without doing anything was meant to
represent "this could conceptually depend on active or not, but I chose to have
it be the same for both".

It might be a good idea then to keep the condition there

::: src/ui/frames.c
@@ +142,3 @@
+                         guint                  n_properties,
+meta_frames_constructor (GType                  gtype,
+static GObject *

This does not compile: metacity/mutter is pretty old, which is why it doesn't use G_DEFINE_TYPE here - so meta_frames_parent_class is not defined.

The variable you are looking for is parent_class ...

On a side note: After the GSEAL work, there'll probably quite some stuff to do to allow building with GTK_DISABLE_DEPRECATED (I only skimmed this file, and it's not pretty ...)

::: src/ui/metaaccellabel.c
@@ +294,2 @@
 	    GTK_WIDGET_CLASS (parent_class)->expose_event (widget, event);
+          gtk_widget_get_allocation (widget, &allocation);

Again quoting Owen:
There's no reason to get the allocation again here - and getting it again
obscures the fact that we are saving the allocation, setting it to something
temporary, and restoring it.

Created attachment 160883 [details] [review]
Use accessor functions instead direct access.v3

Thanks again for the quickly review.

Here a new patch with your comments

(In reply to comment #22)
> On a side note: After the GSEAL work, there'll probably quite some stuff to do
> to allow building with GTK_DISABLE_DEPRECATED (I only skimmed this file, and
> it's not pretty ...)

There already is a bug to remove deprecated symbols:  bug #587991

Review of attachment 160883 [details] [review]:

One comment left, then it should be good to go (I'll double-check with Owen later)

::: src/ui/draw-workspace.c
@@ +147,2 @@
     color = &widget->style->fg[state];
   if (is_active)

Sorry for the confusion - of course, the widget->style still has to be adjusted (just like you did before):

if (is_active)
  color = &style->fg[state];
else
  color = &style->fg[state];

Attachment 160842 [details] pushed as 5526e91 - Replace left-over accesses of struct members
Attachment 160843 [details] pushed as c6c7b05 - Add compatibility with GTK+ 2.18
Attachment 160833 [details] pushed as 9e12369 - Use accessor functions instead direct access. (with the addition pointed out in the comment above)