After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 618258 - Use accessor functions instead of direct access
Use accessor functions instead of direct access
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 585391
 
 
Reported: 2010-05-10 12:22 UTC by Florian Müllner
Modified: 2010-05-10 17:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use accessor functions instead of direct access (21.38 KB, patch)
2010-05-10 12:22 UTC, Florian Müllner
needs-work Details | Review
Use accessor functions instead of direct access (18.56 KB, patch)
2010-05-10 15:48 UTC, Florian Müllner
committed Details | Review
Add compatibility with GTK+ 2.18 (3.95 KB, patch)
2010-05-10 15:49 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2010-05-10 12:22:14 UTC
GTK+ master has merged the 2.90 branch, so it seems like a good time to support building shell with -DGSEAL_ENABLE.
Comment 1 Florian Müllner 2010-05-10 12:22:18 UTC
Created attachment 160714 [details] [review]
Use accessor functions instead of direct access

With the transition to GTK+ 3.0, direct access to struct members
will no longer be possible.
To replace all calls to deprecated code, GTK+ 2.20 is required - due
to some basic compatibility code, it is still possible to build the
shell with GTK+ 2.18 when not using -DGSEAL_ENABLE.
Comment 2 Florian Müllner 2010-05-10 12:33:31 UTC
Most changes should be pretty trivial, I'd be interested on input on the following though:

1. Compatibility with GTK+ 2.18:
   GTK+ 2.20 is still pretty new, so a straight dependency is probably not
   a good idea - I think the solution in this patch is pretty straightforward
   (and easy to remove when bumping the GTK+ requirement), nevertheless some
   comments would be nice.
2. Change in shell_embedded_window_show():
   I'm pretty sure I removed an optimization there, which is bad. I doubt it
   can be avoided, but I'd be glad to be proven wrong on that.

(Completely unrelated: it would be awfully cool if git-bz edit supported adding
 blocks/depends)
Comment 3 Owen Taylor 2010-05-10 15:00:06 UTC
Review of attachment 160714 [details] [review]:

I think the way you handled 2.18 here is fine. I think we are going to have to bump to a GTK+-2.90 dependency quite soon, but until that happens I don't want to introduce a 2.20 dependency just for accessors.

::: src/gtk-compat.h
@@ +1,2 @@
+#ifndef __GTK_COMPAT_H__
+#define __GTK_COMPAT_H__

A comment here about the purpose of this file would be good.

@@ +6,3 @@
+#if !GTK_CHECK_VERSION(2, 20, 0)
+
+#define gtk_widget_get_realized(w) GTK_WIDGET_REALIZED((w))

Double parens here are unnecessary

::: src/shell-embedded-window.c
@@ +64,3 @@
     }
+
+  GTK_WIDGET_CLASS (shell_embedded_window_parent_class)->show (widget);

Not happy with this. This will mean round trips to the X server waiting for ConfigureNotify events to arrive (GtkWindow properly doesn't trust that gdk_window_resize() on a toplevel will be honored by the window manager), and a whole lot of complexity.

Since our current hack was taken away from us, we can add another. If we look at 
gtk_widget_real_show(), it is:

static void
gtk_widget_real_show (GtkWidget *widget)
{
  if (!gtk_widget_get_visible (widget))
    {
      GTK_WIDGET_SET_FLAGS (widget, GTK_VISIBLE);

      if (widget->parent &&
          gtk_widget_get_mapped (widget->parent) &&
          GTK_WIDGET_CHILD_VISIBLE (widget) &&
          !gtk_widget_get_mapped (widget))
        gtk_widget_map (widget);
    }
}

So, we can replace GTK_WIDGET_SET_FLAGS(widget, GTK_VISIBLE)

with:

 GtkWidgetClass *widget_class;

 /* Skip GtkWindow, but run the default GtkWidget handling which
  * marks the widget visible */
 widget_class = g_type_class_peek (GTK_TYPE_WIDGET);
 widget_class->show (widget);

Note that widget->parent is NULL for us (as I recall), so we need to do the widget_map() as we did in the old code - this just replaces the SET_FLAGS().
Comment 4 Owen Taylor 2010-05-10 15:03:20 UTC
Also, if you want to pick up jjardon's Mutter patch (bug 595496), review the latest version, and do the same thing there as to 2.18 compatibility, that would be useful.

(What might make sense for committing it would be to commit it with the 2.20 dep, then do an immediate followup patch that adds back 2.18 compatibility.)
Comment 5 Florian Müllner 2010-05-10 15:48:56 UTC
Created attachment 160733 [details] [review]
Use accessor functions instead of direct access

With the transition to GTK+ 3.0, direct access to struct members
will no longer be possible.
This bumps the required minimum version of GTK+ to 2.20.

Adress Owen's comment about shell_embedded_window_show() and fix some warnings in na-tray-child.c
Comment 6 Florian Müllner 2010-05-10 15:49:28 UTC
Created attachment 160734 [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 is still possible to build the
shell with GTK+ 2.18 when not using -DGSEAL_ENABLE.

Split out from the previous patch version with Owen's comments adressed (hopefully)
Comment 7 Owen Taylor 2010-05-10 15:58:21 UTC
Review of attachment 160733 [details] [review]:

looks good except for one tiny thing noted below.

My comments about separate commits were actually about the Mutter patch, not this one, up to you whether you commit this one split or squashed.

::: src/tray/na-tray-child.c
@@ +388,2 @@
   child->composited = composited;
+  if ((window = gtk_widget_get_window (GTK_WIDGET (child))))

We avoid this as a style thing - just put the assignment on a separate line.
Comment 8 Owen Taylor 2010-05-10 15:58:44 UTC
Review of attachment 160734 [details] [review]:

Looks good (though can be squashed, see other comment.)
Comment 9 Florian Müllner 2010-05-10 16:22:09 UTC
(In reply to comment #8)
> Looks good (though can be squashed, see other comment.)

Mmmh, when we bump the requirement to post 2.18, the split will allow us to remove the compatibility stuff with a simple git revert - or do you expect that code to stick around "forever"?
Comment 10 Owen Taylor 2010-05-10 16:52:32 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Looks good (though can be squashed, see other comment.)
> 
> Mmmh, when we bump the requirement to post 2.18, the split will allow us to
> remove the compatibility stuff with a simple git revert - or do you expect that
> code to stick around "forever"?

Yes, that's a reasonable reason to keep the split.

Right now I'm thinking that we'll go for a hard 2.9x requirement within a few months, so hopefully there won't be intervening commits that interfere.
Comment 11 Florian Müllner 2010-05-10 17:11:29 UTC
Attachment 160733 [details] pushed as 8c5bb86 - Use accessor functions instead of direct access
Attachment 160734 [details] pushed as 6318c8e - Add compatibility with GTK+ 2.18