469880 - Add support for notifications on Linux using libnotify

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Michael Ventnor]

Attachment: Patch

This bug is separate from bug 404738, as the patch in this bug won't immediately fix it for Mobile.

This patch puts the code in the mozgnome library but is off by default since our build farm doesn't have the libnotify dev headers yet.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You should document somewhere what the difference is between "@mozilla.org/system-alerts-service;1" and "@mozilla.org/alerts-service;1" (in particular, why we need both).

+  if (imgStatus == imgIRequest::STATUS_ERROR) {
+    // We have an error getting the image. Display the notification with no icon.
+    ShowAlert(NULL);
+  }

Should check mLoadedFrame here; if an error occurs after the first frame we could show the alert twice!

Did you copy the code for getting the application name from somewhere else? Can we share it somehow?

+  nsCAutoString mAlertTitle;
+  nsCAutoString mAlertText;

nsCString

+  PRBool mLoadedFrame;

PRPackedBool

+  nsCOMPtr<nsAlertsIconListener> alertListener = new nsAlertsIconListener();
+  return alertListener->InitAlertAsync(aImageUrl, aAlertTitle, aAlertText);

OOM check

Needs build-system review from Ted

Comment 2

[Reed Loden [:reed]]

Comment on attachment 353279 [details] [diff] [review]
Patch

>   { "GnomeVFS Service",
>     NS_GNOMEVFSSERVICE_CID,
>     NS_GNOMEVFSSERVICE_CONTRACTID,
>-    nsGnomeVFSServiceConstructor }
>+    nsGnomeVFSServiceConstructor },
>+#ifdef MOZ_ENABLE_LIBNOTIFY
>+  { "Gnome Alerts Service",
>+    NS_SYSTEMALERTSSERVICE_CID,
>+    NS_SYSTEMALERTSERVICE_CONTRACTID,
>+    nsAlertsServiceConstructor },
>+#endif
> };

What if MOZ_ENABLE_LIBNOTIFY isn't #define'd... that trailing comma will be there. Will that cause a compilation error?

Comment 3

[Michael Ventnor]

No. Notice how a trailing comma is on the end even if it is defined. Its all over the code to make it easier to add elements.

Comment 4

[Michael Ventnor]

Attachment: Patch 2

Fix roc's comments.

Comment 5

[Michael Ventnor]

Comment on attachment 353295 [details] [diff] [review]
Patch 2

Ted, I need your approval on the build system changes.

Comment 6

[Axel Hecht [:Pike]]

... make sure that --disable-compile-environment ignores the libnotify test. I'd do so even if it is currently not switched on by default.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 353295 [details] [diff] [review]
Patch 2

Looks fine. As Pike said, that whole block in configure should really be inside
if test -z "$SKIP_LIBRARY_CHECKS";
fi

In fact, the dbus stuff up above (and maybe some more) should go inside a block like that as well.

Comment 8

[Michael Ventnor]

Attachment: Patch 3 (Checked in)

This adds support for that compile time check.

Comment 9

[Michael Ventnor]

Just a note, this patch will be off by default until the build machines get libnotify dev headers installed (which I will talk to IT about after landing).

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 9891b174d871

Comment 11

[Shawn Wilsher :sdwilsh]

why did this end up in toolkit/system/gnome and not toolkit/components/alerts/src/gnome (similar to how the mac implementation is)?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So it can live in the dynamically linked libmozgnome, so we can link to libnotify and if it's not there the browser will still work.

Comment 13

[Alexander Sack]

Thanks! Would be nice if we'd get this on 1.9.1

Comment 14

[Michael Ventnor]

Yes it would be, but since string freeze has passed (or if not, coming up extremely soon) I don't know if it will make it considering quite a few strings may have to be changed to remove the implied "click me for further details" action.

I just hope 1.9.2 will be quicker than this "quick" 1.9.1 release cycle was :)

Comment 15

[Alexander Sack]

1.9.0 backport would require the backport in bug 393936 -> depends.

Comment 16

[Alexander Sack]

Comment on attachment 355343 [details] [diff] [review]
Patch 3 (Checked in)

improvements on the linux desktop integration front seems to justify to request for 1.9.1 approval here. Even more so as ubuntu is getting a new notification system: http://www.markshuttleworth.com/archives/253

Thanks for considering this.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 355343 [details] [diff] [review]
Patch 3 (Checked in)

Clearing the nomination to take this in 1.9.1, the lack of callback support feels like a usability regression, and I don't think we want to do that now, and probably ever.  Need to sync with the right people to find a way forward.

Comment 18

[Mike Connor [:mconnor]]

So, this never had ui-review, and it's clearly making user experience changes.  I'm inclined to back it out until it's not a UX regression, or until we've decided that the regression is worth the integration win.

I'm completely unconvinced right now that the current behaviour is an improvement.  I've read https://wiki.ubuntu.com/NotificationDesignGuidelines (now that Ubuntu made the spec public) and I've discussed it with the designers.  I think they're wrong, and I don't think we win by following platform specs if the platform specs are less useful.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

I have no particular opinion about this, but I've CCed a few folks who might.

Comment 20

[Mike Connor [:mconnor]]

Actually, the API supports callbacks, but for some reason they weren't implemented?  Bah, this is just multiple layers of wrong.

Comment 21

[Mike Connor [:mconnor]]

So, it's possible that some notification servers won't support actions.  It's silly, but it's possible.  We should implement actions in toolkit, or back this out (as much as the ugly toaster is something I want to kill).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #18)
> I'm completely unconvinced right now that the current behaviour is an
> improvement.  I've read https://wiki.ubuntu.com/NotificationDesignGuidelines
> (now that Ubuntu made the spec public) and I've discussed it with the
> designers.  I think they're wrong, and I don't think we win by following
> platform specs if the platform specs are less useful.

OK, but don't they get some leeway to be wrong? Not unlimited leeway, but some?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, is this a toolkit issue or a product issue? We could easily enable/disable native notifications per-product.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> Also, is this a toolkit issue or a product issue? We could easily
> enable/disable native notifications per-product.
IMO this is a toolkit issue.  This patch changes the behavior of how something works consistently on all platforms to something that isn't consistent (namely, callback support).  A toolkit peer probably would have flagged that, but one didn't even look at this patch...

Comment 25

[Mike Connor [:mconnor]]

I think this is a toolkit issue.  We shouldn't partially implement libnotify and have nsIAlertsService be less capable on Linux.  If distros disable actions in their notification server, that's their call, but I don't think our impl should ignore callbacks.

Comment 26

[Mike Connor [:mconnor]]

As a note, even Ubuntu's implementation supports actions, it's just not "as shiny" right now.  We should let the caller decide whether they want a callback or not.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. I believe at the time this was implemented Ubuntu was planning to not support callbacks. I'm glad this has changed.

Comment 28

[Michael Ventnor]

I can work on adding callback support, libnotify seems to have a good API for this at first glance.

Comment 29

[Michael Ventnor]

Attachment: Add callbacks, and turn it on

This adds support for callbacks, and seems to work well.

I can still request ui-r if necessary (since I want to flip the switch now for 1.9.2) but with the callback support implemented as is in this patch, it works just like the toaster; clicking anywhere in the bubble (except the X) activates the callback, and the bubble disappears after the system default delay.

I'm still on ubuntu intrepid, I don't know what's happening for the final jaunty in the area of notifications (although jaunty will be well obsolete by the time 1.9.2 is even close to release...)

Comment 30

[David Humphrey (:humph)]

> This adds support for callbacks, and seems to work well.

If we do take this, and get proper callback support, I'm wondering if we'd still consider it for 1.9.1, since it would let me switch the Thunderbird notifications over to use it.

Comment 31

[Shawn Wilsher :sdwilsh]

Can we get some tests too please?  Pretty please, with sugar, and spice, and everything nice?

Comment 32

[Mike Connor [:mconnor]]

I think it's almost certainly too late for 1.9.1.  A set of tests would be great in the service of changing that.  There isn't a matter of "switching anything over" since it's the same API, it'll just be cleaner/nicer/better integrated.

As for ui-review, if it's now identical for the interaction model, and isn't requiring string changes because the behaviour is different, then ui-r=me.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsAutoString alertCookie;

nsString (you normally don't want to use nsAuto* in structs)

+  nsAutoString mAlertCookie;

Same here

Instead of creating NotifyCallbackInfo, why don't you make the notify_ callbacks take the nsAlertsIconListener as their closure-data?

Comment 34

[Michael Ventnor]

(In reply to comment #31)
> Can we get some tests too please?  Pretty please, with sugar, and spice, and
> everything nice?

Litmus is the only way I can think of testing this, as this calls into the native toolkit and any tests I can think of would be no different to what we have for the toaster.

(In reply to comment #33)
> Instead of creating NotifyCallbackInfo, why don't you make the notify_
> callbacks take the nsAlertsIconListener as their closure-data?

I guess so, I was worried the nsAlertsIconListener would be destroyed before the callback happened but I suppose that's what XPCOM is for...

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> Litmus is the only way I can think of testing this, as this calls into the
> native toolkit and any tests I can think of would be no different to what we
> have for the toaster.
I don't even think we have those...

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I guess so, I was worried the nsAlertsIconListener would be destroyed before
> the callback happened but I suppose that's what XPCOM is for...

Right, NS_ADDREF the nsAlertsIconListener before you set the callback and NS_RELEASE it in the destroy callback.

Comment 37

[Michael Ventnor]

Precisely my plan, yes. I'm just trying to think of a mochitest. I could probably test the alertfinished callback, but the alertclickcallback listener seems impossible to test due to the myriad of notification systems we now use.

Comment 38

[Michael Ventnor]

Attachment: Callbacks 2

This includes a mochitest.

What I'm worried about is what happens if Growl is absent. Does the alerts service throw? If not, I'll have to update this patch to make an exception for non Windows and Linux platforms.

Comment 39

[Michael Ventnor]

Attachment: Callback 2.1

Oops, that patch had some cruft in it.

Comment 40

[Shawn Wilsher :sdwilsh]

You'll hit two cases with Growl:
1) if growl is not installed, the call to getService will fail (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/mac/nsAlertsService.mm#150)
2) if growl is not running, dispatching the notification will fail (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/mac/nsAlertsService.mm#97)

For the unit test boxes, you'll hit (1)

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  alertTimeout = setTimeout(killAndFail, ALERT_TIMEOUT);

Don't do this. Let the normal mochitest timeout mechanism trigger test failure.

+  nsAlertsIconListener* alert = (nsAlertsIconListener*) user_data;

static_cast

Comment 42

[Michael Ventnor]

Attachment: Callbacks 2.2

Fix review comments.

Comment 43

[Michael Ventnor]

Wow, that was fast.

Comment 44

[Alexander Sack]

(In reply to comment #26)
> As a note, even Ubuntu's implementation supports actions, it's just not "as
> shiny" right now.  We should let the caller decide whether they want a callback
> or not.

while this is true, it doesn't mean that those actions should be used.

In general, apps should not add actions if there is no "actions" capability announced by the server through:

/**
 * Returns the capabilities of the notification server.
 *
 * @return A list of capability strings. These strings must be freed.
 */
GList *notify_get_server_caps(void);

So what we can do is two things (or maybe both):
 
1. omit the actions in system alerts service if the there is no actions support available
2. add a supportsActions method to system alerts component to allow the caller to decide on its own what to do in case there are no actions available on system.

Comment 45

[Dão Gottwald [::dao]]

1) has already been discussed and is the reason this bug has been reopened. I don't see what you expect the caller to do in 2). Seems like we should avoid that too. Ideally we'd fall back to our cross-platform implementation if the notification server isn't capable of actions.

Comment 46

[Alexander Sack]

(In reply to comment #45)
> 1) has already been discussed and is the reason this bug has been reopened. I

1) might have been discussed, but it wasn't implemented (the implementation just dropped actions no matter what.

> don't see what you expect the caller to do in 2). Seems like we should avoid
> that too. Ideally we'd fall back to our cross-platform implementation if the
> notification server isn't capable of actions.

I think the alertsservice - which currently delegates to the system one - could do that automatic fallback; anyway, in order to decide this, the system alertsservice needs to exports that info first. Thats basically what i meant with 2). Other xulapps could then look up the system alerts service and do their own decision (e.g. if they don't use actions they can just happily use system notification even when no actions are supported)

Comment 47

[Alexander Sack]

1. Capabilities of notify-osd (shipped by ubuntu default):
'body', 'body-markup', 'icon-static', 'image/svg+xml', 'x-canonical-private-
synchronous', 'x-canonical-append', 'x-canonical-private-icon-only', 'x-canonical-truncation', 'private-synchronous', 'append', 'private-icon-only', 'truncation'


2. Capabilities of notification-daemon:
'actions', 'body', 'body-hyperlinks', 'body-markup', 'icon-static'

Comment 48

[Dão Gottwald [::dao]]

(In reply to comment #46)
> 1) might have been discussed, but it wasn't implemented (the implementation
> just dropped actions no matter what.

Right, but it's bad for the same reason. We can't drop callbacks like the original implementation did it, and we also can't do it depending on the notification server -- unless we care only about a know set of servers and are confident that they'll continue to support actions.

Comment 49

[Alexander Sack]

(In reply to comment #48)
> (In reply to comment #46)
> > 1) might have been discussed, but it wasn't implemented (the implementation
> > just dropped actions no matter what.
> 
> Right, but it's bad for the same reason. We can't drop callbacks like the
> original implementation did it, and we also can't do it depending on the
> notification server -- unless we care only about a know set of servers and are
> confident that they'll continue to support actions.

but if the server does not announce "actions" capability adding them does not make it better :) ... anyway, i think the right way to move forward would be 2) as in comment 46

Comment 50

[Michael Ventnor]

One thing we could do is provide the click callback immediately as the alert appears, if the server doesn't support actions. For example, the downloads window will appear when the downloads complete alert is also shown. That would be compatible with our current system but might be annoying in some circumstances.

Comment 46 sounds good in theory I suppose, but changing the interface as well as all its users (not to mention third party extensions!) sounds a little much.

Comment 51

[Michael Ventnor]

But that could probably be a topic of another bug, after this is checked in?

Comment 52

[David Humphrey (:humph)]

> One thing we could do is provide the click callback immediately as the alert
> appears, if the server doesn't support actions. For example, the downloads
> window will appear when the downloads complete alert is also shown. That would
> be compatible with our current system but might be annoying in some
> circumstances.

+1 -- we really should have parity with other notification systems we use (e.g. Growl), so that all the existing "click" logic works here too.

Comment 53

[Dão Gottwald [::dao]]

(In reply to comment #50)
> One thing we could do is provide the click callback immediately as the alert
> appears, if the server doesn't support actions. For example, the downloads
> window will appear when the downloads complete alert is also shown. That would
> be compatible with our current system but might be annoying in some
> circumstances.

That seems to destroy the idea of lightweight notifications.

(In reply to comment #51)
> But that could probably be a topic of another bug, after this is checked in?

Well, you want to turn it on in this bug, right? Yet notify-osd doesn't seem to
support actions officially. That doesn't seem right to me.

Comment 54

[Dão Gottwald [::dao]]

Specifically, we use notifications because we don't want to steal focus.

Comment 55

[Alexander Sack]

(In reply to comment #50)
> Comment 46 sounds good in theory I suppose, but changing the interface as well
> as all its users (not to mention third party extensions!) sounds a little much.

This was not exactly the idea I had in mind. AFAIK, currently alertsservice is the only place that decides whether to delegate to system service or not. ATM this decision only depends on whether the component is available or not.

We could extend that check to also consider whether actions are available; if there is no actions support, it would fall back to the xul implemention - thus, preserving the old contract.

However, toolkit apps/extensions that are happy with the actionless approach could explicitly opt-out from that by using a different method and/or by directly looking up/using the system-alerts component.

Comment 56

[Alexander Sack]

(In reply to comment #51)
> But that could probably be a topic of another bug, after this is checked in?

The current patch should at least not add actions when the server doesn't announce "actions" capability. That would also be in line with comment 25.

Comment 57

[Michael Ventnor]

(In reply to comment #56)
> (In reply to comment #51)
> > But that could probably be a topic of another bug, after this is checked in?
> 
> The current patch should at least not add actions when the server doesn't
> announce "actions" capability. That would also be in line with comment 25.

I'd rather do this than fallback to XUL, this bug seems like a giant waste if the most popular distribution doesn't benefit from it in any way.

Comment 58

[u294409]

> this bug seems like a giant waste if
> the most popular distribution doesn't benefit from it in any way.

What does this mean? Lack of unity in question of notification (notify-osd, notification-daemon) caused, mostly by Ubuntu, is the problem?

Comment 59

[Michael Ventnor]

(In reply to comment #58)
> > this bug seems like a giant waste if
> > the most popular distribution doesn't benefit from it in any way.
> 
> What does this mean? Lack of unity in question of notification (notify-osd,
> notification-daemon) caused, mostly by Ubuntu, is the problem?

Sort of, yes. I don't see how this can be resolved on our side if the notification server doesn't support actions, but I also want to kill that ugly popup for as many people as humanly possible, and adopt the native library.

Comment 60

[Michael Ventnor]

I suppose I can just not add the action callback if the server doesn't support them.

Comment 61

[Dão Gottwald [::dao]]

(In reply to comment #57)
> > The current patch should at least not add actions when the server doesn't
> > announce "actions" capability. That would also be in line with comment 25.
> 
> I'd rather do this than fallback to XUL, this bug seems like a giant waste if
> the most popular distribution doesn't benefit from it in any way.

I'm not sure that I agree with comment 25 there, or maybe I misunderstand it. If the caller wants a callback (which might even be reflected in the notification message) and the server tells us that it doesn't support it, falling back to XUL is the only sane solution, IMHO. I think this needs to be evangelized on the Ubuntu side.

Comment 62

[u294409]

> I think this needs to be evangelized on the Ubuntu side.

Talk to Mark Shuttleworth. It was his idea to make _another_ notification daemon, but with less possibilities (completely without actions).

Comment 63

[Alexander Sack]

(In reply to comment #60)
> I suppose I can just not add the action callback if the server doesn't support
> them.

So:

#define NOTIFY_CAPS_ACTIONS_KEY "actions"

GList *server_caps = notify_get_server_caps();
if (g_list_find (server_caps, NOTIFY_CAPS_ACTIONS_KEY))
...

if you do this, consider to cache the result somehow as you would otherwise do another dbus call for each notification.

Comment 64

[Mike Connor [:mconnor]]

We should absolutely fall back to XUL if there's no actions support, IMO.  I think the Ubuntu system is unnecessarily restrictive, and the design guidelines actually are more damaging than helpful.  I guess I need to write up that critique after all...

Let's do the sane thing, that Windows and OS X (via Growl) already do, and we can work to convince Ubuntu of the error of their ways... :)

Comment 65

[Michael Ventnor]

Attachment: Callbacks 3

This falls back to XUL if the server doesn't support actions.

Comment 66

[Dão Gottwald [::dao]]

I tried to land this (http://hg.mozilla.org/mozilla-central/rev/99cdecb3734f), but will have to back it out because of this:

checking for libnotify >= 0.4... Package libnotify was not found in the pkg-config search path. Perhaps you should add the directory containing `libnotify.pc' to the PKG_CONFIG_PATH environment variable No package 'libnotify' found
configure: error: Library requirements (libnotify >= 0.4) not met; consider adjusting the PKG_CONFIG_PATH environment variable if your libraries are in a nonstandard prefix so pkg-config can find them.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241424892.1241424932.5678.gz

Comment 67

[Michael Ventnor]

Er, I remember libnotify dev headers being installed on mozilla-central, I'll take another look at the bug.

Comment 68

[Michael Ventnor]

Bug 473831 was marked fixed, but they mention only i386 packages.
The red only occurred on the 64-bit box, but the builds that happened around the same time on the 32-bit boxes still seem to be going.

Comment 69

[Nick Thomas [:nthomas] (UTC+12)]

linux64 slave now has a libnotify-devel installed, previously only had libnotify. Details in bug 473831 comment #9.

Comment 70

[Michael Ventnor]

Wonderful, thanks.

Comment 71

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/c2ba27e9e639

For some reason, Linux x86-64 mozilla-central hasn't started building yet...

Comment 72

[yannubuntu]

Dear all,
according to https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/610362 , Firefox's notifications don't work with Ubuntu's implementation.

is there something mozilla can do to help solving this ?

Comment 73

[Dão Gottwald [::dao]]

(In reply to comment #72)
> Dear all,
> according to https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/610362 ,
> Firefox's notifications don't work with Ubuntu's implementation.
> 
> is there something mozilla can do to help solving this ?

Please read through this bug ... We need action callback support for our notifications.