371266 - "default web browser: check on launch" preference does not update or persist

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Sean Murphy]

In the General preference pane, modifying the "Default Web Browser: Check On Launch" checkbox doesn't appear to have any effect nor does it update in user defaults.

STR:
1) Open Camino's preferences
2) Modify the "Default Web Browser: Check On Launch" button.
3) Switch to another preference pane, and then back to General
4) Notice the check on launch pref failed to stick has reverted back to the previous value

The most recent, relevant changes introduced to this pref pane were in Bug 353221: <http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=Navigation.mm&branch=&root=/cvsroot&subdir=/mozilla/camino/PreferencePanes/Navigation&command=DIFF_FRAMESET&rev1=1.34&rev2=1.35>

The behavior in which preferences are actually updated is somewhat inconsistent throughout this pane.  Some options have their own dedicated IBAction and are changed in the defaults immediately upon modification, while others seem to be updated ONLY when the pane's main view was closed (via the NSPreferencePane -didUnselect notification).

These General prefs are modified with their own IBAction:
- (IBAction)checkboxStartPageClicked:(id)sender;
- (IBAction)defaultBrowserChange:(id)sender;
- (IBAction)defaultFeedViewerChange:(id)sender;
- (IBAction)warningCheckboxClicked:(id)sender;
- (IBAction)rememberWindowStateCheckboxClicked:(id)sender;

Searching through the source code for checkboxCheckDefaultBrowserOnLaunch (the IBOutlet for the button) indicated the only time where it is ever modified was in didUnselect.

The fix for bug 353221 was based upon the fact that modifying the preference in didUnselect was just a double-checking mechanism to ensure it had the correct value.  It seems as though, in the case of the "check default browser on launch" preference, that this was the only area of code in which it was ever synced.

The two preferences fixed by the aforementioned bug (close when warnings tabs and check default browser on startup) are unique in that they can also be changed from a dialog outside of preferences. Reversing the patch and reintroducing the removed lines will reopen bug 353221, meaning those prefs could become stale and changes made will counteract parallel choices set via a dialog box while the prefs window was still open.

Keeping preferences/views in sync requires a lot of glue code, especially in this case of dialogs also providing alternative access to the same options.

Two simple fixes are to:
A) Bring back just the first removed line of attachment 239090 [details] [diff] [review]
B) Give checkboxCheckDefaultBrowserOnLaunch its own IBAction for immediate updating (which is expected anyway: 
	> (From bug 353221 comment #2)
	> Makes sense. Checkboxes' and radiobuttons' effect in prefs should be immediate
	
Something more robust might also be necessary here, though.

Comment 1

[Stuart Morgan]

B) is the right solution. (Eventually we should look into maybe making a KVC layer for prefs, so we could just bind them, but that's longer-term.)

Comment 2

[Sean Murphy]

Attachment: fix

Implements option B, giving checkboxCheckDefaultBrowserOnLaunch its own dedicated IBAction.

Comment 3

[Sean Murphy]

Attachment: nib to go along.

updated with the new target/action connection.

Comment 4

[Stuart Morgan]

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

sr=smorgan, r=cl from irc.

Comment 5

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH. Sorry I forgot to give you attribution on the trunk checkin :(

Comment 6

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

Did this regress bug 353221?  There's a post on the forum in the 1.1 beta thread:

On a related note, clicking "Don't show this warning again" when closing a page with multiple tabs doesn't seem to update the state of this preference in the General prefpane until the next launch. 

Comment 7

[Stuart Morgan]

No, that would have been a pr-existing problem. There's an issue with the pref panes (there's a bug I filed somewhere) where closing the pane and opening it doesn't trigger all the same things that changing to another pane and back does, so we wouldn't re-fetch the prefs if you just kept opening and closing the same pane. If that person tried switching to another pane and back, I bet it would worked as well as relaunching.