Closed Bug 857647 Opened 7 years ago Closed 7 years ago
Don't show balloon notification on new mail if tray icon is disabled
(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.
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.
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.
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 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) && 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.
This patch moves "|| sBiffIconData.szInfo" 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.
Attachment #733520 - Flags: review?(neil)
Comment on attachment 733520 [details] [diff] [review] Proposed fix OK, this seems simple enough.
Attachment #733520 - Flags: review?(neil) → review+
Thanks, push for trunk please.
Whiteboard: [c-n: comm-central]
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 23.0
You need to log in before you can comment on or make changes to this bug.