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

RESOLVED FIXED in mozilla1.9

Status

()

Toolkit
Preferences
--
major
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Karsten Düsterloh, Assigned: Karsten Düsterloh)

Tracking

({dataloss})

unspecified
mozilla1.9
dataloss
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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!

Updated

10 years ago
Flags: blocking1.9?
why would this block? please provide rationale when requesting blocking...
Flags: blocking1.9?
(Assignee)

Comment 2

10 years ago
Deleting prefs while in non-instant-apply mode comes very unexpected, hence raising severity and setting dataloss keyword.
Severity: normal → major
Keywords: dataloss
(Assignee)

Comment 3

10 years ago
Created attachment 304614 [details] [diff] [review]
use 'undefined' keyword to defer reset of preferences values

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?
(Assignee)

Updated

10 years ago
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.
(Assignee)

Comment 5

10 years ago
Created attachment 304818 [details] [diff] [review]
show default in UI for undefined values

(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)
(Assignee)

Comment 6

9 years ago
Created attachment 309811 [details] [diff] [review]
chrome test for prefwindow

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+
(Assignee)

Updated

9 years ago
Attachment #304818 - Flags: approval1.9?
(Assignee)

Comment 9

9 years ago
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+
(Assignee)

Comment 12

9 years ago
Landed patch (with bracing fixed) and tests (despite Makefile.in bitrot) on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

9 years ago
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".)

Updated

9 years ago
Depends on: 446536
Target Milestone: --- → mozilla1.9
You need to log in before you can comment on or make changes to this bug.