Closed Bug 410562 Opened 17 years ago Closed 16 years ago

<preference>.reset() does not honor instantApply setting

Categories

(Toolkit :: Preferences, defect)

Product:
Toolkit
Component:
Preferences
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: mnyromyr, Assigned: mnyromyr)

References

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

show default in UI for undefined values
3.38 KB, patch
Gavin
: review+
damons
: approval1.9+
Details | Diff | Splinter Review
chrome test for prefwindow
27.43 KB, patch
Gavin
: review+
damons
: approval1.9+
Details | Diff | Splinter Review 
The reset method for preferences does not adhere to the instantApply setting: 
  <method name="reset">
   <body>
    this.preferences.rootBranch.clearUserPref(this.name);
   </body>
  </method>
This means that with instantApply set to false, preferences will still get deleted even if the user hits Cancel!
Flags: blocking1.9?
why would this block? please provide rationale when requesting blocking...
Flags: blocking1.9?
Deleting prefs while in non-instant-apply mode comes very unexpected, hence raising severity and setting dataloss keyword.
Severity: normal → major
Keywords: dataloss
This patch defers the resetting of a pref until normal pref syncing happens by using the sepcial undefined value.
Assignee: nobody → mnyromyr
Status: NEW → ASSIGNED
Attachment #304614 - Flags: review?
Attachment #304614 - Flags: review? → review?(gavin.sharp)
Doesn't this leave the UI in an inconsistent state if you call reset() on a preference associated with an element, in the non-instantApply case? It's .value will be "undefined", and setElementValue doesn't seem to deal with that.
(In reply to comment #4)
> It's .value will be "undefined", and setElementValue doesn't seem to deal 
> with that.

Yeah, true. Especially, reset string prefs would be displayed as the string literal "undefined". This patch syncs that with the behaviour for prefs not overridden by the user: show default value in this case.

>
Attachment #304614 - Attachment is obsolete: true
Attachment #304818 - Flags: review?(gavin.sharp)
Attachment #304614 - Flags: review?(gavin.sharp)
There were no tests so far for preferences windows, so I added some basic tests along with the actual tests for this specific bug.
This chrome test fails without the actual patch in this bug.
Attachment #309811 - Flags: review?(gavin.sharp)
Comment on attachment 304818 [details] [diff] [review]
show default in UI for undefined values

r=me, I apologize for the delay. one nit that applies to both patches: bracing style doesn't match toolkit conventions (opening { on same line). I don't care too much about the test since it's a new file, but please do fix for the patch.
Attachment #304818 - Flags: review?(gavin.sharp) → review+
Comment on attachment 309811 [details] [diff] [review]
chrome test for prefwindow

... and thanks a lot for these tests, they're great!
Attachment #309811 - Flags: review?(gavin.sharp) → review+
Attachment #304818 - Flags: approval1.9?
Comment on attachment 309811 [details] [diff] [review]
chrome test for prefwindow

Tests for preferences, incl. for attachment 304818 [details] [diff] [review].
Attachment #309811 - Flags: approval1.9?
Comment on attachment 309811 [details] [diff] [review]
chrome test for prefwindow

a1.9+=damons
Attachment #309811 - Flags: approval1.9? → approval1.9+
Comment on attachment 304818 [details] [diff] [review]
show default in UI for undefined values

a1.9+=damons
Attachment #304818 - Flags: approval1.9? → approval1.9+
Landed patch (with bracing fixed) and tests (despite Makefile.in bitrot) on trunk.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 309811 [details] [diff] [review]
chrome test for prefwindow

>Index: toolkit/content/tests/chrome/Makefile.in
>===================================================================
>+      ok(found.file_data    === expected.file_data,    "instant reset element file"   );
...
>+      ok(found.file_data    === expected.file_data,    "instant reset element file"   );

These tests failed on Windows and thus got commented out in a bustage fix.
We need OS-dependent data here once the pref type file has UI elements.
(Also, the second comment (block) should read "non-instant".)
Depends on: 446536
Target Milestone: --- → mozilla1.9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: