Last Comment Bug 857647 - Don't show balloon notification on new mail if tray icon is disabled
: Don't show balloon notification on new mail if tray icon is disabled
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86_64 Windows 7
: -- minor (vote)
: Thunderbird 23.0
Assigned To: rsx11m
:
Mentors:
Depends on:
Blocks: 605972 856454
  Show dependency treegraph
 
Reported: 2013-04-03 10:15 PDT by rsx11m
Modified: 2013-04-07 20:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Proposed fix (1.18 KB, patch)
2013-04-04 13:05 PDT, rsx11m
neil: review+
Details | Diff | Splinter Review

Description rsx11m 2013-04-03 10:15:24 PDT
(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 neil@parkwaycc.co.uk 2013-04-03 12:02:54 PDT
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 2013-04-03 13:21:09 PDT
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 2013-04-04 12:37:21 PDT
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 2013-04-04 13:05:05 PDT
Created attachment 733520 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2013-04-04 13:29:21 PDT
Comment on attachment 733520 [details] [diff] [review]
Proposed fix

OK, this seems simple enough.
Comment 6 rsx11m 2013-04-04 13:38:59 PDT
Thanks, push for trunk please.
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-04-05 05:37:34 PDT
https://hg.mozilla.org/comm-central/rev/1d584cce7616

Note You need to log in before you can comment on or make changes to this bug.