Created attachment 163885 [details] [review]
[gnome-shell.modules] Fix gtk+ dep, add librsvg

Comment on attachment 163885 [details] [review]
[gnome-shell.modules] Fix gtk+ dep, add librsvg

add a comment please. (I assume we need librsvg because of sealing?)

comment as in "commit message"

Created attachment 163955 [details] [review]
Switch over to building full stack

We now start building the gtk-2-20 stack, along with
librsvg (since otherwise we won't have an SVG loader).

Created attachment 164237 [details] [review]
Switch build to Gtk+-3.0

Update the build dependencies to gtk+-3.0 and adjust the javascript
to run on gtk+-3. Obviously depends on mutter compiled with gtk+-3
as well.

Note that the patch does not modify the moduleset; there is a dependency on bug 622303 and the open question of Clutter's GdkPixbuf-2.0 dependency.

Created attachment 164653 [details] [review]
Switch build to Gtk+-3.0

Rebased on master.

in src/Makefile.am the --include=GdkPixbuf-3.0 line should be reverted back to GdkPixbuf-2.0 to reflect the separation of the GdkPixbuf from the Gtk+ source tree.

Created attachment 164830 [details] [review]
Replace deprecated GDK symbols

The fix depends on API introduced in this cycle, so the required
GTK+ version is bumped to 2.21.1.

No idea how these managed to escape before ...

Created attachment 164831 [details] [review]
Switch build to Gtk+-3.0

(In reply to comment #7)
> in src/Makefile.am the --include=GdkPixbuf-3.0 line should be reverted back to
> GdkPixbuf-2.0 to reflect the separation of the GdkPixbuf from the Gtk+ source
> tree.

Thanks - I really thought I had updated the patch here already. Anyway, attached patch reverts back the GdkPixbuf version in src/Makefile.am and js/ui/main.js.

Review of attachment 164830 [details] [review]:

Patch looks good.

[ gdk_visual_get_red_pixel_details (visual, NULL, NULL, &red_prec);

 that's an ugly api! ]

Review of attachment 164831 [details] [review]:

::: js/ui/environment.js
@@ -94,3 @@
-                 'gjs\'s handling of Gdk.ModifierType is broken. See bug 597292.');
-    _blockMethod('Gdk.Window.get_pointer', 'global.get_pointer',
-                 'gjs\'s handling of Gdk.ModifierType is broken. See bug 597292.');

Why are you removing these blocks? I don't think anything has changed here with the way that GJS handles enums.

Created attachment 164917 [details] [review]
Switch build to Gtk+-3.0

(In reply to comment #11)
> Why are you removing these blocks? I don't think anything has changed here with
> the way that GJS handles enums.

I was under the false impression that those methods had been removed (unconditionally). _blockMethod does a sanity check for the method to exist and will throw an exception if it does not - updated the patch to catch this error instead of removing the calls.

Review of attachment 164917 [details] [review]:

A typelib compiled with -DGDK_MULTIDEVICE_SAFE is a miscompiled typelib. 

That flag is a way of testing application code to see if it's "safe" to use with multiple pointers being driven at once, but cannot affect the GTK+ API/ABI. 

If the typelib is getting compiled with -DGDK_MULTIDEVICE_SAFE, then GTK+ needs to be fixed; we shouldn't be working around it.

Review of attachment 163955 [details] [review]:

OK, I think we are ready to move on this:

 * I want to switch to GTK+ 3
   
   - Because that's what GNOME is doing for core modules
   - Because it slightly reduces the affect on launched apps if launched from within jhbuild - if they are using GTK+-2.0, they'll still pick up the system theme and input method modules. 
     (Colin points out we could add LD_LIBRARY_PATH stripping logic, but that gets complex in other ways.)

   More for the first

 * Along with librsvg as added here, we need to build:

    pixman, cairo, gdk-pixbuf, gtk-engines

* The patch should get rid of all references to gir-repository, which it doesn't currently

* If we keep the <after> vs. <dep> distinction, we need to make sure it is meaningful. I'm not sure in what would make it meaningful. In the future, we might want to restore a full vs. non-full distinction, but I'm not sure we can predict which modules fall into what set. So I'd prefer to see all the <after/> just switched to <dep/> for now and we can add a split setup with <after/> later.

(In reply to comment #14)
>  * Along with librsvg as added here, we need to build:
> 
>     pixman, cairo, gdk-pixbuf, gtk-engines

i thought with the new gdk-pixbuf we don't need master librsvg any more?

(In reply to comment #15)
> (In reply to comment #14)
> >  * Along with librsvg as added here, we need to build:
> > 
> >     pixman, cairo, gdk-pixbuf, gtk-engines
> 
> i thought with the new gdk-pixbuf we don't need master librsvg any more?

If we didn't build gdk-pixbuf we wouldn't need to build librsvg. And it would be possible to build GTK+ 3 using the system gdk-pixbuf if we weren't building with introspection. But we need to build gdk-pixbuf to build the GIR, since Gtk-2.0.gir depends on that, so we need to build librsvg to get the loader installed into jhbuild.

(clarified on IRC: the problem isn't pixbuf-loader ABI, it's pixbuf-loader install prefix)

(In reply to comment #13)
> A typelib compiled with -DGDK_MULTIDEVICE_SAFE is a miscompiled typelib. 
> 
> If the typelib is getting compiled with -DGDK_MULTIDEVICE_SAFE, then GTK+ needs
> to be fixed; we shouldn't be working around it.

Results that GTK+ is innocent - both methods are included in the typelib, but marked as deprecated, so they get filtered out by Gjs. This is the log output after adding "JS G OBJ" to the log topics:

  JS G OBJ: Defined class Display prototype 0x8722480 class 0x87625e0 in object 0x86d2880
  JS G OBJ: Ignoring definition of deprecated method get_pointer in prototype for GdkDisplay (Gdk.Display)
  JS ERROR: !!!   Exception was: Error: Bad method name "Gdk.Display.get_pointer"
  JS ERROR: !!!     lineNumber = '43'
  JS ERROR: !!!     fileName = '/home/florian/src/gnome-shell/js/ui/environment.js'

So I'd say we got those possible solutions:
(1) Modify Gjs to not filter deprecated methods
    (or make filtering optional)
(2) Modify _blockMethod to handle methods missing
    due to deprecation
(3) Don't call _blockMethod for the deprecated
    methods

I won't deny that I fancy (3) ... :-)

Created attachment 165128 [details] [review]
Use MetaRectangle for allocated returns

The conversion of GdkRectangle to a typedef for cairo_rectangle_int_t
in GTK+-3 makes it no longer a proper boxed type (it's still registered
boxed, but gobject-introspection doesn't know that.) So, switch to using
MetaRectangle, which is now registered as a boxed type by Mutter.

Created attachment 165321 [details] [review]
Build the full GTK+ 3.0 stack

Here's a patch that combines the "full stack" patch, with the correct full
stack for GTK+ 3.0.

Review of attachment 165321 [details] [review]:

::: tools/build/gnome-shell-build-setup.sh
@@ +106,3 @@
     libxml2-devel gstreamer-devel gstreamer-plugins-base gstreamer-plugins-good \
     python-devel pygobject2 readline-devel xulrunner-devel libXdamage-devel libcroco-devel \
+    glx-utils startup-notification-devel xorg-x11-server-Xephyr gnome-terminal zenity \

For Fedora, just:

yum-builddep gobject-introspection gjs gtk2 mutter gnome-shell

Is probably good enough.  (Similar would apply to Ubuntu but I'm less confident suggesting a change)

::: tools/build/gnome-shell.modules
@@ +86,3 @@
   </autotools>
+    <branch checkoutdir="gtk+-3" repo="git.gnome.org" module="gtk+"/>
+  <autotools id="gtk+-3">

Can we go with "gtk3" instead of the unwieldy "gtk+-3"?  Might as well use this opportunity to kill the "+" character.

(In reply to comment #21)
> Review of attachment 165321 [details] [review]:
> 
> ::: tools/build/gnome-shell-build-setup.sh
> @@ +106,3 @@
>      libxml2-devel gstreamer-devel gstreamer-plugins-base
> gstreamer-plugins-good \
>      python-devel pygobject2 readline-devel xulrunner-devel libXdamage-devel
> libcroco-devel \
> +    glx-utils startup-notification-devel xorg-x11-server-Xephyr gnome-terminal
> zenity \
> 
> For Fedora, just:
> 
> yum-builddep gobject-introspection gjs gtk2 mutter gnome-shell
> 
> Is probably good enough.  (Similar would apply to Ubuntu but I'm less confident
> suggesting a change)

A) We're using PackageKit not yum for installation
B) I don't like relying on GTK+-2.0 builddeps when building GTK+-3.0. and we can't yum-builddep gtk3
 
> ::: tools/build/gnome-shell.modules
> @@ +86,3 @@
>    </autotools>
> +    <branch checkoutdir="gtk+-3" repo="git.gnome.org" module="gtk+"/>
> +  <autotools id="gtk+-3">
> 
> Can we go with "gtk3" instead of the unwieldy "gtk+-3"?  Might as well use this
> opportunity to kill the "+" character.

Wanted to stay consistent with standard jhbuild moduleset. If you want to submit a change there (Note there's a lot of -3 modules in it, so gtk+-3 is probably chosen to be consistent with say, gtk-engines-3), I'm happy to follow.

Review of attachment 164917 [details] [review]:

::: js/ui/environment.js
@@ +99,3 @@
+        // Both Gdk.Display.get_pointer() and Gdk.Window.get_pointer()
+        // are undefined if the typelib has been compiled without
+        // -DGDK_MULTIDEVICE_SAVE - _blockMethod will throw an exception,

SAFE not SAFE. BUt actually, that's not the correct comment, the correct comment is:

 // Gdk.Display.get_pointer(), Gdk.Window.get_pointer() are deprecated
 // in GTK+-3.0, so are stripped out of the interface by GJS

Or something like that.

(In reply to comment #23)
> SAFE not SAFE. 

Woops.


> BUt actually, that's not the correct comment, the correct
> comment is:
> 
>  // Gdk.Display.get_pointer(), Gdk.Window.get_pointer() are deprecated
>  // in GTK+-3.0, so are stripped out of the interface by GJS
> 
> Or something like that.

Yeah, I noticed - see comment 18. As we are gonna switch to 3.0 unconditionally, I'd say removing the _blockMethod() calls entirely is safe (not save ;-) )

(In reply to comment #24)
> (In reply to comment #23)
> > SAFE not SAFE. 
> 
> Woops.
> 
> 
> > BUt actually, that's not the correct comment, the correct
> > comment is:
> > 
> >  // Gdk.Display.get_pointer(), Gdk.Window.get_pointer() are deprecated
> >  // in GTK+-3.0, so are stripped out of the interface by GJS
> > 
> > Or something like that.
> 
> Yeah, I noticed - see comment 18. As we are gonna switch to 3.0
> unconditionally, I'd say removing the _blockMethod() calls entirely is safe
> (not save ;-) )

But the problem hasn't gone away - it's just been transfered to the new inconvenient alternatives:

GdkWindow *
gdk_window_get_device_position (GdkWindow       *window,
                                GdkDevice       *device,
                                gint            *x,
                                gint            *y,
                                GdkModifierType *mask);


void
gdk_display_get_device_state (GdkDisplay            *display,
                              GdkDevice             *device,
                              GdkScreen            **screen,
                              gint                  *x,
                              gint                  *y,
                              GdkModifierType       *mask);

so, if anything, we should simply change which methods we block.

Created attachment 165425 [details] [review]
Switch build to Gtk+-3.0

(In reply to comment #25)
> But the problem hasn't gone away - it's just been transfered to the new
> inconvenient alternatives:
> [...]
> so, if anything, we should simply change which methods we block.

OK.

Attachment 164830 [details] pushed as a45021b - Replace deprecated GDK symbols
Attachment 165128 [details] pushed as ab0d57d - Use MetaRectangle for allocated returns
Attachment 165321 [details] pushed as e48d119 - Build the full GTK+ 3.0 stack
Attachment 165425 [details] pushed as 643c7fd - Switch build to Gtk+-3.0