Last Comment Bug 874899 - Hide new options in Notifications preference pane when the old alert is selected
: Hide new options in Notifications preference pane when the old alert is selected
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
-- minor (vote)
: seamonkey2.21
Assigned To: rsx11m
Depends on:
Blocks: 856454
  Show dependency treegraph
Reported: 2013-05-22 08:12 PDT by rsx11m
Modified: 2013-05-28 11:33 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proposed patch (1.65 KB, patch)
2013-05-22 08:23 PDT, rsx11m
iann_bugzilla: review+
neil: ui‑review+
iann_bugzilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description User image rsx11m 2013-05-22 08:12:01 PDT
(Quoting rsx11m from bug 840474 comment #19)
> Note that bug 856454 will provide a UI for the associated preferences in the
> Notifications pane with SeaMonkey 2.20; if the new alert remains off until
> then, those will have to be hidden to avoid user confusion.
> If mail.biff.show_new_alert will stay even after the new alert if finalized,
> it may be a good idea either way to let visibility of those prefs depend on
> its value.
Comment 1 User image rsx11m 2013-05-22 08:23:13 PDT
Created attachment 752766 [details] [diff] [review]
Proposed patch

This is the least intrusive way to make the additional options hide when the new alert is switched off, thus the UI should be safe for further releases shipped with the old alert selected by default.

It should also be easy to be backed out again if the old code ever gets removed (restricted to a single block of code in a single file and easily searchable by the preference name). As written, an exception is thrown in Startup() if the pref does no longer exist, thus it should be easily discoverable if missed.

Sure, it would be nicer to combine the checkboxes in a vbox and hide that box rather than the individual prefs, but that would imply changes in the XUL part and may result in a redundant vbox if mail.biff.show_new_alert gets removed.

No conflict with bug 872133 attachment 749385 [details] [diff] [review], thus both patches can be reviewed and pushed independently.
Comment 2 User image rsx11m 2013-05-22 08:27:36 PDT
BTW: alerts.totalOpenTime should apply also for the old notifications, thus I'm not touching it (and it would be tricky string-wise anyway).
Comment 3 User image rsx11m 2013-05-26 09:04:32 PDT
Thanks for the reviews, push for comm-central please.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2013-05-28 10:17:07 PDT
Comment 5 User image rsx11m 2013-05-28 10:24:08 PDT
Comment on attachment 752766 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): Follow-up fix to bug 856454
User impact if declined: SeaMonkey 2.20 would have a different behavior / UX
Testing completed (on m-c, etc.): tested and works fine with 2.20
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2013-05-28 11:33:10 PDT

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