191410 - modifying bool prefs in about:config should not let the user type a value in

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

In about:config,

when modifying a pref, rather than manually typing in true or false (which
doesn't do anything if the value is not one of those two) two radio options
should be present for true or false.

Comment 1

[[e-mail]]

Attachment: Sort of patch...

If you want anything more spectacular I'd appreciate more description.

Comment 2

[[e-mail]]

*** Bug 235197 has been marked as a duplicate of this bug. ***

Comment 3

[Jesse Ruderman]

From the dup:

For bonus points, call the context menu item "Toggle" when the the pref is a
bool and "Modify..." (with an ellipsis) when the pref is not a bool.  (Another
possibility: "Change...", "Change to true", "Change to false".)

Comment 4

[Jesse Ruderman]

It shouldn't put up a dialog with a checkbox.  It should just change it to the
other value with no additional user interaction.

Comment 5

[Steffen Wilberg]

Please don't forget to also patch
mozilla/toolkit/components/viewconfig/content/config.js. These files are now
almost identical.
I'd suggest to remove the remaining differences while we're at it: The xpfe
version still has the NPL-license, whereas toolkit's config.js is tri-licensed.
Blake also removed the unnecesary toString() from line 353 in the toolkit version.

Comment 6

[Steffen Wilberg]

I played a bit with the "Sort of patch". How about these lines:
  if (entry.typeCol == nsIPrefBranch.PREF_BOOL) {
      var value = gPrefBranch.getBoolPref(entry.prefCol);
      gPrefBranch.setBoolPref(entry.prefCol, !value);

Works like a charm. Select the pref and hit enter to toggle. Or doubleclick it.

Comment 7

[[e-mail]]

OK, so what are y'all expecting for a new boolean preference?

Comment 8

[Steffen Wilberg]

Modifying a bool pref should just change it to the other value with no
additional user interaction, as Jesse pointed out. Doubleclicking should be
enough to toggle the pref. That's what the code I suggested in comment 6
(modyfying Neil's "sort of patch") does. I can make a patch if you want.

Comment 9

[[e-mail]]

That's all very well but the point of my question was to avoid confusing the
user by having different UI for new and existing prefs, especially since the UI
for new prefs switches to modifying if you type in the name of an existing pref.

Comment 10

[Steffen Wilberg]

Now I understand your question. If the user creates a new bool pref, he has to
set it to true or false. The popup with the checkbox you proposed in the "sort
of patch" would be fine for that.
But I think creating a new bool pref is a rare case. Most of the time I just
want to toggle an existing pref. Just doubleclicking is much more convenient
than a popup with a checkbox and buttons.
I therefore suggest to have the popup with the checkbox only for new prefs. I
don't think that's too confusing. 

The code could be like this in ModifyPrefs:
  if (entry.typeCol == nsIPrefBranch.PREF_BOOL) {
    if (entry.valueCol) { // existing pref
      var value = gPrefBranch.getBoolPref(entry.prefCol);
      gPrefBranch.setBoolPref(entry.prefCol, !value);
    } else { // new pref
      var check = { value: true };
      if (!gPromptService.confirmCheck(window, title, null, entry.prefCol, check))
        return false;
      gPrefBranch.setBoolPref(entry.prefCol, check.value);
    }

Comment 11

[[e-mail]]

Attachment: Steffen's approach

Comment 12

[Steffen Wilberg]

Comment on attachment 146498 [details] [diff] [review]
Steffen's approach

That's just fine for modifying an existing bool pref, but if you create a new
bool pref, you still have to enter a value.
There should be a checkbox instead, like in your first patch and in the not so
elegant code I posted before.

Comment 13

[[e-mail]]

Attachment: Combined

Steffen, how does this look to you? Note that I also added some code to make
the New option available without selecting an existing option first.

Comment 14

[Steffen Wilberg]

Comment on attachment 146501 [details] [diff] [review]
Combined

Great!

Comment 15

[[e-mail]]

Comment on attachment 146501 [details] [diff] [review]
Combined

Let's ask caillon for review after all he filed the bug in the first place ;-)

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

[:::::::::::: Enter boolean value ::::::::::]
|  _._                                      |
| (_? ) Enter boolean value                 |
|   \|                                      |
|       [x] foo.bar.foopy                   |
|___________________________________________|


Hmm.  I dunno.  That seems a bit weird to me....

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 146501 [details] [diff] [review]
Combined

Cancelling request until we can figure out what the UI should be, but I'm
pretty sure that isn't it.

One suggestion would be to stop using the default prompts dialogs and write
your own where you can enter both the pref name and value on the same dialog. 
Then you could use a radio group if you have to in order to make it more
explicit that the pref needs to have a true or false value.  The checkbox is
quite vague and confusing IMO.

Another (probably more idealistic) approach would be to allow users to edit the
prefs inline without a prompt, but I don't think our trees support that yet. 
IOW, the value column would be a textfield which users could click in and edit.
 There would be a "New" button somewhere which would add an untitled pref to
the end of the list which would let the user edit the name inline and the value
inline.  Modifying bool prefs could keep the toggle behavior you have now....

Comment 18

[[e-mail]]

(In reply to comment #17)
>One suggestion would be to stop using the default prompts dialogs and write
>your own where you can enter both the pref name and value on the same dialog. 
What happens if the user types in the name of an existing pref?
(the current code just edits the existing pref)

Comment 19

[Alec Flett]

Comment on attachment 146501 [details] [diff] [review]
Combined

can we break up this chunk of code in updateContextMenu to add some comments?
And the 2nd large chunk of code too?

overall the code looks correct, but it is so muddled with obscure checks, it
takes a while to understand (or review!)

Also, fix variable names (like the ambiguous "copy" and "pref" variables -
"copy" is even doing double-duty here, and its not clear what nodes it is
referring to. Seems like "pref" should be "currentPrefRow" or something. even
"modify" and "reset" are pretty ambiguous to me

I'm going to sr- this for now, just so I can see the more readable code :)

Comment 20

[[e-mail]]

Attachment: Take two

Comment 21

[Steffen Wilberg]

Comment on attachment 147568 [details] [diff] [review]
Take two

>+function updateContextMenu() {
...
>+  var copyDisabled = false;
>+  if (view.selection.currentIndex >= 0) {
...
>+    copyDisabled = true;
>+  }
>+
>+  var copyName = document.getElementById("copyName");
>+  copyName.setAttribute("disabled", copyDisabled);
copyDisabled should set to true first (=disable the menuitem). Inside the if,
it should be set to false (=enable the menuitem).

You also forgot to include the changes to config.dtd.

While you're at it, I'd prefer right-clicks to work on the pref just under the
mouse pointer, instead of the previously selected pref.

Comment 22

[[e-mail]]

Attachment: Take three

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 151982 [details] [diff] [review]
Take three

>Index: content/config.js

>+function updateContextMenu() {

Nit: I think most of the rest of the file uses K&R bracing.  Please use that
here, too.


>+  var lockCol = PREF_IS_LOCKED;
>+  var typeCol = nsIPrefBranch.PREF_STRING;
>+  var valueCol = "";
>+  var copyDisabled = true;

Nit: a line break here would be nice.

>+  if (view.selection.currentIndex >= 0) {
>+    var prefRow = gPrefView[view.selection.currentIndex];
>+    lockCol = prefRow.lockCol;
>+    typeCol = prefRow.typeCol;
>+    valueCol = prefRow.valueCol;
>+    copyDisabled = false;
>+  }
>+
>+  var copyName = document.getElementById("copyName");
>+  copyName.setAttribute("disabled", copyDisabled);
>+
>+  var copyValue = document.getElementById("copyValue");
>+  copyValue.setAttribute("disabled", copyDisabled);
>+
>+  var resetSelected = document.getElementById("resetSelected");
>+  resetSelected.setAttribute("disabled", lockCol != PREF_IS_USER_SET);
>+
>+  // Only hide the modify item for boolean preferences with values

A more useful comment would explain why you should hide the item for said
prefs, rather than (or in addition to) tell you what the code does (or, what it
says the code does, my next comment shows otherwise).

>+  var modifySelected = document.getElementById("modifySelected");
>+  modifySelected.setAttribute("disabled", lockCol == PREF_IS_LOCKED);
>+  modifySelected.hidden = typeCol == nsIPrefBranch.PREF_BOOL && valueCol;

This doesn't seem right.  This will set hidden to false if the pref has a false
value, won't it?  I'm not so sure that's what you want.

>+

A similar comment to the above seems required here.

>+  var toggleSelected = document.getElementById("toggleSelected");
>+  toggleSelected.setAttribute("disabled", lockCol == PREF_IS_LOCKED);
>+  toggleSelected.hidden = typeCol != nsIPrefBranch.PREF_BOOL || !valueCol;

And since the two checks are mutually exclusive of each other, perhaps an
if(){}else{} is in order?  Or toggleSelected.hidden = !modifySelected.hidden or
something.

Comment 24

[[e-mail]]

Attachment: Take four

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 152036 [details] [diff] [review]
Take four

>+  var canToggle = typeCol == nsIPrefBranch.PREF_BOOL && valueCol != "";

This is still broken.  you want valueCol !== ""

r=caillon with that.

Comment 26

[[e-mail]]

Comment on attachment 152036 [details] [diff] [review]
Take four

No I don't, valueCol is explicitly always a string.

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 152036 [details] [diff] [review]
Take four

sr=sspitzer

Comment 28

[Jesse Ruderman]

This patch (for Mozilla) conflicts with one line of my patch in bug 247606 (for
Firefox, already checked in).  It would be great if you'd merge the patches and
check both into Mozilla, Firefox, and Firefox-aviary.

Comment 29

[Steffen Wilberg]

*** Bug 238955 has been marked as a duplicate of this bug. ***

Comment 30

[Steffen Wilberg]

Ready for checkin?

Comment 31

[[e-mail]]

Just waiting for the tree to thaw for 1.8b

Comment 32

[[e-mail]]

Fix checked in.

Comment 33

[Steffen Wilberg]

Attachment: port to aviary toolkit

This is a port to toolkit on the aviary branch.
The only difference is to preserve this line from Jesse's patch in bug 247606:
  gPrefBranch.setIntPref(entry.prefCol, parseInt(result.value, 10));
instead of xpfe:
  gPrefBranch.setIntPref(entry.prefCol, eval(result.value));

The patch applies to the trunk as well, but config.dtd still lives here:
mozilla/toolkit/components/viewconfig/locale/config.dtd

Comment 34

[Steffen Wilberg]

Comment on attachment 154101 [details] [diff] [review]
port to aviary toolkit

This is just a port from xpfe. It has l10n impact.

Comment 35

[Steffen Wilberg]

Toolkit port checked into aviary branch and trunk 2004-07-26 10:05/10:07.