pref mail.biff.show_alert works only on Windows

RESOLVED FIXED

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: wolfiR, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed1.8.1})

1.8 Branch
x86
Linux
fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
BTW: I found the Mailnews: Notification component only in Product Mozilla Application Suite but it should have been Core.
(Reporter)

Comment 2

12 years ago
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

12 years ago
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?
(Reporter)

Comment 4

12 years ago
(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.
(Assignee)

Comment 5

12 years ago
Created attachment 213581 [details] [diff] [review]
How it might work
Assignee: mail → neil
Status: NEW → ASSIGNED
Attachment #213581 - Flags: superreview?(bienvenu)

Comment 6

12 years ago
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.
Attachment #213581 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 7

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #213581 - Flags: review?(mnyromyr)

Comment 8

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

Comment 9

11 years ago
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. ;-)
(Assignee)

Updated

11 years ago
Attachment #213581 - Flags: review?(mnyromyr) → review?(iann_bugzilla)
(Reporter)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
(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

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

Comment 13

11 years ago
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?
Attachment #213581 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 14

11 years ago
Suite fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

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

Comment 16

11 years ago
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
(Assignee)

Comment 17

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

Comment 18

11 years ago
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
(Assignee)

Comment 19

11 years ago
Um, yes, I forgot to check nsMessengerWinIntegration.h; thanks for poking me.
(Assignee)

Comment 20

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

Comment 21

11 years ago
Could we get this patch on the 1.8 branch?
(Assignee)

Comment 22

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

Do you want a new patch including bustage fixes?
Attachment #213581 - Flags: approval-branch-1.8.1?(mscott)

Updated

11 years ago
Attachment #213581 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
(Assignee)

Comment 23

11 years ago
Fix checked in to the branch.
Keywords: fixed1.8.1

Updated

11 years ago
Blocks: 332160

Updated

9 years ago
Component: MailNews: Notification → MailNews: Message Display
QA Contact: search
You need to log in before you can comment on or make changes to this bug.