857647 - Don't show balloon notification on new mail if tray icon is disabled

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[rsx11m]

(Quoting bug 856454 comment #16)
> > > This implies that if you uncheck "Show a tray icon" the "Show a balloon"
> > > checkbox is unchecked as well and disabled as the balloon implies the tray
> > > icon.
> > No, just disabled, in the same way that unchecking "Show an alert" will
> > disable all related fields withut clearing them.
> 
> This won't work, unfortunately. I have to actively reset the preference (and
> thus the checkbox) if "Show tray icon" is cleared. That's a backend issue,
> if you have mail.biff.show_balloon set it will show an icon regardless of
> the mail.biff.show_tray_icon value. I don't know which trade-offs may have
> been necessary to implement the feature, but as of now the patch needs to
> uncheck the box as is.

Steps to reproduce:

 - mail.biff.show_balloon = false
 - mail.biff.show_tray_icon = true
 - new mail > icon but no balloon notification is shown (expected)

 - mail.biff.show_balloon = true
 - mail.biff.show_tray_icon = true
 - new mail > icon with balloon notification is shown (expected)

 - mail.biff.show_balloon = false
 - mail.biff.show_tray_icon = false
 - new mail > no icon and no balloon notification is shown (expected)

 - mail.biff.show_balloon = true
 - mail.biff.show_tray_icon = false
 - new mail > icon with balloon notification is shown (unexpected)

If the tray icon itself is disabled, its balloon should be disabled as well, given that it implies the tray icon being available.

Tentatively assigning to myself as I may need it in bug 856454.

Comment 1

[[e-mail]]

I think I may have wanted to avoid the strange logic there is as to when to show the icon, but as you don't think that balloons and alerts mix then I don't think that will be a problem.

Comment 2

[rsx11m]

That shouldn't depend on the regular alerts. The only check I'd like to add is that showing the balloon is tied to the mail.biff.show_tray_icon preference and doesn't just depend on mail.biff.show_balloon alone.

Comment 3

[rsx11m]

Analysis: The entire action is confined within a single file, that's mailnews/base/src/nsMessengerWinIntegration.cpp, which is good. There is a global Windows Shell struct NOTIFYICONDATAW sBiffIconData that collects all information for the icon, and two locations (lines 431 and 477) where it's either filled or left empty based on SHOW_BALLOON_PREF. This can all stay as it is.

The relevant part seems to be the following hunk of bug 605972, http://hg.mozilla.org/comm-central/rev/740d1cf79d2b#l1.197 modifying nsMessengerWinIntegration::AlertFinished(), which contains the logic regarding SHOW_TRAY_ICON_PREF (line 605). Apparently the idea was to show the icon only in certain cases, thus mSuppressBiffIcon can be set to skip that part. The logic before bug 605972 was

>  !mSuppressBiffIcon && SHOW_TRAY_ICON_PREF

and then became

> (!mSuppressBiffIcon && SHOW_TRAY_ICON_PREF) || sBiffIconData.szInfo[0]

where the final term yields true if sBiffIconData was filled with a string, which can only happen before if SHOW_BALLOON_PREF is set. Thus, changing the logic to

> (!mSuppressBiffIcon || sBiffIconData.szInfo[0]) && SHOW_TRAY_ICON_PREF

should provide the desired result, i.e., won't call GenericShellNotify() to show the icon if !SHOW_TRAY_ICON_PREF applies, despite SHOW_BALLOON_PREF being set.

Comment 4

[rsx11m]

Attachment: Proposed fix

This patch moves "|| sBiffIconData.szInfo[0]" up so that it is included in the check for SHOW_TRAY_ICON_PREF per comment #3.

I assume that a single review is sufficient here as it is a local change only without any impact on API or other modules, otherwise please let me know.

Comment 5

[[e-mail]]

Comment on attachment 733520 [details] [diff] [review]
Proposed fix

OK, this seems simple enough.

Comment 6

[rsx11m]

Thanks, push for trunk please.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/1d584cce7616