Closed Bug 358318 Opened 18 years ago Closed 17 years ago

Fix didSelect/didUnselect semantics of pref pane controller

Categories

(Camino Graveyard :: Preferences, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.6

People

(Reporter: stuart.morgan+bugzilla, Assigned: mark)

References

Details

(Keywords: fixed1.8.1.12)

Attachments

(1 file)

There should be a one-to-one relationship between didSelect and didUnselect, but there currently isn't.  Most notably, it appears that closing the window doesn't send a didUnselect to the current pane, but re-opening prefs sends it a didSelect.  This makes doing correct memory management of things we only need while the pane is showing pretty much impossible (we currently deal with it by caching things much longer than we need them).
It should be mentioned that this can also cause behavioral problems (see dupe), or with STR like this:

1. open prefs
2. close prefs
3. use about:config to change a pref that was on the pref pane you had open
4. open prefs

In cases like these, the UI becomes inconsistent with reality.
From MVPreferencesController.mm

http://mxr.mozilla.org/mozilla/source/camino/src/preferences/MVPreferencesController.mm#313

314 - (void) windowWillClose:(NSNotification *) notification
315 {
316   // we want to behave as if we're unselecting, but we're not really unselecting. We are leaving
317   // the current pref pane selected per Apple's recommendation.
318   [[self currentPane] willUnselect];
319   [[self currentPane] didUnselect];

Anybody know what recommendation this is referring to? And in light of this code, is comment 0 really what's happening? It looks like we're indeed sending the didUnselect message.
Comment 0 is flipped.  We don't don't send willSelect or didSelect when reopening the window.

The comment is confusing, but I believe it refers to:

http://developer.apple.com/documentation/UserExperience/Conceptual/PreferencePanes/Concepts/Running.html#//apple_ref/doc/uid/20000704-9728

"Apple's recommendation" is about the unselect selectors, not to leaving the current pref pane selected.  This is a little bit clearer in light of what used to happen before this comment was introduced:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/camino/src/preferences&command=DIFF_FRAMESET&file=MVPreferencesController.mm&rev2=1.7&rev1=1.6

The comment should be reworded or removed.

Incidentally, that same checkin was the source of this regression.
We should start sending willSelect and didSelect in the new "is the window reopening?" block introduced in showPreferences: by bug 371484.  We should send both before calling makeFirstResponderValid, to remain compatible with the sequence of events in selectPreferencePaneByIdentifier:.

And we should still reword or remove the comment cl pointed to.
Assignee: nobody → mark
Depends on: 371484
Comment on attachment 295258 [details] [diff] [review]
Send willSelect/didSelect to panes when the Preferences window reopens (willUnselect/didUnselect were already sent when the window closed)

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?
Attachment #295258 - Flags: review?(stuart.morgan) → review+
I can send mainViewDidLoad before willSelect and didSelect easily and in this bug, but technically, I prefer the latter option, too.  It's easy, but it's more work and I don't want to do it in this bug, though - let's do it in a new bug.  I sort of assumed (without saying aloud) that we'd go in and fix the panes once the controller fix was in.
Blocks: 411189
Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1 as-is.  Bug 411189 is the follow-up.
No longer blocks: 411189
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1.12
Resolution: --- → FIXED
Target Milestone: --- → Camino1.6
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: