289549 - [l10n] localized version in Preferences can't change Monospaced font

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Michel Mouric]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050407 Camino/0.8+
Build Identifier: Camino 0.8.3-Multilang

Impossible to change the monospace font with the "Preferences Appearence Fonts"
window in the FRENCH LOCALIZED version.

Reproducible: Always

Steps to Reproduce:
1. Open Preferences
2. Try to change the monospaced font
3

Actual Results:  
The change is not reflected in the field for the name of the monospaced font

Expected Results:  
The change is not made

Comment 1

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

Haven't confirmed. Updated summary with [l10n]. CCing Ludo to look into this.

Comment 2

[Ludovic Hirlimann [:Usul]]

I need to sort where the problems comes from.
sources might be:

     1) Broken mib after translations (hand editing of the panel thru IB).
     2) AppleGlot bug.
     3) Other.

Comment 3

[Ludovic Hirlimann [:Usul]]

Problem is present in italian version also.

Comment 4

[Marcello Testi]

If I try to open the Appearance.nib file from the original (English only)
package, I get an error from Interface Builder:
"Open Interface
An inconsistency in the document was found and repaired. Please save the document"

This doesn't happen with any of the other prefpanes' .nib files.

Comment 5

[waverider]

Attachment: screenshot : we should not change Alt.Title in NSButtonCell for monospace.

I check appearance.mm and find this line:
" [matrixChooseFont selectedCell] alternateTitle] "
this method depend on Alt. Title in NSButtonCell.
We should not change Alt. Title from "monospace".

Comment 6

[Marcello Testi]

(In reply to comment #5)
> We should not change Alt. Title from "monospace".

By doing so we make things work, but I think it's not the right way to go.
The other button (for proportional fonts) has the alt. text localized and works
ok. I guess there's something broken in the prefpane code.

Comment 7

[Marcello Testi]

I think I get it now: the problem must be in the blank spaces. If I translate
the alt. text with a string with blank spaces, it doesn't work. With a single
word string, the button works ok.
Can anyone else confirm this behaviour?

Comment 8

[Markus Amalthea Magnuson]

Hmm, I can't reproduce this in the Swedish localization, which uses a multi-word
translation of "monospace".

Comment 9

[waverider]

Attachment: patch : use tag instead of Alt. Title

We should not use Alt. Title for l10n.

Comment 10

[Ludovic Hirlimann [:Usul]]

Comment on attachment 180180 [details] [diff] [review]
patch : use tag instead of Alt. Title

>Index: Appearance.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/Appearance/Appearance.mm,v
>retrieving revision 1.6
>diff -u -9 -r1.6 Appearance.mm
>--- Appearance.mm	27 Apr 2003 08:40:51 -0000	1.6
>+++ Appearance.mm	9 Apr 2005 15:34:20 -0000
>@@ -500,31 +500,40 @@
> {
>   if ([fontType isEqualToString:@"monospace"])
>     return @"fixed";
> 
>   return @"variable";
> }
> 
> - (void)syncFontPanel
> {
>-  NSString 	*fontType = [[matrixChooseFont selectedCell] alternateTitle];
>-  NSFont		*newFont	= [[self getFontSampleForType:fontType] font];
>+  NSString* fontType;
>+  switch([[matrixChooseFont selectedCell] tag]) {
>+    case 1:
>+      fontType = @"monospace";
>+      break;
>+    case 0:
>+    default:

We should log in default and do what is currently being done in the case 0.
Else this is the solution.

Waverider would you mind respining the patch.

Tested on the 0.8 branch on 10.3.

Comment 11

[Ludovic Hirlimann [:Usul]]

Waverider mind respining the patch ?

Comment 12

[waverider]

Attachment: patch : case 0: case 1: correct wrong order.

Comment 13

[Mike Pinkerton (not reading bugmail)]

landed on the 1.7branch for 084, keep this open to land on the trunk.

Comment 14

[Mike Pinkerton (not reading bugmail)]

this patch totally doesn't apply to the trunk, looks like the entire panel has
been rewritten. someone needs to make the appropriate patch there so it'll work.

Comment 15

[Ludovic Hirlimann [:Usul]]

taking

Comment 16

[Ludovic Hirlimann [:Usul]]

(In reply to comment #14)
> this patch totally doesn't apply to the trunk, looks like the entire panel has
> been rewritten. someone needs to make the appropriate patch there so it'll work.
No more radio buttons involved. Trunk code has 
- (IBAction)proportionalFontChoiceButtonClicked:(id)sender
- (IBAction)monospaceFontChoiceButtonClicked:(id)sender

Hence nothing to do for trunk. Marking Fixed because the pref rewrite fixed it.

Comment 17

[Ludovic Hirlimann [:Usul]]

Still present on the new code.

Comment 18

[Ludovic Hirlimann [:Usul]]

Attachment: Patch v1.0 for trunk releases

Don't compare strings on alternate attributes in a .nib file.
The process used by Caminol10n implies that those .nib alternate attributes
*will* be translated, and the string comparison won't work in the translated
version. Hence I'm not using the altrenate text anymore.

Comment 19

[Bruce Davidson]

Comment on attachment 184532 [details] [diff] [review]
Patch v1.0 for trunk releases

Looks good. Haven't got a localised build to test, but makes sense and doesn't
change operation in normal en-us build.
r=me.

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 184532 [details] [diff] [review]
Patch v1.0 for trunk releases

This breaks stuff because the alternate title is used to hold "serif" or
"sans-serif" depending on your default proprotional font type. See calls to
-setAlternateTitle:.

Comment 21

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

(In reply to comment #20)
> (From update of attachment 184532 [details] [diff] [review] [edit])
> This breaks stuff because the alternate title is used to hold "serif" or
> "sans-serif" depending on your default proprotional font type. See calls to
> -setAlternateTitle:.

FYI, the serif/sans-serif switching (as well as the proportional hint) is broken
right now in the nightlies; see bug 293561.

Comment 22

[Simon Fraser [no longer active]]

Mine.

Comment 23

[Simon Fraser [no longer active]]

Fix checked in (with some other cleanup too). We no longer use -alternateTitle
at all.