251431 - about:config doesn't show new prefs while filter is applied

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Steve Chapel]

Reproducibility: Always

Steps to Reproduce:
1. Go to about:config
2. Type in any string for the Filter (e.g. test)
3. Add a new pref that matches that filter (e.g. http.test)

Expected Results:
The newly added pref immediately shows up because it matches the filter.

Actual Results:
The newly added pref is at first not visible. You must press Enter in the Filter
field to get it to appear.

First noticed in Mozilla 1.7. Reproduced in Mozilla trunk build 2004071108.

Comment 1

[[e-mail]]

Yeah, I know, I was just too lazy to code it.

Comment 2

[baffclan]

Dup/related with bug 243737?

Comment 3

[[e-mail]]

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

Comment 4

[Stefan Borggraefe]

Attachment: Patch

Fix this bug by calling FilterPrefs() which calls gotoPref() instead of only
calling gotoPref(). Tested with Seamonkey and FF.

Comment 5

[[e-mail]]

I see this patch only works for prefs added by NewPref...
You could improve on your prefCol assignment by testing if (!prefCol &&
view.selection.currentIndex > 0)
Also see if you can spot the coding error in FilterPrefs().

Comment 6

[Dave Townsend [:mossop]]

This also happens in firefox.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502
Firefox/1.0+

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 173126 [details] [diff] [review]
Patch

r=me, with neil's comments addressed.  This doesn't completely fix the bug
either, but that's a bit harder.

Comment 8

[Ger Teunis]

This bug also doesn't display the correct value when a pref is changed twice
while a filter is used.

1. Use text "http" as filter
2. Switch network.http.pipelining pref: change is displayed.
3. Switch network.http.pipelining pref again: change is NOT displayed
4. When applying the filter again the new value is visible for 3.

It works correct when no filter is used.

Comment 9

[Steve Chapel]

(In reply to comment #8)
> This bug also doesn't display the correct value when a pref is changed twice
> while a filter is used.

WFM with Mozilla trunk build 2005052005, using both Toggle and Reset to set
network.http.pipelining to false while the http filter is in effect. What build
were you using?

Comment 10

[Ger Teunis]

Hmm, it seems to happen at random here. Can't reproduct it myself anymore at
this moment, weird. Will try to trigger it again.

My build is:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050519
Firefox/1.0+

Comment 11

[Peter van der Woude [:Peter6]]

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

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: Toolkit-only patch

This already had r=mconnor, asking for approval.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186592 [details] [diff] [review]
Toolkit-only patch

a=shaver

Comment 14

[[e-mail]]

Wait a minute, the movement of the assignment to prefCol is incorrect...

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 186592 [details] [diff] [review]
Toolkit-only patch

(In reply to comment #14)
> Wait a minute, the movement of the assignment to prefCol is incorrect...

Sorry, I hadn't noticed the reviews comments. New patch coming up.

Comment 16

[[e-mail]]

(In reply to comment #15)
>Sorry, I hadn't noticed the reviews comments. New patch coming up.
Actually my comment 14 may refer to a new issue that my comment 5 missed.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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

Comment 19

[Sylvain Pasche]

Attachment: alternative patch

What about this one? (from bug 399713)

Comment 21

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fast patch that seems to fix this bug without any of the problems of the other patches.

This patch applies a big hammer to the problem and redraws the view when a preference is changed and a filter is being applied.

This is better than the previous patches as it will even update the view when a preference is changed by an outside source but:

This is worse as it is really is a big hammer. We can make this faster by only changing certain parts of the view as needed. Furthermore, if the filter being applied is tiny (for example, the letter 'a'), the time to redraw could be large.

Best solution I think would be:
Refactor the code so that the current filter (if any) is stored as a regex for any method to access, when a new preference is added or changed, run the preference against the regex and redraw appropriately.

I have not taken an indepth look at this code though and am not sure if there are some big pitfalls I'm missing.

Comment 22

[Mehdi Mulani [:mmm] (I don't check this)]

Comment on attachment 454964 [details] [diff] [review]
Fast patch that seems to fix this bug without any of the problems of the other patches.

adding dolske for feedback to offset load from gavin.

Comment 23

[Mehdi Mulani [:mmm] (I don't check this)]

Comment on attachment 454964 [details] [diff] [review]
Fast patch that seems to fix this bug without any of the problems of the other patches.

moving this bad boy to review, as per shawn's advice.

Comment 24

[[e-mail]]

Comment on attachment 454964 [details] [diff] [review]
Fast patch that seems to fix this bug without any of the problems of the other patches.

>     if (prefName in gPrefHash) {
>       index = getViewIndexOfPref(gPrefHash[prefName]);
>       fetchPref(prefName, getIndexOfPref(gPrefHash[prefName]));
...
>     } else {
>       fetchPref(prefName, index);
>       if (index == gFastIndex) {
>         // Keep the array sorted by reinserting the pref object
>         var pref = gPrefArray.pop();
>         index = getNearestIndexOfPref(pref);
>         gPrefArray.splice(index, 0, pref);
>         gFastIndex = gPrefArray.length;
>       }
>       if (gPrefView == gPrefArray)
>         view.treebox.rowCountChanged(index, 1);
>     }
>+
>+    var textbox = document.getElementById("textbox");
>+    if (textbox.value)
>+      FilterPrefs();
Actually we can do better than this, for instance modifying an existing pref already works. Hint: look at where the code adds the row to the tree in the case of adding a pref to an unfiltered view.

Comment 25

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #24)
> Actually we can do better than this, for instance modifying an existing pref
> already works. Hint: look at where the code adds the row to the tree in the
> case of adding a pref to an unfiltered view.

Yeah, I stated that in comment 21. Handling the filtered case nicely would require refactoring the FilterPrefs method.

If we changed how how this adds preferences, we wouldn't handle the use case of an outside method adding a preference (such as an extension might).

Comment 26

[[e-mail]]

(In reply to comment #25)
> (In reply to comment #24)
> > Actually we can do better than this, for instance modifying an existing pref
> > already works. Hint: look at where the code adds the row to the tree in the
> > case of adding a pref to an unfiltered view.
> Yeah, I stated that in comment 21. Handling the filtered case nicely would
> require refactoring the FilterPrefs method.
I was commenting on the code that I specifically quoted, rather than trying to suggest that only God's algorithm was acceptable.

Comment 27

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changed situation when added code is executed for less redundant paths.

(In reply to comment #26)
> I was commenting on the code that I specifically quoted, rather than trying to
> suggest that only God's algorithm was acceptable.

Sorry, I misunderstood your earlier comment :P
I changed the context where the FilterPrefs is run to a different execution path that still seems to exhaust the case we want to fix.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

I think Neil should review this.

Comment 29

[[e-mail]]

Comment on attachment 466080 [details] [diff] [review]
Changed situation when added code is executed for less redundant paths.

>-      if (gPrefView == gPrefArray)
>+      if (gPrefView == gPrefArray) {
>         view.treebox.rowCountChanged(index, 1);
>+      }
>+      else {
>+        var textbox = document.getElementById("textbox");
>+        if (textbox.value)
>+          FilterPrefs();
So close, and yet so far ;-)
If the textbox has no value, then gPrefView == gPrefArray, and we update the view the fast way. So in the else case we know the value must exist.

Also file style is not to wrap single lines in braces.

Comment 30

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed as per neil's comments.

As per Gavin's comments I moved to Neil as a reviewer, I'm sure dolske won't mind with a lighter load :P

Neil: tell me what you think. I tested this out and it seems to work fine.

Comment 31

[Mehdi Mulani [:mmm] (I don't check this)]

Comment on attachment 466160 [details] [diff] [review]
Fixed as per neil's comments.

Sorry for the spam, put the wrong Neil up for review.

Comment 32

[[e-mail]]

Comment on attachment 466160 [details] [diff] [review]
Fixed as per neil's comments.

It's not as if there's much left for me to be able to complain about ;-)

Comment 33

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch with check-in information!

Comment 34

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch with even more check-in information!

Comment 35

[Dave Townsend [:mossop]]

Landed, but was this really approved without a test!
http://hg.mozilla.org/mozilla-central/rev/c7966f640a46