175924 - Fonts tab of Appearance prefpane uses unlabeled buttons

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Steve Smart]

The buttons used in the Fonts tab of the Appearance preference pane are round
buttons designed to be used with the qestion mark icon to open the relevent
section of an application's comprehensive help files, or with another small icon
for another task. Chimera's are currently unlabeled; it would be much more
helpful to the user to use push buttons labeled "Set..." (see TextEdit.app's
font preferences, which operate similarly to ours).

There is enough room to fit in standard push buttons where the round ones are
now, and the preview wells could be moved to the right to be right-aligned with
the pop-up menu above them if more space was needed.

Observed in 2002102104.

Comment 1

[Simon Fraser [no longer active]]

Standard push buttons don't quite work here, because the blue buttons also act
as radio buttons (setting which font type is being edited).

Comment 2

[Steve Smart]

TextEdit just selects the appropriate font if you push the other push button;
this  might seem slightly confusing, but it really makes a lot of sense that
you're editing whichever buttont you pushed last (and the selected font will
match the one in the preview pane).

In contrast, I was thoroughly confused by what I was supposed to do the first
time I saw the unlabeled round buttons; at the very least I would like to see
some kind of label indicating what they do.

Comment 3

[Brad Kemper]

I too was a bit confused by those buttons the first time time. I thought  they 
were overly-large radio buttons, which didn't make much sense for 
actuating a new windoid. Similar round buttons elsewhere in OS X tend to 
have an icon inside them related to their function. I would suggest a letter 
icon such as "T" (for text) or "ABC" or "A".

Comment 4

[Jasper]

Attachment: Proposed patch - modified .nib file.

The problem, as Steve reported, is that the round buttons do not indicate what
their function is. Only with trial and error will the user learn that clickking
the blank buttuns will open a font panel from which the user can select another
font. 

This modified Apperance .nib file changes the round buttun into regular push
buttons (with out loosing functionality of course) and giving them a title
(Set...) which explains the user what they stand for.

Comment 5

[Jasper]

Attachment: Mozified .nib file

Fixed a minor size issue.

Comment 6

[Jasper]

Ludo, Dave, Josh, Simo or Mike. Please review, the nib file that is attached
fixes this bug.

Comment 7

[David Haas (gone, not reading mail)]

Comment on attachment 130829 [details]
Mozified .nib file

looks ok to me

Comment 8

[Jasper]

Mike could you take a look

Comment 9

[Stuart Morgan]

To consider/fix during the pref rewrite.

Comment 10

[Josh Aas]

Comment on attachment 130829 [details]
Mozified .nib file

This is too old now and its getting fixed in the 0.9 prefs rewrite.

Comment 11

[Josh Aas]

Attachment: appearance cleanup v1.0

The attached patch does the following:

1. fixes this bug
2. reformats all the appearance pref code to mozilla style
3. updates nib to 10.2+ format

Comment 12

[Josh Aas]

Attachment: appearance cleanup v2.0

fixed up the patch formatting

Comment 13

[Josh Aas]

Attachment: appearance cleanup v3.0

arg! its been a long day. this puts the right nib file in the patch archive!

Comment 14

[Geoff "temporarily off bugmail" Beier]

As I'm reviewing this, I'm seeing some minor stuff. Since part of the goal of
the patch is cleaning up Appearance.mm, I'm including this in the review, though
it may perhaps be better addressed in a different bug.

Line 420 of Appearance.mm:
There's an NSLog line in an exception handler:
NSLog(@"Exception %@ getting [font displayName] for %@", localException, font);
Is that OK?

Line 649 of Appearance.mm:
    }
    else {
      // use the name from the font family info?
    }
Should the empty else be left in?


Line 671 of Appearance.mm:
- (BOOL)fontManager:(id)theFontManager willIncludeFont:(NSString *)fontName
{
  NSLog(@"theFontManager willIncludeFont %@", fontName);
  // filter out fonts for the selected language
  return YES;
}

Should this NSLog statement be checked in? As far as I can tell, this function
is never called. (I never found a reference to it and never got the debugger to
break on it.)

In several places, there is code of the form

if (x) {
...
}
else {

which should probably be changed to 

if (x) {
...
} else {

if you respin the patch.



Anyway, these are all clearly style questions that may well be due to me being
new at this :-). The patch works as described and looks good. If these are OK,
let me know and I'll r+.

Comment 15

[Josh Aas]

I'll respin the patch and do a more thorough cleanup. I just planned to fix this
bug and clean up what I got to along the way, but I can do more. I might as well
since it would be good to remove that unused method anyway. I'm not finding any
references to it either.

Comment 16

[Josh Aas]

Not removing the method after all as its a harmless part of being a FontManager
delegate. Not removing emtpy else statement as it helps to illustrate a comment
that may be useful in the futurem and it will get compiled out anyway.

Comment 17

[Josh Aas]

Attachment: appearance cleanup v4.0

Comment 18

[Mike Pinkerton (not reading bugmail)]

lots and lots of cleanup here, not sure how much was really necessary. it also
hides a bug that you introduced (inverted a pointer check by removing == nil).

probably in the future we don't need so much spacing cleanup on code that
already works just fine.

Comment 19

[Mike Pinkerton (not reading bugmail)]

should the 'choose' buttons be the small button size?

Comment 20

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 157710 [details]
appearance cleanup v4.0

1) inverted if check
2) cmd-w no longer works when appearance tab is chosen, works for all others
3) choose buttons should be small size

r-

Comment 21

[Josh Aas]

Attachment: appearance cleanup v5.0

This fixes everything in Mike's comments. I won't do so much cleanup next time
- its just a pet peeve of mine :)

Comment 22

[Hiro]

Although a font setup is updated by these bug and bug158180, does the problem of
CJK(bug175651) Fix?
It is a troublesome problem that Japanese fonts cannot be set up by font prefpane.

Comment 23

[Geoff "temporarily off bugmail" Beier]

While reviewing patch 161350 (appearance cleanup v5.0):

Mike observed with v4.0 that:
> 2) cmd-w no longer works when appearance tab is chosen, works for all others

With v5.0, cmd-w works when the "Colors and links" tab of appearance is chosen
but not when fonts is chosen. It just beeps. Also, fonts is now the default,
which seems counter-intuitive since Colors and links is on the left. ISTM it
should either default to Colors and links or remember which I was using last.

Comment 24

[Jasper]

Why not move the Fonts tab to the left and the colors and Link o the right. As I
agree that the Fonts tab is used more often.

Comment 25

[Josh Aas]

Comment on attachment 161350 [details]
appearance cleanup v5.0

Geoff - I don't think you updated the nib files between patches. You need to
touch it in order to change the modification date even after you do the
replacement. None of the problems you are having show up for me.

Comment 26

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #25)
> (From update of attachment 161350 [details])
> Geoff - I don't think you updated the nib files between patches. You need to
> touch it in order to change the modification date even after you do the
> replacement. None of the problems you are having show up for me.
> 
That must've been it. I thought rm -rf'ing the application bundle after dropping
the new .nib down using the finder was enough to make the nib changes get copied
into the bundle. What's really weird, though, is that I got the changed buttons
but still got the problems I described. (This was a tree I had never tested an
earlier version of this patch on.)

Anyway, this looks good now.

r=[e-mail]

Comment 27

[Mike Pinkerton (not reading bugmail)]

-  [[matrixChooseFont cellWithTag:0] setAlternateTitle:defaultFontType];
+  [chooseProportionalFontButton setAlternateTitle:defaultFontType];

not a bad thing, just curious why bother making this change. 

-    /*
-      For each region in the array, there is a dictionary of 
...

why remove this big comment? was it wrong? out of date? can it be fixed to say
what is really going on?

Comment 28

[Josh Aas]

-  [[matrixChooseFont cellWithTag:0] setAlternateTitle:defaultFontType];
+  [chooseProportionalFontButton setAlternateTitle:defaultFontType];

> not a bad thing, just curious why bother making this change. 

I don't like using matrix controls very much. I thought it was unnecessarily
complicated. And matrixChooseFont is a bad name :)

-    /*
-      For each region in the array, there is a dictionary of 
...

> why remove this big comment? was it wrong? out of date? can it be fixed to say
> what is really going on?

My bad. I think we should leave it in. If you could copy it back in when
landing, that would be great. If you don't want to, I'll respin the patch.

Comment 29

[Josh Aas]

Comment on attachment 161350 [details]
appearance cleanup v5.0

got sr on IRC

Comment 30

[Josh Aas]

landed