492263 - Change "Restart to apply theme" dialog from OK to Restart Now/Later

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

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]

Attachment: patch file containing the changes made to the source code for Bug 492263

Comment 2

[Philip Chee]

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

[[e-mail]]

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]

Attachment: changes made to the source code for this bug

Comment 5

[[e-mail]]

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]

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]

Attachment: Patch v1.2 Fix bit-rot and review nits. p=[e-mail] r=neil

Fixed bit-rot and review nits. Patch by vdloo <[e-mail]>. Carrying forward r=neil

Comment 8

[[e-mail]]

Comment on attachment 402578 [details] [diff] [review]
Patch v1.2 Fix bit-rot and review nits. p=[e-mail] 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]

Attachment: Patch v1.3 p=[e-mail] [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)]

Comment on attachment 402839 [details] [diff] [review]
Patch v1.3 p=[e-mail]
[Checkin: Comment 10]


http://hg.mozilla.org/comm-central/rev/99d2ca34a23a

Comment 11

[Philip Chee]

Oh well. I'll take ownership of this bug if nobody else wants it.