646894 - [Restart in Safe Mode] Add a checkbox in the prompt to choose between safe and normal restart.

Status: VERIFIED FIXED

(SeaMonkey :: General, enhancement)

Description

[Philip Chee]

From Bug 574612 Comment 21:
> [e-mail]      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

[[e-mail]]

Attachment: Possible patch

I reused the label to avoid l10n issues.

Comment 2

[Robert Kaiser]

(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)]

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]

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]

p.s. the patch works. so f+ on that.

Comment 6

[[e-mail]]

(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]

Hmm.
  var checkboxText = menuitem;
  var checkbox = menuitem ? { value: true } : {};

Comment 8

[[e-mail]]

Attachment: Addressed review comments [Checked in: Comment 15]

I'm going to be annoying and post several slightly different versions of this...

Comment 9

[[e-mail]]

Attachment: Alternative patch

Comment 10

[[e-mail]]

Attachment: Another alternative patch

Comment 11

[Philip Chee]

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]

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]

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]

Comment on attachment 523610 [details] [diff] [review]
Another alternative patch

r=me in case you prefer this one.

Comment 15

[[e-mail]]

Pushed changeset bf54745a7aec to comm-central.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

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]

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)]

Attachment: possible l10n improvement [Checkin: comment 20]

Comment 19

[Philip Chee]

rs=me

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

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]]

I happen to notice that this bug hasn't yet been VERIFIED

Comment 22

[Tony Mechelynck [:tonymec]]

...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