Closed
Bug 492263
Opened 16 years ago
Closed 15 years ago
Change "Restart to apply theme" dialog from OK to Restart Now/Later
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0
People
(Reporter: kairo, Assigned: philip.chee)
References
Details
(Keywords: fixed-seamonkey2.0, polish, Whiteboard: [L10n-impact])
Attachments
(1 file, 3 obsolete files)
4.71 KB,
patch
|
philip.chee
:
review+
philip.chee
:
superreview+
|
Details | Diff | Splinter Review |
As we have the ability to internally trigger a restart (see EM), we should switch the dialog we put up when switching themes from View > Apply Theme from only having an OK button to having "Restart Now" and "Restart Later", with the former of course performing the restart, and the latter just closing the dialog, like OK does right now.
![]() |
Assignee | |
Comment 2•15 years ago
|
||
1. Your patch introduces some DOS newlines (\x0d\x0a) into the file. Please use unix newlines instead. > +function restartApp() { > + Remove blank line. > + Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(nsIAppStartup) > + .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit); > + } Remove spaces before the closing "}" > + var promptMsg = gNavigatorBundle.getFormattedString("switchskins", [brandName]); > + > + var check = {value: false}; > + > + var flags = promptService.BUTTON_POS_0 * promptService.BUTTON_TITLE_IS_STRING + promptService.BUTTON_POS_1 * promptService.BUTTON_TITLE_IS_STRING; Wrap this line. > + > + var pressedVal = promptService.confirmEx(window, promptTitle, promptMsg, flags, "Restart Now", "Restart Later", null, null, check); Wrap this line. > + Please remove unnecessary blank lines.
Updated•15 years ago
|
Attachment #391570 -
Flags: review?(neil) → review-
Comment 3•15 years ago
|
||
Comment on attachment 391570 [details] [diff] [review] patch file containing the changes made to the source code for Bug 492263 In addition to the other comments already mentioned, these are mostly style corrections but there is one major issue which makes the patch unacceptable. >+ var cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"] >+ .createInstance(Components.interfaces.nsISupportsPRBool); >+ os.notifyObservers(cancelQuit, "quit-application-requested", "restart"); >+ >+ // Something aborted the quit process. >+ if (cancelQuit.data) >+ return; Somehow these lines of code got 3 spaces of indent instead of 2. >+ Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(nsIAppStartup) >+ .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit); Add the const nsIAppStartup just before these lines instead at the top. >+ var promptTitle = gNavigatorBundle.getString("switchskinstitle"); You need to do something similar for promptNow/switchskinsnow and promptLater/switchskinslater, and then add these strings to navigator.properties - see http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/browser/navigator.properties#24 (you can find navigator.properties in en-US.jar in a nightly build). This allows us to have the button labels translated for other locales. >+ if (pressedVal == 0) restartApp(); restartApp should be indented on its own line.
Attachment #391570 -
Attachment is obsolete: true
Attachment #391854 -
Flags: review?(neil)
Updated•15 years ago
|
Attachment #391854 -
Flags: review?(neil) → review+
Comment 5•15 years ago
|
||
Comment on attachment 391854 [details] [diff] [review] changes made to the source code for this bug Well, this is basically code complete, although the style nits needs to be addressed before this can be checked in. >-} >+} Looks like you still have line ending problems. Unlike CVS which would helpfully fix up line endings for you, under Mercurial you have to make sure you use Unix line endings so as to not annoy users of other platforms. >+ And trailing spaces are just... a waste of space! >+const nsIAppStartup = Components.interfaces.nsIAppStartup; Actually I meant this to be inside the restartApp function (compare BrowserReloadSkipCache for instance). >+ // Something aborted the quit process. >+ if (cancelQuit.data) >+ return; >+ >+ Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(nsIAppStartup).quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit); These lines still have one space too much indentation. >+ if (pressedVal == 0) >+ restartApp(); And this has way too much indent! Should only be 2 extra spaces.
![]() |
Assignee | |
Comment 6•15 years ago
|
||
vdloo: The next step is to revise your patch fixing those nits. Then attach the revised patch. This time you need to set ? on the super-review flag (to Neil again). SeaMonkey patches require both review+ and super-review+ before being accepted.
Assignee: nobody → jaco_vdloo
![]() |
Assignee | |
Comment 7•15 years ago
|
||
Fixed bit-rot and review nits. Patch by vdloo <jaco_vdloo@hotmail.com>. Carrying forward r=neil
Attachment #391854 -
Attachment is obsolete: true
Attachment #402578 -
Flags: superreview?(neil)
Attachment #402578 -
Flags: review+
![]() |
Assignee | |
Updated•15 years ago
|
Flags: wanted-seamonkey2.0?
Whiteboard: [good first bug] → [good first bug][L10n-impact]
![]() |
Reporter | |
Updated•15 years ago
|
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0+
Updated•15 years ago
|
Attachment #402578 -
Flags: superreview?(neil) → superreview+
Comment 8•15 years ago
|
||
Comment on attachment 402578 [details] [diff] [review] Patch v1.2 Fix bit-rot and review nits. p=jaco_vdloo@hotmail.com r=neil >+ const nsIAppStartup = Components.interfaces.nsIAppStartup; That looks lonely there. Maybe it belongs just before it gets used. >+ Components.classes["@mozilla.org/toolkit/app-startup;1"] >+ .getService(nsIAppStartup) >+ .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit); Now that we have session store, maybe we should set the resume_once pref?
![]() |
Assignee | |
Comment 9•15 years ago
|
||
Carrying forward r=neil sr=neil > (From update of attachment 402578 [details] [diff] [review]) > >+ const nsIAppStartup = Components.interfaces.nsIAppStartup; > That looks lonely there. Maybe it belongs just before it gets used. Fixed. > >+ Components.classes["@mozilla.org/toolkit/app-startup;1"] > >+ .getService(nsIAppStartup) > >+ .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit); > Now that we have session store, maybe we should set the resume_once pref? Fixed.
Attachment #402578 -
Attachment is obsolete: true
Attachment #402839 -
Flags: superreview+
Attachment #402839 -
Flags: review+
![]() |
Assignee | |
Updated•15 years ago
|
Keywords: checkin-needed
Comment 10•15 years ago
|
||
Comment on attachment 402839 [details] [diff] [review] Patch v1.3 p=jaco_vdloo@hotmail.com [Checkin: Comment 10] http://hg.mozilla.org/comm-central/rev/99d2ca34a23a
Attachment #402839 -
Attachment description: [for checkin] Patch v1.3 p=jaco_vdloo@hotmail.com r=neil sr=neil → Patch v1.3 p=jaco_vdloo@hotmail.com
[Checkin: Comment 10]
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED
Whiteboard: [good first bug][L10n-impact] → [L10n-impact]
Target Milestone: --- → seamonkey2.0
![]() |
Assignee | |
Comment 11•15 years ago
|
||
Oh well. I'll take ownership of this bug if nobody else wants it.
Assignee: nobody → philip.chee
You need to log in
before you can comment on or make changes to this bug.
Description
•