"Don't warn again" checkboxes can become polluted

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Preferences
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: froodian (Ian Leue), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

1.8 Branch
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment)

1.08 KB, patch
Håkan Waara
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
bug 340412 comment 18:
> >-
> >-  [self setPref:"camino.warn_when_closing" toBoolean:([checkboxWarnWhenClosing state] == NSOnState)];
> 
> Why is this being removed (and if there's a good reason, why is the previous
> one not being removed)?

We manually set some prefs on closing the General prefpane, "just to be sure."  In actuality, this can easily compromise the "Don't warn again" checkboxes.  For instance:

1. Open Preferences.  Make sure "warn me when closing..." is on.  Leave the window open.
2. Open a bunch of tabs, cmd-shift-W, check the "don't warn me again" checkbox, hit cancel.
3. Close the preferences window.

What happens: Closing the prefs window sets the preference back to true.
What should happen: We should just trust that we're doing the right thing, and let the pref stay as it is (as we do in all other prefpanes)

There are easier ways this can come up too, since |mainViewDidLoad| doesn't necessarily get called every time you open the prefs window, but that may clouding the issue with what very well may be a different bug.  For instance:

1. Open Preferences.  Make sure "warn me when closing..." is on.  Close preferences.
2. Open a bunch of tabs, cmd-shift-W, check the "don't warn me again" checkbox, hit cancel.
3. Open Preferences (checkbox is on, since it doesn't call |mainViewDidLoad|, but pref is off internally)
4. Close Preferences (since checkbox is on, we set the pref to on)
(Assignee)

Comment 1

11 years ago
Created attachment 239090 [details] [diff] [review]
Patch

Gets rid of the manual pref setters at prefpane de-selection.
Attachment #239090 - Flags: review?(hwaara)

Comment 2

11 years ago
Comment on attachment 239090 [details] [diff] [review]
Patch

Makes sense. Checkboxes' and radiobuttons' effect in prefs should be immediate (while the textboxes require closing the prefs window).
Attachment #239090 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #239090 - Flags: superreview?(mikepinkerton)

Comment 3

11 years ago
(In reply to comment #2)
> (From update of attachment 239090 [details] [diff] [review] [edit])
> Makes sense. Checkboxes' and radiobuttons' effect in prefs should be immediate

Yes.

> (while the textboxes require closing the prefs window).

No. Text boxes should take effect as soon as focus leaves the text input. (And in fact, I believe we've largely addressed that issue.)

cl
> > (while the textboxes require closing the prefs window).
> 
> No. Text boxes should take effect as soon as focus leaves the text input. (And
> in fact, I believe we've largely addressed that issue.)

History days still doesn't auto-apply, iirc (I wonder if that's because you can't really "leave" text fields unless there's a second text field or you have FKA on?).  But yeah, they should auto-apply.

Comment 5

11 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 239090 [details] [diff] [review] [edit] [edit])
> > Makes sense. Checkboxes' and radiobuttons' effect in prefs should be immediate
> 
> Yes.
> 
> > (while the textboxes require closing the prefs window).
> 
> No. Text boxes should take effect as soon as focus leaves the text input. (And
> in fact, I believe we've largely addressed that issue.)

If that's the case, then I'm wondering what the lines above froodian's changes do. Unfortunately, the diff doesn't let me see the method name, but I suspect it is when we're closing the window?

Textfields have a didEndEditing delegate method that can be used to apply the pref when we "leave" it somehow, I think.

Comment 6

11 years ago
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 239090 [details] [diff] [review] [edit] [edit] [edit])
> > > Makes sense. Checkboxes' and radiobuttons' effect in prefs should be immediate
> > 
> > Yes.
> > 
> > > (while the textboxes require closing the prefs window).
> > 
> > No. Text boxes should take effect as soon as focus leaves the text input. (And
> > in fact, I believe we've largely addressed that issue.)
> 
> If that's the case, then I'm wondering what the lines above froodian's changes
> do. Unfortunately, the diff doesn't let me see the method name, but I suspect
> it is when we're closing the window?

They ensure that the edit gets committed if focus is still in the field when the prefs are closed (or when a different pref pane is selected).

cl
Comment on attachment 239090 [details] [diff] [review]
Patch

sr=pink
Attachment #239090 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 8

11 years ago
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.