346209 - Font changes do not take effect until preferences are closed

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Trevor Johns]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2

Changes to the browser's font in Perferences/Appearance/Fonts do not take effect until the preferences window is closed or another preference pane is selected.

Reproducible: Always

Steps to Reproduce:
1. Launch Camino.
2. Open Preferences/Appearance/Fonts.
3. Use the Change button to change the font.
Actual Results:  
The browser window does not use the newly reflected font until the preferences window is closed.

Expected Results:  
The browser window should update itself to match the currently selected font in the font dialog, even though the preferences window is still open.

Comment 1

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

This is a dupe.

Comment 2

[Trevor Johns]

Bug 191032 maybe? That bug is for something else, but this issue is discussed in the comments.

Comment 3

[Chris Lawson (gone)]

Yeah, I mentioned it there, but I don't think anyone ever filed it.

cl

Comment 4

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

Confirming and putting on the 1.1 list for now, but if we can't take it in the current prefPane work, then off it goes ;)

Julian noted on irc tonight that Reset auto-applies (or did when he wrote the patch, so there should be some code to auto-apply we can perhaps copy (see also bug 158180 comment 15).

Also note, though, that the reset code is buggy; it Resets all fonts instead of only the current langgroup's fonts (bug 281770).

Comment 5

[froodian (Ian Leue)]

Attachment: Patch

Comment 6

[Wevah (Nate Weaver)]

Comment on attachment 244504 [details] [diff] [review]
Patch

codewise r=me

Comment 7

[Stuart Morgan]

Comment on attachment 244504 [details] [diff] [review]
Patch

The advanced prefs don't apply on change, or even when the advanced prefs sheet is dismissed, making them feel broken if the rest of the pane causes live updates.  We need them both to live update if either is going to.

Comment 8

[Chris Lawson (gone)]

I assume Ian's not working on this at the moment, and won't mind if I steal it from him...

Comment 9

[Chris Lawson (gone)]

Attachment: fix v1.1

This addresses the Advanced sheet.

The changes don't auto-apply while the sheet is open, but I'm not sure how to do that, or if we really care. They do auto-apply as soon as the "done" button is clicked, which I thought to be the more important part of comment 7, and when bug 422576 gets done, we won't care anyway.

Comment 10

[Chris Lawson (gone)]

Note to self: there are a ton of whitespace problems in that file, so when a patch here gets sr, generate a diff without -w for checkin.

Comment 11

[Stuart Morgan]

(In reply to comment #10)
> when a patch here gets sr, generate a diff without -w for checkin.

Once again, no. If you want to fix whitespace, do it in a *different checkin*. Mixing tons of whitespace changes with actual code changes makes blame very unpleasant.

Comment 12

[Jeff Dlouhy]

Comment on attachment 323963 [details] [diff] [review]
fix v1.1

Advanced sheet is now working as expected.

r=jeff

Comment 13

[Stuart Morgan]

Comment on attachment 323963 [details] [diff] [review]
fix v1.1

This saves every type in every region any time anything changes. The only thing that needs saving for the two buttons is the specific region/type that was modified, and for the sheet only the current region.

Comment 14

[Chris Lawson (gone)]

Attachment: fix v1.2

Addresses sr comments, but due to changes, should be re-reviewed first.

Comment 15

[Chris Lawson (gone)]

Comment on attachment 324727 [details] [diff] [review]
fix v1.2

Stuart said he was OK with just giving this another sr, so that way Jeff doesn't have to worry about it.

Comment 16

[Stuart Morgan]

Comment on attachment 324727 [details] [diff] [review]
fix v1.2

>+  // apply the changes for the current region
>+  [self saveFontNamePrefsForRegion:regionDict forFontType:@"serif"];
>+  [self saveFontNamePrefsForRegion:regionDict forFontType:@"sans-serif"];
>+  [self saveFontNamePrefsForRegion:regionDict forFontType:@"monospace"];
>+  [self saveDefaultFontTypePrefForRegion:regionDict];
>+  [self saveFontSizePrefsForRegion:regionDict];

Rather than cloning this logic from saveFontPrefs, make a new private saveAllFontPrefsForRegion: method with this body, and make both saveFontPrefs and your code call that.

sr=smorgan with that change.

Comment 17

[Chris Lawson (gone)]

Attachment: fix v1.21

sr comments addressed, ready for checkin.

Comment 18

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

Hooray!  This is in on cvs trunk, and we finally behave like a real Mac app here!

Comment 19

[philippe (part-time)]

Lovely :-)
One less click just to turn minimum font-size on/off.

v. with 2.0a1pre (1.9.0.3pre 2008090900)