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

Proposed by Jeremy Munsch on 2015-10-04
Status: Work in progress
Proposed branch: lp:~jeremy-munsch/synapse-project/fix-notification
Merge into: lp:synapse-project
Diff against target: 113 lines (+17/-35)
5 files modified
configure.ac (+1/-3)
debian/control (+0/-1)
src/plugins/imgur-plugin.vala (+8/-15)
src/plugins/pastebin-plugin.vala (+8/-15)
src/ui/synapse-main.vala (+0/-1)
To merge this branch: bzr merge lp:~jeremy-munsch/synapse-project/fix-notification
Reviewer Review Type Date Requested Status
Rico Tzschichholz 2015-10-04 Needs Fixing on 2015-11-15
Review via email: mp+273323@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Rico Tzschichholz (ricotz) wrote :

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) wrote :

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) wrote :

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

Jeremy Munsch (jeremy-munsch) wrote :

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) wrote :

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

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) wrote :

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

review: Needs Information
Jeremy Munsch (jeremy-munsch) wrote :

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

Jeremy Munsch (jeremy-munsch) wrote :

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.

620. By Jeremy Munsch on 2015-11-14

chromium: Set mime-type of BookmarkMatch

Rico Tzschichholz (ricotz) wrote :

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) wrote :

I'll make some tests on a virtual machine asap

Jeremy Munsch (jeremy-munsch) wrote :

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) wrote :

> 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) wrote :

> 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) wrote :

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

Jeremy Munsch (jeremy-munsch) wrote :

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*

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

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) wrote :

Just corrected according your side note.

Jeremy Munsch (jeremy-munsch) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configure.ac'
2--- configure.ac 2015-11-14 20:50:37 +0000
3+++ configure.ac 2015-11-15 20:04:27 +0000
4@@ -62,7 +62,6 @@
5 gee-0.8 >= $MIN_GEE_VERSION \
6 json-glib-1.0 >= $MIN_JSON_VERSION \
7 keybinder-3.0 \
8- libnotify
9 )
10 SYNAPSE_MODULES_VALAFLAGS=" --pkg gdk-x11-3.0 \
11 --pkg gtk+-3.0 \
12@@ -70,8 +69,7 @@
13 --pkg gio-unix-2.0 \
14 --pkg gee-0.8 \
15 --pkg json-glib-1.0 \
16- --pkg keybinder-3.0 \
17- --pkg libnotify"
18+ --pkg keybinder-3.0"
19 AC_SUBST(SYNAPSE_MODULES_VALAFLAGS)
20
21 SYNAPSE_COMMON_VALAFLAGS=" --target-glib=2.40 --thread -g"
22
23=== modified file 'debian/control'
24--- debian/control 2014-07-04 19:46:35 +0000
25+++ debian/control 2015-11-15 20:04:27 +0000
26@@ -13,7 +13,6 @@
27 libgee-0.8-dev (>= 0.5.2),
28 libjson-glib-dev (>= 0.10.0),
29 libkeybinder-3.0-dev,
30- libnotify-dev,
31 librest-dev,
32 libappindicator3-dev (>= 0.0.7)
33 Vcs-Bzr: http://bazaar.launchpad.net/~synapse-core/synapse-project/trunk/
34
35=== modified file 'src/plugins/imgur-plugin.vala'
36--- src/plugins/imgur-plugin.vala 2014-07-11 16:10:19 +0000
37+++ src/plugins/imgur-plugin.vala 2015-11-15 20:04:27 +0000
38@@ -166,21 +166,14 @@
39 msg = _("An error occurred during upload, please check the log for more information.");
40 }
41
42- try
43- {
44- // yey for breaking API!
45- var notification = Object.new (
46- typeof (Notify.Notification),
47- summary: _("Synapse - Imgur"),
48- body: msg,
49- icon_name: "synapse",
50- null) as Notify.Notification;
51- notification.set_timeout (10);
52- notification.show ();
53- }
54- catch (Error err)
55- {
56- warning ("%s", err.message);
57+ try {
58+ var notification = new GLib.Notification (_("Synapse - Imgur"));
59+ notification.set_body (msg);
60+ var gicon = new GLib.ThemedIcon ("synapse");
61+ notification.set_icon (gicon);
62+ GLib.Application.get_default ().send_notification (null, notification);
63+ } catch (Error e) {
64+ warning ("%s", e.message);
65 }
66 }
67
68
69=== modified file 'src/plugins/pastebin-plugin.vala'
70--- src/plugins/pastebin-plugin.vala 2014-07-10 11:39:28 +0000
71+++ src/plugins/pastebin-plugin.vala 2015-11-15 20:04:27 +0000
72@@ -155,21 +155,14 @@
73 msg = _("An error occurred during upload, please check the log for more information.");
74 }
75
76- try
77- {
78- // yey for breaking API!
79- var notification = Object.new (
80- typeof (Notify.Notification),
81- summary: _("Synapse - Pastebin"),
82- body: msg,
83- icon_name: "synapse",
84- null) as Notify.Notification;
85- notification.set_timeout (10);
86- notification.show ();
87- }
88- catch (Error err)
89- {
90- warning ("%s", err.message);
91+ try {
92+ var notification = new GLib.Notification (_("Synapse - Pastebin"));
93+ notification.set_body (msg);
94+ var gicon = new GLib.ThemedIcon ("synapse");
95+ notification.set_icon (gicon);
96+ GLib.Application.get_default ().send_notification (null, notification);
97+ } catch (Error e) {
98+ warning ("%s", e.message);
99 }
100 }
101
102
103=== modified file 'src/ui/synapse-main.vala'
104--- src/ui/synapse-main.vala 2015-10-05 07:03:23 +0000
105+++ src/ui/synapse-main.vala 2015-11-15 20:04:27 +0000
106@@ -285,7 +285,6 @@
107 /* Custom style loading must be done before Gtk.init */
108 load_custom_style ();
109 Gtk.init (ref argv);
110- Notify.init ("synapse");
111
112 var app = new GLib.Application ("org.gnome.Synapse", ApplicationFlags.FLAGS_NONE);
113 if (!app.register () || app.get_is_remote ()) {