Closed
Bug 469880
Opened 16 years ago
Closed 16 years ago
Add support for notifications on Linux using libnotify
Categories
(Firefox :: Shell Integration, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3.6a1
People
(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
22.97 KB,
patch
|
Details | Diff | Splinter Review | |
11.30 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #353279 -
Flags: superreview?(roc)
Attachment #353279 -
Flags: review?(roc)
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•16 years ago
|
||
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?
Assignee | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
Fix roc's comments.
Attachment #353279 -
Attachment is obsolete: true
Attachment #353295 -
Flags: superreview?(roc)
Attachment #353295 -
Flags: review?(roc)
Attachment #353279 -
Flags: superreview?(roc)
Attachment #353279 -
Flags: review?(roc)
Attachment #353295 -
Flags: superreview?(roc)
Attachment #353295 -
Flags: superreview+
Attachment #353295 -
Flags: review?(roc)
Attachment #353295 -
Flags: review+
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 353295 [details] [diff] [review] Patch 2 Ted, I need your approval on the build system changes.
Attachment #353295 -
Flags: review?(ted.mielczarek)
Comment 6•16 years ago
|
||
... 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•16 years ago
|
||
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.
Attachment #353295 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 8•16 years ago
|
||
This adds support for that compile time check.
Attachment #353295 -
Attachment is obsolete: true
Assignee | ||
Comment 9•16 years ago
|
||
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).
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed 9891b174d871
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Comment 11•16 years ago
|
||
why did this end up in toolkit/system/gnome and not toolkit/components/alerts/src/gnome (similar to how the mac implementation is)?
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•16 years ago
|
||
Thanks! Would be nice if we'd get this on 1.9.1
Assignee | ||
Comment 14•16 years ago
|
||
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•16 years ago
|
||
1.9.0 backport would require the backport in bug 393936 -> depends.
Depends on: 393936
Updated•16 years ago
|
Attachment #355343 -
Flags: approval1.9.1?
Comment 16•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #355343 -
Flags: approval1.9.1?
Updated•16 years ago
|
Target Milestone: --- → Firefox 3.2a1
Comment 18•16 years ago
|
||
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•16 years ago
|
||
I have no particular opinion about this, but I've CCed a few folks who might.
Comment 20•16 years ago
|
||
Actually, the API supports callbacks, but for some reason they weren't implemented? Bah, this is just multiple layers of wrong.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 21•16 years ago
|
||
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).
(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?
Also, is this a toolkit issue or a product issue? We could easily enable/disable native notifications per-product.
Comment 24•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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.
OK. I believe at the time this was implemented Ubuntu was planning to not support callbacks. I'm glad this has changed.
Assignee | ||
Comment 28•16 years ago
|
||
I can work on adding callback support, libnotify seems to have a good API for this at first glance.
Assignee | ||
Comment 29•16 years ago
|
||
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...)
Attachment #373102 -
Flags: superreview?(roc)
Attachment #373102 -
Flags: review?(roc)
Comment 30•16 years ago
|
||
> 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•16 years ago
|
||
Can we get some tests too please? Pretty please, with sugar, and spice, and everything nice?
Flags: in-testsuite?
Comment 32•16 years ago
|
||
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.
+ 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?
Assignee | ||
Comment 34•16 years ago
|
||
(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•16 years ago
|
||
(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...
> 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.
Assignee | ||
Comment 37•16 years ago
|
||
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.
Assignee | ||
Comment 38•16 years ago
|
||
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.
Attachment #373102 -
Attachment is obsolete: true
Attachment #373274 -
Flags: superreview?(roc)
Attachment #373274 -
Flags: review?(roc)
Attachment #373102 -
Flags: superreview?(roc)
Attachment #373102 -
Flags: review?(roc)
Assignee | ||
Comment 39•16 years ago
|
||
Oops, that patch had some cruft in it.
Attachment #373274 -
Attachment is obsolete: true
Attachment #373275 -
Flags: superreview?(roc)
Attachment #373275 -
Flags: review?(roc)
Attachment #373274 -
Flags: superreview?(roc)
Attachment #373274 -
Flags: review?(roc)
Comment 40•16 years ago
|
||
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)
+ 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
Assignee | ||
Comment 42•16 years ago
|
||
Fix review comments.
Attachment #373275 -
Attachment is obsolete: true
Attachment #373592 -
Flags: superreview?(roc)
Attachment #373592 -
Flags: review?(roc)
Attachment #373275 -
Flags: superreview?(roc)
Attachment #373275 -
Flags: review?(roc)
Assignee | ||
Updated•16 years ago
|
Attachment #355343 -
Attachment description: Patch 3 → Patch 3 (Checked in)
Attachment #373592 -
Flags: superreview?(roc)
Attachment #373592 -
Flags: superreview+
Attachment #373592 -
Flags: review?(roc)
Attachment #373592 -
Flags: review+
Comment 44•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
(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
Assignee | ||
Comment 50•16 years ago
|
||
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.
Assignee | ||
Comment 51•16 years ago
|
||
But that could probably be a topic of another bug, after this is checked in?
Comment 52•16 years ago
|
||
> 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•16 years ago
|
||
(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•16 years ago
|
||
Specifically, we use notifications because we don't want to steal focus.
Comment 55•16 years ago
|
||
(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•16 years ago
|
||
(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.
Assignee | ||
Comment 57•16 years ago
|
||
(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•16 years ago
|
||
> 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?
Assignee | ||
Comment 59•16 years ago
|
||
(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.
Assignee | ||
Comment 60•16 years ago
|
||
I suppose I can just not add the action callback if the server doesn't support them.
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 61•16 years ago
|
||
(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•16 years ago
|
||
> 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•16 years ago
|
||
(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•16 years ago
|
||
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... :)
Assignee | ||
Comment 65•16 years ago
|
||
This falls back to XUL if the server doesn't support actions.
Attachment #373592 -
Attachment is obsolete: true
Attachment #375563 -
Flags: superreview?(roc)
Attachment #375563 -
Flags: review?(roc)
Attachment #375563 -
Flags: superreview?(roc)
Attachment #375563 -
Flags: superreview+
Attachment #375563 -
Flags: review?(roc)
Attachment #375563 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 66•16 years ago
|
||
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
Keywords: checkin-needed
Assignee | ||
Comment 67•16 years ago
|
||
Er, I remember libnotify dev headers being installed on mozilla-central, I'll take another look at the bug.
Assignee | ||
Comment 68•16 years ago
|
||
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•16 years ago
|
||
linux64 slave now has a libnotify-devel installed, previously only had libnotify. Details in bug 473831 comment #9.
Comment 71•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c2ba27e9e639 For some reason, Linux x86-64 mozilla-central hasn't started building yet...
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
See Also: → https://launchpad.net/bugs/501393
Comment 72•14 years ago
|
||
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•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•