Closed
Bug 646894
Opened 14 years ago
Closed 14 years ago
[Restart in Safe Mode] Add a checkbox in the prompt to choose between safe and normal restart.
Categories
(SeaMonkey :: General, enhancement)
SeaMonkey
General
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.1b3
People
(Reporter: philip.chee, Assigned: neil)
References
(Blocks 2 open bugs)
Details
(Keywords: ux-control, ux-error-recovery)
Attachments
(2 files, 3 obsolete files)
2.92 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•14 years ago
|
||
I reused the label to avoid l10n issues.
Attachment #523535 -
Flags: feedback?(philip.chee)
Attachment #523535 -
Flags: feedback?(jh)
![]() |
||
Comment 2•14 years ago
|
||
(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•14 years ago
|
||
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.
Attachment #523535 -
Flags: feedback?(jh) → feedback+
![]() |
Reporter | |
Comment 4•14 years ago
|
||
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?
Attachment #523535 -
Flags: feedback?(philip.chee) → feedback-
![]() |
Reporter | |
Comment 5•14 years ago
|
||
p.s. the patch works. so f+ on that.
Assignee | ||
Comment 6•14 years ago
|
||
(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.
![]() |
Reporter | |
Comment 7•14 years ago
|
||
Hmm.
var checkboxText = menuitem;
var checkbox = menuitem ? { value: true } : {};
Assignee | ||
Comment 8•14 years ago
|
||
I'm going to be annoying and post several slightly different versions of this...
Assignee: nobody → neil
Attachment #523535 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #523608 -
Flags: review?(philip.chee)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #523609 -
Flags: review?(philip.chee)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #523610 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Comment 11•14 years ago
|
||
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.
Attachment #523608 -
Flags: review?(philip.chee) → review+
![]() |
Reporter | |
Comment 12•14 years ago
|
||
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.
Attachment #523609 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Comment 13•14 years ago
|
||
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]
Attachment #523610 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Comment 14•14 years ago
|
||
Comment on attachment 523610 [details] [diff] [review]
Another alternative patch
r=me in case you prefer this one.
Attachment #523610 -
Flags: review+
Assignee | ||
Comment 15•14 years ago
|
||
Pushed changeset bf54745a7aec to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 16•14 years ago
|
||
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).
![]() |
Reporter | |
Comment 17•14 years ago
|
||
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
Attachment #523609 -
Flags: review+
![]() |
Reporter | |
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•14 years ago
|
||
Attachment #523866 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Updated•14 years ago
|
Attachment #523866 -
Flags: review?(philip.chee) → review+
![]() |
Reporter | |
Comment 19•14 years ago
|
||
rs=me
Comment 20•14 years ago
|
||
Comment on attachment 523866 [details] [diff] [review]
possible l10n improvement [Checkin: comment 20]
http://hg.mozilla.org/comm-central/rev/21b306aef054
Attachment #523866 -
Attachment description: possible l10n improvement → possible l10n improvement [Checkin: comment 20]
Updated•14 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
Updated•14 years ago
|
Attachment #523608 -
Attachment description: Addressed review comments → Addressed review comments
[Checked in: Comment 15]
Updated•14 years ago
|
Attachment #523609 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #523610 -
Attachment is obsolete: true
Comment 21•14 years ago
|
||
I happen to notice that this bug hasn't yet been VERIFIED
Comment 22•14 years ago
|
||
...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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•