jjk-jacky
commented
9 years ago On 06/23/15 07:13, Vladimir wrote:
Adds two new functions:
status_notifier_set_context_menu status_notifier_get_context_menu
Look good, thanks. Although to be consistent the context menu should be exposed as a property (e.g. "menu") on the StatusNotifier GObject as well.
And I think it would be good in the example if, when dbusmenu support was enabled, instead of always using it this was based on an option. So running the example would still have it handle ContextMenu() and popup the menu itself, but when ran with e.g. --dbus-menu then it would simply set the menu property and let the Host display it when/as needed. That way it's easy for one to test/see differences between the two.
new ./configure options: --enable-dbusmenu --with-dbusmenu-gtk-version=[2,3]
the only problem, i don't know how to make this functions optional in documentation.
I don't think this is a problem, let's just have them always in there, and simply add a note in each one stating that they will only be available if StatusNotifier was compiled with dbusmenu support.
(Besides, I usually generate the html doc myself and include it inside the tarball, so it's probably best that way.)
Means the functions need to be added in docs/reference/statusnotifier-sections.txt though.
Some other (mostly minor) little things below.
---> configure.ac | 53 +++++++++++++++++++++++++++ example/sn-example.c | 28 +++++++++++--- src/interfaces.h | 3 ++ src/statusnotifier.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/statusnotifier.h | 10 +++++ 5 files changed, 190 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac index fbcc75c..2afa4f4 100644 --- a/configure.ac +++ b/configure.ac @@ -45,6 +45,14 @@ AC_ARG_ENABLE([warning-flags], AS_HELP_STRING([--enable-warning-flags], [enable extra compiler warning flags]), [warningflags=$enableval], [warningflags=no])
+AC_ARG_ENABLE([dbusmenu],
- AS_HELP_STRING([--enable-dbusmenu], [enable extra dbusmenu functionality via libdbusmenu]),
- [dbusmenu=$enableval], [dbusmenu=no]) + +AC_ARG_WITH([dbusmenu-gtk-version],
- AS_HELP_STRING([--with-dbusmenu-gtk-version], [Select libdbusmenu-gtk version to be built against if dbusmenu support enabled. Possible values: 2,3. Default: 3 ]),
- [dbusmenugtkversion=$enableval], [dbusmenugtkversion=3]) +
Checks for libraries.
PKG_CHECK_MODULES(GOBJECT, [gobject-2.0], , AC_MSG_ERROR([GLib/GObject is required])) PKG_CHECK_MODULES(GIO, [gio-2.0], , AC_MSG_ERROR([GLib/GIO is required])) @@ -137,6 +145,49 @@ else AC_MSG_RESULT([no]) fi
+if test "x$dbusmenu" = "xyes"; then
- if test "x$dbusmenugtkversion" = "x2"; then
- AC_MSG_NOTICE([dbusmenu support will be built against dbusmenu-gtk (gtk2)])
- elif test "x$dbusmenugtkversion" = "x3"; then
- AC_MSG_NOTICE([dbusmenu support will be built against dbusmenu-gtk3 (gtk3)])
- else
- dbusmenugtkversion=3;
- AC_MSG_NOTICE([dbusmenu support will be built against dbusmenu-gtk3 (gtk3)])
- fi +
- if test "x$dbusmenugtkversion" = "x2"; then
- PKG_CHECK_MODULES(DBUSMENU, [dbusmenu-glib-0.4 dbusmenu-gtk-0.4], [
- DEP_PACKAGES="${DEP_PACKAGES} dbusmenu-glib-0.4 dbusmenu-gtk-0.4"
- ], [
- AC_MSG_WARN([dbusmenu-glib and dbusmenu-gtk are required for dbusmenu support])
- dbusmenu=no
Should be an error, not a warning. If a dependency if missing, as is the case here, configure should stop & fail.
- ])
- elif test "x$dbusmenugtkversion" = "x3"; then
- PKG_CHECK_MODULES(DBUSMENU, [dbusmenu-glib-0.4 dbusmenu-gtk3-0.4], [
- DEP_PACKAGES="${DEP_PACKAGES} dbusmenu-glib-0.4 dbusmenu-gtk3-0.4"
- ], [
- AC_MSG_WARN([dbusmenu-glib and dbusmenu-gtk3 are required for dbusmenu support])
- dbusmenu=no
Same here.
- ])
- fi +fi + +AC_MSG_CHECKING([for dbusmenu support]) +if test "x$dbusmenu" = "xyes"; then
- AC_MSG_RESULT([yes])
- DEP_CFLAGS="$DEP_CFLAGS $DBUSMENU_CFLAGS"
- DEP_LIBS="$DEP_LIBS $DBUSMENU_LIBS"
- AC_SUBST(DEP_PACKAGES)
- AC_SUBST(DEP_CFLAGS)
- AC_SUBST(DEP_LIBS)
- AC_DEFINE([USE_DBUSMENU], , [Use dbusmenu])
- dbusmenu="will be built against GTK ${dbusmenugtkversion}" +else
- AC_MSG_RESULT([no])
dbusmenu=no +fi +AM_CONDITIONAL(USE_DBUSMENU, test "x$dbusmenu" != "xno") + AC_CONFIG_FILES([Makefile statusnotifier.pc example/Makefile docs/reference/Makefile docs/reference/version.xml]) AC_OUTPUT @@ -150,9 +201,11 @@ echo "
build html documentation : ${enable_gtk_doc} example : ${enable_example}
dbusmenu : ${dbusmenu}
Install paths: binaries : $(eval echo $(eval echo ${bindir}))
- libraries : $(eval echo $(eval echo ${libdir})) documentation : $(eval echo $(eval echo ${docdir})) man pages : $(eval echo $(eval echo ${mandir})) " diff --git a/example/sn-example.c b/example/sn-example.c index e0c0b97..69ff264 100644 --- a/example/sn-example.c +++ b/example/sn-example.c @@ -20,6 +20,8 @@
- statusnotifier. If not, see http://www.gnu.org/licenses/ */
+#include "config.h" +
include <gtk/gtk.h>
include
include
@@ -56,14 +58,18 @@ set_status (GObject item, StatusNotifier sn) status_notifier_set_status (sn, GPOINTER_TO_UINT (g_object_get_data (item, "sn-status"))); }
-static gboolean -sn_menu (StatusNotifier sn, gint x, gint y, GMainLoop loop) +GtkMenu * get_menu(StatusNotifier sn, GMainLoop loop)
For coding style consistency, please no space between the * and the function name, but add one between the function name & parenthesis: GtkMenu get_menu (StatusNotifier sn, GMainLoop *loop)
{
- GtkMenu *menu;
- static GtkMenu *menu = 0;
menu = NULL
GtkMenu *submenu; GtkWidget *item; guint i; StatusNotifierStatus status;+
- if (menu)
- {
- return menu;
- }
Drop the brackets.
menu = (GtkMenu *) gtk_menu_new (); submenu = (GtkMenu *) gtk_menu_new ();@@ -112,7 +118,14 @@ sn_menu (StatusNotifier sn, gint x, gint y, GMainLoop loop) ++i;
g_object_ref_sink (menu);
- g_signal_connect_swapped (menu, "hide", (GCallback) g_object_unref, menu);
- return menu; +} + +static gboolean +sn_menu (StatusNotifier sn, gint x, gint y, GMainLoop loop) +{
- GtkMenu *menu = get_menu(sn, loop);
Again, space before the parenthesis.
Also, it seems you've dropped the signal_connect() to "hide" but I don't think that was intended, it should be back here before gtk_menu_popup().
+ gtk_menu_popup (menu, NULL, NULL, NULL, NULL, 0, gtk_get_current_event_time ()); return TRUE; } @@ -343,8 +356,13 @@ main (gint argc, gchar *argv[]) if (cfg.tooltip_body) status_notifier_set_tooltip_body (sn, cfg.tooltip_body);
- g_signal_connect (sn, "registration-failed", (GCallback) sn_reg_failed, loop); +#ifdef USE_DBUSMENU
- status_notifier_set_context_menu(sn, GTK_WIDGET(get_menu(sn, loop)));
As mentioned before, would be nice to make things based on a runtime option here. Also, spaces before parenthesis; And for consistency sake, a simple typecast instead: status_notifier_set_context_menu (sn, (GtkWidget *) get_menu (sn, loop));
Lastly, set_context_menu() does add its own ref, so unless I'm missing something we have a leak here. I wonder if it might not be better to make set_context_menu() call g_object_ref_sink() on the menu instead of g_object_ref(), and remove the g_object_ref_sink() call in get_menu().
That way here we would not need to do anything, which looks like a typical use case; and we'd have to add a g_object_ref_sink() in sn_menu()
+#else g_signal_connect (sn, "context-menu", (GCallback) sn_menu, loop); +#endif +
- g_signal_connect (sn, "registration-failed", (GCallback) sn_reg_failed, loop); g_signal_connect_swapped (sn, "activate", (GCallback) sn_activate, loop); status_notifier_register (sn); g_main_loop_run (loop); diff --git a/src/interfaces.h b/src/interfaces.h index 507bfa7..d260dc5 100644 --- a/src/interfaces.h +++ b/src/interfaces.h @@ -62,6 +62,9 @@ static const gchar item_xml[] = " <property name='AttentionIconPixmap' type='(iiay)' access='read' />" " <property name='AttentionMovieName' type='s' access='read' />" "
" +#ifdef USE_DBUSMENU - "
" +#endif " " " " " " diff --git a/src/statusnotifier.c b/src/statusnotifier.c index e258ed1..0cff9c8 100644 --- a/src/statusnotifier.c +++ b/src/statusnotifier.c @@ -29,6 +29,12 @@ include "interfaces.h"
include "closures.h"
+#ifdef USE_DBUSMENU +#include <libdbusmenu-glib/menuitem.h> +#include <libdbusmenu-glib/server.h> +#include <libdbusmenu-gtk/parser.h> +#endif + /**
- SECTION:statusnotifier
@Short_description: A StatusNotifierItem as per KDE's specifications @@ -146,6 +152,10 @@ struct _StatusNotifierPrivate guint dbus_owner_id; guint dbus_reg_id; GDBusProxy *dbus_proxy; +#ifdef USE_DBUSMENU
- DbusmenuServer *menuservice;
- GtkWidget menu; +#endif GDBusConnection dbus_conn; GError dbus_err; }; @@ -797,6 +807,17 @@ dbus_free (StatusNotifier sn) g_object_unref (priv->dbus_proxy); priv->dbus_proxy = NULL; } +#ifdef USE_DBUSMENU
- if (priv->menu)
- {
- g_object_unref(priv->menu);
- priv->menu = NULL;
- }
- if (priv->menuservice) {
- g_object_unref (priv->menuservice);
- priv->menuservice = NULL;
} +#endif if (priv->dbus_reg_id > 0) { g_dbus_connection_unregister_object (priv->dbus_conn, priv->dbus_reg_id); @@ -1621,6 +1642,21 @@ get_prop (GDBusConnection *conn,
return variant; } +#ifdef USE_DBUSMENU
- else if (!g_strcmp0 (property, "Menu"))
- {
- if (priv->menuservice != NULL) {
- GValue strval = { 0 };
- g_value_init(&strval, G_TYPE_STRING);
- g_object_get_property (G_OBJECT (priv->menuservice), DBUSMENU_SERVER_PROP_DBUS_OBJECT, &strval);
- GVariant * var = g_variant_new("o", g_value_get_string(&strval));
Please put all declarations on top of the block.
- g_value_unset(&strval);
- return var;
- } else {
- return g_variant_new("o", "/NO_DBUSMENU");
- }
} +#endif
g_return_val_if_reached (NULL); } @@ -1932,3 +1968,68 @@ status_notifier_get_state (StatusNotifier _sn) g_return_val_if_fail (IS_STATUS_NOTIFIER (sn), FALSE); return sn->priv->state; } + +#ifdef USEDBUSMENU +/*
- * status_notifier_set_context_menu:
- * @sn: A #StatusNotifier
- * @menu: A #GtkWidget
A #GtkWidget of the menu to set as context menu
- *
- * Exports specified context menu via dbus.
- * If menu is set, no #StatusNotifier::context_menu signals will be emited
typo: emitted
Also, there should probably be a mention of how things work re: references, as mentioned earlier.
- *
- / +void +status_notifier_set_context_menu (StatusNotifier sn,
- GtkWidget *menu) +{
- StatusNotifierPrivate *priv;
- DbusmenuMenuitem *root = NULL; +
- g_return_if_fail (IS_STATUS_NOTIFIER (sn));
- g_return_if_fail (GTK_IS_MENU (menu));
- priv = sn->priv; +
- if (priv->menu)
- {
- g_object_unref(priv->menu);
- }
Drop the brackets.
+
- priv->menu = menu;
- g_object_ref(menu);
Space before parenthesis. (And as mentioned, maybe do a ref_sink() instead?)
+
- root = dbusmenu_gtk_parse_menu_structure(priv->menu); +
- if (priv->menuservice == NULL)
- {
- priv->menuservice = dbusmenu_server_new ("/MenuBar");
- }
Same here.
+
- dbusmenu_server_set_root (priv->menuservice, root); +
- /* Drop our local ref as set_root should get it's own. */
- if (root != NULL)
- {
- g_object_unref(root);
- }
Same here.
+} + +/**
- * status_notifier_set_context_menu:
- * @sn: A #StatusNotifier
- *
- * Returns #GtkWidget of currently setuped menu. If none was setuped
- null will be returned.
Or simply: Returns the #GtkWidget set as context menu, or %NULL
- *
- * Returns: #GtkWidget or null (if menu was not setuped)
Same here, with the "(transfer none)" annotation.
- / +GtkWidget +status_notifier_get_context_menu (StatusNotifier *sn) +{
- StatusNotifierPrivate *priv; +
- g_return_if_fail (IS_STATUS_NOTIFIER (sn));
- priv = sn->priv; +
return priv->menu; +} +#endif diff --git a/src/statusnotifier.h b/src/statusnotifier.h index 3834064..e5ecdbb 100644 --- a/src/statusnotifier.h +++ b/src/statusnotifier.h @@ -27,6 +27,9 @@
include
include <gio/gio.h>
include <gdk-pixbuf/gdk-pixbuf.h>
+#ifdef USE_DBUSMENU +#include <gtk/gtk.h> +#endif
G_BEGIN_DECLS
@@ -298,6 +301,13 @@ void status_notifier_register ( StatusNotifier sn); StatusNotifierState status_notifier_get_state ( StatusNotifier sn); +#ifdef USE_DBUSMENU +void status_notifier_set_context_menu (
- StatusNotifier *sn,
- GtkWidget menu); +GtkWidget \ status_notifier_get_context_menu (
StatusNotifier *sn);+#endif
G_END_DECLS
vovochka404
Adds two new functions:
status_notifier_set_context_menu status_notifier_get_context_menu
new ./configure options: --enable-dbusmenu --with-dbusmenu-gtk-version=[2,3]
the only problem, i don't know how to make this functions optional in documentation.