452890 - move System Defaults from General to Advanced -> General

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement, P5)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

The system defaults preference is necessary to have somewhere in the preferences, however it's not needed in the general tab.  The general tab is for general interface behavior.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: patch to move startup check to advanced

I moved the preference over to the Advanced / General tab.

Instead of a caption of "System Defaults" I changed it to "System".  The groupbox is at the bottom of the tab and encompasses the about:config editor.

I changed the about config editor to be Configuration and ( Advanced Editor... ), updating the key combos that had to change as well.

The other items in the Adv. General tab are planned to get some cleanup as well so the limited amount of space left in the tab shouldn't be a problem.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screenshot of advanced -> general tab

here's a screenshot of the change in the advanced -> general tab.

the main tab is just missed the system default item and isn't really worth a screenshot of it's own.

Comment 3

[Steve Simms]

Bryan's bugs should be defaulting to NEW rather than UNCONFIRMED, but the Bugzilla upgrade last week broke that (hopefully just temporarily).

Confirming.

Comment 4

[Magnus Melin [:mkmelin]]

I definitely agree this shouldn't be on the front page, but it does look a bit crammed in there, probably needs more grouping. 

Since firefox uses "System Defaults", I think that's a good argument to use in tb too. Not to mention it does say a lot more. (I realize that's to accomodate the config editor, but maybe that should move elsewhere instead.)

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

> I definitely agree this shouldn't be on the front page, but it does look a bit
> crammed in there, probably needs more grouping. 

once bug 456872 gets finished we'll have a lot more room in this page.

> Since firefox uses "System Defaults", I think that's a good argument to use in
> tb too. Not to mention it does say a lot more. (I realize that's to accomodate
> the config editor, but maybe that should move elsewhere instead.)

Good point.  Lets keep the System Defaults and we can probably give the about:config button a "Configuration" category.  

Return Receipts would be all alone then and could possibly use a "Sending" category however I'm unsure how bug 452909 is going to land.

Comment 6

[Ronald Killmer]

I am also following Bug 456872 so I know this will fit there when the new Read/Show tab is added. Agreed this usually is a one shot selection that fits the Advanced description, so concur with the proposed move.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Taking this and setting the right flags up.  I really need to get my other work done, but this should be fast...

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: 452890-v2.patch

Here's an updated patch.  This doesn't actually change any strings, though it does move some around and remove the ".height" value that doesn't seem necessary anymore.  I'm not sure if dtd churn means breaking string freeze or not; if it does I might as well add a new string, otherwise this should be good to get in.

Will attach screenshot shortly

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screenshot of advanced -> general tab v2

Comment 10

[Ronald Killmer]

A suggestion for post Beta 1.

Where the Config editor button is, place a Text Leadin Description "Config Editor" to the left of the button and change the button label to "Edit".  The button in the screen shot looks like an out of context orphan.  It's is not consistent with the flow of the other tab content.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Good suggestion, we can keep this bug open for the post b1 changes.  I'd like to add that "Sending" group as well for the return receipts section.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 348648 [details] [diff] [review]
452890-v2.patch

magnus: would you mind looking this over for review?

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 348648 [details] [diff] [review]
452890-v2.patch

You're mostly just moving code, but since you're touching it... a few nits. The screenshot looks good!

>+#ifdef HAVE_SHELL_SERVICE
>+  /**
>+   * Checks whether Thunderbird is currently registered with the operating
>+   * system as the default app for mail, rss and news.  If Thunderbird is not currently the
>+   * default app, the user is given the option of making it the default for each type;
>+   * otherwise, the user is informed that Thunderbird is already the default.
>+   */

Wrap @ ~ 80 chars.

>+  checkDefaultNow: function (aAppType) 
>+  {   
>+    var nsIShellService = Components.interfaces.nsIShellService;
>+    var shellSvc;
>+    try {
>+      shellSvc = Components.classes["@mozilla.org/mail/shell-service;1"].getService(nsIShellService);

Wrap and align as
                  Components.classes
                            .getService

>+    
>+    // if we are already the default for all the types we handle, then alert the user.

Capital i.

>+    if (shellSvc.isDefaultClient(false, nsIShellService.MAIL | nsIShellService.NEWS | nsIShellService.RSS))
>+    {
>+      var brandBundle = document.getElementById("bundleBrand");
>+      var preferencesBundle = document.getElementById("bundlePreferences");
>+      var brandShortName = brandBundle.getString("brandShortName");
>+      var promptTitle = preferencesBundle.getString("alreadyDefaultClientTitle");
>+      var promptMessage;
>+      const IPS = Components.interfaces.nsIPromptService;
>+      var psvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                           .getService(IPS);

IPS is just used once, so I'd drop it and just use getService(Components.interfaces.nsIPromptService)

>+
>+        promptMessage = preferencesBundle.getFormattedString("alreadyDefault", [brandShortName]);
>+        psvc.alert(window, promptTitle, promptMessage);

Fix indention.

>+    }
>+    else
>+    {
>+      // otherwise, bring up the default client dialog
>+      window.openDialog("chrome://messenger/content/defaultClientDialog.xul", "Default Client", 
>+                        "modal,centerscreen,chrome,resizable=no");
>+    }
>+  },
>+#endif


>+                <button id="configEditor" label="&configEdit.label;"
>+                        accesskey="&configEdit.accesskey;" oncommand="gAdvancedPane.showConfigEdit();"/>

Wrap. (Might also be some other places.)

Simon might be able to tell us what effect moving entities have on the string freeze.

Comment 14

[Simon Paquet [:sipaq]]

The moving around of strings in advanced.dtd is fine from a string freeze perspective, but removing the strings in general.dtd and moving them to advanced.dtd is not.

This will turn all locales red. So please postpone that until after b1 is out.

I also noticed that you removed the entity alwaysCheckDefault.height in general.dtd without a replacement in advanced.dtd. Was that intentional?

Comment 15

[rsx11m]

Any chance that this can still make b1, despite comment #14 raising a valid point here, but with the code freeze being moved by a week or so now?

The General pref pane is now over its boundaries after bug 456869 added the addon manager to that pane. The patch here would resolve this issue and present the first TB3 beta with a cleaner pref pane.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> Any chance that this can still make b1, despite comment #14 raising a valid
> point here, but with the code freeze being moved by a week or so now?

Although the code freeze has been moved out, we are still trying to keep the strings frozen. For example, right now we have 27 locales that are ready to go for beta 1. If we take this bug (which hasn't been reviewed yet) they will all have to do extra work.

> The General pref pane is now over its boundaries after bug 456869 added the
> addon manager to that pane. The patch here would resolve this issue and present
> the first TB3 beta with a cleaner pref pane.

Whilst that is an ideal, I don't think it would stop us shipping beta. The alternative would be for Bryan to come up with two patches, one for beta that doesn't reorganise the strings, and one post beta which does (assuming no additions/removals are needed pre beta).

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

> I also noticed that you removed the entity alwaysCheckDefault.height in
> general.dtd without a replacement in advanced.dtd. Was that intentional?

Intentional cleanup? ... This looked like a layout bug that has since been fixed.  I found the change entered in 2006 and could not find a related bug or reason for it.  The text wrapping seems to work out correctly now and Firefox doesn't include the same code in their preferences.  

> Whilst that is an ideal, I don't think it would stop us shipping beta. The
> alternative would be for Bryan to come up with two patches, one for beta that
> doesn't reorganise the strings, and one post beta which does (assuming no
> additions/removals are needed pre beta).

I might try including the general.dtd file inside the advanced.xul such that I can pull in the strings I need without swapping them around in the files just yet.  Assuming there aren't conflicts (and with a quick glance this seemed to be true).  Then I can throw up an alternate patch for later that cleans up the dtd files and reverts the multi-import.

If I can get this done, and soon, I'll put up the new patch that also addresses the comments from magnus.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: [checked in] 452890-v3.patch

here is the updated patch

I pulled the general.dtd file into advanced.xul so that I don't have to make the strings dance for now.  I made a separate patch for the dtd changes that I can put up later for committing post-freeze.

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 349305 [details] [diff] [review]
[checked in] 452890-v3.patch

I often forget this part.

All nits should have been addressed.

I reordered the prompt variables a little so it made more sense to me.

Also note that I brought in the general.dtd file.  This would just be a hack until the strings are cleaned up after the string freeze.

Thanks magnus!

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 349305 [details] [diff] [review]
[checked in] 452890-v3.patch

A bunch of trailing spaces snuck in. Other than that, looks good. I've checked this in for you with the spaces removed

changeset:   1166:c694b3450f90
http://hg.mozilla.org/comm-central/rev/c694b3450f90

Leaving open until the hack is handled after the string freeze.

Comment 21

[rsx11m]

Thanks for the preliminary beta1 fix, this looks much better now.

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: 452890-cleanup-v1.patch

Here's the patch that does the cleanup of the advanced.xul file and swaps the strings properly in the dtd files.

Additional changes like suggested in Comment #10 should be added but I wanted to make sure that we at least had this patch to get things cleaned up

Comment 23

[Simon Paquet [:sipaq]]

Bryan, any update here? Beta1 is now out for a few weeks. IMO it's time that you seek review for your cleanup patch, right?

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 349773 [details] [diff] [review]
452890-cleanup-v1.patch

Right.  Magnus, can you take a look at this patch?  I haven't checked that it still applies cleanly, but I don't think anything has changed in the prefs area.  

Comment 10 should likely get a separate bug now.

Comment 25

[Magnus Melin [:mkmelin]]

changeset:   1563:ecf94b87d3b0
http://hg.mozilla.org/comm-central/rev/ecf94b87d3b0

->FIXED

Comment 26

[Rimas Kudelis [use needinfo to ping]]

WARNING: advanced.dtd now has two instances of checkNow.label. This should be fixed.

Comment 27

[Magnus Melin [:mkmelin]]

Attachment: patch - remove unused checkNow.label

The checkNow.label for software update one is unused...

Comment 28

[Magnus Melin [:mkmelin]]

changeset:   1574:9c788dc9e472
http://hg.mozilla.org/comm-central/rev/9c788dc9e472

Comment 29

[Wayne Mery (:wsmwk)]

noting the regression ... is this testable / in-testsuite?