Last Comment Bug 492263 - Change "Restart to apply theme" dialog from OK to Restart Now/Later
: Change "Restart to apply theme" dialog from OK to Restart Now/Later
Status: RESOLVED FIXED
[L10n-impact]
: fixed-seamonkey2.0, polish
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0
Assigned To: Philip Chee
:
Mentors:
: 400734 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-10 06:53 PDT by Robert Kaiser (not working on stability any more)
Modified: 2010-03-05 10:41 PST (History)
2 users (show)
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch file containing the changes made to the source code for Bug 492263 (2.67 KB, patch)
2009-07-30 05:29 PDT, vdloo
neil: review-
Details | Diff | Review
changes made to the source code for this bug (2.71 KB, patch)
2009-07-31 04:47 PDT, vdloo
neil: review+
Details | Diff | Review
Patch v1.2 Fix bit-rot and review nits. p=jaco_vdloo@hotmail.com r=neil (4.64 KB, patch)
2009-09-24 07:43 PDT, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Review
Patch v1.3 p=jaco_vdloo@hotmail.com [Checkin: Comment 10] (4.71 KB, patch)
2009-09-25 08:46 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2009-05-10 06:53:02 PDT
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.
Comment 1 vdloo 2009-07-30 05:29:26 PDT
Created attachment 391570 [details] [diff] [review]
patch file containing the changes made to the source code for  Bug 492263
Comment 2 Philip Chee 2009-07-30 08:10:44 PDT
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.
Comment 3 neil@parkwaycc.co.uk 2009-07-30 13:51:37 PDT
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.
Comment 4 vdloo 2009-07-31 04:47:02 PDT
Created attachment 391854 [details] [diff] [review]
changes made to the source code for this bug
Comment 5 neil@parkwaycc.co.uk 2009-07-31 05:32:13 PDT
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.
Comment 6 Philip Chee 2009-07-31 06:03:03 PDT
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.
Comment 7 Philip Chee 2009-09-24 07:43:05 PDT
Created attachment 402578 [details] [diff] [review]
Patch v1.2 Fix bit-rot and review nits. p=jaco_vdloo@hotmail.com r=neil

Fixed bit-rot and review nits. Patch by vdloo <jaco_vdloo@hotmail.com>. Carrying forward r=neil
Comment 8 neil@parkwaycc.co.uk 2009-09-24 14:00:58 PDT
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?
Comment 9 Philip Chee 2009-09-25 08:46:40 PDT
Created attachment 402839 [details] [diff] [review]
Patch v1.3 p=jaco_vdloo@hotmail.com
[Checkin: Comment 10]

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.
Comment 10 Serge Gautherie (:sgautherie) 2009-09-25 09:59:35 PDT
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
Comment 11 Philip Chee 2009-09-25 19:39:52 PDT
Oh well. I'll take ownership of this bug if nobody else wants it.
Comment 12 Philip Chee 2010-03-05 10:41:08 PST
*** Bug 400734 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.