Closed Bug 552770 Opened 12 years ago Closed 5 years ago

Implement nsIAlertsService for Qt

Categories

(Core Graveyard :: Widget: Qt, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: dougt, Assigned: MikeK)

Details

Attachments

(1 file, 3 obsolete files)

We depend on the system alert service.  Currently gnome has an implementation here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsAlertsService.cpp#50

it is registered here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsGnomeModule.cpp#78

Basically we need something similar for Qt.
Attached patch mozilla-central part of patch (obsolete) — Splinter Review
Note - in this version the icon on the alert dialog is missing in FF qt builds - works in mobile qt builds
(In reply to comment #1)
> Created an attachment (id=435947) [details]
> mozilla-central part of patch

Which happens to be all that is needed :)
Attached patch Now without debug code (obsolete) — Splinter Review
Fits mozilla-central:

changeset:   40005:cc980f275250
tag:         tip
user:        Robert Longson <longsonr@gmail.com>
date:        Tue Mar 30 13:21:18 2010 +0100
summary:     Bug 388547 - reenable reftest.
Attachment #435947 - Attachment is obsolete: true
Assignee: nobody → mkristoffersen
Attachment #435958 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #440384 - Flags: review?(dougt)
btw - note that the qt stuff is identical to the Gnome stuff, except the GDK dependent code has been commented out (makes it easier to put it in again, if we decide on allowing GDK together with QT).  

Result of patch is as expected on mobile, we still have the icon issue in pure FF Qt builds (due to the GDK dependency of libnotify)
Mike, maybe we should create a new directory called libnotify as a peer to gnome.  Then move the libnotify specific files into that new directory.  We can have libnotify configure tests that we key on in our Makefiles.

Michael -- we want Qt to also use some of the code that is in the gnome directory (stuff that is really specific to libnotify, not gnome).  What do you think?
Attachment #440384 - Flags: review?(dougt) → review-
Hrmmm... libnotify is really a library for the GTK world, as evident by its API that you had to comment out. Isn't there a Qt API you could use?
Well, what we want to use is actually the notification deamon (libnotify is just a nice wrapper interface for it) but since libnotify was available for use in Qt on Maemo ( http://wiki.forum.nokia.com/index.php/How_to_show_notification_from_Qt_aplication_in_Maemo_5 ) we agreed that it was easier to at least temporary use libnotify for QT also.

If we were (not saying that we wont) to use the D-bus interface to get to the notification deamon, then it would probably make sense to remove our use of libnotify for Gnome also (so we don't have two ways of doing the same thing).

And create a shared notification folder?  The question is, how much effort do we want to put into suporting FF qt builds and do we want to support it? - as it turns out we are not using this code in Maemo anyway (its handled in JS).
This is the right way to do it for ifdef maemo5 and QT.
http://wiki.forum.nokia.com/index.php/How_to_show_notification_from_Qt_aplication_in_Maemo_5

Also for desktop:
Gnome - libnotify or gnome notify
Kde - their own notification wrapper I guess.

Maemo 6 - don't know yet, need to ask our architect....
(In reply to comment #9)
> This is the right way to do it for ifdef maemo5 and QT.
> http://wiki.forum.nokia.com/index.php/How_to_show_notification_from_Qt_aplication_in_Maemo_5
> 
> Also for desktop:
> Gnome - libnotify or gnome notify
> Kde - their own notification wrapper I guess.
> 
> Maemo 6 - don't know yet, need to ask our architect....

Is there a way around the GDK dependency that libnotify has for icons?  (when doing a QT "only" build)
Attachment #440384 - Attachment is obsolete: true
Change summary
Summary: toolkit/system/qt → Implement nsIAlertsService for Qt
While I have attached a patch updated to trunk, it won't be needed as the code show correct behavior on trunk - while it doesn't do it the way indicated by the reporter, I assume it instead use the JS version of the dialogs.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Hmm - my mistake doesn't work on FF Qt build - so the question becomes if we should support it on FF Qt?  and if the JS implementation is what we want
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
A lot of talking above, let me see if I can summarize where we are:

For mobile, we have a working solution on trunk - working in the sense that it will display notifications (with icons) on the display - this is not ideal, as it use our own layout to do so, meaning it doesn't integrate very well with the platform that it is running on.

libnotify uses the D-bus to talk with the notification daemon to show notifications, the problem with libnotify is that it depends on GDK.

Maemo 6 has their own wrapper - I assume it is also a wrapper for the D-bus interface?

The Desktop notification Specification (http://www.galago-project.org/specs/notification/) is the proposed standard that describes the D-bus interface for doing notifications.

-----

To limit the amount of code we need to maintain, my proposal is that we create some new code that talks directly to the D-Bus, and abandon our current libnotify solution (assuming this will work on maemo 6).

However as the code actually displays the notifications now, I see this as a change with lower priority for mobile qt builds (its cosmetic).  We don't have any plans to ship Firefox in a Qt version, so for the moment it seems safe to ignore that qt builds of firefox won't have alert dialogs.
In bug 1282866, I have removed the QT code from the Firefox tree with the approval of the active peer. That code is not currently maintained by any team.

I have looked through the bugs in the Widget: Qt bugzilla component and I believe all of these are no longer relevant because the Qt code has been removed. If you believe that this bug is still valid, please move it to another more appropriate bugzilla component as you reopen it.
Status: REOPENED → RESOLVED
Closed: 12 years ago5 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.