Merge lp:~jeremy-munsch/synapse-project/fix-notification into lp:synapse-project

[Jeremy Munsch]

Remove usage of libnotify as it was breaking imgur and pastbin plugins and making desktop hangs before failling.

Not making use of http://valadoc.org/#!api=gio-2.0/GLib.Notification which works great.

[Rico Tzschichholz (ricotz)]

Looks reasonable, but do not be lazy! "n" is not a good variable name at all. So "notification" it is.

More of a problem is the implicit bump to glib >= 2.40 which could use some thinking.

review: Needs Fixing

[Rico Tzschichholz (ricotz)]

Please use descriptive commit messages mainly in the present tense.

"topic/source-target": "short description of the actual change"
\n
"long description if needed"

e.g. "Replace usage of libnotify with GLib.Notification"

And use "bzr --fixes lp:XXXXXX" if a bug is available to reference the bug and don't explicitly mention the bug number in the commit message.

This concerns all your merge-proposals

[Jeremy Munsch (jeremy-munsch)]

Ok, thank you for the review, i'll look for these.

[Jeremy Munsch (jeremy-munsch)]

Also i just saw https://launchpad.net/ubuntu/+source/glib2.0,
and precise is still at supported until late 2018 https://wiki.ubuntu.com/LTS.

I guess that this merge is dead for now sadly, because GLib is still at 2.32, there might be no workaround this is guess ?

[Jeremy Munsch (jeremy-munsch)]

Ok I have now updated my other merge proposals as well as this one with your commit recommendations. :P

[lp:~jeremy-munsch/synapse-project/fix-notification]

615. By Rico Tzschichholz on 2015-10-05

main: Properly setup internationalization

616. By Jeremy Munsch on 2015-11-14

core: Handle UriMatches pointing to executable files

617. By Rico Tzschichholz on 2015-11-14

build: Drop optional dependency on gnome-common

618. By Rico Tzschichholz on 2015-11-14

build: Bump requirements glib-2.0 >= 2.40, valac >= 0.24.0

619. By Jeremy Munsch on 2015-11-14

Add support for "Additional applications actions"

Source and evaluate "Desktop Action" groups in desktop-files.

[Rico Tzschichholz (ricotz)]

Please rebase and test it again. Seems not to work here.

review: Needs Information

[Jeremy Munsch (jeremy-munsch)]

I just put modifications on last branch version and it is working here.

[Jeremy Munsch (jeremy-munsch)]

To test it i:
- had a selection and search for text -> send pastebin - OK
- search an image -> send to imgur - OK

No hang, no crash.

[lp:~jeremy-munsch/synapse-project/fix-notification]

620. By Jeremy Munsch on 2015-11-14

chromium: Set mime-type of BookmarkMatch

[Rico Tzschichholz (ricotz)]

OK, it is *not* working here (gnome-shell 3.18.2 and glib 2.27.1) while the existing solution (libnotify) seems to work flawlessly.

review: Needs Information

[Jeremy Munsch (jeremy-munsch)]

I'll make some tests on a virtual machine asap

[Jeremy Munsch (jeremy-munsch)]

Just had a few tests on
Ubuntu 15.10 fresh install (libglib 2.46.1-1) - *OK*
Lubuntu 15.10 fresh install (libglib 2.46.1-1) - *OK*
Kubuntu 15.10 (libglib 2.46.1-1) - *OK*

-- Alright, i was about to test debian + gnome DE, but i checked in devhelp as you adviced me once,
i found that there is an implicit bump to glib 2.40, which is actually what you said on #1 lol

So i think it should need a preprocessor instruction on compile to check which method to use.
I'll try to figure out this and update PR.

[Rico Tzschichholz (ricotz)]

> Just had a few tests on
> Ubuntu 15.10 fresh install (libglib 2.46.1-1) - *OK*
> Lubuntu 15.10 fresh install (libglib 2.46.1-1) - *OK*
> Kubuntu 15.10 (libglib 2.46.1-1) - *OK*

Hmm, I see. I wonder why it isn't working with my setup.

> -- Alright, i was about to test debian + gnome DE, but i checked in devhelp as
> you adviced me once,
> i found that there is an implicit bump to glib 2.40, which is actually what
> you said on #1 lol
>
> So i think it should need a preprocessor instruction on compile to check which
> method to use.
> I'll try to figure out this and update PR.

You are not making much sense or did you really miss
https://bazaar.launchpad.net/~synapse-core/synapse-project/trunk/revision/618

(It is not a dependency or build problem)

[Jeremy Munsch (jeremy-munsch)]

> OK, it is *not* working here (gnome-shell 3.18.2 and glib 2.27.1) while the existing solution > (libnotify) seems to work flawlessly.

> You are not making much sense or did you really miss
> https://bazaar.launchpad.net/~synapse-core/synapse-project/trunk/revision/618
>
> (It is not a dependency or build problem)

Yes it got out of my mind as i saw you had glib < 2.40.

I'm instal fresh debian + Gnome DE right now to make a test

[Rico Tzschichholz (ricotz)]

Ah, I am sorry, that was a typo which meant to be "glib 2.47.1"

[Jeremy Munsch (jeremy-munsch)]

Just made other tests :
Fedora 23, glib 2.46.1, gnome-shell 3.18.1, valac 0.30 - *Not Working*
Debian Jessy, glib 2.42.1-1, gnome-shell 3.14.4.1.deb8u1, valac {0.26,0.30} - *Not Working*

[lp:~jeremy-munsch/synapse-project/fix-notification]

621. By Rico Tzschichholz on 2015-11-15

ui: Use get_allocation() instead of multiple calls to get_allocated_*()

622. By Rico Tzschichholz on 2015-11-15

ui: Use some unowned variables

623. By Rico Tzschichholz on 2015-11-15

ui: Fix usage of get_state_flags()

624. By Rico Tzschichholz on 2015-11-15

ui: Use some unowned variables

625. By Rico Tzschichholz on 2015-11-15

ui: Clean up struct initializations

626. By Rico Tzschichholz on 2015-11-15

widgets: Cache struct in loop

627. By Rico Tzschichholz on 2015-11-15

view-base: Verify presence of GdkX11Screen before using it

628. By Rico Tzschichholz on 2015-11-15

tile: Fix regression of r623

[Jeremy Munsch (jeremy-munsch)]

I think this is obviously a bug,
I ran this sample code:
http://paste.ubuntu.com/13283891/
with valac --pkg=gio-2.0 notif.vala

Tested on all previous VM i mentionned, and it is not workin on fedora and debian.

I'm thinking of filling a bug, still to found from which lib occurs the bug, my guess is it is likely to be gio having a problem with gnome-shell, even this sounds weird; i'm thinking of filling a bug in https://bugzilla.gnome.org.

Is using a fallback method would be acceptable ? It may be checking env var XDG_CURRENT_DESKTOP which is filled by gnome DE with GNOME.

I'll start working on this.

[Jeremy Munsch (jeremy-munsch)]

Just tested this solution and it makes every distro happier :)
So both use libnotify and GLib.Notify based on XDG_CURRENT_DESKTOP == "GNOME"

[Rico Tzschichholz (ricotz)]

Please reinstantiate your original proposal. I won't accept this conditional stuff anyway.

Maybe look at gnome-clocks which is written in vala and is using GNotification too.

(gnome-clocks' notifications are working here)

review: Needs Fixing

[Jeremy Munsch (jeremy-munsch)]

Well i checked a few times to really be sure https://mail.gnome.org/archives/commits-list/2013-October/msg08602.html
It seems that GLib.notification is used exactly the same way.
Even the sample code i linked above is not working.

How ever it saw in
https://bugzilla.gnome.org/show_bug.cgi?id=710913
https://wiki.gnome.org/HowDoI/GNotification

> Warning: gnome-shell uses desktop files to find extra information (app icon, name) about the
> sender of the notification. If you don't have a desktop file whose base name matches the
> application id, then your notification will not show up.

> This will be the preferred way of launching applications, so install a
> DBus .service file, rename the .desktop file to use reverse DNS notation
> and mark it as DBus activatable.
> Using reverse DNS notation for the .desktop file is also a prerequisite
> for using GLib's new notification API.

I don't know what are the "reverse DNS notation" and "DBus .service file". I'll try to find more info

[Rico Tzschichholz (ricotz)]

As a side-note: "var gicon = new GLib.ThemedIcon ("synapse");"

[lp:~jeremy-munsch/synapse-project/fix-notification]

629. By Jeremy Munsch on 2015-11-15

plugins imgur/ pastebin: Replace usage of libnotify with GLib.Notification

Remove dependency of libnotify therefore implicit bump to GLib >= 2.40

[Jeremy Munsch (jeremy-munsch)]

Just corrected according your side note.

[Jeremy Munsch (jeremy-munsch)]

So what i understand, is that to use GLib.Notification with gnome-shell, Synapse has to be DBus launchable at least, yes, I have no idea how to implement that a this time.
What I know is that instead of having a classic fork checking for an existing previous instance, would have to be replaced by a DBus listener, with at least one action.

According to what I read, I could be a small change though, not sure.

Unmerged revisions

629. By Jeremy Munsch on 2015-11-15

plugins imgur/ pastebin: Replace usage of libnotify with GLib.Notification

Remove dependency of libnotify therefore implicit bump to GLib >= 2.40

=== modified file 'configure.ac'
--- configure.ac	2015-11-14 20:50:37 +0000
+++ configure.ac	2015-11-15 20:04:27 +0000
@@ -62,7 +62,6 @@
   gee-0.8 >= $MIN_GEE_VERSION \
   json-glib-1.0 >= $MIN_JSON_VERSION \
   keybinder-3.0 \
-  libnotify
   )
 SYNAPSE_MODULES_VALAFLAGS=" --pkg gdk-x11-3.0 \
   --pkg gtk+-3.0 \
@@ -70,8 +69,7 @@
   --pkg gio-unix-2.0 \
   --pkg gee-0.8 \
   --pkg json-glib-1.0 \
-  --pkg keybinder-3.0 \
-  --pkg libnotify"
+  --pkg keybinder-3.0"
 AC_SUBST(SYNAPSE_MODULES_VALAFLAGS)
 
 SYNAPSE_COMMON_VALAFLAGS=" --target-glib=2.40 --thread -g"

=== modified file 'debian/control'
--- debian/control	2014-07-04 19:46:35 +0000
+++ debian/control	2015-11-15 20:04:27 +0000
@@ -13,7 +13,6 @@
  libgee-0.8-dev (>= 0.5.2),
  libjson-glib-dev (>= 0.10.0),
  libkeybinder-3.0-dev,
- libnotify-dev,
  librest-dev,
  libappindicator3-dev (>= 0.0.7)
 Vcs-Bzr: http://bazaar.launchpad.net/~synapse-core/synapse-project/trunk/

=== modified file 'src/plugins/imgur-plugin.vala'
--- src/plugins/imgur-plugin.vala	2014-07-11 16:10:19 +0000
+++ src/plugins/imgur-plugin.vala	2015-11-15 20:04:27 +0000
@@ -166,21 +166,14 @@
           msg = _("An error occurred during upload, please check the log for more information.");
         }
 
-        try
-        {
-          // yey for breaking API!
-          var notification = Object.new (
-            typeof (Notify.Notification),
-            summary: _("Synapse - Imgur"),
-            body: msg,
-            icon_name: "synapse",
-            null) as Notify.Notification;
-          notification.set_timeout (10);
-          notification.show ();
-        }
-        catch (Error err)
-        {
-          warning ("%s", err.message);
+        try {
+          var notification = new GLib.Notification (_("Synapse - Imgur"));
+          notification.set_body (msg);
+          var gicon = new GLib.ThemedIcon ("synapse");
+          notification.set_icon (gicon);
+          GLib.Application.get_default ().send_notification (null, notification);
+        } catch (Error e) {
+          warning ("%s", e.message);
         }
       }
 

=== modified file 'src/plugins/pastebin-plugin.vala'
--- src/plugins/pastebin-plugin.vala	2014-07-10 11:39:28 +0000
+++ src/plugins/pastebin-plugin.vala	2015-11-15 20:04:27 +0000
@@ -155,21 +155,14 @@
           msg = _("An error occurred during upload, please check the log for more information.");
         }
 
-        try
-        {
-          // yey for breaking API!
-          var notification = Object.new (
-            typeof (Notify.Notification),
-            summary: _("Synapse - Pastebin"),
-            body: msg,
-            icon_name: "synapse",
-            null) as Notify.Notification;
-          notification.set_timeout (10);
-          notification.show ();
-        }
-        catch (Error err)
-        {
-          warning ("%s", err.message);
+        try {
+          var notification = new GLib.Notification (_("Synapse - Pastebin"));
+          notification.set_body (msg);
+          var gicon = new GLib.ThemedIcon ("synapse");
+          notification.set_icon (gicon);
+          GLib.Application.get_default ().send_notification (null, notification);
+        } catch (Error e) {
+          warning ("%s", e.message);
         }
       }
 

=== modified file 'src/ui/synapse-main.vala'
--- src/ui/synapse-main.vala	2015-10-05 07:03:23 +0000
+++ src/ui/synapse-main.vala	2015-11-15 20:04:27 +0000
@@ -285,7 +285,6 @@
         /* Custom style loading must be done before Gtk.init */
         load_custom_style ();
         Gtk.init (ref argv);
-        Notify.init ("synapse");
 
         var app = new GLib.Application ("org.gnome.Synapse", ApplicationFlags.FLAGS_NONE);
         if (!app.register () || app.get_is_remote ()) {