Closed
Bug 646894
Opened 12 years ago
Closed 12 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•12 years ago
|
||
I reused the label to avoid l10n issues.
Attachment #523535 -
Flags: feedback?(philip.chee)
Attachment #523535 -
Flags: feedback?(jh)
![]() |
||
Comment 2•12 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•12 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•12 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•12 years ago
|
||
p.s. the patch works. so f+ on that.
Assignee | ||
Comment 6•12 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•12 years ago
|
||
Hmm. var checkboxText = menuitem; var checkbox = menuitem ? { value: true } : {};
Assignee | ||
Comment 8•12 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•12 years ago
|
||
Attachment #523609 -
Flags: review?(philip.chee)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #523610 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Comment 11•12 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•12 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•12 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•12 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•12 years ago
|
||
Pushed changeset bf54745a7aec to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 16•12 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•12 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•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•12 years ago
|
||
Attachment #523866 -
Flags: review?(philip.chee)
![]() |
Reporter | |
Updated•12 years ago
|
Attachment #523866 -
Flags: review?(philip.chee) → review+
![]() |
Reporter | |
Comment 19•12 years ago
|
||
rs=me
Comment 20•12 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•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
Updated•12 years ago
|
Attachment #523608 -
Attachment description: Addressed review comments → Addressed review comments
[Checked in: Comment 15]
Updated•12 years ago
|
Attachment #523609 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #523610 -
Attachment is obsolete: true
Comment 21•12 years ago
|
||
I happen to notice that this bug hasn't yet been VERIFIED
Comment 22•12 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
•