550589 - Get the font chooser working properly with font.name-list.*

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: WIP

There are currently two issues with the font chooser and font.name-list.*:

1. The Advanced dialog inexplicably ignores the font list if it is a default pref.
2. The Display pane doesn't work at all.

Writing a test for this required a fair amount of extension to mozmill. asuth, am I being stupid somewhere and needlessly complicating a part of the patch?

Comment 1

[Rain [:rain1] (inactive)]

Attachment: patch, v1

Blake, could you look at the font bits, and asuth at the extension to mozmill?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 430975 [details] [diff] [review]
patch, v1

Very forward-looking! Nice.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 430975 [details] [diff] [review]
patch, v1

>+++ b/mail/components/preferences/display.js
>@@ -108,21 +111,21 @@ var gDisplayPane = {
>       var element = document.getElementById(prefs[i].element);
>       if (element) {
>-        element.setAttribute("preference", preference.id);
>-
>         if (prefs[i].fonttype)
>           FontBuilder.buildFontList(aLanguageGroup, prefs[i].fonttype, element);
> 
>+        element.setAttribute("preference", preference.id);
>+

Why did you move this down?

>         preference.setElementValue(element);
>       }
>     }
>   },
> 
>   /**
>    * Returns the type of the current default font for the language denoted by
>    * aLanguageGroup.
>@@ -138,16 +141,52 @@ var gDisplayPane = {
>       preference.setAttribute("name", defaultFontTypePref);
>       preference.setAttribute("type", "string");
>       preference.setAttribute("onchange", "gDisplayPane._rebuildFonts();");
>       document.getElementById("displayPreferences").appendChild(preference);
>     }
>     return preference.value;
>   },
> 
>+  /**
>+   * Determine the appropriate value to select for defaultFont, for the following cases:

This line could be split to make it under 80 characters.

>+++ b/mail/test/mozmill/pref-window/wrapper.py
>@@ -0,0 +1,64 @@

>+_pref_file_names = {
>+    "nt": "windows-prefs.js",
>+    "mac": "mac-prefs.js",
>+    "posix": "linux-prefs.js",
>+}
>+
>+def on_profile_created(profiledir):
>+    """
>+    On profile creation, this copies prefs.js from the current folder to
>+    profile_dir/preferences. This is a somewhat undocumented feature -- anything
>+    in profile_dir/preferences gets treated as a default pref, which is what we
>+    want here.
>+    """
>+    prefdir = os.path.join(profiledir, "preferences")
>+    # This needs to be a directory, so if it's a file, raise an exception
>+    if os.path.isfile(prefdir):
>+        raise Exception("%s needs to be a directory, but is a file" % prefdir)
>+    if not os.path.exists(prefdir):
>+        os.mkdir(prefdir)
>+    # The pref file is in the same directory this script is in
>+    preffile = os.path.join(os.path.dirname(__file__), _pref_file_names[os.name])
>+    shutil.copy(preffile, prefdir)

So, sadly, the test fails on my mac, with the following output:
TEST-PASS |  setupModule
TEST-PASS |  test_font_name_displayed
TEST-UNEXPECTED-FAIL |  test_font_name_not_present
  EXCEPTION: The second font in font.name-list.serif.x-western (serif) should be present on this computer, but isn't.
    at: test-font-chooser.js line 189
       Error("The second font in font.name-list.serif.x-western (serif) should be present on this computer, but isn't.")  0
       test_font_name_not_present() test-font-chooser.js 189
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  teardownModule
make: *** [mozmill-one] Error 1

Digging into it a little, I find that os.name on Mac OS X 10.6 is "posix", so I'm getting the linux prefs, which isn't so good.  However the following might be useful:
  python -c "import sys;print sys.platform"
  darwin
http://stackoverflow.com/questions/446209/possible-values-from-sys-platform has more possible values.

I'm going to give it an r- because of that, but other than that bug, I'm pretty happy with the patch, so I expect the next review cycle to be pretty quick.  ;)

Later,
Blake.

Comment 4

[Rain [:rain1] (inactive)]

Attachment: patch, v1.5

I haven't tested this on Linux, but the stackoverflow page leads me to believe it'll work.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 433131 [details] [diff] [review]
patch, v1.5

The changes look good to me, and the test passes, so here's the r+.

Later,
Blake.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(Oh, and remember to fix that dict lookup thing that we talked about in IRC, while you're checking in, okay?  ;)

Comment 7

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/7b60a657c20c

Comment 8

[Rain [:rain1] (inactive)]

Pushed a followup, rs=philor over IRC: http://hg.mozilla.org/comm-central/rev/5ad4853677c7