Closed
Bug 158180
Opened 22 years ago
Closed 20 years ago
Preferences - Appearance section should have default button
Categories
(Camino Graveyard :: Preferences, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino0.9
People
(Reporter: chrispetersen, Assigned: jpellico)
References
Details
Attachments
(2 files, 3 obsolete files)
15.58 KB,
application/octet-stream
|
me
:
review+
|
Details |
4.59 KB,
patch
|
jaas
:
review+
me
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
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.
Updated•22 years ago
|
QA Contact: winnie → sairuh
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.7
Updated•21 years ago
|
Target Milestone: Camino0.8 → Camino0.9
Assignee | ||
Comment 4•20 years ago
|
||
I can look at this if you like.
Assignee | ||
Comment 5•20 years ago
|
||
I got this one. Note the diff is from the PreferencePanes/Appearance directory...I have other stuff floating around in my tree.
Assignee | ||
Comment 6•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Attachment #160993 -
Flags: review?(me)
Assignee | ||
Comment 8•20 years ago
|
||
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 10•20 years ago
|
||
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 11•20 years ago
|
||
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-
Assignee | ||
Comment 12•20 years ago
|
||
(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.
Assignee | ||
Comment 13•20 years ago
|
||
Attachment #160993 -
Attachment is obsolete: true
Comment 14•20 years ago
|
||
This will need to be re-spun and re-reviewed once bug 175924 lands.
Depends on: 175924
Assignee | ||
Comment 15•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #162042 -
Flags: review?(joshmoz)
Assignee | ||
Comment 16•20 years ago
|
||
Comment on attachment 162042 [details]
new nib
Geoff, I dunno if I need to get re-approved from you...?
Attachment #162042 -
Flags: review?(me)
Assignee | ||
Comment 17•20 years ago
|
||
I spun the patch from PreferencePanes dir...hope that's ok.
Comment 18•20 years ago
|
||
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-
Assignee | ||
Comment 19•20 years ago
|
||
(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.
Assignee | ||
Comment 20•20 years ago
|
||
Attachment #161627 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #162042 -
Flags: review?(joshmoz)
Assignee | ||
Updated•20 years ago
|
Attachment #162445 -
Flags: review?(joshmoz)
Assignee | ||
Comment 21•20 years ago
|
||
Geoff, do you want to test this patch again? I see you gave an r+ for the original patch already.
Assignee | ||
Updated•20 years ago
|
Attachment #162445 -
Attachment description: against current CVS (gah) → source code patch, against current CVS (gah)
Assignee | ||
Updated•20 years ago
|
Attachment #162445 -
Flags: review?(me)
Assignee | ||
Comment 22•20 years ago
|
||
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•20 years ago
|
||
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+
Updated•20 years ago
|
Attachment #162042 -
Flags: review- → review+
Comment 24•20 years ago
|
||
+ 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)
Comment 25•20 years ago
|
||
(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•20 years ago
|
||
+ // 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?
Assignee | ||
Comment 27•20 years ago
|
||
(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 :)
Assignee | ||
Comment 28•20 years ago
|
||
(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•20 years ago
|
||
> 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.
Updated•20 years ago
|
Attachment #162445 -
Flags: superreview?(pinkerton) → superreview+
Comment 30•20 years ago
|
||
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.
Description
•