GNOME Bugzilla – Bug 601691
Refactoring of libnotify code
Last modified: 2010-03-24 11:36:48 UTC
The following branch introduces some refactoring in the libnotify branch: - A new object has been created EmpathyNotifyManager. It doesn't do that much atm but that's a first step to separate the status icon and the libnotify bubbles. - This mgr caches the capabilities of the server which can be useful to do capabilities specific things with notifications (as bug #588054 and bug #589851) for example. - The code of the not very well named "empathy-misc" has been merged to this mgr.
Created attachment 147575 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/notify-refactoring-601691 libempathy-gtk/Makefile.am | 6 +- libempathy-gtk/empathy-notify-manager.c | 188 +++++++++++++++++++++++++++++++ libempathy-gtk/empathy-notify-manager.h | 95 ++++++++++++++++ src/Makefile.am | 1 - src/empathy-chat-window.c | 11 ++- src/empathy-misc.c | 77 ------------- src/empathy-misc.h | 49 -------- src/empathy-status-icon.c | 28 ++--- 8 files changed, 306 insertions(+), 149 deletions(-)
For the record here is the result of GetCapabilities() and GetServerInformation() on Ubuntu Karmic and Debian Sid: Ubuntu ===== GetCapabilities() [u'body', u'body-markup', u'icon-static', u'image/svg+xml', u'x-canonical-private-synchronous', u'x-canonical-append', u'x-canonical-private-icon-only', u'x-canonical-truncation', u'private-synchronous', u'append', u'private-icon-only', u'truncation'] GetServerInformation() (u'notify-osd', u'Canonical Ltd', u'1.0', u'1.1') Debian ===== GetCapabilities() [u'actions', u'body', u'body-hyperlinks', u'body-markup', u'icon-static'] GetServerInformation() (u'Notification Daemon', u'Galago Project', u'0.4.0', u'1.0')
Review of attachment 147575 [details] [review]: Looks mostly fine; I inlined some comments: ::: libempathy-gtk/Makefile.am @@ +67,3 @@ empathy-kludge-label.c \ + empathy-ui-utils.c \ + empathy-notify-manager.c We should probably try to keep this list in an alphabetical order. ::: libempathy-gtk/empathy-notify-manager.c @@ +71,3 @@ + list = notify_get_server_caps (); + for (l = list; l != NULL; l = g_list_next (l)) + { I'd move this code into empathy_notify_manager_init(), as it's not usual to see something else than singleton stuff in _constructor(). @@ +85,3 @@ +static void +notify_manager_dispose (GObject *object) +{ I'd entirely remove _dispose() if we're not actually disposing anything. @@ +134,3 @@ +{ + return EMPATHY_NOTIFY_MANAGER (g_object_new (EMPATHY_TYPE_NOTIFY_MANAGER, + NULL)); No need to cast to EMPATHY_NOTIFY_MANAGER, as g_object_new returns a gpointer. ::: src/empathy-status-icon.c @@ +636,3 @@ icon); /* Query the notification daemon to check if it supports actions */ This comment is now useless.
(In reply to comment #3) > Review of attachment 147575 [details] [review]: > > Looks mostly fine; I inlined some comments: > > ::: libempathy-gtk/Makefile.am > @@ +67,3 @@ > empathy-kludge-label.c \ > + empathy-ui-utils.c \ > + empathy-notify-manager.c > > We should probably try to keep this list in an alphabetical order. Agreed. I'll cook another branch doing that in all the Makefile.am > ::: libempathy-gtk/empathy-notify-manager.c > @@ +71,3 @@ > + list = notify_get_server_caps (); > + for (l = list; l != NULL; l = g_list_next (l)) > + { > > I'd move this code into empathy_notify_manager_init(), as it's not usual to see > something else than singleton stuff in _constructor(). done. > @@ +85,3 @@ > +static void > +notify_manager_dispose (GObject *object) > +{ > > I'd entirely remove _dispose() if we're not actually disposing anything. done > @@ +134,3 @@ > +{ > + return EMPATHY_NOTIFY_MANAGER (g_object_new (EMPATHY_TYPE_NOTIFY_MANAGER, > + NULL)); > > No need to cast to EMPATHY_NOTIFY_MANAGER, as g_object_new returns a gpointer. good point; fixed. > ::: src/empathy-status-icon.c > @@ +636,3 @@ > icon); > > /* Query the notification daemon to check if it supports actions */ > > This comment is now useless. done.
Review of attachment 147575 [details] [review]: This probably sounds stupid, but seriously, no-one ever uses the abbreviation "capa". ::: libempathy-gtk/empathy-notify-manager.c @@ +72,3 @@ + for (l = list; l != NULL; l = g_list_next (l)) + { + gchar *capa = l->data; "cap" or "capability" instead of "capa" @@ +139,3 @@ +gboolean +empathy_notify_manager_has_capability (EmpathyNotifyManager *self, + const gchar *capa) "cap" or "capability" instead of "capa" ::: libempathy-gtk/empathy-notify-manager.h @@ +43,3 @@ +#define EMPATHY_NOTIFY_MANAGER_CAPA_X_CANONICAL_PRIVATE_ICON_ONLY "x-canonical-private-icon-only" +#define EMPATHY_NOTIFY_MANAGER_CAPA_X_CANONICAL_PRIVATE_SYNCHRONOUS "x-canonical-private-synchronous" +#define EMPATHY_NOTIFY_MANAGER_CAPA_X_CANONICAL_TRUNCATION "x-canonical-truncation" For all of these, "CAP" or "CAPABILITY" instead of "CAPA" @@ +81,3 @@ + +gboolean empathy_notify_manager_has_capability (EmpathyNotifyManager *self, + const gchar *capa); "cap" or "capability" instead of "capa"
Merged to master. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
private-synchronous private-icon-only append shouldn't those values be removed? they don't follow the galago stanadard: they were left in notify-osd only for backwards compatibility, but should not be used by newer apps. The galago standard imposes that everyone can add its own values, but must use x-COMPANY_NAME-VALUE Should I open a new bug?
(In reply to comment #7) > private-synchronous > private-icon-only > append > > shouldn't those values be removed? they don't follow the galago stanadard: they > were left in notify-osd only for backwards compatibility, but should not be > used by newer apps. The galago standard imposes that everyone can add its own > values, but must use x-COMPANY_NAME-VALUE > Should I open a new bug? Yes please.