Merge lp:~kalikiana/midori/alwaysnotify into lp:midori

Proposed by Cris Dywan
Status: Merged
Approved by: Paweł Forysiuk
Approved revision: 6182
Merged at revision: 6183
Proposed branch: lp:~kalikiana/midori/alwaysnotify
Merge into: lp:midori
Diff against target: 98 lines (+5/-30)
3 files modified
README (+2/-2)
midori/midori-app.c (+0/-23)
wscript (+3/-5)
To merge this branch: bzr merge lp:~kalikiana/midori/alwaysnotify
Reviewer Review Type Date Requested Status
gue5t gue5t Needs Fixing
Paweł Forysiuk Approve
André Stösel Approve
Review via email: mp+166480@code.launchpad.net

Commit message

Make libnotify mandatory except on Windows

Description of the change

Make libnotify mandatory except for now on Windows

To post a comment you must log in.
Revision history for this message
Cris Dywan (kalikiana) wrote :

A very interesting discussion the other day showed that some people may disable libnotify to "customize" Midori's behavior. I think this is misguided and apart from something like openbox libnotify is very unusual to not have available these days.
I'm leaving an exception for Windows for now because we don't have the same expectations there and at least currently don't think it's typical to have notifications there.

Revision history for this message
Cris Dywan (kalikiana) wrote :

Consider https://code.launchpad.net/~kalikiana/midori/manydowns/+merge/166483 for solving some of the behavior aspects people might've been trying to work-around.

Revision history for this message
André Stösel (ivaldi) wrote :

Works fine with Linux, but to be sure it would be nice if Paweł Forysiuk could review it (windows), too.

(Although the changes are really obviously - it's probably unnecessary ;)

review: Approve
Revision history for this message
Paweł Forysiuk (tuxator) wrote :

Seems to work fine on win32

review: Approve
Revision history for this message
gue5t gue5t (gue5t) wrote :

In my opinion it's perfectly sane for users to want some programs to not produce notifications. It's also sane for non-Windows systems to not have libnotify installed, and given that libnotify does not provide essential functionality I don't see why it should be made a hard-dependency. However, I do agree that ./configure is not the right place for users to configure whether on typical desktop systems whether they would like to see notifications from Midori. We should instead have a preference to control notifications.

Removing code that manually calls notify-send is a very good thing. Executing random system binaries is awful and Midori built on a system which wants notification features should depend on the requisite software, i.e. libnotify.

review: Needs Fixing
Revision history for this message
Cris Dywan (kalikiana) wrote :

> It's also sane for non-Windows systems to not have libnotify installed

I'm very doubtful here. All DEs roughly following freedesktop.org use notifications. It's also been proposed for inclusion with GTK+ which by itself says something.

> …libnotify does not provide essential functionality…

I disagree with this assumption and others voiced concerns that for example "launcher created" may not show up without libnotify. It is expected in the typical case, except custom kiosk setups and the like which may intentionally block the notification daemon.

I'll file a bug to re-consider the Windows case. I simply didn't want to block on it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'README'
2--- README 2013-05-16 21:25:22 +0000
3+++ README 2013-05-30 11:15:49 +0000
4@@ -12,9 +12,9 @@
5 * Customizable interface, extensions written in C and Vala.
6
7 Requirements: GLib 2.22, GTK+ 2.16, WebkitGTK+ 1.1.17, libXML2,
8- libsoup 2.27.90, sqlite 3.0, Vala 0.14
9+ libsoup 2.27.90, sqlite 3.0, Vala 0.14, libnotify
10
11-Optional: GTK+ 3.0, Unique 0.9, libnotify, gcr, Granite 0.2, WebKit2GTK+ 1.11.91/ 2.0.0
12+Optional: GTK+ 3.0, Unique 0.9, gcr, Granite 0.2, WebKit2GTK+ 1.11.91/ 2.0.0
13
14 For installation instructions read INSTALL.
15
16
17=== modified file 'midori/midori-app.c'
18--- midori/midori-app.c 2013-05-19 09:33:02 +0000
19+++ midori/midori-app.c 2013-05-30 11:15:49 +0000
20@@ -76,10 +76,6 @@
21
22 MidoriBrowser* browser;
23 MidoriAppInstance instance;
24-
25- #if !HAVE_LIBNOTIFY
26- gchar* program_notify_send;
27- #endif
28 };
29
30 static gchar* app_name = NULL;
31@@ -795,8 +791,6 @@
32
33 #if HAVE_LIBNOTIFY
34 notify_init (PACKAGE_NAME);
35- #else
36- app->program_notify_send = g_find_program_in_path ("notify-send");
37 #endif
38 }
39
40@@ -824,8 +818,6 @@
41 #if HAVE_LIBNOTIFY
42 if (notify_is_initted ())
43 notify_uninit ();
44- #else
45- katze_assign (app->program_notify_send, NULL);
46 #endif
47
48 G_OBJECT_CLASS (midori_app_parent_class)->finalize (object);
49@@ -1312,21 +1304,6 @@
50 notify_notification_show (note, NULL);
51 g_object_unref (note);
52 }
53- #else
54- /* Fall back to the command line program "notify-send" */
55- if (app->program_notify_send)
56- {
57- gchar* msgq = g_shell_quote (message);
58- gchar* titleq = g_shell_quote (title);
59- gchar* command = g_strdup_printf ("%s -i midori %s %s",
60- app->program_notify_send, titleq, msgq);
61-
62- g_spawn_command_line_async (command, NULL);
63-
64- g_free (titleq);
65- g_free (msgq);
66- g_free (command);
67- }
68 #endif
69 }
70
71
72=== modified file 'wscript'
73--- wscript 2013-05-22 20:17:41 +0000
74+++ wscript 2013-05-30 11:15:49 +0000
75@@ -193,12 +193,11 @@
76 uselib_store=var, errmsg=name + ver_str + ' not found')][have])
77 return have
78
79- if option_enabled ('libnotify'):
80- if not check_pkg ('libnotify', mandatory=False):
81- option_checkfatal ('libnotify', 'notifications')
82- else:
83+ if is_win32 (os.environ):
84 conf.define ('LIBNOTIFY_VERSION', 'No')
85 conf.check_message_custom ('libnotify', '', 'disabled')
86+ else:
87+ check_pkg ('libnotify', mandatory=True)
88 conf.define ('HAVE_LIBNOTIFY', [0,1][conf.env['LIBNOTIFY_VERSION'] != 'No'])
89
90 if option_enabled ('granite'):
91@@ -419,7 +418,6 @@
92
93 group = opt.add_option_group ('Optional features', '')
94 add_enable_option ('unique', 'single instance support', group, disable=is_win32 (os.environ))
95- add_enable_option ('libnotify', 'notification support', group)
96 add_enable_option ('granite', 'new notebook, pop-overs', group, disable=True)
97 add_enable_option ('addons', 'building of extensions', group)
98 add_enable_option ('tests', 'install tests', group, disable=True)

Subscribers

People subscribed via source and target branches

to all changes: