332160 - Hook up new mail alert notifications for UNIX

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

Neil just did some work for the mail notification in seamonkey (see Bug 327613) to make it work for UNIX builds.

We should add this to the build for Thunderbird and expose the pref UI to control it.

Comment 1

[Scott MacGregor]

actually it looks like we don't hide the show an alert pref in the options UI for unix, so the UI is already there! This might be as easy as just adding Neil's new file to the Thunderbird build.

Comment 2

[Magnus Melin [:mkmelin]]

OS -> linux :)

Comment 3

[Scott MacGregor]

Magnus, are you on linux? If so, do you want to hook this up? Should be pretty straight forward.

Comment 4

[Magnus Melin [:mkmelin]]

Working on it...

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This is mostly copy-paste from the windows integration, making unix tb use the same alert as on windows.

Comment 6

[Scott MacGregor]

Comment on attachment 218189 [details] [diff] [review]
proposed fix

awesome. How does it look on unix? Does the existing pref UI work ok for turning it off?

I can land this on the trunk and branch if you need me too.

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: screenshot - new mail notification in linux/kde

> How does it look on unix? Does the existing pref UI work ok for turning it off?

As you can see in the screenshot, the close notification x is cut off on top by 1px or so for me, I don't know if it's something with my system...? But other than that, I think it looks quite nice! The pref works fine.

Yep, do check it in, I don't have cvs checkin rights.

Comment 8

[Scott MacGregor]

Hmm, I wonder why the blue border  and the white background aren't showing up. Some of the style rules don't seem to be working compared to the windows version:

http://www.mozilla.org/projects/thunderbird/specs/images/newmailalert.png

Comment 9

[Scott MacGregor]

does dom inspector show anything interesting in the window that would explain why the blue border is missing? The border style is coming from:

http://lxr.mozilla.org/mozilla/source/mail/themes/qute/mail/newmailalert.css#47

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: screenshot with css fix - new mail notification in linux/kde

Ok, I got it working... Possible patch coming up.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: proposed additional fix, make the alert look like on windows -- does not work on win

This patch fixes the color issues for linux. Hope it works for windows too.

I'm not sure what the problem was. Earlier, I was able to change fonts and stuff, just not the border-color and background-color. I think the border was actually there, only not the in correct color. At least I was able to change it's width.

Comment 12

[Scott MacGregor]

Comment on attachment 219340 [details] [diff] [review]
proposed additional fix, make the alert look like on windows -- does not work on win

Very strange, this doesn't work on windows. The styles need to be on the window element not the stack for Windows, but on the stack element for Linux? How odd.

Comment 13

[Scott MacGregor]

Rob, this is the bug that's going to add the nice alert notification to linux that I was telling you about.

nsMessengerUnixIntegration.cpp is where we would also want to add code to add a system tray icon.

Comment 14

[Scott MacGregor]

*** Bug 342210 has been marked as a duplicate of this bug. ***

Comment 15

[Scott MacGregor]

jens, can you help us figure out why the first patch in this bug doesn't seem to look right on linux? 

Comment 16

[Jens Bannmann]

(In reply to comment #15)
> jens, can you help us figure out why the first patch in this bug doesn't seem
> to look right on linux? 

I cannot see anything obvious right now. I will have a more detailed look at it this weekend, though.

Comment 17

[Michael Cronenworth]

Yes! I hope this makes Thunderbird 2.0 (or come on a fix for 1.5). I've been missing out on new mail alerts since I've moved to Linux from Windows.

Comment 18

[Rob]

Does this alert rely on a system tray?  How about window managers that don't have one (eg. WindowMaker).

Ideally I would like to see the application icon change for the minimised window if I have unread mail (not new mail), or if this is not possible change the window title to 'unread'.  This means I can set up mail filters so I don't get notified and disrupted for new spam etc.

Comment 19

[Scott MacGregor]

Jens, do you have any time to help shed some light on this?

Comment 20

[Jens Bannmann]

(In reply to comment #19)
> Jens, do you have any time to help shed some light on this?

Scott, I can't explain the win32/unix difference in styles at all. I guess this is some oddity in widget implementations (not that I know any details in that area). You should ask some widget folks for explanations and/or workarounds.

If the behaviour cannot be circumvented or fixed, we could use the chrome registry's flags for win/unix/mac here to include different CSS files. Perhaps #ifdef'ing the relevant style rules between both platforms is also possible.

Comment 21

[Scott MacGregor]

Ok, let's do this for now. Unfortunately we can't use the xul pre-processor on CSS files because the # used for element ids confuses the pre-processor.

But we have a couple options.

Option A:

1) Add a new theme file newMailAlertUnix.css in mail\themes\qute\mail
2) Add the styles for #alertContainer
3) we can ifdef newMailAlert.xul to include newMailAlertUnix.css for unix

Option B:
If that doesn't work, we can fork newMailAlert.css all together and have a copy that gets loaded for windows and another version that gets loaded for unix.

Jens or Magnus, do one of you want to try out option A? I don't think it will be too hard and I'll gladly help get it checked in if it works.

Comment 22

[Adam Guthrie]

*** Bug 360545 has been marked as a duplicate of this bug. ***

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

This is basically the same patch as before + "Option A". Applying it will make the UNIX alert look like in attachment 219336 [details].

Comment 24

[Scott MacGregor]

Comment on attachment 246611 [details] [diff] [review]
proposed fix, v2

awesome thanks for doing this.

Comment 25

[Scott MacGregor]

I just checked this into the trunk.

The unix mail notification class added for bug 327613 isn't on the branch so we need to land that first.

Comment 26

[Scott MacGregor]

(In reply to comment #25)
> I just checked this into the trunk.
> 
> The unix mail notification class added for bug 327613 isn't on the branch so we
> need to land that first.
> 

I'm being stupid. 327613 is indeed on the branch.

Comment 27

[Tony Mechelynck [:tonymec]]

see regression crash bug 362225 on today's Tb 2.0a1 nightly. Rollback?

Comment 28

[Tony Mechelynck [:tonymec]]

(In reply to comment #27)
> see regression crash bug 362225 on today's Tb 2.0a1 nightly. Rollback?
> 

Oops. 2.0b1, namely "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1pre) Gecko/20061129 Thunderbird/2.0b1pre" - Build ID: 2006112903

Comment 29

[Scott MacGregor]

I've backed this out of the branch where it was causing that crash. It's still checked into the trunk. 

Comment 30

[Tony Mechelynck [:tonymec]]

(In reply to comment #29)
> I've backed this out of the branch where it was causing that crash. It's still
> checked into the trunk. 
> 

"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1pre) Gecko/20061130 Thunderbird/2.0b1pre" - Build ID: 2006113004

Crash has indeed disappeared. Alert again can't be displayed regardless of "Edit => Preferences => General => When new messages arrive, [x] Show an alert" (just like it was until day before yesterday).

Comment 31

[Scott MacGregor]

Magnus, what happens if you back out say just the unix specific CSS changes on the branch, leaving the alert itself running with broken CSS, does is still crash? Just trying to isolate the specific thing that's triggering the crash since the stack traces seem random.

Comment 32

[Magnus Melin [:mkmelin]]

Attachment: the backed out branch changes with problem fixed

This seems to fix it... I'm no longer crashing after I removed the unix specific rules for min-height and background-color on #newMailAlertNotification. Turns out they were not necessary.

Comment 33

[Scott MacGregor]

Comment on attachment 247444 [details] [diff] [review]
the backed out branch changes with problem fixed

thanks for the extra effort Magnus.

Comment 34

[Scott MacGregor]

I've landed this new branch patch on the branch.

Comment 35

[Mike Cowperthwaite]

*** Bug 315387 has been marked as a duplicate of this bug. ***