Last Comment Bug 646894 - [Restart in Safe Mode] Add a checkbox in the prompt to choose between safe and normal restart.
: [Restart in Safe Mode] Add a checkbox in the prompt to choose between safe a...
Status: VERIFIED FIXED
: ux-control, ux-error-recovery
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1b3
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on: 574612
Blocks: 646897 1194744
  Show dependency treegraph
 
Reported: 2011-03-31 10:24 PDT by Philip Chee
Modified: 2015-08-14 08:35 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible patch (2.97 KB, patch)
2011-04-01 01:12 PDT, neil@parkwaycc.co.uk
philip.chee: feedback-
jh: feedback+
Details | Diff | Splinter Review
Addressed review comments [Checked in: Comment 15] (2.92 KB, patch)
2011-04-01 09:15 PDT, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review
Alternative patch (2.49 KB, patch)
2011-04-01 09:17 PDT, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review
Another alternative patch (1.72 KB, patch)
2011-04-01 09:18 PDT, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review
possible l10n improvement [Checkin: comment 20] (3.01 KB, patch)
2011-04-03 05:48 PDT, Jens Hatlak (:InvisibleSmiley)
philip.chee: review+
Details | Diff | Splinter Review

Description Philip Chee 2011-03-31 10:24:44 PDT
From Bug 574612 Comment 21:
> neil@parkwaycc.co.uk      2011-03-31 08:00:14 PDT
> 
> Comment on attachment 523268 [details] [diff] [review]
> Patch v1.1 Restart in Safe Mode only.
> 
> Jens had the neat idea of providing a checkbox in the prompt to choose between
> safe and normal restart, but we could do that in a followup. (I don't know how
> easy it would be to share the label of the checkbox with that of the menuitem.)
Comment 1 neil@parkwaycc.co.uk 2011-04-01 01:12:18 PDT
Created attachment 523535 [details] [diff] [review]
Possible patch

I reused the label to avoid l10n issues.
Comment 2 Robert Kaiser 2011-04-01 06:13:39 PDT
(In reply to comment #1)
> I reused the label to avoid l10n issues.

In that case, you probably should file a followup to disentangle that once we have forked 2.1 from 2.next.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-04-01 08:03:41 PDT
Comment on attachment 523535 [details] [diff] [review]
Possible patch

This is a nice gimmick for sure! With this you can quickly restart the app in normal mode, and get back to normal mode if in safe mode. -> f=me

Maybe we should change the checkbox label to "Disable Add-ons for this restart" or something equally down to the point, but that's mostly nice to have and can therefore be moved to a follow-up.
Comment 4 Philip Chee 2011-04-01 08:40:54 PDT
Comment on attachment 523535 [details] [diff] [review]
Possible patch

>+  var checkboxText = menuitem ? menuitem.getAttribute("label") : "";
>+  var checkbox = { value: true };

Under what circumstances is menuitem null?

>   var rv = Services.prompt.confirmEx(window, promptTitle, promptMessage,
>                                      buttonFlags, restartText, null, null,
>-                                     null, {});
>+                                     checkboxText, checkbox);

If checkboxText can be "" the prompt will come up with a checkbox without any descriptive label right? How would a user respond to that?
Comment 5 Philip Chee 2011-04-01 08:41:21 PDT
p.s. the patch works. so f+ on that.
Comment 6 neil@parkwaycc.co.uk 2011-04-01 08:55:31 PDT
(In reply to comment #4)
>>+  var checkboxText = menuitem ? menuitem.getAttribute("label") : "";
>>+  var checkbox = { value: true };
>Under what circumstances is menuitem null?
In case an extension tries to call it.

>>   var rv = Services.prompt.confirmEx(window, promptTitle, promptMessage,
>>                                      buttonFlags, restartText, null, null,
>>-                                     null, {});
>>+                                     checkboxText, checkbox);
> If checkboxText can be "" the prompt will come up with a checkbox without any
> descriptive label right? How would a user respond to that?
Yeah, my bad, I guess I should have used menuitem && instead of ? : above.
Comment 7 Philip Chee 2011-04-01 09:12:34 PDT
Hmm.
  var checkboxText = menuitem;
  var checkbox = menuitem ? { value: true } : {};
Comment 8 neil@parkwaycc.co.uk 2011-04-01 09:15:39 PDT
Created attachment 523608 [details] [diff] [review]
Addressed review comments
[Checked in: Comment 15]

I'm going to be annoying and post several slightly different versions of this...
Comment 9 neil@parkwaycc.co.uk 2011-04-01 09:17:04 PDT
Created attachment 523609 [details] [diff] [review]
Alternative patch
Comment 10 neil@parkwaycc.co.uk 2011-04-01 09:18:33 PDT
Created attachment 523610 [details] [diff] [review]
Another alternative patch
Comment 11 Philip Chee 2011-04-01 09:44:01 PDT
Comment on attachment 523608 [details] [diff] [review]
Addressed review comments
[Checked in: Comment 15]

I like this one. It allows an extension to pass null to remove the checkbox as an option.
Comment 12 Philip Chee 2011-04-01 09:45:52 PDT
Comment on attachment 523609 [details] [diff] [review]
Alternative patch

> safeModeRestartPromptTitle=Restart with Add-ons Disabled
> safeModeRestartPromptMessage=Are you sure you want to disable all add-ons and restart?
> safeModeRestartButton=Restart
>+safeModeRestartCheckbox=Restart with Add-ons Disabled
Hardly worth a additional string since it'll be string frozen on 2.1 and would likely be removed or changed for 2.next.
Comment 13 Philip Chee 2011-04-01 09:49:52 PDT
Comment on attachment 523610 [details] [diff] [review]
Another alternative patch

Nothing wrong with this. I just like the flexibility in attachment 523608 [details] [diff] [review]
Comment 14 Philip Chee 2011-04-02 02:31:32 PDT
Comment on attachment 523610 [details] [diff] [review]
Another alternative patch

r=me in case you prefer this one.
Comment 15 neil@parkwaycc.co.uk 2011-04-02 14:39:49 PDT
Pushed changeset bf54745a7aec to comm-central.
Comment 16 Jens Hatlak (:InvisibleSmiley) 2011-04-02 16:17:32 PDT
I wonder whether we should add a localization note for the fact that we reuse the menuitem label for the checkbox now. Otherwise localizers might make wrong assumptions (I mean, in other languages, a menuitem label string might be subject to different rules/constraints than a checkbox label string).
Comment 17 Philip Chee 2011-04-02 22:21:52 PDT
Comment on attachment 523609 [details] [diff] [review]
Alternative patch

> I wonder whether we should add a localization note for the fact that we reuse
> the menuitem label for the checkbox now. Otherwise localizers might make wrong
> assumptions (I mean, in other languages, a menuitem label string might be
> subject to different rules/constraints than a checkbox label string).

I hadn't considered that. In that case rs=me for adding something like:
> +safeModeRestartCheckbox=Restart with Add-ons Disabled
If you can get this in before Callek spins 2.1b3
Comment 18 Jens Hatlak (:InvisibleSmiley) 2011-04-03 05:48:42 PDT
Created attachment 523866 [details] [diff] [review]
possible l10n improvement [Checkin: comment 20]
Comment 19 Philip Chee 2011-04-03 06:18:25 PDT
rs=me
Comment 20 Jens Hatlak (:InvisibleSmiley) 2011-04-03 06:28:33 PDT
Comment on attachment 523866 [details] [diff] [review]
possible l10n improvement [Checkin: comment 20]

http://hg.mozilla.org/comm-central/rev/21b306aef054
Comment 21 Tony Mechelynck [:tonymec] 2011-04-12 22:07:43 PDT
I happen to notice that this bug hasn't yet been VERIFIED
Comment 22 Tony Mechelynck [:tonymec] 2011-04-12 22:09:12 PDT
...let's do it.

Mozilla/5.0 (X11; Linux x86_64; rv:2.2a1pre) Gecko/20110412 Firefox/4.2a1pre SeaMonkey/2.2a1pre ID:20110412003004

The checkbox is present, and considering that no-one said it wasn't: → VERIFIED

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