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)

enhancement
Not set
normal

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)

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.)
Attached patch Possible patch (obsolete) — Splinter Review
I reused the label to avoid l10n issues.
Attachment #523535 - Flags: feedback?(philip.chee)
Attachment #523535 - Flags: feedback?(jh)
(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 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+
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-
p.s. the patch works. so f+ on that.
(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.
Hmm. var checkboxText = menuitem; var checkbox = menuitem ? { value: true } : {};
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)
Attached patch Alternative patch (obsolete) — Splinter Review
Attachment #523609 - Flags: review?(philip.chee)
Attached patch Another alternative patch (obsolete) — Splinter Review
Attachment #523610 - Flags: review?(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.
Attachment #523608 - Flags: review?(philip.chee) → review+
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)
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)
Comment on attachment 523610 [details] [diff] [review] Another alternative patch r=me in case you prefer this one.
Attachment #523610 - Flags: review+
Pushed changeset bf54745a7aec to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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 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+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #523866 - Flags: review?(philip.chee) → review+
rs=me
Attachment #523866 - Attachment description: possible l10n improvement → possible l10n improvement [Checkin: comment 20]
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
Attachment #523608 - Attachment description: Addressed review comments → Addressed review comments [Checked in: Comment 15]
Attachment #523609 - Attachment is obsolete: true
Attachment #523610 - Attachment is obsolete: true
I happen to notice that this bug hasn't yet been VERIFIED
...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
Blocks: 1194744
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: