Closed
Bug 358318
Opened 18 years ago
Closed 17 years ago
Fix didSelect/didUnselect semantics of pref pane controller
Categories
(Camino Graveyard :: Preferences, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.6
People
(Reporter: stuart.morgan+bugzilla, Assigned: mark)
References
Details
(Keywords: fixed1.8.1.12)
Attachments
(1 file)
1.95 KB,
patch
|
stuart.morgan+bugzilla
:
review+
|
Details | Diff | Splinter Review |
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).
Comment 2•17 years ago
|
||
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.
Comment 3•17 years ago
|
||
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.
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
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 | ||
Comment 6•17 years ago
|
||
Attachment #295258 -
Flags: review?(stuart.morgan)
Reporter | ||
Comment 7•17 years ago
|
||
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+
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
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.
Description
•