158180 - Preferences - Appearance section should have default button

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, enhancement)

Description

[Chris Petersen]

Build: 2002-07-18-05
Platform: OS X 10.1.5
Expected Results: Default button should be available to remove any user changes
What I got: Can't reset back to original settings


Steps to reproduce:

1) Go Navigator -Preferences
2) Click on Appearance icon
3) Notice no default button in either Color & Links or Fonts tab.

Comment 1

[Greg K.]

Confirmed using Chimera/20020718.

Comment 2

[sairuh (rarely reading bugmail)]

over to simon (with his permission).

Comment 3

[Winnie Lam]

removing from 147975 meta bug list.

Comment 4

[Julian Pellico]

I can look at this if you like.

Comment 5

[Julian Pellico]

Attachment: fix

I got this one.
Note the diff is from the PreferencePanes/Appearance directory...I have other
stuff floating around in my tree.

Comment 6

[Julian Pellico]

Attachment: updated objects.nib

Comment 7

[Julian Pellico]

taking

Comment 8

[Julian Pellico]

I could take out the retain on the const string, to avoid propagationg such
ridiculousness.

Comment 9

[Josh Aas]

If we do decide to fix this bug, patches for it should land after the big patch
for bug 175924.

Comment 10

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 160993 [details] [diff] [review]
fix

Why are you reordering these?

+  NSString* fontPrefName = [NSString stringWithFormat:@"font.name.%@.%@",
fontType, regionCode];
+  NSString* fontName = [fontTypeDict objectForKey:@"fontfamily"];

-  NSString	*fontName			= [fontTypeDict
objectForKey:@"fontfamily"];
-  NSString	*fontPrefName	= [NSString
stringWithFormat:@"font.name.%@.%@", fontType, regionCode];
-

Apart from that apparently unneeded change, this looks fine.

One note: for future reference it is IMO easier if you just post a tarball of
the .nib file rather than including part of it in the diff and attaching the
rest.

Comment 11

[Josh Aas]

Comment on attachment 160993 [details] [diff] [review]
fix

Please clean up the patch by removing nib file changes from it, and then respin
the patch after bug 175924 has landed. It will have the be done for the merge
to be successful. I will try to get the patch for bug 175924 in quickly so we
can move forward with this.

Comment 12

[Julian Pellico]

(In reply to comment #10)

i reordered them because I thought I was going to need some of the information
earlier. It turns out I didn't use it. It was too complicated to undo it in the
editor when I decided I didn't need it; I'll strip it from the patch.

(In reply to comment #11)
I'll respin the patch with regards to above and the nib. Then I'll post the nib
in its entirety separately.

Comment 13

[Julian Pellico]

Attachment: source code changes, updated wrt comment 10

Comment 14

[Geoff "temporarily off bugmail" Beier]

This will need to be re-spun and re-reviewed once bug 175924 lands.

Comment 15

[Julian Pellico]

Attachment: new nib

I updated my Appearance directory and my source changes still appear to be
fine. Here's the nib.

One thing I'd like to point out about this patch...when the reset button is
pressed on the Fonts tab, I save the changes immediately. This means that
loaded pages may be re-rendered. Meanwhile, when you just change the fonts, the
changes aren't saved until the Appearance pane closes. Any objections?

Comment 16

[Julian Pellico]

Comment on attachment 162042 [details]
new nib

Geoff, I dunno if I need to get re-approved from you...?

Comment 17

[Julian Pellico]

I spun the patch from PreferencePanes dir...hope that's ok.

Comment 18

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 162042 [details]
new nib

This patch looks OK and works fine, but the patch is not against the current
CVS version of Camino. While I was able to apply it, I think it needs to be
re-spun against current CVS before it can be reviewed and landed. Josh, if I am
incorrect here, please undo my r- and I'll r+, as the patch is otherwise OK.
(e.g. this patch is against Appearance.h r1.4 and current is Appearance.h r1.5,
etc.)

Comment 19

[Julian Pellico]

(In reply to comment #18)
> (From update of attachment 162042 [details])
> This patch looks OK and works fine, but the patch is not against the current
> CVS version of Camino. While I was able to apply it, I think it needs to be
> re-spun against current CVS before it can be reviewed and landed. Josh, if I am
> incorrect here, please undo my r- and I'll r+, as the patch is otherwise OK.
> (e.g. this patch is against Appearance.h r1.4 and current is Appearance.h r1.5,
> etc.)
> 
gah, it looks like I might have copied stuff from the first patch even though I
know I respun it. Fixing.

Comment 20

[Julian Pellico]

Attachment: source code patch, against current CVS (gah)

Comment 21

[Julian Pellico]

Geoff, do you want to test this patch again? I see you gave an r+ for the
original patch already.

Comment 22

[Julian Pellico]

You two wanna review this? It's getting stale :-P Seriously, the nib cuold
change again, and that would suck. (Maybe it *has* changed already.)

If you want me to put the nib together with the diff, that's fine, I can do
that. Just ask me

Comment 23

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 162445 [details] [diff] [review]
source code patch, against current CVS (gah)

This looks good and works as advertised. r=[e-mail] for patch and
nib. Next time, it might be a good idea to combine them into one archive, but
that's not necessary this time.

Comment 24

[Josh Aas]

+  else
+    // If the preferences were reset to defaults, this key could be gone.
+    [self clearPref:[fontPrefName cString]];

This makes my eyes nervous as it has two lines after an unbracketed else statement.

+  for (unsigned int i = 0; i < [regionMappingTable count]; i++)
+  {

Bracket should go on the same line as the conditional statement (moz style).

Thats all I have to say after glancing at the code. No need to update your patch
to take these things into account - just keep it in mind. Off to test the patch
now...

Comment 25

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #24)
> +  else
> +    // If the preferences were reset to defaults, this key could be gone.
> +    [self clearPref:[fontPrefName cString]];
> 
> This makes my eyes nervous as it has two lines after an unbracketed else
statement.

makes me nervous as well. :)

Comment 26

[Mike Pinkerton (not reading bugmail)]

+  // reset default font type
+  [defaultFontType release];
+  defaultFontType = @"serif"; // the default default font type...wish there
were a constant for it ;)
+  [defaultFontType retain];

does this get released when the panel closes?

Comment 27

[Julian Pellico]

(In reply to comment #25)
> (In reply to comment #24)
> > +  else
> > +    // If the preferences were reset to defaults, this key could be gone.
> > +    [self clearPref:[fontPrefName cString]];
> > 
> > This makes my eyes nervous as it has two lines after an unbracketed else
> statement.
> 
> makes me nervous as well. :)

But I don't dare use braces because it's a one-line block. I assume that means
you want me to move the comment :)

Comment 28

[Julian Pellico]

(In reply to comment #26)
> +  // reset default font type
> +  [defaultFontType release];
> +  defaultFontType = @"serif"; // the default default font type...wish there
> were a constant for it ;)
> +  [defaultFontType retain];
> 
> does this get released when the panel closes?

No. It persists once the panel is loaded. It's released in |dealloc|. I mimiced
some other code.
574   // save the default font
575   [defaultFontType release];
576   defaultFontType = ([[defaultFontMatrix selectedCell] tag] ==
kDefaultFontSerifTag) ? @"serif" : @"sans-serif";
577   [defaultFontType retain];

Since defaultFontType is always assigned to a constant string, retaining and
releasing it are meaningless anyway.

Comment 29

[Mike Pinkerton (not reading bugmail)]

> But I don't dare use braces because it's a one-line block. I assume that means
> you want me to move the comment :)

use braces in this case. it's ok. really.

>> does this get released when the panel closes?
> 
> No. It persists once the panel is loaded. It's released in |dealloc|. I mimiced
> some other code.

ok. sr=pink.

Comment 30

[Josh Aas]

landed on trunk