305375 - Alerts service not built on non-Win32 platforms (gtk1/2)

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Jens Bannmann]

Currently, the alerts service, the component for sliding in notification popups
in the lower right corner of the screen, is only built on Windows platforms -
although its implementation is cross-platform. The result is that e.g. Linux
users don't get the global notifications like "Downloads complete" or "New mail"
Windows users are used to. In the case of Thunderbird, this means the user
doesn't get *any* visual notification of new mail on linux, as we don't have a
'new mail' tray icon there, either. (Tb needs additional code to actually call 
the alerts service, but that is another story.)

Therefore, I propose to remove the ifdefs around the alerts service code.

Zach on irc:
> it was restricted to windows for a very good reason. the existing UI
> convention on windows is notification through the task bar, no such
> convention exists on linux or mac

IMO, we should do this even though there's no such convention, because there's
little reason to "discriminate" users of non-Windows platforms. They have as
much use for visual notifications as everyone else. However, this is a user
experience question, so it's up to you, Mike. What do you think?

Comment 1

[Jens Bannmann]

Attachment: patch

Removes the ifdef XP_WIN around the alerts service, and no longer alters the
window height of the alert to avoid trouble with minimal window sizes. Makes
alerts work on Linux (to be tested on Mac).

Comment 2

[Jens Bannmann]

Attachment: patch v2

Win32 doesn't like sliding in the full window, so this patch keeps the old
method for Win32 while not altering window.outerHeight for other platforms.

Comment 3

[Jens Bannmann]

Comment on attachment 193332 [details] [diff] [review]
patch v2

Screw this. Neil was so kind and reminded me of multiple monitor setups and
taskbars which aren't always-on-top, two scenarios where sliding in the full
window isn't an option. The solution is to stay with altering
window.outerHeight, but just start with 1 pixel instead of 0. New patch follows
once I verified things on Win32.

Comment 4

[Jens Bannmann]

Attachment: patch v3

Improved and simplified version. Additionally, fixes a bug where a 1px border
appeared on the right side of the content (white/gray).

Comment 5

[[e-mail]]

(In reply to comment #4)
>Additionally, fixes a bug where a 1px border
>appeared on the right side of the content (white/gray).
Also mentioned in bug 227951 and other bugs, but not actually fixed as such :-(

Comment 6

[Benjamin Smedberg]

Jens, I'm not sure I understand what this patch *does* in terms of UI. What does
this actually look like (on mac, for instance).

Mike & Mike: what is the desired behavior of alerts on non-windows?

Comment 7

[Benjamin Smedberg]

Comment on attachment 193346 [details] [diff] [review]
patch v3

This is almost certainly not what we want to do on mac, at least without
significant UI thinking.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

While less sure about Linux, I'm positive that this really isn't what we want to
be doing on Mac without instead considering using an existing OS X notification
system like Growl (see: Adium) or making better use of the available frameworks
for dock icon decorators.

Comment 9

[Jens Bannmann]

(In reply to comment #8)
> While less sure about Linux, I'm positive that this really isn't what we want to
> be doing on Mac without instead considering using an existing OS X notification
> system like Growl (see: Adium) or making better use of the available frameworks
> for dock icon decorators.

Agreed. Regarding Linux, I saw some screenshots of Gnome applications doing
these kinds of notifications. It seems not as common as on Windows, but it isn't
formally forbidden either. KDE even has an API for it, though using that one
would probably be a separate matter, and to be done by the people maintaining
the Qt port.

Given that unlike to the "new mail" notification icon in the tray area itself,
this is an exposed generic API that can be (and is) used by extensions (or
XULRunner apps) for all sorts of things, I think having this on Linux could be
valuable.

So what do you all think about providing the implementation of the notifications
on Linux, but disabling the download manager pref that triggers its usage by
default? I think this approach would be analogous to other preferences that
activate features not intended for all platforms, but which can be activated by
toggling a hidden pref for those that want it (e.g. the instantApply pref for
the new options dialog, or the "middle mouse click loads selected URL" function).

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

On Linux I'm less concerned, especially if we default the pref to off so that we
don't start changing behaviour wildly. Not a 1.5 blocker, though, IMO.

Comment 11

[Jens Bannmann]

Attachment: patch v4

- Alerts service IDL is now always built.
- Alerts implementation is only built on windows, gtk1 and gtk2.
- In download manager, the alert call is no longer wrapped in an #ifdef.
Instead, it is silently skipped if do_GetService fails.
- The showAlertOnComplete pref defaults to true for windows, false on other
platforms.

Comment 12

[Benjamin Smedberg]

Comment on attachment 194690 [details] [diff] [review]
patch v4

r=me, assuming that any resulting UI changes have been accepted by mconnor or
beltzner.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #12)
> (From update of attachment 194690 [details] [diff] [review] [edit])
> r=me, assuming that any resulting UI changes have been accepted by mconnor or
> beltzner.

I'm fine with Jens's suggestion in comment 9 (enable function in Linux builds
but default the pref to "off").

Comment 14

[Allan Beaufour]

Checked in to trunk

Comment 15

[Jens Bannmann]

Comment on attachment 194690 [details] [diff] [review]
patch v4

Is it possible to get this onto the branch? Summary: No UI change, very low
risk (only makefile changes plus small JS tweaks), high value for
XULRunner/Extension developers, lets gtk users optionally enable download
alerts via the existing hidden pref.

Comment 16

[Jens Bannmann]

Checked in to 1.8 branch by bz, resolving. Thanks, everyone :-)

Comment 17

[Scott MacGregor]

Jens, this patch caused some unpleasant changes for the alert service on
Windows. Please see Bug 310226. The alert is no longer coming up with the right
height on Windows anymore. 

Comment 18

[Scott MacGregor]

I put a patch in that bug which backs out the alert.js change made here to
restore things on Windows until such time as Jens or others come up with a fix
that doesn't regress Windows.

Comment 19

[Jens Bannmann]

See also bug 310900.