411189 - Fix pref pane construction and cleanup

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Mark Mentovai]

Bug 358318 comment 7:

>Stuart Morgan  2008-01-07 07:09:08 PST
>(From update of attachment 295258 [details] [diff] [review])
>To fix the dup (and the other issues like it), we need to either:
>- also send a mainViewDidLoad, or
>- move everything that's pref setup rather than view construction (like
>setting up table formatters) into willSelect, now that doing so will work
>correctly. (I'd vote for the latter, since it makes it clear how to balance
>object lifetimes.)
>
>Shall we take care of all of that at once, or do you want to have that in a
>follow-up?

I prefer the second option, too, and this is bug is the follow-up.

The dup in question is bug 373158.

Comment 1

[Chris Lawson (gone)]

Apple's own preference pane documentation uses |mainViewDidLoad| to trigger all the preference reads/updates/etc. That's all well and good, but Camino (and other Gecko browsers) is sort of a special case, since there are two different ways prefs can be set (prefs window and about:config).

I, too, would prefer the second option above (move stuff into willSelect as appropriate), but I have two questions:

1) Why willSelect rather than didSelect?
2) Should I be putting something about this into the pref pane documentation (bug 384800)? My gut says yes, since I just wrestled with this mess for about an hour and a half trying to figure out why Apple's example code wasn't working in CBSamplePane, but I'm wondering if the did/willSelect thing matters much (if at all).

cl

Comment 2

[Chris Lawson (gone)]

...or should we do like the Security preferences pane does, be very paranoid about always getting it right, and put the prefs setup in |didActivate| instead, which (apparently) gets called every time the prefs window comes to the foreground?

Comment 3

[Chris Lawson (gone)]

I have a patch ready to go for this, pending an answer to comment 1 and comment 2 (mostly comment 2).

Comment 4

[Stuart Morgan]

(In reply to comment #1)
> Why willSelect rather than didSelect?

Because we want things to be correct before they are displayed, not after.

(In reply to comment #2)
> ...or should we do like the Security preferences pane does, be very paranoid
> about always getting it right, and put the prefs setup in |didActivate|
> instead

No, we should only do that when we have an explicit need to do so.

Comment 5

[Chris Lawson (gone)]

Attachment: fix v1

Moves pref setup stuff to the willSelect/willUnselect pairing. Also fixes the last couple instances of OrgMozillaChimeraPreferences in our pref panes.

Comment 6

[Chris Lawson (gone)]

Attachment: new Downloads nib

Comment 7

[Chris Lawson (gone)]

Attachment: new Security nib

Comment 8

[Chris Lawson (gone)]

Attachment: new History nib

Comment 9

[Stuart Morgan]

Comment on attachment 323810 [details] [diff] [review]
fix v1

>Index: camino/PreferencePanes/Appearance/Appearance.mm
>+- (void)willSelect
>...
>   [self setupFontRegionPopup];

This does some relatively expensive work based on static resources that shouldn't be done repeatedly, so it will need som refactoring.

>-@interface OrgMozillaChimeraPreferenceDownloads : PreferencePaneBase
>+@interface OrgMozillaCaminoPreferenceDownloads : PreferencePaneBase

This patch doesn't contain any of the necessary accompanying Info.plist changes for these.

> - (void)mainViewDidLoad
> {
>+  // set up default browser menu
>+  [self updateDefaultBrowserMenu];
>+
>+  // set up the feed viewer menu
>+  [self updateDefaultFeedViewerMenu];

I'm not convinced we want to move these, since we don't know when a user might install a new reader/browser.

>-- (void) didUnselect
>+- (void) willUnselect

There's no reason to change this (in any of the panes)

>-- (void)mainViewDidLoad
>-{
>-  [self updateButtons];
>-}

This should be changed to willSelect, not removed. didActivate is only called when the window changes from non-key to key.

>-  // Set tab focus popup.
>-  [mTabBehaviorPopup selectItemAtIndex:[self popupIndexForCurrentTabFocusPref]];

This is a pref value, not view setup, so it shouldn't be moved.

Comment 10

[Chris Lawson (gone)]

Attachment: fix v1.1

Incorporates all fixes from comment 9. This does the right thing in the Font tab of the Appearance pref pane and I'm reasonably certain it's the right fix.

Comment 11

[Stuart Morgan]

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

Sorry, a couple things I missed before in General:
- Move the disabling of the update checkbox into mainViewDidLoad (the part to set the state in willSelect will need to check for the checkbox being enabled and the pref)
- Given bug 353433, please move the two application popups to mainViewDidLoad and put a comment explaining that they are only there because of the slowness (and reference the bug)

Also, I get an exception (then crash, courtesy of Core putting itself in the codepath of every single event in every window) in History's NonNegativeIntegerFormatter stringForObjectValue (sending stringValue to an NSString) now that the int value isn't being set first. That method should be changed to check the class, and return @"" for any non-NSNumber.

The rest looks good though, including the font setup separation.

Comment 12

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

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

(Cancelling since the patch has sr-)

Comment 13

[Chris Lawson (gone)]

Attachment: fix v1.2

The nibs are still good AFAIK. Here's a fixed patch that addresses all the previous comments.

Comment 14

[Stuart Morgan]

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

r=smorgan. This conflicts with my Sparkle update though, and since it was easier for me to resolve this patch against that than the other way around, I'll post a patch that can apply and land after bug 445629.

Comment 15

[Stuart Morgan]

Attachment: resolved against the Sparkle changes.

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 339729 [details] [diff] [review]
resolved against the Sparkle changes.

sr=pink

Comment 17

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

Comment on attachment 339729 [details] [diff] [review]
resolved against the Sparkle changes.

This doesn't apply cleanly (hunk 2 in General.mm) and I can't figure out how to resolve it.

Comment 18

[Chris Lawson (gone)]

Attachment: Fix, now un-bitrotted

This ought to do it. Someone should probably test it just to make sure, though.

Comment 19

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

So, besides some "simple" order changes between neighboring bits of code, the major difference between Stuart's "resolved" patch and the new patch from Chris is where in the file this block lives:

>   if (![[[NSBundle mainBundle] objectForInfoDictionaryKey:@"SUEnableAutomaticChecks"] boolValue]) {
>     // Disable update checking if it's turned off for this build.  The tooltip comes
>     // from the main application because it's used there too.
>     [checkboxAutoUpdate setEnabled:NO];
>     [checkboxAutoUpdate setToolTip:NSLocalizedString(@"AutoUpdateDisabledToolTip", @"")];
>   }

In Stuart's patch, it's moved up to the - (void)mainViewDidLoad group, whereas in Chris's it remains where it was (which is now in the - (void) willSelect group).

Comment 20

[Stuart Morgan]

(In reply to comment #19)
> In Stuart's patch, it's moved up to the - (void)mainViewDidLoad group, whereas
> in Chris's it remains where it was (which is now in the - (void) willSelect
> group).

My version is correct (except for whatever got messed up that prevents it from applying, obviously).

Comment 21

[Chris Lawson (gone)]

Oh, right, comment 11.

Comment 22

[Chris Lawson (gone)]

Attachment: fix v1.4

No mo' bitrot, more Sparkle-y.

Comment 23

[Serge Gautherie (:sgautherie)]

Cancelling 'checkin-needed', due to pending reviews.

Comment 24

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

Serge, please let us manage the keywords in our bugs ourselves; thanks.  I'd also recommend excluding bugs in the Camino module from your checkin-needed query.

Comment 25

[Stuart Morgan]

Comment on attachment 340821 [details] [diff] [review]
fix v1.4

This doesn't need to go to sr again; the existing one is fine.

>-  else if ([[SUUpdater sharedUpdater] automaticallyChecksForUpdates]) {
>-    [checkboxAutoUpdate setState:NSOnState];
>-  }
>-
>   [textFieldHomePage setStringValue:[self currentHomePage]];
> 
>[...]
>+  if ([[SUUpdater sharedUpdater] automaticallyChecksForUpdates] && [checkboxAutoUpdate isEnabled])
>+    [checkboxAutoUpdate setState:NSOnState];

I'm not sure why you are moving this to after the homepage line, but that doesn't really matter I guess. You should be doing less expensive [checkboxAutoUpdate isEnabled] check first of the two conditions though.

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> I'd also recommend excluding bugs in the Camino module from your checkin-needed query.

Done.

Comment 27

[Chris Lawson (gone)]

Attachment: fix v1.41

For checkin; I told Stuart on IRC that I was just going to rearrange all the pref setters in |willSelect| according to the order they currently are in the UI, which he gave a verbal OK for. This also does the less expensive check first.

Comment 28

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

Landed on cvs trunk.