Closed Bug 499557 Opened 15 years ago Closed 13 years ago

New mail alert should not be displayed when running a full-screen application

Categories

(Thunderbird :: OS Integration, enhancement)

x86_64
Windows 7
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: the.assimilator, Assigned: gcp)

References

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.1b99) Gecko/20090605 Firefox/3.5b99 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090223 Thunderbird/3.0b2

If I am running a full-screen application (e.g. videogame) with Thunderbird running in the background, and I receive a new email, Thunderbird pops up the new mail alert dialog, which causes flickering in the bottom-right corner of the screen while the alert is appearing/disappearing.

Reproducible: Always

Steps to Reproduce:
1. Run Thunderbird.
2. Run a full-screen application.
3. Send a mail to one of the accounts configured in Thunderbird.
4. Notice how the full-screen application is "disturbed" by the new mail alert.
Actual Results:  
New mail alert always displays.

Expected Results:  
If the foreground application is running in full-screen mode, the new mail alert should not be displayed (or at least, there should be an option to prevent this).
This has been an issue since Thunderbird 1.x IIRC.
Sid is there an easy way to detect that the Foreground app is running full screen ?
For Vista, there's SHQueryUserNotificationState: http://msdn.microsoft.com/en-us/library/bb762242(VS.85).aspx, which would resolve the issue for games as well as presentations, etc.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We should fix this. Thanks Jim for the pointer.
Patch attached which prevents popping up notifications when Windows does not think it's appropriate (in fullscreen game, when presenting, ...).

Who on the thunderbird team can review this patch/get it landed?
Comment on attachment 564079 [details] [diff] [review]
Patch 1. Don't notify when fullscreen/presenting

I know Sid's done a bunch of good work on Windows platform integration, so he might be a good person to look at this. Feel free to pass this review on to anyone else though, Sid. :)
Attachment #564079 - Flags: review?(sagarwal)
One thing we should think about: should we show a new mail alert during "quiet time" (defined as "the first hour after a new user logs into his or her account for the first time")? Quiet time is supposed to be a period when notifications are minimized, but new email is probably pretty high-priority, so maybe we should show the new mail alert during quiet time too.
The documentation says "If this function returns QUNS_QUIET_TIME, notifications should be displayed only if critical."

While new mail *might* be important ("You have new mail. It's not spam!"), it does not qualify as critical. Note that the audio jingle will still play.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> While new mail *might* be important ("You have new mail. It's not spam!"),
> it does not qualify as critical. Note that the audio jingle will still play.

I wonder if we shouldn't play the audio jingle either. CCing Blake for UX input.
If we don't show the popup, we shouldn't play the audio.  I'm not totally enthused about not notifying the user when they get new mail, though.  Particularly since it sounds like it's only the new users who would be affected by this, and not showing them that we can actually check their mail and tell them about it seems like it would be a worse user experience.

I would consider a patch which restricted us to one notification in the quiet period, so that the user can tell that we can check that they have new mail, but we don't overwhelm them with notifications…

Thanks,
Blake.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #10)
> If we don't show the popup, we shouldn't play the audio.  I'm not totally
> enthused about not notifying the user when they get new mail, though. 
> Particularly since it sounds like it's only the new users who would be
> affected by this, and not showing them that we can actually check their mail
> and tell them about it seems like it would be a worse user experience.
> 
> I would consider a patch which restricted us to one notification in the
> quiet period, so that the user can tell that we can check that they have new
> mail, but we don't overwhelm them with notifications…
> 
> Thanks,
> Blake.

Why not add a preference (maybe something in about:config) to enable/disable the patch behavior, and set it to disabled by default? That way, TB works exactly as it currently does - but users like me who don't want to see notifications in this scenario, can enable the setting.
I'm not a big fan of preferences, in general.  I suspect we can think long and hard about what the right thing to do here is, and figure it out.

(Oh, and I'm totally on board with the original proposal of not showing alerts when an application is full-screened.  My comment is only for the quiet-time period.)
I don't think there is any user that will want the current behavior of being thrown out of full-screen applications [*], or having a presentation interrupted, when he gets new mail.

Do we agree that is far more serious than whether or not the jingle still plays, or the almost philosophical question whether or not a user should get new mail notifications in the first hour he uses his computer?

If so, please consider merging a simple fix first (like the attached patch), and implement the more advanced behavior Blake asks for in follow-up bugs. This bug hasn't moved in 2 years - it'd be painful if it keeps languishing here because nobody has the time to implement the "perfect" solution.

[*] This is what actually happens for me instead of just flickering.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #13)
> I don't think there is any user that will want the current behavior of being
> thrown out of full-screen applications [*], or having a presentation
> interrupted, when he gets new mail.
> 
> Do we agree that is far more serious than whether or not the jingle still
> plays, or the almost philosophical question whether or not a user should get
> new mail notifications in the first hour he uses his computer?
> 
> If so, please consider merging a simple fix first (like the attached patch),
> and implement the more advanced behavior Blake asks for in follow-up bugs.
> This bug hasn't moved in 2 years - it'd be painful if it keeps languishing
> here because nobody has the time to implement the "perfect" solution.
> 
> [*] This is what actually happens for me instead of just flickering.

Agreed 100%. Being thrown violently out of the current fullscreen app, just so TB can tell me I have new mail that I most likely don't want to deal with right now, is the very reason I opened this bug. Besides, the new mail icon will still be displayed, so users will know they have received mail while they were otherwise occupied.
Huh, I didn't think I asked for particularly advanced behaviour…

Anyways, I'm happy for the "quiet time" patch to be a different bug, and the "fullscreen" patch to be this bug, if that makes everyone happy.

Thanks,
Blake.
>Huh, I didn't think I asked for particularly advanced behaviour…

It's advanced enough that I can't whip up a patch for it as easily (I know almost nothing about TB). If someone on the TB team has time to implement it, of course my point is moot. But this bug being open for >2 years suggests you guys have your hands full :-)
Comment on attachment 564079 [details] [diff] [review]
Patch 1. Don't notify when fullscreen/presenting

Although Blake's been commenting here, this is a UX change so we should still get explicit ui-review.
Attachment #564079 - Flags: ui-review?(bwinton)
Comment on attachment 564079 [details] [diff] [review]
Patch 1. Don't notify when fullscreen/presenting

This sounds like something we want to do, although I don't have good steps to test how it works in practice.  Still, I'm going to say ui-r=me on the premise.
Attachment #564079 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 564079 [details] [diff] [review]
Patch 1. Don't notify when fullscreen/presenting

Review of attachment 564079 [details] [diff] [review]:
-----------------------------------------------------------------

I've tested this patch by launching a full-screen application (a game) and sending a message to an email account. Without the patch the alert showed up, while with the patch the sound played but the alert didn't show up.

r+ modulo comments.

::: mailnews/base/src/nsMessengerWinIntegration.cpp
@@ +83,5 @@
>  #define PROFILE_COMMANDLINE_ARG " -profile "
>  
>  #define XP_SHSetUnreadMailCounts "SHSetUnreadMailCountW"
>  #define XP_SHEnumerateUnreadMailAccounts "SHEnumerateUnreadMailAccountsW"
> +#define XP_SHQueryUserNotificationState "SHQueryUserNotificationState"

The XP here is misleading because the function is Vista+. Let's rename it to Vista_SHQueryUserNotificationState. I wouldn't mind it if you simply used the literal string in the GetProcAddress call either.

@@ +537,5 @@
>    if (prefBranch)
>      prefBranch->GetBoolPref(SHOW_ALERT_PREF, &showAlert);
>  
> +  // check if we are allowed to show a notification
> +  if (mSHQueryUserNotificationState) {

Let's make this if (showAlert && mSHQueryUserNotificationState).

::: mailnews/base/src/nsMessengerWinIntegration.h
@@ +60,5 @@
> +    QUNS_BUSY = 2,
> +    QUNS_RUNNING_D3D_FULL_SCREEN = 3,
> +    QUNS_PRESENTATION_MODE = 4,
> +    QUNS_ACCEPTS_NOTIFICATIONS = 5
> +} QUERY_USER_NOTIFICATION_STATE;

- Please add QUNS_QUIET_TIME = 6 to this.
- I'm worried about the name conflicting with Windows' own at some point in the future. Could you change the name of the enum to MOZ_QUERY_USER_NOTIFICATION_STATE or similar?
Attachment #564079 - Flags: review?(sagarwal) → review+
Landed with comments addressed: http://hg.mozilla.org/comm-central/rev/46b0cf2f1a85
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Ludo, will it be possible to have a litmus test for this (Windows Vista and 7 only)? I don't know what we could use as a fullscreen app and how we could send an email to the account. I've been using a fullscreen game on Steam and my smartphone to do the testing, but not everyone will have access to them.
Flags: in-litmus?(ludovic)
Another way is to ask people to switch to presentation mode in Windows, but unfortunately that's Professional and above only. :/
I've filed a bug for the toolkit equivalent: bug 699014.
(In reply to Siddharth Agarwal [:sid0] from comment #21)
> Ludo, will it be possible to have a litmus test for this (Windows Vista and
> 7 only)? I don't know what we could use as a fullscreen app and how we could

I'll add them this week and or week-end.
Blocks: 706520
https://litmus.mozilla.org/show_test.cgi?id=40445
Flags: in-litmus?(ludovic) → in-litmus+
See Also: → 621323
You need to log in before you can comment on or make changes to this bug.