175651 - CJK (at least Chinese and Japanese) font Preferences changes not applied to any CJK Web page

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Andrew Jacobson]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021020 Chimera/0.5+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021020 Chimera/0.5+

Chimera will only display Japanese as well as Simplified and Traditional Chinese
(and probably Korean) Web pages in the default system fonts that were common in
System 6 through OS 9.x (i.e., Beijing and Song for Simplified Chinese, Taipei
and Apple LiGothic or LiSung for Traditional Chinese, Osaka and Osaka-Mono for
Japanese), regardless of what serif and sans-serif fonts the user selects in
Preferences for each language.

Reproducible: Always

Steps to Reproduce:
1. In Preferences, select the following fonts in Preferences for both serif and
sans-serif: Biaukai for Traditional Chinese, STKaiti for Simplified Chinese,
Hiragino Mincho Pro W3 or preferably some third-party calligraphic font for
Japanese, in both Chimera and Mozilla.
2. Visit the following test sites and any others for the respective languages in
the latest versions of both Chimera and Mozilla:
http://www.yzzk.com/current/yzmain.htm for Traditional Chinese,
http://www.peopledaily.com.cn/ for Simplified Chinese, and
http://www.yomiuri.co.jp/ or http://www.mainichi.co.jp/ for Japanese.
3. Compare Chimera rendering to Mozilla’s to see how the pages should appear
with the font settings.
Actual Results:  
Mozilla displays CJK pages in whatever fonts are installed and selected in
Preferences, but Chimera displays the same pages only in Apple's default system
bitmap fonts, possibly from ROM instead of the actual properly and fully
installed fonts. Actually, Taipei and especially Beijing have always looked like
illegible bitmap fonts even after Apple finally came out with TrueType versions
at about OS 8.6. Therefore, the default system TT or OTC fonts may still be used
but only appear as bitmap fonts.

Expected Results:  
Chimera should display pages in whatever fonts are selected in Preferences for
each language, as Mozilla properly does, instead of only the default system
fonts for each language.

Before any non-Asian language reader considers lowering the severity status to
Normal, note that according to the definition, a major feature is in fact broken
and is thus considered Major.

Comment 1

[kerim]

My experience was similar if a little different. I was able to set the serif
font for Traditional Chinese - and it worked when visiting sites -  but unable
to even assign a new monospace font. The button brings up the font selection
box, but changes are not registered upon closing the box.

Comment 2

[Glenn D. Tiffert]

My experience under late builds of Chimera 0.5 and (after initial testing) the official 
release of 0.6, is the same as the original report.  Changes to the font or font size prefs for 
Chinese have no effect.

Comment 3

[Joe]

I can confirm this for Traditional Chinese. The Mac OS X font panel
comes up just fine and I'm able to select, but the browser continues to
render the Taipei font for Big-5 encoded pages.

Comment 4

[Stephane Moureau]

FYI:
http://sourceforge.net/projects/monafont/
Mona Font is a Japanese proportional font which allows you to view Japanese text
arts correctly.

Comment 5

[Simon Fraser [no longer active]]

Need to look into this.

Comment 6

[Michael Lam]

I think that Chimera is defaulting to the ugly Taipei for Traditional Chinese 
font. Even MSIE is displaying TW Chinese better than Chimera. Something must be 
done to fix this because the font is so ugly that I cannot use Chimera to 
browse TW Chinese sites.

Comment 7

[Ludovic Hirlimann [:Usul]]

Reporter could you verify this bug with a newer (nigtly) build of Camino and
update this bug please. To me it seems that 2003090102 on 10.2.6 displays your
URLs correctly.

Comment 8

[[e-mail]]

The URL for Traditional Chinese is gone. But if you check http://tw.yahoo.com
and you can see all the text are still rendered in bitmap.

Interestingly, Safari does the same thing. But IE renders the text in smooth
outline.

Changing font settings in preferences has no effect.

Comment 9

[waverider]

Font setting save prefs.js.
example:user_pref("font.name.serif.zh-TW", "FONTNAME");
"FONTNAME" should set Font file name,but Camino preference set "Font full name".
("Font full name" can check on the Info panel of Finder.)

example:
Font file name = nouvelle.ttf
Font full name = new-world
correct = user_pref("font.name.serif.zh-TW", "nouvelle");
incorrrect = user_pref("font.name.serif.zh-TW", "new-world");
Camino is the latter.

If file name and full name are same fortunately,no problem.
but If there're not same,Changing fails and selected system bitmap font
automatically.

I think camino font preference should be rewrite to set Font file name to user_pref.

Comment 10

[Simon Fraser [no longer active]]

Thanks for the analysis.

Comment 11

[Hiro]

This problem exists all the time, after Chimera appears.
For a CJK user, a font setup of Camino is very complicated.
Supposing it is possible, please fix this problem by 0.8 releases.

Comment 12

[[e-mail]]

#11:

It's not complicated. It's just not working. No matter how I set it, Camino always uses Apple LiGothic 
Medium for Chinese text. With Panther, we should at least be able to use the new LiHei Pro which looks 
much better than the old ones in small point size situations.

Comment 13

[Hiro]

Too, it is very inconvenient that a Japanese font cannot be chosen from Preference.
Even if it considers a version called 0.8 releases, it desires to already fix soon.

Comment 14

[waverider]

Camino's font preferences is Cocoa, and return font name of Cocoa type.
but Gecko is Carbon, and Camino should return font name of Carbon type.

We need Carbon guru...

Comment 15

[[e-mail]]

http://developer.apple.com/technotes/tn2002/tn2058.html

Maybe this can help?

Comment 16

[waverider]

Attachment: rough patch.

This patch only fix Menuitems at Appearance - Fonts - Advanced...
Call Carbon API and enumerate QuickDraw Fontnames.
reference is "nsDeviceContextMac :: InitFontInfoList() at
mozilla/gfx/src/mac/nsDeviceContextMac.cpp"

Camino developers, Please brush up this rough patch.

Comment 17

[Simon Fraser [no longer active]]

There was a bug where if a font was missing, subsequent attempts to choose a
different font did not stick. That will be fixed in the 3/1 nightly.

Comment 18

[Simon Fraser [no longer active]]

I can't reproduce the bug here (now that other stuff has been fixed.

I downloaded Minya Nouvell.ttf, renamed it Nouvelle.ttf so that the file name
doesn't match the font family name, and installed it. I chose it in the font
prefs (it shows up as "Minya Nouvelle"), and pages render in that font.

I think the real problem here is that the default fonts are all missing, and
there was a bug that caused you to not be able to change a font that is marked
as missing.

Comment 19

[waverider]

Comment on attachment 175300 [details] [diff] [review]
rough patch.

>Index: Appearance.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/Appearance/Appearance.mm,v
>retrieving revision 1.11
>diff -u -9 -r1.11 Appearance.mm
>--- Appearance.mm	16 Feb 2005 16:40:32 -0000	1.11
>+++ Appearance.mm	23 Feb 2005 15:12:19 -0000
>@@ -33,18 +33,20 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
>  
> #import "Appearance.h"
> 
>+#define USE_QUICKDRAW_FONTNAME 1
>+
> @interface OrgMozillaChimeraPreferenceAppearance(Private)
> 
> - (void)setupFontRegionPopup;
> - (void)updateFontPreviews;
> - (void)loadFontPrefs;
> - (void)saveFontPrefs;
> 
> - (NSMutableDictionary *)makeDictFromPrefsForFontType:(NSString*)fontType andRegion:(NSString*)regionCode;
> - (NSMutableDictionary *)makeDefaultFontTypeDictFromPrefsForRegion:(NSString*)regionCode;
>@@ -735,18 +737,68 @@
> {
>   NSMenu* menu = [popupButton menu];
> 
>   [menu setAutoenablesItems:NO];
> 
>   // remove existing items
>   while ([menu numberOfItems] > 0)
>     [menu removeItemAtIndex:0];
> 
>+#ifdef USE_QUICKDRAW_FONTNAME
>+  // We should enumerate QuickDraw fontname while Gecko use QuickDraw API
>+  // reference is "nsDeviceContextMac :: InitFontInfoList() at mozilla/gfx/src/mac/nsDeviceContextMac.cpp"
>+  OSStatus err;
>+  FMFontFamilyIterator iter;
>+  err = FMCreateFontFamilyIterator(NULL, NULL, kFMDefaultOptions, &iter);
>+  if (err != noErr)
>+    return;
>+  TextEncoding unicodeEncoding = ::CreateTextEncoding(kTextEncodingUnicodeDefault, 
>+															kTextEncodingDefaultVariant,
>+													 		kTextEncodingDefaultFormat);
>+  // enumerate all fonts
>+  TECObjectRef converter = 0;
>+  TextEncoding oldFontEncoding = 0;
>+  FMFontFamily fontFamily;
>+  while (FMGetNextFontFamily(&iter, &fontFamily) == noErr) {
>+    Str255 fontName;
>+    err = ::FMGetFontFamilyName(fontFamily, fontName);
>+    if (err != noErr || fontName[0] == 0 || fontName[1] == '.' || fontName[1] == '%')
>+      continue;
>+    TextEncoding fontEncoding;
>+    err = ::FMGetFontFamilyTextEncoding(fontFamily, &fontEncoding);
>+    if (oldFontEncoding != fontEncoding) {
>+      oldFontEncoding = fontEncoding;
>+      if (converter)
>+        err = ::TECDisposeConverter(converter);
>+      err = ::TECCreateConverter(&converter, fontEncoding, unicodeEncoding);
>+      if (err != noErr)
>+        continue;
>+    }
>+    // convert fontname to UNICODE
>+    UniChar unicodeFontName[sizeof(fontName)];
>+    ByteCount actualInputLength, actualOutputLength;
>+    err = ::TECConvertText(converter, &fontName[1], fontName[0], &actualInputLength, 
>+                           (TextPtr)unicodeFontName , sizeof(unicodeFontName), &actualOutputLength);
>+    unsigned int len = actualOutputLength / sizeof(UniChar);
>+    unicodeFontName[len] = '\0';
>+    
>+    // build Menu items
>+    CFStringRef tmpstr;
>+    tmpstr = CFStringCreateWithCharacters(kCFAllocatorDefault, unicodeFontName, len);
>+    NSString* QDFontName = [[[NSString alloc] initWithString:(NSString*) tmpstr] autorelease];
>+    NSMenuItem* newItem = [[[NSMenuItem alloc] initWithTitle:QDFontName action:nil keyEquivalent:@""] autorelease];
>+    [menu addItem:newItem];
>+  }
>+  if (converter)
>+    err = ::TECDisposeConverter(converter);
>+  err = FMDisposeFontFamilyIterator(&iter);
>+
>+#else        
>   NSArray*	fontList = [[NSFontManager sharedFontManager] availableFontFamilies];
>   NSArray*  sortedFontList = [fontList sortedArrayUsingSelector:@selector(caseInsensitiveCompare:)];
>   
>   for (unsigned int i = 0; i < [sortedFontList count]; i ++) {
>     NSString* fontFamilyName = [sortedFontList objectAtIndex:i];
>     unichar firstChar = [fontFamilyName characterAtIndex:0];
>     
>     if (firstChar == unichar('.') || firstChar == unichar('#'))
>       continue; // skip fonts with ugly names
>@@ -767,23 +819,23 @@
>         NSMenuItem* newSubmenuItem = [[NSMenuItem alloc] initWithTitle:fontItemName action:nil keyEquivalent:@""];
>         [familySubmenu addItem:newSubmenuItem];
>       }
>       
>       [newItem setSubmenu:familySubmenu];
>     } else {
>       // use the name from the font family info?
>     }
> #endif
>-
>     [menu addItem:newItem];
>   }
>-}
> 
>+#endif
>+}
> @end
> 
> @implementation OrgMozillaChimeraPreferenceAppearance (FontManagerDelegate)
> 
> - (void)changeFont:(id)sender
> {
>   if (fontButtonForEditor) {
>     NSString *fontType = [fontButtonForEditor alternateTitle];
>     [self updateFontSampleOfType:fontType];

Comment 20

[waverider]

sorry, comment#19 is my mistake.

I test 2005030108 nightly and this bug exist yet.

1. About some font, name refered by QuickDraw and that refered by Quartz are
different.
2. Gecko require font name refered by QuickDraw.
3. Camino's preference use NSFontManager and this class don't use font name
refered by QuickDraw.
there are cause of this bug.

p.s
http://developer.apple.com/fonts/OSXTools.html
Install this tool and run command:
% ftxinstalledfonts -q
We get Output the installed fonts' QuickDraw names. This list is necessary for
Gecko font setting.
This output don't agree the font list of Camino's advanced font preference setting.
Please check it.

Comment 21

[Simon Fraser [no longer active]]

Please give me an example of a font that you can choose in the Camino prefs, but
that shows incorrect rendering. I was able to choose Japanese fonts, and see Ja
pages render OK in the latest Camino build.

Comment 22

[waverider]

Attachment: screenshot of result.

re:comment#21

Steps to Reproduce:
1. open http://www.yahoo.co.jp
2. open camino preference - appearance - font
3 .choose "Japanese" and click "Advanced..."
4. set serif to "ヒラギノ丸ゴ Pro"
5. set default font to serif.
6. close camino preference window

at present, nightly build show wrong rendering .

http://caminofreak.hp.infoseek.co.jp/subset/caminol10nJP.html
access this page and download "Camino0.8.2_bug175651".
this camino is applied my patch.
Please test my camino too.
(When test my camino, set serif to
"ヒラギノ丸ゴ Pro W4")

Comment 23

[waverider]

Attachment: sorry. set serif to this font and please test.

Comment 24

[Jungshik Shin]

 waverider , please set View | Character Encoding to UTF-8 before posting
non-ASCII characters in bugzilla. Your comments with Japanese fonts names are
all in NCRs (which I can convert back to Kanzi's, but it's inconvenient)

Also, note that unless 'Japanese' is the first in the list of languages in the
international system preference, Japanese font names don't come up in Japanese.
That is, with a language other than 'Japanese' at the top of the list, I have
'Hiragino Kaku Gothic Pro' instead of 'ヒラギノ　角？　Pro W3'. The current
all.js lists Japanese font names in Japanese and Camino regards them as missing.

sfraser, the bug you mentioned in comment #17 seems to be still an issue.
Changing the font has no effect. For instance, the default traditional Chinese
serif font is 'Apple LiSung Medium (missing)', change it to 'LiHei Pro' and then
'LiSong Pro'. The former is 'sans-serif' style while the latter is 'serif' style
so that the rendering should be distinctly different, but it's not. 

Comment 25

[Jungshik Shin]

With waverider's patch applied, I get '儷宋 Pro' and '儷黑 Pro' instead of
'LiSong Pro' and 'LiHei Pro'. Selecting either of them does change the font. 

A simple test case: 

<ul lang="zh-TW">
<li style="font-family: serif;">新聞 - 股市 - 理財 - 健康 1234 Abcde lmnop</li>
<li style="font-family: sans-serif;">新聞 - 股市 - 理財 - 健康 1234 Abcde lmnop<li>
<li style="font-family: monospace;">新聞 - 股市 - 理財 - 健康 1234 Abcde lmnop</li>
</ul>

Comment 26

[Jungshik Shin]

Attachment: test case

Hmm. it's more complex than that. Attachment 175300 [details] [diff] only touched the part which
enumerates font for 'Advanced'. Fonts selected via 'Advanced' are later
labelled as 'missing' apparently because QuickDraw name selsected is not
recognized. 

Anyway, this test case will help test and diagnose the problem.

Comment 27

[Jungshik Shin]

For each language, three lines of text are given. The first is styled with
'serif', the second is with 'sans-serif' and the last is with 'monospace'

Comment 28

[Simon Fraser [no longer active]]

Thank you for the testcase.

Comment 29

[Jungshik Shin]

(In reply to comment #26)
> Fonts selected via 'Advanced' are later
> labelled as 'missing' apparently because QuickDraw name selsected is not
> recognized. 

  because Carbon names selected in 'Advanced' is not recognized by Cocoa APIs.
This point was already mentioned in comment #16

Comment 30

[Simon Fraser [no longer active]]

There must be APIs to map between Cocoa and Carbon fonts. I'll make some enquiries.

Comment 31

[Jungshik Shin]

Please, ignore the paragraph that begins with the following in  comment #24. I
was mistaken and I don't want to confuse anyone with my incorrect statement.
 
> Also, note that unless 'Japanese' is the first in the list of languages in the
> international system preference, Japanese font names don't come up in Japanese.

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

There are a couple of bugs out there that appear that they would be fixed if
Camino would write CJK fonts prefs in Carbon to please Gecko; one was even
targetted for 0.9.

There also is a "revised" fonts prefpane available that (so I am understand from
actual CJK users) does this correctly http://www.fan.gr.jp/~sakai/moz.php and is
available under the MPL license, so there is someone/some code out there that
can fix this bug.  Can it be fixed for 0.9, though?

Comment 33

[Simon Fraser [no longer active]]

I tried that "Extra Prefs" panel, and note that it could now be a drop-in prefs
panel (the signature just needs to be changed to "CHIM"). It needs a bit of
work, and I'm not a big fan of the UI (navigating through huge long font menus
is hard). But it seems to function OK.

Comment 34

[waverider]

Attachment: rough patch 02 : improve ability of advanced font setting

a compromise for camino 0.9.
fix CJK font selecting bug and "missing" display bug only about advanced font
setting panel.

I have no skill and time to fix this bug completely.
If we fix only advanced font setting panel for the present, CJK user will be
happy in camino 0.9.

Comment 35

[Jungshik Shin]

Could you attach your patch in plain text instead of 'sit'? 

Comment 36

[waverider]

Attachment: text patch

I put this patch and new "Appearance.nib" into .sit file.
Appearance.nib contain binary file (keyedobjects.nib) and I can't upload it as
text.

Comment 37

[Simon Fraser [no longer active]]

Thanks for the patch. This seems like a reasonable thing to do: i'll look at it.

Comment 38

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 258750 has been marked as a duplicate of this bug. ***

Comment 39

[Eiichi]

Just for reference;
Hiroto Sakai releases "Camino Appearance.prefPane alternative".
http://www.fan.gr.jp/~sakai/moz.php

Download caa-050619.dmg：http://www.fan.gr.jp/~sakai/files/caa-050619.dmg

Comment 40

[Samuel Sidler (old account; do not CC)]

This definitely needs to be in 0.9 if it fixes bug 278255. Josh, can look at this?

I'm targeting for 0.9. If this is wrong, please change it.

Comment 41

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #40)
> This definitely needs to be in 0.9 if it fixes bug 278255. 

Yes, the re-worked prefpane in comment 39 (just like its predecessor in comment
32) fixes all the sample urls in bug 278255 and its dupe, bug 275949 comment 4,
at least with the default Gecko fonts (Apple LiSung Light and Apple LiGothic
Medium) once they're set properly (i.e., the default "missing" fonts if you've
never touched your CJK font prefs).  At this point the UI and Gecko are in sync
over the name of the font that should be used.  They also work when I then
select "BiauKai" (mentioned in comment 1) for the Traditional Chinese font
(serif and sans).

What we need is to hook up the font-name-Carbon-Cocoa-conversion stuff so that
we can use the nice Cocoa font panel rather than being stuck with the awful UI
of an endlessly-scrolling drop-down list. :-(

Comment 42

[Simon Fraser [no longer active]]

(In reply to comment #41)
> What we need is to hook up the font-name-Carbon-Cocoa-conversion stuff so that
> we can use the nice Cocoa font panel rather than being stuck with the awful UI
> of an endlessly-scrolling drop-down list. :-(

I'm not sure that such a conversion is possible. I believe you actually have to
generate the font listing using Carbon APIs, which means that we can't use the
Font panel, which sucks.

Comment 43

[Mike Pinkerton (not reading bugmail)]

what's the status of this? do we want this patch, or do we also need a new
version of the pref panel?

Comment 44

[Simon Fraser [no longer active]]

yeah, we should probably take this patch, or something like it.

Comment 45

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #42)
> I'm not sure that such a conversion is possible. I believe you actually have to
> generate the font listing using Carbon APIs, which means that we can't use the
> Font panel, which sucks.

Simon, can we do this, or is it only for Carbon apps?
http://developer.apple.com/technotes/tn2002/tn2058.html

Alternatively, the UI in v1.0.2 of Camino Extra Fonts
http://www.fan.gr.jp/~sakai/moz_en.php is better than the drop-down list and
perhaps something to consider as a starting point for an alternative UI if we
have to roll our own.

Comment 46

[Simon Fraser [no longer active]]

(In reply to comment #45)
> (In reply to comment #42)
> > I'm not sure that such a conversion is possible. I believe you actually have to
> > generate the font listing using Carbon APIs, which means that we can't use the
> > Font panel, which sucks.
> 
> Simon, can we do this, or is it only for Carbon apps?
> http://developer.apple.com/technotes/tn2002/tn2058.html

I don't know. I'd be curious to know if the "Carbon" font panel really was the
Cocoa font panel with some munging APIs, and, if so, whether we can make use of
similar Cocoa->Carbon font name mapping.

> Alternatively, the UI in v1.0.2 of Camino Extra Fonts
> http://www.fan.gr.jp/~sakai/moz_en.php is better than the drop-down list and
> perhaps something to consider as a starting point for an alternative UI if we
> have to roll our own.

Well, the patch in this bug is also a good start. I just would really like to
avoid the popup-heavy UI that has been suggested.

Comment 47

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Simon, is this going to make 1.0?

Comment 48

[Eiichi]

Most of Japanese users eager to be solved this problem.
So this bug won't be fixed for 1.0, please present tentatively Camino Extra Fonts
(http://www.fan.gr.jp/~sakai/moz_en.php) in the release notes.

Comment 49

[Mike Pinkerton (not reading bugmail)]

i would rather see this release noted then radically change the UI at this point. we should also add this to the faq on the website.

Comment 50

[Simon Fraser [no longer active]]

The plan is not to change the UI, but just to build the popups in the "Advanced" dialog using Carbon font iteration.

Comment 51

[Eiichi]

Just for reference;
Hiroto Sakai released "Camino ExtraFonts.prefPane 1.0.3".

In English : http://www.fan.gr.jp/~sakai/moz_en.php
In Japanese: http://www.fan.gr.jp/~sakai/moz.php

Comment 52

[waverider]

Attachment: simple patch : fix menu popup of advanced font setting

Comment 53

[Eiichi]

Simon, if you have a chance, please take a look at an attachment (id=203708) before Camino  RC 1 is shipped.

Comment 54

[YAMASHITA Makoto]

(In reply to comment #52)
> +    err = ::FMGetFontFamilyName(fontFamily, fontName);
> +    if (err != noErr || fontName[0] == 0 || fontName[1] == '.' || fontName[1] == '%')
> +      continue;
> +    TextEncoding fontEncoding;
> +    err = ::FMGetFontFamilyTextEncoding(fontFamily, &fontEncoding);
You are using FM funcs, but they have a name-encoding problem for Symbol and Wingdings, see bug 213702.

Comment 55

[Samuel Sidler (old account; do not CC)]

I think this is important enough for our CJK users that it should be included in 1.0.1. Requesting approval for 1.0.1.

Comment 56

[waverider]

Attachment: simple patch v0.2 : use ATSFontFamily API (comment #54)

Comment 57

[Mark Mentovai]

Comment on attachment 218577 [details] [diff] [review]
simple patch v0.2 : use ATSFontFamily API (comment #54)

+  [mQDFontList release];

mQDFontList is never declared anywhere - bet you're missing a diff to Appearance.h here.

Comment 58

[Mark Mentovai]

Not gonna make 1.0.1, but we can do it for 1.0.2 if we get something tested in time for that.

Comment 59

[waverider]

Attachment: sorry, I added Appearance.h part.

Comment 60

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 218960 [details] [diff] [review]
sorry, I added Appearance.h part.

The font lists generated by this patch seem to include fonts which are disabled in Font Book.

Comment 61

[Simon Fraser [no longer active]]

Maybe useful: http://developer.apple.com/qa/qa2006/qa1471.html

Comment 62

[Ludovic Hirlimann [:Usul]]

pushing to future 1.0 is out.

Comment 63

[Samuel Sidler (old account; do not CC)]

This needs to happen well before "Future". Retargeting for 1.1.

Comment 64

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per previous comments, this bug still has a shot at making 1.0.x if we get a tested patch by then.

Comment 65

[waverider]

Re:comment #60
I tested comment #59's patch as adminisrater user and confirmed the font list don't include fonts which are disabled in Font Book.

Comment 66

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #65)
> Re:comment #60
> I tested comment #59's patch as adminisrater user and confirmed the font list
> don't include fonts which are disabled in Font Book.

Hmm, you're right; there's a bug somewhere else, and I'd bet it's Apple's (I see the same problem in Firefox).

The fonts in question are non-Unicode non-Roman fonts in my Classic folder, and I have all non-Unicode non-Roman fonts disabled in Font Book for Mac OS X.  Except, the fonts in question don't show up in Font Book, so they're not actually disabled :-(  They don't appear in the Cocoa Font Panel, though...odd.

Sorry about that.

My only other comment about the listings generated by this patch is that the fonts seem to be listed in alphabetical order within certain subgroups, e.g. A-Z, start over A-Z, start over A-Z, rather than the entire list being alphabetized A-Z as a whole (Fx does the latter).

Comment 67

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

BTW, this issue was included in the release notes for Camino 1.0.1; sorry about missing the boat on the relnote for 1.0 :-(

Lets see if we can lots of testing of this patch (particularly of the issues described in the bugs in the blocking list), including in some Cm1.0.1-based builds to account for any branch vs. trunk Gecko changes, so we can get this reviewed and in 1.0.2.

Then we should be good until Cairo completely redoes fonts ;)

Comment 68

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

To facilitate testing of waverider's patch in hopes of getting it in 1.0.3, I've made a Camino 1.0.x build (PPC-only; sorry) with the patch and it's available for testers at http://www.jonagold.org/caminotmp/Camino-10x-bug175651.dmg

Please pass the word to CJK users; we need to make sure that the issues raised in the bugs on the blocking list above are all fixed by this patch and that no other problems arise.  Note that you will have to use the pop-up lists in the "Advanced" sheet in the Fonts preferences to change fonts (the main screen of the Fonts prefpane is still broken, and it will still report fonts as "missing" when they are not).

Comment 69

[philippe (part-time)]

(In reply to comment #68)
10.4.6, with a fresh profile.

Gave this build a run through some of the Japanese sites I designed (coded shift_jis, utf-8), and switched font regularly. I haven't seen any problems at all .
All my sites specify a font-family in the stylesheet though, they should work anyway. Some other sites I visited (like yahoo.co.jp) don't specify anything, as do some of my test files. 
User set prefs are correctly applied.

Tested with the (Japanese) fonts that ship with OS X 10.4, and some third-party Japanese fonts (Morisawa Shin-go, some from Dynafonts - 'pop' fonts).

Selected fonts are correctly listed in about:config.

A couple of things, however:
1/ when accessing fonts > advanced, the sorting is kind of messy (comment #66).
2/ for Japanese at least, fonts > advanced still lists 'cursive' and 'fantasy' as XXX.cursive (missing) and XXX.fantasy (missing). Selecting a font for those does work correctly.
3/ If I understand this correctly, the user will have to use the advanced panel. One limit is that one can't set the font-size there (the default for Japanese is 14pt - Gecko Mac core. Gecko windows sets 16pt, Safari uses 16pt as well).

Point 2 is probably a detail. Neither Safari nor Firefox give an UI for those two.

Comment 70

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #69)
> 2/ for Japanese at least, fonts > advanced still lists 'cursive' and 'fantasy'
> as XXX.cursive (missing) and XXX.fantasy (missing). 

That's the Gecko default for not assigning a default font ;)  Since Mac OS X only includes 1 or 2 fonts for many languages/scripts (or only 1 or 2 that Gecko can understand), the Mac font prefs have a lot of XXX.cursive and XXX.fantasy entries.  (IOW, what philippe saw in point 2 is expected behavior there.)

> 3/ If I understand this correctly, the user will have to use the advanced
> panel. One limit is that one can't set the font-size there (the default for
> Japanese is 14pt - Gecko Mac core. Gecko windows sets 16pt, Safari uses 16pt as
> well).

One should be able to set the size using the panel and then select the correct  font by using the Advanced sheet and drop-downs.  It's an ugly work-around, but this whole bug is pretty ugly until we get Gecko using Cocoa font names :-(

Comment 71

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This won't make 1.0.2, but we would like to get this in 1.0.x if possible.

waverider, have you had a chance to look at the issue of the order in which the fonts are sorted (last paragraph of comment 66)?

Comment 72

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 339912 has been marked as a duplicate of this bug. ***

Comment 73

[Samuel Sidler (old account; do not CC)]

Comment on attachment 218960 [details] [diff] [review]
sorry, I added Appearance.h part.

We really need some traction on this bug. Mark, do you have time to take a look at this?

Comment 74

[Joe Wiz]

(In reply to comment #73)
> (From update of attachment 218960 [details] [diff] [review] [edit])
> We really need some traction on this bug. Mark, do you have time to take a look
> at this?

I'd be interested in testing builds that fix this issue - which I've only now just noticed.  Is the build listed above in Comment #68 the best one to use?

Comment 75

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Missed 1.0.3, but we can try again for 1.0.4 if the patch gets a positive review.

Comment 76

[Chris Lawson (gone)]

(In reply to comment #74)

> I'd be interested in testing builds that fix this issue - which I've only now
> just noticed.  Is the build listed above in Comment #68 the best one to use?

Yes, that'd be the one.

Comment 77

[Chris Lawson (gone)]

Simon, if you still plan to work on this, feel free to re-take it.

Comment 78

[Stuart Morgan]

*** Bug 278255 has been marked as a duplicate of this bug. ***

Comment 79

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This won't block 1.1.  If we get a solid patch with reviews, we'll certainly take it, though.

Comment 80

[Samuel Sidler (old account; do not CC)]

Comment on attachment 218960 [details] [diff] [review]
sorry, I added Appearance.h part.

Sean, do you think you could take a look at this? It's an old bug and, while it missed 1.5, we'd like to take it in a maintenance release for both Camino 1.0.x and Camino 1.5.x.

Comment 81

[Sean Murphy]

Comment on attachment 218960 [details] [diff] [review]
sorry, I added Appearance.h part.

waverider, thank you for your patch.  There are a few areas which I'd like to see changed before it's accepted.

Much of the Apple Type Services interaction and font list generation in patch appears to have been borrowed from nsDeviceContextMac.cpp:
http://mxr.mozilla.org/mozilla1.8/source/gfx/src/mac/nsDeviceContextMac.cpp#806

>+#define QUICKDRAW_FONTLIST 1

We should comment why this directive exists, possibly even referencing the bug report.

>+#if QUICKDRAW_FONTLIST
>+  NSEnumerator* tmpEnum = [mQDFontList objectEnumerator];
>+  NSString* tmpStr;
>+  BOOL foundFont = NO;
>+  while (tmpStr = [tmpEnum nextObject]) {
>+    if ([tmpStr isEqualToString:defaultValue]) {
>+      foundFont = YES;
>+      break;
>+    }
>+  }

I think we should avoid obscure variable names and strive for more clarity among them.  This concerns, most importantly, anything with "tmp" in its name.  Moreover, this block can be simplified to just:

BOOL foundFont = [mQDFontList containsObject:defaultValue];

>+#if QUICKDRAW_FONTLIST
>+  // we should enumerate QuickDraw fontname while Gecko use QuickDraw API
>+  mQDFontList = [[NSMutableArray array] retain];

Rather than allocate an object which has been autoreleased, and then retain it, lets just allocate an object:

mQDFontLost = [[NSMutableArray alloc] init];

Using initWithCapacity: would be nice if we could easily get a rough estimate as to how many fonts there will be.

>+  if (err != noErr)
>+    return;

I'd simplify this error check just slightly, avoiding an inequality:

if (err)
  return;

Concerning the iteration of ATSFontFamilies and the use of ATSFontFamilyIteratorNext:
>+  while (::ATSFontFamilyIteratorNext(iter, &fontFamily) == noErr) {

We are not accounting for changes which might be made to the font database during iteration.  

From the documentation:
"If any changes are made to the font database while you are using the font family iterator, the iterator is invalidated and the function ATSFontFamilyIteratorNext returns the error kATSIterationScopeModified. To remedy this error, your application must either restart or cancel the enumeration by calling the ATSFontFamilyIteratorReset or the ATSFontFamilyIteratorRelease functions."

>+    if (err != noErr || fontName[0] == 0 || fontName[1] == '.' || fontName[1] == '%')
>+      continue;

Like above, I'd just check: if (err || ...)

>+      if (err != noErr)
>+        continue;

if (err)

>+    // build QuickDraw fontname array
>+    CFStringRef tmpStr;
>+    tmpStr = CFStringCreateWithCharacters(kCFAllocatorDefault, unicodeFontName, len);
>+    NSString* QDFontName = [[[NSString alloc] initWithString:(NSString*) tmpStr] autorelease];
>+    [mQDFontList addObject:QDFontName];
>+  }

A few concerns with this block:
Again, unclear "tmp" variable naming.
You're leaking the CFString tmpStr, which follows the create rule and therefore must balanced with a call to CFRelease (make sure != NULL before attempting to release it).
Moreoever, we can nix that intermediary NSString and insert the CFString directly into mQDFontList:

[mQDFontList addObject:(NSString*)tmpStr]; // (still using tmpStr name)

>+  // build menu items from mQDFontList
>+  NSEnumerator* tmpEnum = [mQDFontList objectEnumerator];
>+  NSString* tmpName;
>+  while (tmpName = [tmpEnum nextObject]) {
>+    NSMenuItem* newItem = [[[NSMenuItem alloc] initWithTitle:tmpName action:nil keyEquivalent:@""] autorelease];
>+    [menu addItem:newItem];
>+  }

Same as above, lets not use "tmp" variable names. Also, add extra parens around the assignment inside of the while condition:

while ((tmpName = [tmpEnum nextObject])) {

>+  NSMutableArray*         mQDFontList; //QUICKDRAW_FONTLIST

Finally, the need for an instance variable and cached font list stems only from the |setupFontPopup:forType:fromDict:| method's checking to see if the previously chosen font has gone missing.  While it's not much harm keeping the generated array around, are there any easy alternatives to doing that?

--

As Smokey mentioned in comment #66, the advanced font popup menus are basically unordered and look pretty disorganized with this patch applied.  We should consider sorting mQDFontList before presenting it to the user.

Also, comment #68 still stands:
> Note that you will have to use the pop-up lists in the
> "Advanced" sheet in the Fonts preferences to change fonts (the main screen of
> the Fonts prefpane is still broken, and it will still report fonts as "missing"
> when they are not).

waverider: Are you able to work up a new patch, or should someone else jump in?

Comment 82

[Stuart Morgan]

(In reply to comment #81)
> >+  if (err != noErr)
> >+    return;
> 
> I'd simplify this error check just slightly, avoiding an inequality:
> 
> if (err)
>   return;

That's not the right way to error-check functions that return OSStatus. The success return for OSStatus is noErr, not 0.

Comment 83

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 218960 [details] [diff] [review]
sorry, I added Appearance.h part.

Clearing this request since the patch has r- from murph.

Comment 84

[Sean Murphy]

Assigning this to myself, per the 2007-04-18 status meeting.

I didn't look closely enough at the creation date of waverider's patch because seeing April led me to assume he was working on it this month.

I think he actually wanted one of us to fix it up anyway:

(comment #16)
> ...
> Camino developers, Please brush up this rough patch.

Comment 85

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Nominating for 1.5.1, too.

Comment 86

[Sean Murphy]

Attachment: patch

As I understand this situation: if we supply a QuickDraw font family name to gecko for non-western fonts, they will be rendered correctly.  I can use some help from CJK users to determine for sure if this is the case.  Based on this approach, the patch generates the advanced panel's font list using Apple Type Services for Fonts to obtain a QuickDraw representation of each name.  I'll admit this isn't the perfect solution, but it's probably the best we can do for now.

Ideally, I'd rather not change around the interface to fix something internally but it seems to be the only way to properly hand gecko an appropriate font name.  I tried to leave the NSFontManager supplied popup menu alone, and instead convert the selected item into a gecko-compatible string later on when the sheet ended.  That didn't work; you can use ATSFontFamilyFindFromName() with names returned by -[NSFontManager availableFontFamilies] for western fonts, but international ones will not be found.  I'm thinking this is because Cocoa will localize all font names so that they are returned in the current locale.  Carbon, when using ATS, will return non-western fonts in their native representation (not in Roman form).  As a result, changing around the font menu in each popup button was warranted, as it seems like the only way to obtain the correct name was to allow the user to directly choose it.

Apple's Documentation states:
"If your goal is to gather information about all fonts installed in the system, you should enumerate once and then cache the results. Enumerating the entire system can be time-consuming and degrade system performance, especially on systems with many fonts installed."

This prompted me to keep an instance variable around for caching the system's font families.  This list is lazily created and avoids the need to enumerate the fonts four times (for each popup button) every time the advanced sheet is displayed.  Preference panes are not deallocated until the application is terminated though, meaning if we rely on releasing this system font list in dealloc, it will persist until then.

Am I correct in assuming that this patch will only be taken by the 1.8 branch, which still utilizes Quickdraw?  I'm hoping that trunk-gecko, which is moving away from the depreciated QuickDraw, can accept international font names returned directly from NSFontManager.

Comment 87

[Sean Murphy]

> +  while (font = [fontNameEnumerator nextObject]) {

I just noticed that I forgot the additional parenthesis here.  It's fixed locally, so you can ignore the omission when reviewing.

Comment 88

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #86)
> Am I correct in assuming that this patch will only be taken by the 1.8 branch,
> which still utilizes Quickdraw?  I'm hoping that trunk-gecko, which is moving
> away from the depreciated QuickDraw, can accept international font names
> returned directly from NSFontManager.

Ideally we'd only need this on the 1.8 branch(es), yes, but I haven't been able to determine exactly what sort of font names are being passed to/utilized by Gecko on the trunk.  As far as I can tell as a non-coder, it looks like they're trying to preserve compatibility with QuickDraw font names, which really sucks for us.

Maybe you can tell something more from bug 362665, bug 364678, and bug 364785?

Comment 89

[philippe (part-time)]

(In reply to comment #86)

> As I understand this situation: if we supply a QuickDraw font family name to
> gecko for non-western fonts, they will be rendered correctly.  I can use some
> help from CJK users to determine for sure if this is the case. 
Yes that is basically how it works.
e.g. to recognise 'Hiragino Kaku Gothic Pro', the font-name must be supplied as 'ヒラギノ角ゴ Pro W3'


> Am I correct in assuming that this patch will only be taken by the 1.8 branch,
> which still utilizes Quickdraw?  I'm hoping that trunk-gecko, which is moving
> away from the depreciated QuickDraw, can accept international font names
> returned directly from NSFontManager.
> 

Trunk doesn't need this, although it still needs some love (bug 364786 does fix most issues, and then some fine-tuning for the preference pane).

--
Sean, or anyone else, can you make a test build available ? I won't be able to build from 1.8 branch atm. I can test it on 10.4.9 ppc and 10.3.9.

Comment 90

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://www.ardisson.org/smokey/moz/Camino-1.1b+cjk.dmg is today's branch + this patch.  PPC-only.

Comment 91

[Hiro]

(In reply to comment #90)
> http://www.ardisson.org/smokey/moz/Camino-1.1b+cjk.dmg is today's branch + this
> patch.  PPC-only.
> 
This build was tried. 
But it is not correctly displayed by the changed Japanese font. 

The font name was set to font.name.sans-serif.ja as follows. 
  Hiragino Maru Gothic Pro

It is correctly displayed if it sets it as follows. 
  ヒラギノ丸ゴ Pro W4

FYI.
The difference of the font name of 10.4 and 10.3 might be influenced according to the 
situation. (The name has been changed because of the difference of OS version though it is 
the same font. )

  10.4    : "Osaka−等幅"
  10.3.9 : "Osaka レギュラー−等幅"
  http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=5515#c6

Mac OS X 10.3.9

Comment 92

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #91)
> The font name was set to font.name.sans-serif.ja as follows. 
>   Hiragino Maru Gothic Pro

That name won't work on the 1.8 branch.

> It is correctly displayed if it sets it as follows. 
>   ヒラギノ丸ゴ Pro W4

This name will work, and should be the name the Advanced panel sets.  Did you use the Advanced Panel to change the font?  This seems to work for me in the test build; at least, I can see in about:config that the font name gets changed to another Carbon font name when I change fonts using the Advanced panel, and fonts used on in http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=5515#c6 change when I change fonts.  I can't say that they change to the right fonts since I don't read Japanese, but they do change.

Note that the main Fonts prefs still uses Cocoa to look for fonts and will still call ヒラギノ丸ゴ Pro W4 and Osaka−等幅 "(missing)".

>   10.4    : "Osaka−等幅"

That's what I see on 10.3.9 (en-US), too, in the Advanced Panel.

Comment 93

[Hiro]

(In reply to comment #92)
> (In reply to comment #91)
> > The font name was set to font.name.sans-serif.ja as follows. 
> >   Hiragino Maru Gothic Pro
> 
> That name won't work on the 1.8 branch.
>
This changed from "Change.." button in the font panel. 
Advanced Font panel is not used. 
 
> > It is correctly displayed if it sets it as follows. 
> >   ヒラギノ丸ゴ Pro W4
> 
> This name will work, and should be the name the Advanced panel sets.  Did you
> use the Advanced Panel to change the font?
This changed the parameter directly from "about:config". 
And, this was tried. 
The change from Advanced panel worked correctly, too. 

Comment 94

[philippe (part-time)]

This works fine overall, on both 10.3.9 (login/os lang: english) and 10.4 ppc (login/os lang: English and Japanese).
I changed between a variety of fonts without any problems.
The one 'issue': the user must use the advanced panel, and the user must close the pref window or change tabs before the choice is applied (should be rel-noted I guess).

Problem: the user cannot make a choice for 'monospace': this can only be done through the cocoa panel, not the advance panel. OK, about config is an option as well.

No problems with the default choice for monospace here on 10.3.9. Setting the choice to 'Osaka レギュラー−等幅' in about:config does cause problems, but causes the same problems for the 'fox as well.

Comment 95

[Stuart Morgan]

Comment on attachment 263176 [details] [diff] [review]
patch

I haven't had time for a full review yet, but some initial comments:

>+- (NSString*)unicodeStringForFontFamily:(ATSFontFamilyRef)fontFamily;

unicodeString is redundant with the return type, so this should be nameForFontFamily: or carbonNameForFontFamily:


>+- (NSArray*)sortedCarbonSystemFontFamilies
>+{
>+  if (mCarbonSystemFontFamilies)
>+    return mCarbonSystemFontFamilies;

This method would be more clear if structured as:
  if (!mCarbonSystemFontFamilies) {
    // do all the setup work
  }
  return mCarbonSystemFontFamilies;

>+      status = ATSFontFamilyIteratorReset(kATSFontContextLocal, nil, nil,
>+                                          kATSOptionFlagsUnRestrictedScope,
>+                                          &fontFamilyIterator);

Don't bother setting status if you don't care what it is.


One general question: I understand that we can't just convert to a Carbon name at the end, but is there any way while we are iterating to build a parallel list of Cocoa names, so that we can continue to use the names users are presumably used to seeing in the UI, but give Gecko a Carbon name? (It's fine if the answer is no; I just didn't see any discussion of whether it's possible.)

Comment 96

[Sean Murphy]

Attachment: updated patch

I didn't like the fact that choosing a carbon name from the advanced menu would falsely indicate that selection is missing in the main pane's Cocoa-based font preview cell.  This patch fixes that.  The cached carbon list is consulted before assuming a missing font.

To accomplish this cleanly and to avoid a really confusing flow, I needed to restructure the if/else statements in |setFontSampleOfType:withFont:andDict:|.

(In reply to comment #95)

> >+      status = ATSFontFamilyIteratorReset(kATSFontContextLocal, nil, nil,
> >+                                          kATSOptionFlagsUnRestrictedScope,
> >+                                          &fontFamilyIterator);
> 
> Don't bother setting status if you don't care what it is.

This value will be checked inside the while loop's condition after a reset, and allows the loop is aborted if there was an error resetting the iterator.
 
> One general question: I understand that we can't just convert to a Carbon name
> at the end, but is there any way while we are iterating to build a parallel
> list of Cocoa names, so that we can continue to use the names users are
> presumably used to seeing in the UI, but give Gecko a Carbon name? (It's fine
> if the answer is no; I just didn't see any discussion of whether it's
> possible.)

I like that idea, but couldn't come up with a way to accomplish it.  There's no way to relate international family names between Cocoa and Carbon, as the latter are returned in their native local and character set (meaning, for example, Cocoa = "Hiragino Maru Gothic Pro" and Carbon = "ヒラギノ丸ゴ Pro W4.")  Additionally, we can't rely on a simple 1:1 relation when iterating, as the Carbon route supplies 100+ more family names than NSFontManaager's availableFontFamilies.  An example of why this happens: Cocoa has only one entry for "Arno Pro," whereas Carbon (for some reason) returns "Arno Pro", "Arno Pro Bold 08pt", "Arno Pro Bold 10pt", and so forth.

The advanced list isn't as friendly using Carbon names, and of course isn't as familiar, but changing the UI still seems like the only choice.

Comment 97

[Stuart Morgan]

Comment on attachment 265086 [details] [diff] [review]
updated patch

> This value will be checked inside the while loop's condition after a reset

Yep, sorry. That's why I shouldn't skim and comment.

Comments:

>+      displayString = [NSString stringWithFormat:@"%@, %dpt", fontName, fontSize];
...
>+      displayString = [NSString stringWithFormat:@"%@, %dpt %@", fontName, fontSize, [self getLocalizedString:@"Missing"]];
...
>     displayString = [NSString stringWithFormat:@"%@, %dpt", [font familyName], (int)[font pointSize]];

Rather than repeat this format string, declare it once at the beginning of the method (and for the version with the appended "Missing" actually go ahead and use the same format string then do an append).

>+      displayString = @"Font missing"; // XXX localize

Nice. Filed as bug 381230.

>+  status = TECConvertText(textEncodingConverter, &quickDrawFontName[1], quickDrawFontName[0], &actualInputLength, 
>+                          (TextPtr)unicodeFontName , sizeof(unicodeFontName), &actualOutputLength);
>+  status = TECDisposeConverter(textEncodingConverter);
>+  if (status != noErr)
>+    return nil;

This returns failure if the dispose fails (which is irrelevant to the caller) rather than if the convert fails, which is what matters.

>+    mCarbonSystemFontFamilies = [fontFamilies sortedArrayUsingSelector:@selector(caseInsensitiveCompare:)];

mCarbonSystemFontFamilies = [[...] retain];

>+    return [mCarbonSystemFontFamilies retain];

Remove this line.

Comment 98

[Sean Murphy]

Attachment: updated with review comments

I'm sorry for allowing so much time to pass before I got around to addressing the last review comments.

> Rather than repeat this format string, declare it once at the beginning of the method

The font information for use in the displayString is obtained in differing ways depending on the NSFont argument's existence.  If the NSFont was supplied, the family name and point size are obtained directly from it.  If there is no NSFont, we dive into the regionDict from prefs and extract the useful information from that instead.

For now, I reduced the display string initializations down to one for each case.  My thinking is to avoid reliance on the regionDict unless we have no other choice.  Can you elaborate on what you had in mind for this single declaration, as far as obtaining the font information goes?

While looking at this method again, I made two additional changes:

Switched around the font status if/else blocks, indicating more obviously the normal/problem-free flow.

Moved the initialization of the following variable closer to where it's used:
> NSTextField *sampleCell = [self getFontSampleForType:fontType];

…and filed bug 382013.

Comment 99

[Stuart Morgan]

(In reply to comment #98)
> Can you elaborate on what you had in mind for this single
> declaration, as far as obtaining the font information goes?

How the font information is obtained isn't relevant to the format string. Declare the format string @"%@, %dpt" once, and use that everywhere (appending the " <whatever>" to the string you generate in the error cases).

Comment 100

[Sean Murphy]

Attachment: patch

(In reply to comment #99)
Ok, I see what you mean now.

Please also disregard the previous patch, I attached the wrong file and it is identical to the one before it.  I'm sorry if you tried to look at it!

Comment 101

[Stuart Morgan]

Comment on attachment 266162 [details] [diff] [review]
patch

>+  if (status != noErr)
>+    return nil;
>+  TECDisposeConverter(textEncodingConverter);

Move the dispose to before the early return.

r=me with that change.

Comment 102

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 266162 [details] [diff] [review]
patch

Simon, will you have a chance in the next few days/week to sr this, or should we have one of the others (mento?) look at it?

Comment 103

[Simon Fraser [no longer active]]

Comment on attachment 266162 [details] [diff] [review]
patch

>Index: PreferencePanes/Appearance/Appearance.mm
>===================================================================

>+  NSEnumerator* fontNameEnumerator = [[self sortedCarbonSystemFontFamilies] objectEnumerator];
>+  NSString* font;

Call this variable 'fontName', since that's what it is.

>+  while (font = [fontNameEnumerator nextObject]) {
>+    NSMenuItem* newMenuItem = [[[NSMenuItem alloc] initWithTitle:font action:nil keyEquivalent:@""] autorelease];
>+    [menu addItem:newMenuItem];
>+  }
>+}

>+- (NSString*)carbonNameForFontFamily:(ATSFontFamilyRef)fontFamily
>+{
>+  OSStatus status = noErr;
>+
>+  // We'd like to use ATSFontFamilyGetName here, but it's ignorant of the
>+  // font encodings, resulting in garbage names for non-western fonts.

Does this comment still apply, since ATSFontFamilyGetEncoding() is being used
to get the font name encoding?

>+  Str255 quickDrawFontName;
>+  status = ATSFontFamilyGetQuickDrawName(fontFamily, quickDrawFontName);
>+
>+  // Fonts with certain prefixes are not useful.
>+  if (status != noErr || quickDrawFontName[0] == 0 || quickDrawFontName[1] == '.' || quickDrawFontName[1] == '#')
>+    return nil;
>+
>+  TextEncoding unicodeTextEncoding = CreateTextEncoding(kTextEncodingUnicodeDefault,
>+                                                        kTextEncodingDefaultVariant,
>+                                                        kUnicode16BitFormat); 
>+  TECObjectRef textEncodingConverter = NULL;
>+  TextEncoding fontEncoding = ATSFontFamilyGetEncoding(fontFamily);
>+  status = TECCreateConverter(&textEncodingConverter, fontEncoding, unicodeTextEncoding);
>+  if (status != noErr)
>+    return nil;
>+
>+  // Convert the QuickDraw name to Unicode.
>+  UniChar unicodeFontName[sizeof(quickDrawFontName)];

This is risky; what if the conversion produces something longer? I'd play safe and make unicodeFontName twice as many characters long.

>+  ByteCount actualInputLength, actualOutputLength;
>+  status = TECConvertText(textEncodingConverter, &quickDrawFontName[1], quickDrawFontName[0], &actualInputLength, 
>+                          (TextPtr)unicodeFontName , sizeof(unicodeFontName), &actualOutputLength);
>+  if (status != noErr)
>+    return nil;
>+  TECDisposeConverter(textEncodingConverter);
>+
>+  unsigned int nameLength = actualOutputLength / sizeof(UniChar);
>+  unicodeFontName[nameLength] = '\0';

Don't you risk whacking one past the length of unicodeFontName here? If TECConvertText() filled the entire string (say 256 Unichars), actualOutputLength will be 512. Then you'll be writing to unicodeFontName[256].

>+  NSString* fontName = (NSString*)CFStringCreateWithCharacters(kCFAllocatorDefault, unicodeFontName, nameLength);

Any reason not to use +[NSString stringWithCharacters:length:]; then no need for the autorelease.

>+  return [fontName autorelease];
>+}

Comment 104

[Sean Murphy]

Attachment: updated from r/sr comments

> >+  // Convert the QuickDraw name to Unicode.
> >+  UniChar unicodeFontName[sizeof(quickDrawFontName)];
> 
> This is risky; what if the conversion produces something longer? I'd play safe
> and make unicodeFontName twice as many characters long.

Is a simple (sizeof(quickDrawFontName) * 2) what you had in mind?  This doubles the character size to twice the total capacity of quickDrawFontName.  Doubling the actual character length of quickDrawFontName would involve looking at its first byte (in Pascal strings, equal to the length).  

> >+  ByteCount actualInputLength, actualOutputLength;	
> >+  status = TECConvertText(textEncodingConverter, &quickDrawFontName[1], quickDrawFontName[0], &actualInputLength, 
> >+                          (TextPtr)unicodeFontName , sizeof(unicodeFontName), &actualOutputLength);
> >+  if (status != noErr)
> >+    return nil;
> >+  TECDisposeConverter(textEncodingConverter);
> >+
> >+  unsigned int nameLength = actualOutputLength / sizeof(UniChar);
> >+  unicodeFontName[nameLength] = '\0';
> 
> Don't you risk whacking one past the length of unicodeFontName here? If
> TECConvertText() filled the entire string (say 256 Unichars),
> actualOutputLength will be 512. Then you'll be writing to unicodeFontName[256].

The more I think about this... Since we're supplying the length, using +[NSString stringWithCharacters:length:], can't we avoid the need to null terminate the string entirely?

The ideas above and all other comments were incorporated into this updated patch; thanks.

Comment 105

[Simon Fraser [no longer active]]

Comment on attachment 266574 [details] [diff] [review]
updated from r/sr comments

(In reply to comment #104)
> Created an attachment (id=266574) [details]
> updated from r/sr comments
> 
> > >+  // Convert the QuickDraw name to Unicode.
> > >+  UniChar unicodeFontName[sizeof(quickDrawFontName)];
> > 
> > This is risky; what if the conversion produces something longer? I'd play safe
> > and make unicodeFontName twice as many characters long.
> 
> Is a simple (sizeof(quickDrawFontName) * 2) what you had in mind?  This doubles
> the character size to twice the total capacity of quickDrawFontName.  Doubling
> the actual character length of quickDrawFontName would involve looking at its
> first byte (in Pascal strings, equal to the length).  

... and a malloc(), rather than stack memory. For that reason, sizeof(quickDrawFontName) * 2 seems simpler.

> 
> > >+  ByteCount actualInputLength, actualOutputLength;	
> > >+  status = TECConvertText(textEncodingConverter, &quickDrawFontName[1], quickDrawFontName[0], &actualInputLength, 
> > >+                          (TextPtr)unicodeFontName , sizeof(unicodeFontName), &actualOutputLength);
> > >+  if (status != noErr)
> > >+    return nil;
> > >+  TECDisposeConverter(textEncodingConverter);
> > >+
> > >+  unsigned int nameLength = actualOutputLength / sizeof(UniChar);
> > >+  unicodeFontName[nameLength] = '\0';
> > 
> > Don't you risk whacking one past the length of unicodeFontName here? If
> > TECConvertText() filled the entire string (say 256 Unichars),
> > actualOutputLength will be 512. Then you'll be writing to unicodeFontName[256].
> 
> The more I think about this... Since we're supplying the length, using
> +[NSString stringWithCharacters:length:], can't we avoid the need to null
> terminate the string entirely?

Yes.

sr=me on the patch.

Comment 106

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can smfr's sr comments be made on checkin, or do we need a new patch for checkin?

If no new patch is needed, then we can land this on the 18branch, CAMINO_1_5_BRANCH, and 180 branch.  (My understanding is that we don't need this on the trunk, but we'll have to drive through some font pref changes there and then do testing to be sure.)

Comment 107

[Sean Murphy]

(In reply to comment #106)
> Can smfr's sr comments be made on checkin, or do we need a new patch for
> checkin?

(from smfr's comment)
> > Is a simple (sizeof(quickDrawFontName) * 2) what you had in mind?  This doubles
> > the character size to twice the total capacity of quickDrawFontName.  Doubling
> > the actual character length of quickDrawFontName would involve looking at its
> > first byte (in Pascal strings, equal to the length).  
> 
> ... and a malloc(), rather than stack memory. For that reason,
> sizeof(quickDrawFontName) * 2 seems simpler.

I believe smfr's comment here was made just for the sake of completeness, expanding upon what would be needed to take that more complicated approach - which is dynamically allocating a buffer based upon the string's actual length (a runtime calculation) as opposed to allocating from the string's capacity (which is ok for the stack and what we're currently doing).

If my understanding is correct, this patch should be ready to go as is.

Comment 108

[froodian (Ian Leue)]

Landed on MOZILLA_1_8_BRANCH, MOZILLA_1_8_0_BRANCH, and CAMINO_1_5_BRANCH.

Comment 109

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We shouldn't need this on trunk; trunk *should* be capable of handling Cocoa font names (though we need a bug on switching the default font prefs to Cocoa names), but testing is desired (can you switch using the normal Font panel, and does the right font get used then).

Since this bug is very long and unwieldy, please put any trunk issues in new bugs. 

Thanks waverider and murph for your work on this bug, and philippe, Hiro, and others for your testing.  This patch (fixes for the Advanced sheet) will appear in Camino 1.0.5, Camino 1.5.1, and in what becomes Camino 1.6.