Migrate SeaMonkey's appearance and child pref panels to new pref pane

RESOLVED FIXED

Status

SeaMonkey
Preferences
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

10.77 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
12.22 KB, patch
Ian Neal
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
53.22 KB, patch
Karsten Düsterloh
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
26.84 KB, patch
Ian Neal
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 295858 [details] [diff] [review]
Patch for fonts and colors

Start with migrating fonts and colors pref panels.
Attachment #295858 - Flags: review?(mnyromyr)
(Assignee)

Updated

10 years ago
Attachment #295858 - Flags: superreview?(neil)

Comment 1

10 years ago
Comment on attachment 295858 [details] [diff] [review]
Patch for fonts and colors

Colours are fine, but fonts are seriously wrong... I'm seeing all manner of errors in the JavaScript console. Also, the old code used to build the popups lazily because otherwise they take seconds to refresh.

>+                  onsyncfrompreference="return document.getElementById('fonts_pane').readFontLanguageGroup();">
This unfortunately seems to fire before the script has loaded...

>+                      onsyncfrompreference="return document.getElementById('fonts_pane').readFontSelection(this);"/>
"this" is the preference element. (Maybe it shouldn't be...)

>+    // Both lists are sorted, and the Fonts-By-Type list is a subset of the
>+    // All-Fonts list, so walk both lists side-by-side, skipping values we've
>+    // already created menu items for.
>+    var builtItem = separator ? separator.nextSibling : popup.firstChild;
>+    var builtItemValue = builtItem ? builtItem.getAttribute("value") : null;
Why not use the fonts array?

>+    separator = document.createElement("menuseparator");
>+    popup.appendChild(separator);
This is incorrect if there are no specific fonts.

>+  aMenuList.appendChild(popup);    
Trailing spaces :-P

>+function readMinFontSize(aElement)
>+function readUseDocumentFonts()
>+function writeUseDocumentFonts(aUseDocumentFonts)
I don't see what use these are.
Attachment #295858 - Flags: superreview?(neil) → superreview-

Comment 2

10 years ago
Comment on attachment 295858 [details] [diff] [review]
Patch for fonts and colors

Also some code style nits for pref-fonts.js:
  - prefix globals with g
  - UpperCaseFunctionNames()
  - no spaces in or after empty {}
Attachment #295858 - Flags: review?(mnyromyr) → review-

Updated

10 years ago
Depends on: 308754
(Assignee)

Comment 3

10 years ago
Created attachment 297067 [details] [diff] [review]
Patch for pref-colors only v0.1 (Checked into trunk)

Let's do this one pane at a time - split off pref-colors changes first.
Only change from this part of last patch is the removal of trailing space just before closing of <!DOCTYPE> element.
Attachment #297067 - Flags: superreview?(neil)
Attachment #297067 - Flags: review?(mnyromyr)

Updated

10 years ago
Attachment #297067 - Flags: superreview?(neil)
Attachment #297067 - Flags: superreview+
Attachment #297067 - Flags: review?(mnyromyr)
Attachment #297067 - Flags: review+
(Assignee)

Comment 4

10 years ago
Comment on attachment 297067 [details] [diff] [review]
Patch for pref-colors only v0.1 (Checked into trunk)

Checking in
preferences.xul;
new revision: 1.9; previous revision: 1.8
preftree.xul;
new revision: 1.113; previous revision: 1.112
pref-colors.xul;
new revision: 1.59; previous revision: 1.58
done
Attachment #297067 - Attachment description: Patch for pref-colors only v0.1 → Patch for pref-colors only v0.1 (Checked into trunk)
(Assignee)

Comment 5

10 years ago
Created attachment 302358 [details] [diff] [review]
Patch for pref-fonts only v0.2

Changes since v0.1
* Use the font array for building the second part of the font list
* Test for non-empty first part of the font list before adding a separator
* Removed trailing spaces and other extra spaces
* Prefixed global variables with g
* Started function names with a capital letter
* Builds the font list lazily

Read/WriteUseDocumentFonts are used to convert the integer pref to a boolean element (checkbox)
ReadMinFontPref is used to deal with when the pref does not exist and what default value to use.
Attachment #295858 - Attachment is obsolete: true
Attachment #302358 - Flags: superreview?(neil)
Attachment #302358 - Flags: review?(mnyromyr)

Comment 6

10 years ago
Comment on attachment 302358 [details] [diff] [review]
Patch for pref-fonts only v0.2

>+        <menulist id="selectLangs" preference="font.language.group"
>+                  onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontLanguageGroup();">
There's a bug on having to use document.getElementById?

>+                      onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontPref(this, null);">
I'd pass in the real default value (0) instead of "assuming" it.
(I'd also pass in the value as a number in the other cases.)

>+const kDefaultFontType          = "font.default.%LANG%";
>+const kFontNameFmtSerif         = "font.name.serif.%LANG%";
>+const kFontNameFmtSansSerif     = "font.name.sans-serif.%LANG%";
>+const kFontNameFmtMonospace     = "font.name.monospace.%LANG%";
>+const kFontNameFmtCursive       = "font.name.cursive.%LANG%";
>+const kFontNameFmtFantasy       = "font.name.fantasy.%LANG%";
>+const kFontNameListFmtSerif     = "font.name-list.serif.%LANG%";
>+const kFontNameListFmtSansSerif = "font.name-list.sans-serif.%LANG%";
>+const kFontNameListFmtMonospace = "font.name-list.monospace.%LANG%";
>+const kFontNameListFmtCursive   = "font.name-list.cursive.%LANG%";
>+const kFontNameListFmtFantasy   = "font.name-list.fantasy.%LANG%";
>+const kFontSizeFmtVariable      = "font.size.variable.%LANG%";
>+const kFontSizeFmtFixed         = "font.size.fixed.%LANG%";
>+const kFontMinSizeFmt           = "font.minimum-size.%LANG%";
There's no point defining contants for these strings, you only use them once, and the name of the constant is no more informative then the string.

>+  if (!gFontEnumerator) {
>+    gFontEnumerator = Components.classes["@mozilla.org/gfx/fontenumerator;1"]
>+                                .createInstance(Components.interfaces.nsIFontEnumerator);
>+    }
Nit: incorrect indentation of }

>+      if (fonts.indexOf(gAllFonts[i]) == -1) {
fonts.lastIndexOf(gAllFonts[i], 0) is cheaper. I'd also change this to == 0 and swap the else case around.

>+    var preference = document.getElementById(prefs[i].format.replace(/%LANG%/, languageGroup));
>+    if (!preference) {
>+      preference = document.createElement("preference");
>+      var name = prefs[i].format.replace(/%LANG%/, languageGroup);
Put the name up above since you need it there anyway. Also I'd simply chop the %LANG% off the format and concatenate the languageGroup instead.

>+  return undefined;
Nit: this is the default return value, and there are no other returns.

>+  var stripWhitespace = /^\s*(.*)\s*$/;
Nit: I prefer .replace(/^\s+|\s+$/g, "")

>+  for (var i = 0; i < fontNames.length; ++i) {
>+    var fontName = fontNames[i].replace(stripWhitespace, "$1");
>+    fontItems = aElement.getElementsByAttribute("value", fontName);
>+    if (fontItems.length)
>+      break;
>   }
>+  if (fontItems.length)
>+    return fontItems[0].getAttribute("value");
Why not return the value inside the loop?

>+function ReadFontPref(aElement, aDefaultValue)
>+{
>+  // Check to see if preference value exists, if not return
>+  // given default value if there is one or first in list.
>+  var preference = document.getElementById(aElement.getAttribute("preference"));
>+  if (!!preference.value)
>+    return undefined;
>+  return aDefaultValue || aElement.firstChild.firstChild.getAttribute("value");
>+}
Why the !!? Does return preference.value || aDefaultValue work?
(See above about not needing to "assume" the value.)

>+function ReadUseDocumentFonts()
>+{
>+  var preference = document.getElementById("browser.display.use_document_fonts");
>+  return preference.value == 1;
>+}
>+ 
>+function WriteUseDocumentFonts(aUseDocumentFonts)
>+{
>+  return aUseDocumentFonts.checked ? 1 : 0;
>+}
Strangely the checkbox seems to work without ReadUseDocumentFonts but not without WriteUseDocumentFonts :-(
Attachment #302358 - Flags: superreview?(neil) → superreview+

Comment 7

10 years ago
I've been looking into the main appearance panel in the last few hours, but the overlays for startup options coming from composer/mailnews/chatzilla give me headaches :(

Updated

10 years ago
Depends on: 417319

Comment 8

10 years ago
Created attachment 303142 [details] [diff] [review]
WIP patch for main appearance pane

Here's a WIP patch for the main appearance pane, including bug 417319 work - note though that due to some yet unresolved issue with loading the overlays to pref-appearance.xul I commented out all of those overlay lines in the patch for now, so only the "browser" startup option appears on the pane with this WIP patch for now. If anyone finds out why the overlays (which are included) make the pane fail, I'd be happy about that...
(Assignee)

Updated

10 years ago
Depends on: 330458

Comment 9

10 years ago
After some IRC talk with IanN about the problem I saw with the WIP patch in comment #8, I found out that the loadOverlay() call in http://mxr.mozilla.org/mozilla/source/toolkit/content/widgets/preferences.xml#692 never notifies the observer about this pane being loaded (we get past the loadOverlay() but never enter the observe() function of obs) - this might be an instance of bug 330458.

Comment 10

10 years ago
Created attachment 303292 [details] [diff] [review]
patch to add a content panel, v1

I'd like to add the language selection to the main appearance panel (I have that in my local tree) and we already once had a problem with space in the appearance pane in bug 341949, so adding zoom or the aggressive favicon pref would probably also break that again, even if we didn't add locale selection there.

Because of that I propose to split off the parts that affect web and message content to a new content pref pane, for which I'm attaching a patch here.
We could probably make the icons part a groupbox that also includes http://kb.mozillazine.org/Browser.chrome.load_toolbar_icons if we keep supporting that on trunk.
Attachment #303292 - Flags: superreview?(neil)
Attachment #303292 - Flags: review?(iann_bugzilla)

Comment 11

10 years ago
patching file `mozilla/suite/common/jar.mn'
Hunk #1 FAILED at 139.
1 out of 1 hunk FAILED -- saving rejects to mozilla/suite/common/jar.mn.rej
patching file `mozilla/suite/common/pref/preferences.xul'
patch: **** malformed patch at line 82:

Comment 12

10 years ago
> patch: **** malformed patch at line 82:

Hrmm, yes, I had to manually edit the diff there, I may have introduced a bug there.
(Assignee)

Comment 13

10 years ago
(In reply to comment #10)
> We could probably make the icons part a groupbox that also includes
> http://kb.mozillazine.org/Browser.chrome.load_toolbar_icons if we keep
> supporting that on trunk.
> 
I'm happy for that to happen. Don't forget to update help as well if you are adding extra pref panel / pref options.

Comment 14

10 years ago
(In reply to comment #13)
> Don't forget to update help as well if you are
> adding extra pref panel / pref options.

Yes, I'll look into that once I have review on the new panel. Should I do the work for that icons groupbox right now or can I do that in a followup?
(Assignee)

Comment 15

10 years ago
(In reply to comment #14)
> (In reply to comment #13)
> > Don't forget to update help as well if you are
> > adding extra pref panel / pref options.
> 
> Yes, I'll look into that once I have review on the new panel. Should I do the
> work for that icons groupbox right now or can I do that in a followup?
> 
Well you're adding at least one extra pref as it is so you might as well do this one too and group them how you intend.

Comment 16

10 years ago
Created attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

Here's an updated patch for the content panel, adding that groupbox (and reducing context on jar.mn so I can hopefully give you something that applies cleanly).
Attachment #303292 - Attachment is obsolete: true
Attachment #303386 - Flags: superreview?(neil)
Attachment #303386 - Flags: review?(iann_bugzilla)
Attachment #303292 - Flags: superreview?(neil)
Attachment #303292 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> Created an attachment (id=303386) [details]
> add content panel, v2 (including site icon groupbox)
> 
> Here's an updated patch for the content panel, adding that groupbox (and
> reducing context on jar.mn so I can hopefully give you something that applies
> cleanly).
> 

So you are planning to put in browser.chrome.load_toolbar_icons in a spin off bug?

Comment 18

10 years ago
Created attachment 303714 [details] [diff] [review]
WIP2 patch for main appearance pane

Here's another WIP patch for the appearance pane, which is almost read from my perspective - you need at least the part of bug 417319 for testing that removes the ChatZilla overlay from pref-appearance so that you aren't disturbed by the bug 330458 problem. I'd like a first review on this even though it cannot go in exactly as it is in the patch (workaround for the overlay problem).
Some Comments along with that:
1) Actually overlaying the mailnews and editor parts is commented out (cZ part also needs to be removed for testing this), but I added checkboxes manually in the pane so that the final layout can be tested. Of course, this needs to be corrected for checkin.
2) I put locale switching into this panel now (and it works!), if that's ok, I need to add removal of the old pref-locales as well.
Attachment #303142 - Attachment is obsolete: true
Attachment #303714 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 19

10 years ago
Comment on attachment 303714 [details] [diff] [review]
WIP2 patch for main appearance pane

This is on code inspection only.

>Index: mozilla/suite/common/pref/pref-appearance.xul
>===================================================================

>+    <groupbox id="generalStartupPreferences" align="start">
Because chatzilla still looks for "generalStartupPreferences" perhaps change the id to "generalStartupPrefs"

>+      <menulist id="switchLocales" flex="1" preference="general.useragent.locale"
preference could be on a new line.
>+                onsyncfrompreference="return document.getElementById('appearance_pane').SelectLocale(this);"/>
Hmmm, must log a bug on the need to call the function that way.

>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>       <treeitem container="true"
>-                id="appearance"
>-                label="&appear.label;"
>+                id="appearance" label="&appear.label;"
id should be appearanceItem and label should be on next line.

>+                prefpane="appearance_pane"
>+                helpTopic="appearance_pref"
helpTopic usually comes before prefpane entries elsewhere in this file.
>                 url="chrome://communicator/content/pref/pref-appearance.xul">
Change needed for pref-locale as per your comment in bug

>Index: mozilla/suite/common/pref/preftree.xul
>===================================================================
Change needed for pref-locale as per your comment in bug

>Index: mozilla/suite/locales/en-US/chrome/common/pref/pref-appearance.dtd
>===================================================================
> <!ENTITY useSiteIcons.label                     "Show Web Site Icons">
> <!ENTITY useSiteIcons.accesskey                 "S">
> <!ENTITY enableAutomaticImageResizing.label     "Resize large images to fit in the browser window">
> <!ENTITY enableAutomaticImageResizing.accesskey "R">
> <!ENTITY useSmoothScroll.label                  "Use smooth scrolling">
> <!ENTITY useSmoothScroll.accesskey              "U">
>+<!ENTITY textZoomOnly.label                     "Zoom only text instead of full pages">
>+<!ENTITY textZoomOnly.accesskey                 "Z">
These entities will now be in the pref-content.dtd so should be removed.
> 
>Index: mozilla/mailnews/jar.mn
>===================================================================
>-% overlay chrome://communicator/content/pref/pref-appearance.xul               chrome://messenger/content/mailPrefsOverlay.xul
>+###% overlay chrome://communicator/content/pref/pref-appearance.xul               chrome://messenger/content/mailPrefsAppearanceOverlay.xul
I prefer mailPrefAppearanceOverlay.xul as the filename.

>+    content/messenger/mailPrefsAppearanceOverlay.xul                           (base/prefs/resources/content/mailPrefsAppearanceOverlay.xul)
See comment above.

/mailPrefsAppearanceOverlay.xul
>===================================================================
See comment above.
>+<overlay id="mailPrefsAppearanceOverlay"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
See comment above.

>+  <groupbox id="generalStartupPreferences">
See comment about changing to generalStartupPrefs earlier.

>Index: mozilla/editor/ui/jar.mn
>===================================================================
>-% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://editor/content/editorPrefsOverlay.xul
>+###% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://editor/content/editorPrefsAppearanceOverlay.xul
I prefer editorPrefAppearanceOverlay.xul as a filename.

>+      content/editor/editorPrefsAppearanceOverlay.xul          (composer/content/editorPrefsAppearanceOverlay.xul)
See comment above.

>Index:  mozilla/editor/ui/composer/content/editorPrefsAppearanceOverlay.xul
>===================================================================
See comment above.

>+<!DOCTYPE overlay [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
Is brand.dtd still needed?

>+<overlay id="editorPrefsAppearanceOverlay"
See comment above.

>+  <groupbox id="generalStartupPreferences">
See comment about changing to generalStartupPrefs earlier.
Attachment #303714 - Flags: review?(iann_bugzilla) → review-

Comment 20

10 years ago
(In reply to comment #17)
> So you are planning to put in browser.chrome.load_toolbar_icons in a spin off
> bug?

Ideally, yes. I'm working on too many different things atm, I'd like to get those done step by step.
(Assignee)

Updated

10 years ago
Attachment #302358 - Flags: review?(mnyromyr)
(Assignee)

Comment 21

10 years ago
Created attachment 303785 [details] [diff] [review]
Patch for pref-fonts only v0.3

Changes since v0.2:
* As per Neil's comments.
* Did some further tweaking of how preference name is generated.

Carrying forward sr= and requesting r=
Attachment #302358 - Attachment is obsolete: true
Attachment #303785 - Flags: superreview+
Attachment #303785 - Flags: review?(mnyromyr)
(Assignee)

Comment 22

10 years ago
Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

Just on reading through the code...
>Index:  mozilla/suite/locales/en-US/chrome/common/pref/pref-content.dtd
>===================================================================
>+<!ENTITY pref.content.description               "Those settings influence how website and message content appears in &brandShortName;. See the separate preferences for colors, fonts and languages to further customize website appearance and the Privacy &amp; Security section for privacy-related website settings.">
"These" instead of "Those", also drop the "website" from the first part of the second sentence so it becomes "to further customize appearance". Perhaps say "separate preference panels" or "pages" or similar?

Comment 23

10 years ago
(In reply to comment #19)
> >+      <menulist id="switchLocales" flex="1" preference="general.useragent.locale"
> preference could be on a new line.
> >+                onsyncfrompreference="return document.getElementById('appearance_pane').SelectLocale(this);"/>
> Hmmm, must log a bug on the need to call the function that way.

Not sure if that's possible. onsyncfrompreference get loaded into the XBL and runs in the scope of that instead of the pane scope, so the function isn't local to it.

Comment 24

10 years ago
Sorry for generating another bugmail, missed this comment:

(In reply to comment #19)
> >+                prefpane="appearance_pane"
> >+                helpTopic="appearance_pref"
> helpTopic usually comes before prefpane entries elsewhere in this file.

Actually, I see it helpTopic after prefpane in most places in the file...

All other comments are addressed in the patch I'm about to attach (and which really works because of the fix for bug 419452, which makes the overlays work).

Comment 25

10 years ago
Created attachment 306108 [details] [diff] [review]
main appearance pane, v1

Here's the first really working patch for the main pref-appearance panel, adressing the comments from the WIP versions.
Attachment #303714 - Attachment is obsolete: true
Attachment #306108 - Flags: superreview?(neil)
Attachment #306108 - Flags: review?(iann_bugzilla)

Comment 26

10 years ago
Comment on attachment 306108 [details] [diff] [review]
main appearance pane, v1

>-  <groupbox id="generalStartupPreferences" align="start">
Why rename this?

>+    <vbox class="box-padded" align="start">
>+      <separator class="thin" />
Nit: put the separator before the vbox

>+      <menulist id="switchLocales" flex="1"
>+                preference="general.useragent.locale"
I don't like the way this menulist consumes the width of the groupbox.

>diff -N mozilla/suite/common/pref/pref-appearance.js
Not added to jar.mn

>+  var popup = document.createElement("menupopup");
Use menulist.appendItem so you don't need this.

>+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Components.interfaces.nsIStringBundleService);
>+  var langNames = sbs.createBundle("chrome://global/locale/languageNames.properties");
>+  var regNames  = sbs.createBundle("chrome://global/locale/regionNames.properties");
These ought to be <stringbundle> elements.

>+      displayName = langNames.GetStringFromName(parts[0]);
I think I saw someone .toLowerCase() this too just in case.

>+          displayName += " (" + regNames.GetStringFromName(parts[1].toLowerCase()) + ")";
This is OK for l10n, right? ;-)

>-<!--
>-          <treeitem label="&locales.label;"
>-                    url="chrome://communicator/content/pref/pref-locales.xul"/>
>         </treechildren>
>       </treeitem>
>--->
>+
You need to remove the close comment too...

>-<!ENTITY languageList.label                     "Choose your preferred language for &brandShortName;:">
Heh, how long has that been there :-)

>+<!ENTITY restartOnLangChange.label              "Language preferences will take effect when you restart &brandShortName;.">
>+<!ENTITY selectLocale.label                     "Select the language for text that appears in dialog boxes, menus, toolbars and button labels:">
Nit: entries are in reverse order

>diff -N mozilla/mailnews/base/prefs/resources/content/mailPrefAppearanceOverlay.xul
Although not applicable in every case, if the overlay is small enough that every element has an id then it doesn't really matter how many times it is used.

sr- based on number of nits and questions.
Attachment #306108 - Flags: superreview?(neil) → superreview-

Comment 27

10 years ago
(In reply to comment #26)
> (From update of attachment 306108 [details] [diff] [review])
> >-  <groupbox id="generalStartupPreferences" align="start">
> Why rename this?

Because IanN suggested that, see comment #19

> >+      displayName = langNames.GetStringFromName(parts[0]);
> I think I saw someone .toLowerCase() this too just in case.

Why? The chrome registry should always gives us correct language IDs, and those have a lowercase language name per spec (RFC 3066).

> >+          displayName += " (" + regNames.GetStringFromName(parts[1].toLowerCase()) + ")";
> This is OK for l10n, right? ;-)

I hope so, I never heard of problems with the locale switcher extension, where this is copied from.

> >--->
> >+
> You need to remove the close comment too...

Erm, "--->" is just that, as the "-" in the first column is the diff "minus" sign.

> >-<!ENTITY languageList.label                     "Choose your preferred language for &brandShortName;:">
> Heh, how long has that been there :-)

No idea, I wondered as well.

> >diff -N mozilla/mailnews/base/prefs/resources/content/mailPrefAppearanceOverlay.xul
> Although not applicable in every case, if the overlay is small enough that
> every element has an id then it doesn't really matter how many times it is
> used.

IMHO, It's actually much cleaner to have separate overlays there which never are supposed to drop elements - not sure about load time though (or even how much of an issue that is).
(Assignee)

Comment 28

10 years ago
(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 306108 [details] [diff] [review] [details])
> > >-  <groupbox id="generalStartupPreferences" align="start">
> > Why rename this?
> 
> Because IanN suggested that, see comment #19
My understanding is that the change would only be needed if the same overlay file was used for both SM2.x and pre SM2.x versions of pref-appearance.xul for chatzilla (unless you do as Neil has done in the latest patch for that).

Comment 29

10 years ago
Comment on attachment 303785 [details] [diff] [review]
Patch for pref-fonts only v0.3

>Index: pref-fonts.xul
>===================================================================
>+<!DOCTYPE overlay SYSTEM "chrome://communicator/locale/pref/pref-fonts.dtd">
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Add an empty line between these two.

>+    <groupbox>
>+      <caption>
>+        <hbox align="center">
>+          <label value="&language.label;" 
>+                 accesskey="&language.accesskey;" 
>+                 control="selectLangs"/>
>+        </hbox>

I don't see much point in this hbox (especially with the following menulist outside of it) since caption already is one itself. You can put the align onto the caption.

>+        <menulist id="selectLangs" preference="font.language.group"
>+                  onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontLanguageGroup();">

There's not much point in returning the value of a function which doesn't give a return value. I think it's clearer that we'll hit the 'undefined' case with this pseudo handler if we drop the return here. 

>+        <rows>
>+          <row align="center">
>+            <spacer/>
>+            <hbox align="center">
>+              <label value="&typefaces.label;"/>
>+            </hbox>  
>+            <hbox align="center">
>+              <label value="&sizes.label;"/>
>+            </hbox>  
>+          </row>

The hboxes are not needed, row is already one itself.

> 
>-</page>
>+          <row>
>+            <separator class="thin"/>
>+          </row>
>+

Drop the empty lines around that row, we don't do that below either.

>+          <row align="center">
>+            <hbox align="center" pack="end">
>+              <label value="&monospace.label;" 
>+                     accesskey="&monospace.accesskey;" 
>+                     control="monospace"/>
>+            </hbox>

Add a thin separator row before the monospace row.

>+    <hbox align="center">
>+      <!-- Unchecking this removes the ability to select dynamic fonts -->
>+      <checkbox id="browserUseDocumentFonts" 

You do like hboxes, do you? ;-)
But this one is superfluous, too...

>Index: pref-fonts.js
>===================================================================

This file used the "curly braces go onto their own line" style. 
I'd rather like to see that retained (despite the overall rewrite).

>+function BuildFontList(aLanguage, aFontType, aMenuList, aPreference)
...
>+  var fonts = GetFontEnumerator().EnumerateFonts(aLanguage, aFontType, {});
>+  if (fonts.length > 0)
>+    defaultFont = GetFontEnumerator().getDefaultFont(aLanguage, aFontType);
>+  else {
>+    fonts = GetFontEnumerator().EnumerateFonts(aLanguage, "", {});
>+    if (fonts.length > 0)
>+      defaultFont = GetFontEnumerator().getDefaultFont(aLanguage, "");

Please always "embrace" both if-branches if at least one needs it.

Furthermore, we don't care if the length is actually > 0, it can't be < 0 anyways, so "if (fonts.length)" will suffice.
This is also true for all of the > 0 tests which follow.

>+  if (!gAllFonts)
>+    gAllFonts = GetFontEnumerator().EnumerateAllFonts({});
>+    

Non-empty empty line. ;-)

>+    for (var i = 0; i < fonts.length; ++i) {
>+      menuitem = document.createElement("menuitem");
>+      menuitem.setAttribute("value", fonts[i]);
>+      menuitem.setAttribute("label", fonts[i]);

You can just use 
  for each (var font in fonts)
  ...
  menuitem.setAttribute("value", font);
here.

>+    for (i = 0; i < gAllFonts.length; ++i) {

Ditto here and for other arrays(!) below.

>+function ReadFontLanguageGroup()
>+{
>+  var prefs = [{ format: "default",       type: "string",  element: "defaultFontType", fonttype: ""         },

No space after {, please, and align the closing }s.

>+        window.setTimeout(BuildFontList, i*100, languageGroup,
>+                          prefs[i].fonttype, element, preference);

First, the * should have spaces around.
Second, eww - do we really care about the order? We don't have control over it anyway, even with this. Can't we just hide the entire row until the dropdown is filled?

>+function ReadFontSelection(aElement)
...
>+    // There is a setting that actually is in the list. Respect it.
>+    if (fontItems.length > 0)
>+      return undefined;

As much as I usually dislike that, I agree that explicitly returning undefined here is more readable.


No real problems, r- just for the number of nits.
Attachment #303785 - Flags: review?(mnyromyr) → review-

Updated

10 years ago
Depends on: 419452
(Assignee)

Comment 30

10 years ago
(In reply to comment #29)
> (From update of attachment 303785 [details] [diff] [review])
> >Index: pref-fonts.js
> >===================================================================
> >+    for (var i = 0; i < fonts.length; ++i) {
> >+      menuitem = document.createElement("menuitem");
> >+      menuitem.setAttribute("value", fonts[i]);
> >+      menuitem.setAttribute("label", fonts[i]);
> 
> You can just use 
>   for each (var font in fonts)
>   ...
>   menuitem.setAttribute("value", font);
> here.
for...in should not be used for arrays - see MDC for details - http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Statements:for...in#Description

> >+        window.setTimeout(BuildFontList, i*100, languageGroup,
> >+                          prefs[i].fonttype, element, preference);
> 
> First, the * should have spaces around.
> Second, eww - do we really care about the order? We don't have control over it
> anyway, even with this. Can't we just hide the entire row until the dropdown is
> filled?
Hiding looks even more ugly than doing it this way. Patch with rest of changes coming up.
(Assignee)

Comment 31

10 years ago
Created attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

Changes since v0.3:
* Now honour lock status of browser.display.languageList
* Made most of the changes per mnyromyr's review (see previous comment for exceptions)
* Used legacy pref-fonts' way of stopping menulists jumping once populated (setting an empty label on the menulist element) - alternatively could set it in the xul but I think it looks better if the menulists go blank and disabled when switching between language groups rather than just disabled.
Attachment #303785 - Attachment is obsolete: true
Attachment #306918 - Flags: superreview?(neil)
Attachment #306918 - Flags: review?(mnyromyr)
(Assignee)

Comment 32

10 years ago
(In reply to comment #31)
> Created an attachment (id=306918) [details]
> pref-fonts patch v0.4
Hmmm, I meant to put something in about browser.display.languageList in either the xul or js files - any preference?
(Assignee)

Comment 33

10 years ago
Comment on attachment 306108 [details] [diff] [review]
main appearance pane, v1

>Index: mozilla/suite/common/pref/pref-appearance.xul
>===================================================================
>+<!DOCTYPE overlay [
>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD;
>+  <!ENTITY % prefAppearanceDTD SYSTEM "chrome://communicator/locale/pref/pref-appearance.dtd"> %prefAppearanceDTD;
> ]>
Add an empty line here, before the <overlay>.

>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

>Index: mozilla/suite/common/pref/pref-appearance.js
>===================================================================
>+function switchLocales_load() {
Functions should be properly Capitalised, so should be SwitchLocales_Load()

>+function SelectLocale(aElement)
>+{
Declare matchItems here, so you are not declaring it twice (strict JS warnings otherwise I think).
>+  var pref = document.getElementById(aElement.getAttribute("preference"));
>+  if (pref.value) {
>+    var matchItems = aElement.getElementsByAttribute("value", pref.value);
>+    // If the pref matches an entry that actually is in the list, use it.
>+    if (matchItems.length > 0)
Only need to do (matchItems.length) as if it exists it will always be > 0.

>+      return pref.value;
>+  }
>+
>+  if (pref.defaultValue) {
>+    var matchItems = aElement.getElementsByAttribute("value", pref.defaultValue);
>+    // If the pref's default matches an entry that actually is in the list, use it.
>+    if (matchItems.length > 0)
As above.
>+      return pref.defaultValue;
>+  }
>+
>+  // If prefs can't point us to a valid value and something is set, leave that.
>+  if (aElement.value)
>+    return aElement.value;
>+
>+  // If somehow we still have no value, return the first value in the list
>+  return aElement.firstChild.firstChild.getAttribute("value");
>+}
>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>     <treechildren id="prefsPanelChildren">
>       <treeitem label="READ THIS!" prefpane="temp_readthis_pane"/>
>-<!-- commenting out yet unmigrated panels
>+      <!-- Appearance items -->
>       <treeitem container="true"
>-                id="appearance"
>+                id="appearanceItem"
>                 label="&appear.label;"
>+                prefpane="appearance_pane"
>+                helpTopic="appearance_pref"
>                 url="chrome://communicator/content/pref/pref-appearance.xul">
You are correct about position of prefpane/helpTopic but I think we have, already, become inconsistent in the layout of each treeitem...
Probably needs to be sorted.
Attachment #306108 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 34

10 years ago
Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>+          <treeitem id="contentItem"
>+                    label="&content.label;"
>+                    helpTopic="appearance_pref_content"
>+                    prefpane="content_pane"
>+                    url="chrome://communicator/content/pref/pref-content.xul"/>
See your previous comments about position of helpTopic / prefpane, you might as well correct the ones for "colorsItem" too.
>+<!-- commenting out yet unmigrated panels
>           <treeitem label="&fonts.label;"
>                     url="chrome://communicator/content/pref/pref-fonts.xul"/>
> -->
>           <treeitem id="colorsItem"
>                     label="&colors.label;"
>                     helpTopic="appearance_pref_colors"
>                     prefpane="colors_pane"
>                     url="chrome://communicator/content/pref/pref-colors.xul"/>

Rest as per previous comment, r=me with those changes.
Attachment #303386 - Flags: review?(iann_bugzilla) → review+

Comment 35

10 years ago
(In reply to comment #22)
> >Index:  mozilla/suite/locales/en-US/chrome/common/pref/pref-content.dtd
> >===================================================================
> >+<!ENTITY pref.content.description               "Those settings influence how website and message content appears in &brandShortName;. See the separate preferences for colors, fonts and languages to further customize website appearance and the Privacy &amp; Security section for privacy-related website settings.">
> "These" instead of "Those", also drop the "website" from the first part of the
> second sentence so it becomes "to further customize appearance". Perhaps say
> "separate preference panels" or "pages" or similar?

I shortened it to "These settings influence how website and message content appears in &brandShortName;." - the rest can be explained in the help docs (done in a followup).

Comment 36

10 years ago
(In reply to comment #28)
>(In reply to comment #27)
>>(In reply to comment #26)
>>>(From update of attachment 306108 [details] [diff] [review] [details] [details])
>>>>-  <groupbox id="generalStartupPreferences" align="start">
>>>Why rename this?
>>Because IanN suggested that, see comment #19
>My understanding is that the change would only be needed if the same overlay
>file was used for both SM2.x and pre SM2.x versions of pref-appearance.xul for
>chatzilla (unless you do as Neil has done in the latest patch for that).
Actually you don't even need it then; the checkboxes are almost identical.

Comment 37

10 years ago
Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

>                     url="chrome://communicator/content/pref/pref-colors.xul"/>
>-<!--
>-          <treeitem label="&locales.label;"
>-                    url="chrome://communicator/content/pref/pref-locales.xul"/>
>         </treechildren>
>       </treeitem>
>--->
>       <treeitem container="true"
Not part of this patch.

>+    </groupbox>
>+
>+    <vbox class="box-padded" align="start">
>+      <separator class="thin" />
Nits: Separator between the groupbox and the vbox please.
      Also lose the space before the /.

>+<!ENTITY useFavIcons.label                      "Aggressively look for Web Site Icons when not defined in page">
Nit: I think this is bad grammar, but I'm not sure what I'd like this to say; maybe it would suffice to reduce it to "Aggressively look for Web Site Icons" (i.e. without mentioning that this only makes a difference when there's no icon link tag in the page) or maybe change the condition to "... when the page does not define one".
Attachment #303386 - Flags: superreview?(neil) → superreview+

Comment 38

10 years ago
Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

Content panel is checked in, thanks.
Attachment #303386 - Attachment description: add content panel, v2 (including site icon groupbox) → add content panel, v2 (including site icon groupbox) [checked in]

Updated

10 years ago
Blocks: 420730

Updated

10 years ago
Blocks: 420732

Comment 39

10 years ago
Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

>+      if (fonts.lastIndexOf(gAllFonts[i]) == 0)
Nit: I said fonts.lastIndexOf(gAllFonts[i], 0) == 0
Attachment #306918 - Flags: superreview?(neil) → superreview+

Comment 40

10 years ago
Created attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

New version of the patch, all comments fixed and tested in current builds (yay, loading overlays works again).
Attachment #306108 - Attachment is obsolete: true
Attachment #308031 - Flags: superreview?(neil)
Attachment #308031 - Flags: review?(iann_bugzilla)

Comment 41

10 years ago
Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

>+    </preferences>
>+
>+  <stringbundleset id="prefAppearanceBundleset">
Nit: indentation of this and the next five lines is wrong

>+    <groupbox id="switchLocaleBox">
Nit: IMHO this groupbox needs align="start" too.

>Index: mozilla/suite/common/pref/pref-appearance.js
>===================================================================
>RCS file: mozilla/suite/common/pref/pref-appearance.js
>diff -N mozilla/suite/common/pref/pref-appearance.js
>--- /dev/null	2008-02-25 20:03:38.000000000 +0100
>+++ mozilla/suite/common/pref/pref-appearance.js	2008-03-07 23:25:07.000000000 +0100
Still no jar.mn change in the patch ;-)
Attachment #308031 - Flags: superreview?(neil) → superreview+

Comment 42

10 years ago
Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

Yeah, looks much better without flickering dropdown boxes. ;-)
Attachment #306918 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 43

10 years ago
Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

With Neil's final comment addressed.
Checking in to trunk
preftree.xul;
new revision: 1.117; previous revision: 1.116
done
preferences.xul;
new revision: 1.14; previous revision: 1.13
pref-fonts.xul;
new revision: 1.87; previous revision: 1.86
pref-fonts.js;
new revision: 1.54; previous revision: 1.53
done
Attachment #306918 - Attachment description: pref-fonts patch v0.4 → pref-fonts patch v0.4 (Patch checked in)
(Assignee)

Comment 44

10 years ago
Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

>Index: mozilla/suite/common/pref/preftree.xul
>===================================================================
>-        <treeitem>
>-          <treerow>
>-            <treecell url="chrome://communicator/content/pref/pref-locales.xul" label="&locales.label;"/>
>-          </treerow>
>-        </treeitem>
In theory isn't this being migrated too even if it is to the main appearance pane?

>Index: mozilla/editor/ui/composer/content/editorPrefsOverlay.xul
>===================================================================
>   <!-- editor startup toggle -->
>-  <groupbox id="generalStartupPreferences">           
>-    <checkbox id="generalStartupEditor" wsm_persist="true"
>+  <preferences id="appearance_preferences">
>+    <preference id="general.startup.editor"
>+                name="general.startup.editor"
>+                type="bool"/>
>+  </preferences>
Nit: Preferences should come before the <!-- editor startup toggle --> comment

r=me with those addressed (and Neil's comments)
Attachment #308031 - Flags: review?(iann_bugzilla) → review+

Comment 45

10 years ago
Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

Patch for main panel including removal of locales panel has been checked in now.
Attachment #308031 - Attachment description: main appearance pane, v2 → main appearance pane, v2 [checked in]

Comment 46

10 years ago
Actually, from what I see, this completes the work in this bug, so marking this FIXED.
This is the first pref category (besides debug) to be completely ported to the new window!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Verifying on tinderbox build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031507 SeaMonkey/2.0a1pre

Appearance
  Content
  Fonts
  Colors

are now on the "new" preferences UI. In the "(Legacy Prefwindow)" they now show an empty page, except "Content" which is absent.
(Assignee)

Comment 48

10 years ago
Don't forget updating help too.

Comment 49

10 years ago
Help for this is done together with the content panel in bug 420730
You need to log in before you can comment on or make changes to this bug.