619356 - About:config not refreshed when prefs changed, if filter active and sorted by 'status' column

Status: VERIFIED FIXED

(Toolkit :: Preferences, defect)

Description

[Ed Morley [:emorley]]

Using:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215 Firefox/4.0b9pre ID:20101215030322

Summary:
If the filter box is being used in about:config and the prefs results are sorted by the 'status' column, the second attempt at changing a pref value results in that row not updating visually, until the filter is adjusted (which forces a refresh).

STR:
1) Start Minefield using clean profile
2) Open about:config in current tab (and accept warning)
3) Type "update" into filter field
4) Press the "status" column heading to activate a sort on the "default/user set" values
5) Double-click "app.update.enabled", to toggle it's state from false to true
6) Repeat step 5, to toggle it back again to false
7) Delete the text in the filter field and type "update" again (to force a refresh)

Expected:
- At step 6, bool state of app.update.enabled can be seen to change.

Actual:
- At step 6, no change occurs in true/false state of app.update.enabled.
- At step 7, the new value can be seen (after a refresh was forced).
- (Error console was checked, but no error message is displayed).

Reproducibility:
- Every time.

Regression Range:
- First known bad = 2004-12-05
- Last known good = 2004-11-25
- Can't reduce range any more, since no nightlies folders exist for the dates in-between?!

Comment 2

[Ed Morley [:emorley]]

Changing platform to all, since bug 635802 occurred on OS X.

Bug 597800 is possibly caused by this too.

Comment 3

[Ed Morley [:emorley]]

I've been trying to figure out what is causing this, and thought it was due to viewconfig config.js changesets in the date range in comment 0, ie:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/components/viewconfig/content&command=DIFF_FRAMESET&file=config.js&rev1=1.7&rev2=1.8&root=/cvsroot

or

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/components/viewconfig/content&command=DIFF_FRAMESET&file=config.js&rev1=1.8&rev2=1.9&root=/cvsroot

However, the second changeset merely backs out the first, so the file didn't change in that range.

This just leaves me with the rather large 10+ day pushlog range of:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-11-24&maxdate=2004-12-06&cvsroot=%2Fcvsroot

Such a pain there aren't any more nightlies in that date range :-(

Comment 4

[Adam]

Attachment: No test, properly reinsert and invalidate range.

Testing one of the preferences up to the last-good nightly that isn't the first in the list will confirm that the bug exists prior to that time (being first, its index didn't change, so it gave a false negative).  An example is update.extensions.autoUpdate preference.  Trying the last-good nightly with that preference shows the same faulty behavior.

The issue is that if the tree is sorted on status (or value), changing the preference might change the status (eg, if it was "default" and then is "user set").  This is what happens:

1. Initial change to the pref value, which gets the row repainted.
2. The next time, getViewIndexOfPref cannot find the pref, because it's not at the correct index for its new value.  So it never gets repainted.

See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewconfig/content/config.js#237

This patch follows the TODO comment there (line 245), reinserting the modified item and invalidating the range (reselecting at its new index and ensuring it's visible).

No test at present; the viewconfig component doesn't have any tests that I found.  If they belong elsewhere, please let me know the appropriate spot.

Comment 5

[Ed Morley [:emorley]]

Nice work :-)

Is this ready for review?

If so, Neil might be a good candidate:
http://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/viewconfig/content/config.js

Comment 6

[Adam]

Attachment: Cleaner version of previous patch.

Cleaned the previous patch up a bit, including fixing some cases and removing some unneeded/duplicated code.

Comment 7

[[e-mail]]

Two notes:
1. This is getting very close to having the pref array sorted at all times. This fixes the case where a preference is changed in a sorted unfiltered view. (But it doesn't know that, so it still marks the array as unsorted...)
2. In the case of adding a preference, we currently call FilterPrefs again to rebuild the view. Could the code be reused to insert the pref there too?

Comment 8

[[e-mail]]

Comment on attachment 535153 [details] [diff] [review]
Cleaner version of previous patch.

>+        if (index > newIndex) { // moving up
>+          view.treebox.invalidateRange(newIndex, index);
>+        } else { // moving down
>+          view.treebox.invalidateRange(index, newIndex);
>+        }
>+        view.selection.select(newIndex);
>+        view.treebox.ensureRowIsVisible(newIndex);
There are two problems with this code, mostly due to the fact that preferences are subject to change at any time, so the change could be unrelated to the fact that about:config happens to be open.
1. Obviously, you can only select the newIndex if the old index was selected.
2. If the selection was between the old and new index you need to adjust it.

Comment 9

[Adam]

Attachment: Reworked to handle all cases.

Thanks for the feedback and suggestions, Neil.

This version handles all of the cases for preference changes and additions while maintaining the sort and filter.

It adds the gRex variable to keep a copy of the current regular expression from filterPrefs().  gRex is used to determine if a newly-added preference belongs in the current, filtered view.

It removes the gFastIndex variable, as it's not needed anymore.

It also cleans up the cycleHeader handler a bit.  Prior to this patch, if the view was filtered, switching column sorts would cause the selection to be set incorrectly.

Comment 10

[[e-mail]]

Comment on attachment 535787 [details] [diff] [review]
Reworked to handle all cases.

This is a long comment, but there are only three real issues.

>+var gRex = null;
I'm not terribly keen on this variable name, perhaps gFilter would be better.

>-        if (gPrefArray != gPrefView)
>-          index = gPrefView.length - index - 1;
Oops, 8 years before someone found my mistake...

>+function getNearestViewIndexOfPref(pref)
Not as part of this bug because it would be too noisy, but does this change allow us to consolidate these functions somehow e.g.
getNearestViewIndexOfPref becomes getNearestIndex(gPrefView, pref)

>     var index = gPrefArray.length;
I only set this to make the "append to sorted view" simpler. If you don't use this value feel free to set it to something else.

>       index = getViewIndexOfPref(gPrefHash[prefName]);
>+      var arrayIndex = getIndexOfPref(gPrefHash[prefName]);
>+      fetchPref(prefName, arrayIndex);
>+      pref = gPrefHash[prefName];
Nit: Move the pref assignment up for when you get the indices.

>       if (index >= 0) {
This confused me. Would it help to rename it to viewIndex?

>+        // Newly added, so the range will begin with the start
That's not true, because the index adjusting code thinks that you're removing the row at this index; gPrefView.length might be a better choice.

>-        view.treebox.rowCountChanged(index, 1);
In the case of a new pref, you need this so that tree knows that it has an extra line.

>+      if (!updateView) {
>+        finalPosition = newArrayIndex;
Nit: You only use newArrayIndex to assign to finalPosition. If you just called it newIndex in the first place you wouldn't even need finalPosition.

>+      else {
>+        // View is filtered, reinsert in the view separately
That's true, but the opposite isn't; we might not be updating the view because the new preference doesn't match the filter, in which case we're done once we've inserted the pref in the array.

>+      else if (low != high && selectedIndex >= low && selectedIndex <= high) {
Nit: low == high implies low == high == index == finalPosition but we've already accounted for selectedIndex >= low && selectedIndex <= high in that case, so the low == high check is unnecessary.

>+        selectedIndex += (finalPosition - index > 0) ? -1 : 1;
Nit: Normally written finalPosition > index ? -1 : 1;

>+        view.treebox.ensureRowIsVisible(selectedIndex);
Only do this if this was the changed pref's index.

>       rex = RegExp(r[1], r[2]);
Couldn't we put this in gRex directly? Something like this:
if (/* looks like a regexp */) {
  try {
    gRex = RegExp(r[1], r[2]);
  } catch (e) {
    return;
  }
} else if (substring) {
  gRex = RegExp(/* convert substring to regexp */, "i");
} else {
  gRex = null;
}

>+      gRex = null;
I'm in two minds about this, after all the existing display is not changed.

Comment 11

[Adam]

Attachment: Addresses issues raised in feedback.

Thanks for the feedback.

The getNearest* and getView* pairs can be consolidated as a separate change.

Moved the pref variable assignment up, but it has to be assigned again afterwards since fetchPref() replaces the pref object with a fresh one.  Added a comment explaining.

Cleaned up filterPref as suggested; no longer sets gFilter to null in the catch block.  If the user were to change the sort without correcting the filter, all of the preferences would suddenly appear.

Comment 12

[[e-mail]]

Comment on attachment 536168 [details] [diff] [review]
Addresses issues raised in feedback.

>+      if (pref)
>+        index = getViewIndexOfPref(pref);
>       gSortedColumn = col.id;
>-      if (pref)
>-        index = getIndexOfPref(pref);
Is it worth moving the gSortedColumn assignment so that the if (pref) line doesn't move?

>-      rex = RegExp(r[1], r[2]);
>+     gFilter = RegExp(r[1], r[2]);
You accidentally lost a space of indentation here. r=me with that fixed.

Comment 13

[Adam]

Attachment: Corrects indentation.

Corrected indentation, moved gSortedColumn assignment up to make patch cleaner.

Comment 14

[Adam]

Attachment: Adds bug number to commit message.

Includes the bug number in the commit message.

Comment 15

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/projects/cedar/rev/ef59fe81e943

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ef59fe81e943

Comment 17

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed using the steps from the description on F7 beta1. Toggling the state of app.update.enabled works fine now.