move System Defaults from General to Advanced -> General

RESOLVED FIXED in Thunderbird 3.0b1

Status

enhancement
P5
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: clarkbw, Assigned: clarkbw)

Tracking

Trunk
Thunderbird 3.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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.
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.
Posted image screenshot of advanced -> general tab (obsolete) —
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.
Bryan's bugs should be defaulting to NEW rather than UNCONFIRMED, but the Bugzilla upgrade last week broke that (hopefully just temporarily).

Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.)
Depends on: 456872
> 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.
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.
Taking this and setting the right flags up.  I really need to get my other work done, but this should be fast...
Assignee: nobody → clarkbw
Severity: normal → enhancement
Priority: -- → P5
Hardware: PC → All
Whiteboard: [waiting on review of bug 456872]
Posted patch 452890-v2.patch (obsolete) — Splinter Review
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
Attachment #336160 - Attachment is obsolete: true
Attachment #336161 - Attachment is obsolete: true
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.
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 on attachment 348648 [details] [diff] [review]
452890-v2.patch

magnus: would you mind looking this over for review?
Attachment #348648 - Flags: review?(mkmelin+mozilla)
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.
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?
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.
(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).
> 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.
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.
Attachment #348648 - Attachment is obsolete: true
Attachment #348648 - Flags: review?(mkmelin+mozilla)
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!
Attachment #349305 - Flags: review?(mkmelin+mozilla)
Attachment #349305 - Attachment description: 452890-v3.patch → [checked in] 452890-v3.patch
Attachment #349305 - Flags: review?(mkmelin+mozilla) → review+
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.
Whiteboard: [waiting on review of bug 456872] → [needs cleanup after 3.0b1 string freeze]
Thanks for the preliminary beta1 fix, this looks much better now.
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
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 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.
Attachment #349773 - Flags: review?(mkmelin+mozilla)
Attachment #349773 - Flags: review?(mkmelin+mozilla) → review+
changeset:   1563:ecf94b87d3b0
http://hg.mozilla.org/comm-central/rev/ecf94b87d3b0

->FIXED
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs cleanup after 3.0b1 string freeze]
WARNING: advanced.dtd now has two instances of checkNow.label. This should be fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The checkNow.label for software update one is unused...
Attachment #355806 - Flags: review?(philringnalda)
Attachment #355806 - Flags: review?(philringnalda) → review+
changeset:   1574:9c788dc9e472
http://hg.mozilla.org/comm-central/rev/9c788dc9e472
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
noting the regression ... is this testable / in-testsuite?
You need to log in before you can comment on or make changes to this bug.