Closed Bug 158180 Opened 22 years ago Closed 20 years ago

Preferences - Appearance section should have default button

Categories

(Camino Graveyard :: Preferences, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino0.9

People

(Reporter: chrispetersen, Assigned: jpellico)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Confirmed using Chimera/20020718.
Severity: normal → enhancement
QA Contact: winnie → sairuh
over to simon (with his permission).
Assignee: saari → sfraser
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.7
Blocks: 147975
removing from 147975 meta bug list.
No longer blocks: 147975
Blocks: 147975
No longer blocks: 147975
Target Milestone: Camino0.7 → Camino0.8
Target Milestone: Camino0.8 → Camino0.9
I can look at this if you like.
Attached patch fix (obsolete) — Splinter Review
I got this one.
Note the diff is from the PreferencePanes/Appearance directory...I have other
stuff floating around in my tree.
Attached file updated objects.nib (obsolete) —
taking
Assignee: sfraser → jpellico
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attachment #160993 - Flags: review?(me)
I could take out the retain on the const string, to avoid propagationg such
ridiculousness.
If we do decide to fix this bug, patches for it should land after the big patch
for bug 175924.
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.
Attachment #160993 - Flags: review?(me) → review+
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.
Attachment #160993 - Flags: review-
(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.
Attachment #160993 - Attachment is obsolete: true
This will need to be re-spun and re-reviewed once bug 175924 lands.
Depends on: 175924
Attached file 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?
Attachment #160994 - Attachment is obsolete: true
Attachment #162042 - Flags: review?(joshmoz)
Comment on attachment 162042 [details]
new nib

Geoff, I dunno if I need to get re-approved from you...?
Attachment #162042 - Flags: review?(me)
I spun the patch from PreferencePanes dir...hope that's ok.
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.)
Attachment #162042 - Flags: review?(me) → review-
(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.
Attachment #161627 - Attachment is obsolete: true
Attachment #162042 - Flags: review?(joshmoz)
Attachment #162445 - Flags: review?(joshmoz)
Geoff, do you want to test this patch again? I see you gave an r+ for the
original patch already.
Attachment #162445 - Attachment description: against current CVS (gah) → source code patch, against current CVS (gah)
Attachment #162445 - Flags: review?(me)
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 on attachment 162445 [details] [diff] [review]
source code patch, against current CVS (gah)

This looks good and works as advertised. r=me@mollyandgeoff.com 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.
Attachment #162445 - Flags: review?(me) → review+
Attachment #162042 - Flags: review- → review+
+  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...
Attachment #162445 - Flags: review?(joshmoz) → superreview?(pinkerton)
(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. :)
+  // 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?
(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 :)

(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.
> 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.
Attachment #162445 - Flags: superreview?(pinkerton) → superreview+
landed on trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: