AppMenu GTK+

Merge lp://qastaging/~hasselmm/appmenu-gtk/gtk3 into lp://qastaging/appmenu-gtk/0.4

gtk3
Merge into trunk

Proposed by Mathias Hasselmann on 2011-05-08

Status:	Merged
Approved by:	Michael Terry on 2011-05-12
Approved revision:	140
Merged at revision:	135
Proposed branch:	lp://qastaging/~hasselmm/appmenu-gtk/gtk3
Merge into:	lp://qastaging/appmenu-gtk/0.4
Diff against target:	154 lines (+43/-17) 5 files modified 80appmenu.in (+1/-1) Makefile.am (+5/-5) configure.ac (+27/-6) src/Makefile.am (+4/-5) src/bridge.c (+6/-0)
To merge this branch:	bzr merge lp://qastaging/~hasselmm/appmenu-gtk/gtk3
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Michael Terry	2011-05-08	Approve on 2011-05-12
Mathias Hasselmann (community)		Needs Resubmitting on 2011-05-11
Review via email: mp+60326@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2011-05-08.

Description of the change

This permits building of a gtk3 version of appmenu-gtk.

lp://qastaging/~hasselmm/appmenu-gtk/gtk3 updated on 2011-05-08

137. By Mathias Hasselmann on 2011-05-08: Generate proper X session script for both gtk flavors

Revision history for this message

Mathias Hasselmann (hasselmm) wrote on 2011-05-08:

Revision history for this message

Mathias Hasselmann (hasselmm) wrote on 2011-05-10:

fixes this bug: https://bugs.launchpad.net/appmenu-gtk/+bug/701295

Revision history for this message

Michael Terry (mterry) wrote on 2011-05-11:

Looks great. I have two comments:

1) Do we really need --with-gtk3? Seems like --with-gtk2 is enough.

2) You need to add the following in src/bridge.c:

+#if GTK_CHECK_VERSION(3, 0, 0)
+#include <libdbusmenu-gtk3/menuitem.h>
+#include <libdbusmenu-gtk3/parser.h>
+#else
#include <libdbusmenu-gtk/menuitem.h>
#include <libdbusmenu-gtk/parser.h>
+#endif

Unfortunately (and this is my fault), the headers use different directories for gtk2/gtk3.

review: Needs Fixing

lp://qastaging/~hasselmm/appmenu-gtk/gtk3 updated on 2011-05-11

138. By Mathias Hasselmann on 2011-05-11: Include proper appmenu-gtk headers.
139. By Mathias Hasselmann on 2011-05-11: Drop somewhat redundant --with-gtk3 switch.
140. By Mathias Hasselmann on 2011-05-11: Bump package version

Revision history for this message

Mathias Hasselmann (hasselmm) wrote on 2011-05-11:

> 1) Do we really need --with-gtk3? Seems like --with-gtk2 is enough.

Dropped.

> 2) You need to add the following in src/bridge.c:

Fixed.

> Unfortunately (and this is my fault), the headers use different directories
> for gtk2/gtk3.

Maybe still can be fixed somehow? The files in that folders seem to be identical.

Revision history for this message

Mathias Hasselmann (hasselmm) on 2011-05-11:

review: Needs Resubmitting

Revision history for this message

Michael Terry (mterry) wrote on 2011-05-12:

Looks great! Thanks!

> Maybe still can be fixed somehow? The files in that folders seem to be identical.

A) Can't guarantee that there will never be differences in gtk2/gtk3 versions, though seems more on the 0.1% chance side.

B) We need both libdbusmenu-gtk3-dev and libdbusmenu-gtk-dev to ship the headers so that a package can depend on either one and not get both. So we have to ship two copies of them anyway (or split the headers into some -dev-common package).

C) The normal way of dealing with it is like so:

#include <libdbusmenu-gtk/header.h>
/usr/include/dbusmenu/libdbusmenu-gtk/header.h
/usr/include/dbusmenu3/libdbusmenu-gtk/header.h
And have the .pc file specify which directory to -I

But with libdbusmenu, it sticks libdbusmenu-glib in the same directory with libdbusmenu-gtk, so we either had to duplicate the -glib headers or stick gtk3 headers in the same directory too. I went with the latter, but I'm not convinced it was optimal. :)

review: Approve