327613 - pref mail.biff.show_alert works only on Windows

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

SeaMonkey and Thunderbird both have the preference mail.biff.show_alert and an UI for setting this pref.
As far as I could see in the code this is only used in

mailnews/base/src/nsMessengerWinIntegration.cpp

The pref and the UI should be removed from Thunderbird and SeaMonkey preferences for Linux platform.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

BTW: I found the Mailnews: Notification component only in Product Mozilla Application Suite but it should have been Core.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

According to bug 158711 this should not happen if no alerts-service is available.
But actually there is a alerts-service installed in SeaMonkey:
xpfe/components/alerts

Although it's registered it doesn't get fired IMHO.
Sorry, I'm not sure what was/is meant by this alert and if it actually should work. At least it doesn't for me so we don't need it at all or fix to do something.

Comment 3

[David :Bienvenu]

for that to get fired on linux, someone would need to write an nsMessengerUnixIntegration.cpp and hook it up to use the alert service.

I believe OS/X shows an alert, but it isn't controlled by this pref (that should be easy enough for someone with OS/X to fix :-) ).

I think the download manager uses the alert service. Not sure if it uses it on all platforms.

Checking if there's an alert service doesn't seem like the right fix anymore. Can we do platform #if's in the XUL, as ugly as that would be?

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #3)
> for that to get fired on linux, someone would need to write an
> nsMessengerUnixIntegration.cpp and hook it up to use the alert service.

A nsMessengerUnixIntegration.cpp isn't possible easily I fear.
Maybe a nsMessengerGnomeIntegration.cpp would be but I'm not the right person
to write something like that. Maybe there is also some freedesktop.org standard for notifications (what I don't know, too).

> I think the download manager uses the alert service. Not sure if it uses it on
> all platforms.

I never saw an alert on Linux and I think there is also none implemented.
 
> Checking if there's an alert service doesn't seem like the right fix anymore.
> Can we do platform #if's in the XUL, as ugly as that would be?

If you've asked me, I simply don't know if we should do it but at least it would be possible. I just want to remove this UI if it doesn't do anything at all ;-)
Given that Thunderbird (and Firefox) is designed for simplicity it's almost a "major" bug there.
Adding Neil to CC for SeaMonkey.

Comment 5

[[e-mail]]

Attachment: How it might work

Comment 6

[David :Bienvenu]

Comment on attachment 213581 [details] [diff] [review]
How it might work

+#if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2)
+    { "OS/2 OS Integration", NS_MESSENGERUNIXINTEGRATION_CID,

literal string should be something like "Unix Integration"

+#ifdef MOZ_THUNDERBIRD
+    // Since the new mail notification alert isn't ready for primetype yet,

OK, my bad for not catching this in the file this code was taken from, but that should be "prime-time".

Does this code work on linux? I ask because of your "How it might work" description. That would be great if it worked!

We might want to have a core shared base class for these integration classes, because a lot of the code is common. If we did that, then we'd have to adjust the license and contributors sections. It should make maintenance easier too.

Comment 7

[[e-mail]]

(In reply to comment #6)
>+    { "OS/2 OS Integration", NS_MESSENGERUNIXINTEGRATION_CID,
>literal string should be something like "Unix Integration"
D'oh, too much copy & paste :-[

>Does this code work on linux? I ask because of your "How it might work"
>description. That would be great if it worked!
I had trouble enabling it due to that RDF bug... but yes, it does work.

>We might want to have a core shared base class for these integration classes,
>because a lot of the code is common. If we did that, then we'd have to adjust
>the license and contributors sections. It should make maintenance easier too.
It wasn't obvious which bits of the Windows code I needed and which I didn't.

Comment 8

[David :Bienvenu]

I can do the TB build changes, Neil. You're right that they're trivial.

Comment 9

[Karsten Düsterloh]

I have currently no Linux SM build system available (although I intend to change that over the coming weeks, I don't know how long it'll take), so I can't properly review this for the time being. Feel free to reappoint or wait. ;-)

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

There are already some comments from David.
I haven't reviewed the patch code-wise but I'm using it and it worked so far since a few weeks.
There is one effect I can see which is not perfect:
If I open mailnews the first time and it fetches my mail it also opens the alert window. But that's only minor issue I haven't expected.

Comment 11

[[e-mail]]

(In reply to comment #10)
>There is one effect I can see which is not perfect:
>If I open mailnews the first time and it fetches my mail it also opens the
>alert window. But that's only minor issue I haven't expected.
I just copied the way the alert works on Windows...

Comment 12

[Kai Engert (:KaiE:)]

I just started using a Linux build that has this patch applied.
I really like it very much.

Comment 13

[Ian Neal]

Comment on attachment 213581 [details] [diff] [review]
How it might work

As what David said, though nsMessengerWinIntegration.cpp has now changed so will you be generating a new patch to reflect those changes?

Comment 14

[[e-mail]]

Suite fix checked in.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

how about adding this for SM 1.1 when it has been baken on trunk?

Comment 16

[Stefan Sitter [:ssitter]]

This seems to have broken the Thunderbird+Lightning build on galactica tinderbox (http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird):

mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:243: 
    syntax error before `do'
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:244: 
    syntax error before `->' token
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:245: 
    syntax error before `->' token
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:249: 
    warning: ISO C++ forbids declaration of `rv' with no type
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:249: 
    redefinition of `int rv'
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:223: 
    `int rv' previously defined here
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:250: 
    `argsArray' was not declared in this scope
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:253: 
    warning: ISO C++ forbids declaration of `mAlertInProgress' with no type
mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:254: 
    syntax error before `}' token
{standard input}: Assembler messages:
{standard input}:966: Error: symbol `rv' is already defined
gmake[6]: *** [nsMessengerUnixIntegration.o] Error 1

Comment 17

[[e-mail]]

Sorry, I'd mismerged the changes from nsMessengerWinIntegration.cpp, fix is in.

Comment 18

[Stefan Sitter [:ssitter]]

The Thunderbird+Lightning build on galactica tinderbox (http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird) is still broken:

mozilla/mailnews/base/src/nsMessengerUnixIntegration.cpp:207: 
  no `nsresult nsMessengerUnixIntegration::ShowNewAlertNotification(int)' 
  member function declared in class `nsMessengerUnixIntegration'
gmake[6]: *** [nsMessengerUnixIntegration.o] Error 1

Comment 19

[[e-mail]]

Um, yes, I forgot to check nsMessengerWinIntegration.h; thanks for poking me.

Comment 20

[[e-mail]]

I changed my mind and reverted the unix version to the old windows version.

Comment 21

[Kai Engert (:KaiE:)]

Could we get this patch on the 1.8 branch?

Comment 22

[[e-mail]]

Comment on attachment 213581 [details] [diff] [review]
How it might work

Do you want a new patch including bustage fixes?

Comment 23

[[e-mail]]

Fix checked in to the branch.