The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
UI Design
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: Philip Chee)

Tracking

({fixed-seamonkey2.0, polish})

Trunk
seamonkey2.0
fixed-seamonkey2.0, polish
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [L10n-impact])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Keywords: polish
Whiteboard: [good first bug]

Comment 1

8 years ago
Created attachment 391570 [details] [diff] [review]
patch file containing the changes made to the source code for  Bug 492263
Attachment #391570 - Flags: review?(neil)
(Assignee)

Comment 2

8 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

8 years ago
Attachment #391570 - Flags: review?(neil) → review-

Comment 3

8 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.

Comment 4

8 years ago
Created attachment 391854 [details] [diff] [review]
changes made to the source code for this bug
Attachment #391570 - Attachment is obsolete: true
Attachment #391854 - Flags: review?(neil)

Updated

8 years ago
Attachment #391854 - Flags: review?(neil) → review+

Comment 5

8 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

8 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

8 years ago
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
Attachment #391854 - Attachment is obsolete: true
Attachment #402578 - Flags: superreview?(neil)
Attachment #402578 - Flags: review+
(Assignee)

Updated

8 years ago
Flags: wanted-seamonkey2.0?
Whiteboard: [good first bug] → [good first bug][L10n-impact]
(Reporter)

Updated

8 years ago
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0+

Updated

8 years ago
Attachment #402578 - Flags: superreview?(neil) → superreview+

Comment 8

8 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

8 years ago
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.
Attachment #402578 - Attachment is obsolete: true
Attachment #402839 - Flags: superreview+
Attachment #402839 - Flags: review+
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED
Whiteboard: [good first bug][L10n-impact] → [L10n-impact]
Target Milestone: --- → seamonkey2.0

Updated

8 years ago
Assignee: jaco_vdloo → nobody
(Assignee)

Comment 11

8 years ago
Oh well. I'll take ownership of this bug if nobody else wants it.
Assignee: nobody → philip.chee
(Assignee)

Updated

7 years ago
Duplicate of this bug: 400734
You need to log in before you can comment on or make changes to this bug.