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

RESOLVED FIXED in mozilla2.0b5

Status

()

Toolkit
Preferences
--
minor
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Steve Chapel, Assigned: mmm)

Tracking

Trunk
mozilla2.0b5
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
Yeah, I know, I was just too lazy to code it.
Keywords: helpwanted

Comment 2

13 years ago
Dup/related with bug 243737?

Comment 3

13 years ago
*** Bug 243737 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey

Comment 4

13 years ago
Created attachment 173126 [details] [diff] [review]
Patch

Fix this bug by calling FilterPrefs() which calls gotoPref() instead of only
calling gotoPref(). Tested with Seamonkey and FF.
Assignee: prefs → Stefan.Borggraefe
Status: NEW → ASSIGNED
Attachment #173126 - Flags: review?(mconnor)

Comment 5

13 years ago
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().
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 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.
Attachment #173126 - Flags: review?(mconnor) → review+

Comment 8

12 years ago
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.
(Reporter)

Comment 9

12 years ago
(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

12 years ago
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+
*** Bug 298007 has been marked as a duplicate of this bug. ***
Keywords: helpwanted
OS: Windows XP → All
Hardware: PC → All
Created attachment 186592 [details] [diff] [review]
Toolkit-only patch

This already had r=mconnor, asking for approval.
Attachment #186592 - Flags: review+
Attachment #186592 - Flags: approval-aviary1.1a2?
Comment on attachment 186592 [details] [diff] [review]
Toolkit-only patch

a=shaver
Attachment #186592 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+

Comment 14

12 years ago
Wait a minute, the movement of the assignment to prefCol is incorrect...
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.
Attachment #186592 - Attachment is obsolete: true
Attachment #186592 - Flags: approval-aviary1.1a2+

Comment 16

12 years ago
(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.
Attachment #186592 - Flags: review+
*** Bug 360083 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 399713

Comment 19

10 years ago
Created attachment 284803 [details] [diff] [review]
alternative patch

What about this one? (from bug 399713)

Updated

9 years ago
Duplicate of this bug: 428879
Assignee: Stefan.Borggraefe → nobody
Status: ASSIGNED → NEW
QA Contact: prefs

Updated

7 years ago
Component: Preferences → Preferences
Product: SeaMonkey → Toolkit
QA Contact: preferences → preferences
Created attachment 454964 [details] [diff] [review]
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.
Attachment #454964 - Flags: feedback?(gavin.sharp)
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.
Attachment #454964 - Flags: feedback?(dolske)
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.
Attachment #454964 - Flags: feedback?(dolske) → review?(dolske)
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.
(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).
(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.
Created attachment 466080 [details] [diff] [review]
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.
Attachment #454964 - Attachment is obsolete: true
Attachment #466080 - Flags: review?(dolske)
Attachment #454964 - Flags: review?(dolske)
Attachment #454964 - Flags: feedback?(gavin.sharp)
I think Neil should review this.
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.
Created attachment 466160 [details] [diff] [review]
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.
Attachment #466080 - Attachment is obsolete: true
Attachment #466160 - Flags: review?(neil)
Attachment #466080 - Flags: review?(dolske)
Comment on attachment 466160 [details] [diff] [review]
Fixed as per neil's comments.

Sorry for the spam, put the wrong Neil up for review.
Attachment #466160 - Flags: review?(neil) → review?(neil)
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 ;-)
Attachment #466160 - Flags: review?(neil) → review+

Updated

7 years ago
Assignee: nobody → mmulani
Status: NEW → ASSIGNED

Updated

7 years ago
Attachment #466160 - Flags: approval2.0?

Updated

7 years ago
Attachment #466160 - Flags: approval2.0? → approval2.0+
Created attachment 468341 [details] [diff] [review]
Patch with check-in information!
Attachment #466160 - Attachment is obsolete: true
Created attachment 468353 [details] [diff] [review]
Patch with even more check-in information!
Attachment #468341 - Attachment is obsolete: true
Attachment #468353 - Flags: approval2.0+
Keywords: checkin-needed
Landed, but was this really approved without a test!
http://hg.mozilla.org/mozilla-central/rev/c7966f640a46
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.