Closed Bug 492263 Opened 13 years ago Closed 13 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

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)

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.
Keywords: polish
Whiteboard: [good first bug]
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.
Attachment #391570 - Flags: review?(neil) → review-
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)
Attachment #391854 - Flags: review?(neil) → review+
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.
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
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+
Flags: wanted-seamonkey2.0?
Whiteboard: [good first bug] → [good first bug][L10n-impact]
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0+
Attachment #402578 - Flags: superreview?(neil) → superreview+
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?
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+
Keywords: checkin-needed
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]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][L10n-impact] → [L10n-impact]
Target Milestone: --- → seamonkey2.0
Assignee: jaco_vdloo → nobody
Oh well. I'll take ownership of this bug if nobody else wants it.
Assignee: nobody → philip.chee
Duplicate of this bug: 400734
You need to log in before you can comment on or make changes to this bug.