Closed Bug 191410 Opened 20 years ago Closed 18 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: caillon, Assigned: neil)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(2 files, 5 obsolete files)

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.
Attached patch Sort of patch... (obsolete) — Splinter Review
If you want anything more spectacular I'd appreciate more description.
*** Bug 235197 has been marked as a duplicate of this bug. ***
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".)
Summary: modifying bool prefs should not let the user type a value in → modifying bool prefs should not let the user type a value in
It shouldn't put up a dialog with a checkbox.  It should just change it to the
other value with no additional user interaction.
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.
OS: Linux → All
Hardware: PC → All
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.
OK, so what are y'all expecting for a new boolean preference?
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.
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.
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);
    }
Attached patch Steffen's approach (obsolete) — Splinter Review
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.
Attached patch Combined (obsolete) — Splinter Review
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.
Attachment #113399 - Attachment is obsolete: true
Attachment #146498 - Attachment is obsolete: true
Comment on attachment 146501 [details] [diff] [review]
Combined

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

Let's ask caillon for review after all he filed the bug in the first place ;-)
Attachment #146501 - Flags: superreview?(alecf)
Attachment #146501 - Flags: review?(caillon)
[:::::::::::: Enter boolean value ::::::::::]
|  _._                                      |
| (_? ) Enter boolean value                 |
|   \|                                      |
|       [x] foo.bar.foopy                   |
|___________________________________________|


Hmm.  I dunno.  That seems a bit weird to me....
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....
Attachment #146501 - Flags: review?(caillon)
(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 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 :)
Attachment #146501 - Flags: superreview?(alecf) → superreview-
Depends on: 242121
No longer depends on: 242121
Attached patch Take two (obsolete) — Splinter Review
Attachment #146501 - Attachment is obsolete: true
Attachment #147568 - Flags: review?(caillon)
Summary: modifying bool prefs should not let the user type a value in → modifying bool prefs in about:config should not let the user type a value in
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.
Attached patch Take three (obsolete) — Splinter Review
Attachment #147568 - Attachment is obsolete: true
Attachment #151982 - Flags: review?(caillon)
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.
Attached patch Take fourSplinter Review
Attachment #151982 - Attachment is obsolete: true
Attachment #152036 - Flags: review?(caillon)
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.
Attachment #152036 - Flags: review?(caillon) → review+
Comment on attachment 152036 [details] [diff] [review]
Take four

No I don't, valueCol is explicitly always a string.
Attachment #152036 - Flags: superreview?(alecf)
Attachment #152036 - Flags: superreview?(alecf) → superreview?(sspitzer)
Comment on attachment 152036 [details] [diff] [review]
Take four

sr=sspitzer
Attachment #152036 - Flags: superreview?(sspitzer) → superreview+
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.
*** Bug 238955 has been marked as a duplicate of this bug. ***
Ready for checkin?
Just waiting for the tree to thaw for 1.8b
Target Milestone: --- → mozilla1.8beta
Attachment #147568 - Flags: review?(caillon)
Attachment #151982 - Flags: review?(caillon)
Fix checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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 on attachment 154101 [details] [diff] [review]
port to aviary toolkit

This is just a port from xpfe. It has l10n impact.
Attachment #154101 - Flags: review?(mconnor)
Flags: blocking-aviary1.0RC1?
Whiteboard: needed-aviary1.0
Attachment #154101 - Flags: review?(mconnor) → review+
Toolkit port checked into aviary branch and trunk 2004-07-26 10:05/10:07.
Flags: blocking-aviary1.0RC1?
Keywords: fixed-aviary1.0
Whiteboard: needed-aviary1.0
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.