Last Comment Bug 207840 - about:config enhancement - filtering/searching preferences
: about:config enhancement - filtering/searching preferences
Status: VERIFIED FIXED
: fixed1.4.1
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
: sairuh (rarely reading bugmail)
Mentors:
Depends on:
Blocks: 103911 224532
  Show dependency treegraph
 
Reported: 2003-06-01 02:04 PDT by Tim
Modified: 2006-08-09 02:38 PDT (History)
13 users (show)
mozilla: blocking1.4.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
about:config filtering patch v1 (20.24 KB, patch)
2003-06-01 02:34 PDT, Tim
no flags Details | Diff | Splinter Review
Screenshot of patched firebird (28.01 KB, image/png)
2003-06-01 02:37 PDT, Tim
no flags Details
Alternative patch (8.57 KB, patch)
2003-06-02 01:23 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Revised patch (9.21 KB, patch)
2003-06-03 01:43 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Un-bitrotted revised patch (8.93 KB, patch)
2003-06-10 09:33 PDT, Tim
caillon: review+
sspitzer: superreview+
mozilla: approval1.4.1+
Details | Diff | Splinter Review
Addressed caillon's comments (9.30 KB, patch)
2003-06-13 09:53 PDT, neil@parkwaycc.co.uk
neil: review+
Details | Diff | Splinter Review
Hard-coded english version (1.54 KB, patch)
2003-08-07 16:28 PDT, Tim
no flags Details | Diff | Splinter Review

Description Tim 2003-06-01 02:04:37 PDT
Here's my current work at allowing some form of "searching" preferences.  The
patch implements searching on the name as a regular expression.  The patch is
also cluttered with various bits of refactoring. Hopefully, I commented
reasonably after myself.

I have not thoroughly tested this patch, though it works for the first couple of
things I tried.

The patch is not ready for prime-time for a couple of reasons: it still needs to
allow Enter to handle either changing the search text or modify the selected
pref, and the one literal I added to config.xul needs to be extracted into
another file. There are probably also places that could use some performance
analysis (honestly, I think there are few enough preferences and the page is
used infrequently enough that speed/readability tradeoffs should side towards
readability).

Patch + readme coming up.
Comment 1 Tim 2003-06-01 02:34:23 PDT
Created attachment 124664 [details] [diff] [review]
about:config filtering patch v1

This applies to both mozilla and firebird, source or unpacked chrome.

Apply in:
  source: mozilla/xpfe/global/resources/content/
  chrome: toolkit.jar, content/global/

Patch command:
  patch -p1 < toolkit.patch
Comment 2 Tim 2003-06-01 02:37:32 PDT
Created attachment 124665 [details]
Screenshot of patched firebird

A screenshot for those who want to see what it does without bothering with the
patch.
Comment 3 Tim 2003-06-01 02:42:14 PDT
Oh yeah, and I have to fix my brace/parenthesis style, I suppose. (reminder to
myself or whoever else wants to take up the cause).
Comment 4 Jo Hermans 2003-06-01 03:14:57 PDT
This should be a dupe of bug 103911, but since you've already attached a patch
to this one ... But they're still different : Ctrl-F versus a textbox (what do
you call that ?). Can you manage to implement a find-dialog too ? Or do we leave
it like it is now, and dupe 103911 against this one ?
Comment 5 Tim 2003-06-01 16:41:46 PDT
I consider this to be fairly different from find-in-page, primarily because this
doesn't even begin to make find-in-page work.  I figure we can leave that bug
open because it would seemingly be nice even with this type of feature.

I mainly wrote this the way I did because I wanted to be able to focus on a
subset of prefs without rewriting about:config to accept find-in-page.
Comment 6 Tim 2003-06-01 16:48:32 PDT
I should note that right now, the patch only searches the pref name--not any
other fields.  It would be trivial to make it search other fields, but name is
sufficient for me, and people would probably want the ability to only search
certain fields (I leave that functionality for someone else to write).  The
matching function is View.objMatchFunc and it is set in the View.searchRE setter
function.

Also, in the firebird spirit, it should at least search the most sane set of
fields by default.
Comment 7 neil@parkwaycc.co.uk 2003-06-02 01:23:00 PDT
Created attachment 124715 [details] [diff] [review]
Alternative patch
Comment 8 neil@parkwaycc.co.uk 2003-06-02 01:25:13 PDT
Comment on attachment 124715 [details] [diff] [review]
Alternative patch

>+  <textbox id="textbox" flex="1" type="timed" timeout="500" oncommand="FilterPrefs();"/>
Doh! I forgot that textboxes still have that silly callback attribute instead
of something sensible - please change oncommand to callback for testing.
Comment 9 Tim 2003-06-02 02:43:26 PDT
Comment on attachment 124715 [details] [diff] [review]
Alternative patch

>Index: content/config.js
[snip]
>@@ -185,12 +221,16 @@
[snip]
>       if (gSortedColumn == "lockCol" || gSortedColumn == "valueCol")
>         gFastIndex = 1; // TODO: reinsert and invalidate range
>-    } else {
>+    } else if (index == gFastIndex) {
>       fetchPref(prefName, index);
>       if (index == gFastIndex) {
>         // Keep the array sorted by reinserting the pref object
else if -> else

Also, don't forget to change all /capability/ to /^capability/ (as discussed on
IRC).
Comment 10 neil@parkwaycc.co.uk 2003-06-03 01:43:47 PDT
Created attachment 124800 [details] [diff] [review]
Revised patch
Comment 11 Tim 2003-06-04 05:30:02 PDT
Neil is handling this.
Comment 12 Tim 2003-06-08 18:03:04 PDT
Comment on attachment 124800 [details] [diff] [review]
Revised patch

Also, can we get this in the on branch?
Comment 13 Grey Hodge (jX) 2003-06-08 18:05:57 PDT
Uselessly seconding the request for branch...
Comment 14 Asa Dotzler [:asa] 2003-06-08 21:57:58 PDT
Comment on attachment 124800 [details] [diff] [review]
Revised patch

unsetting approval request since this doesn't yet have sufficient reviews.
Comment 15 neil@parkwaycc.co.uk 2003-06-10 05:46:19 PDT
Comment on attachment 124800 [details] [diff] [review]
Revised patch

Sigh, bitrotted again; well unbitrotted really: you'll need to change
callback="..." back to oncomand="..." :-)
Comment 16 Tim 2003-06-10 09:33:23 PDT
Created attachment 125317 [details] [diff] [review]
Un-bitrotted revised patch

s/callback/oncommand/; Untested by me.
Comment 17 Christopher Aillon (sabbatical, not receiving bugmail) 2003-06-13 09:35:52 PDT
Comment on attachment 125317 [details] [diff] [review]
Un-bitrotted revised patch

>Index: content/config.js

> var gPrefHash = {};
> var gPrefArray = [];
>+var gPrefView = gPrefArray;

Just make this |var gPrefView = [];|


>-    if (/capability/.test(prefName)) // avoid displaying "private" preferences
>+    if (/^capability/.test(prefName)) // avoid displaying "private" preferences
>       return;

Should this be checking simply "capability" ?  Or would it be better to check
"capability."?	Same below.



>@@ -307,6 +345,34 @@
>   document.getElementById("configTree").view = null;
> }
> 
>+function FilterPrefs()
>+{
>+  var substring = document.getElementById("textbox").value.toString();
>+  var prefCol = view.selection.currentIndex < 0 ? null : gPrefView[view.selection.currentIndex].prefCol;
>+  var array = gPrefView;
>+  gPrefView = gPrefArray;
>+  if (substring) {
>+    gPrefView = [];
>+    for (var i = 0; i < gPrefArray.length; i++)

A few lines up, you use ++i.  Do the same here.


>@@ -400,10 +466,15 @@
>   }
> }
> 
>-function gotoPref(prefCol) {
>-  var index = getIndexOfPref(gPrefHash[prefCol]);
>-  view.selection.select(index);
>-  view.treebox.ensureRowIsVisible(index);
>+function gotoPref(pref) {
>+  var index = pref in gPrefHash ? getViewIndexOfPref(gPrefHash[pref]) : -1;
>+  if (index >= 0) {
>+    view.selection.select(index);
>+    view.treebox.ensureRowIsVisible(index);
>+  } else {
>+    view.selection.clearSelection();
>+    view.selection.currentIndex = -1;
>+  }


Just personal taste perhaps, but I think I prefer:

  if (typeof gPrefHash[pref] == "number" && gPrefHash[pref] >= 0) {
    ...select(gPrefHash[pref]);
    ...ensureRowIsVisible(gPrefHash[pref]);
  } else {
    ...clearSelection();
    ...curentIndex = -1;
  }

Feel free to ignore the last one, though I think it makes things more readable.

r=caillon with the comments addressed.
Comment 18 (not reading, please use seth@sspitzer.org instead) 2003-06-13 09:49:41 PDT
Comment on attachment 124800 [details] [diff] [review]
Revised patch

clearing sr request, since this patch is obsolete
Comment 19 neil@parkwaycc.co.uk 2003-06-13 09:53:30 PDT
Created attachment 125585 [details] [diff] [review]
Addressed caillon's comments
Comment 20 neil@parkwaycc.co.uk 2003-06-13 09:55:26 PDT
Comment on attachment 125585 [details] [diff] [review]
Addressed caillon's comments

Transferring caillon's review as per irc request
Comment 21 (not reading, please use seth@sspitzer.org instead) 2003-06-13 09:58:03 PDT
Comment on attachment 125317 [details] [diff] [review]
Un-bitrotted revised patch

sorry for the delay.

sr=sspitzer for the trunk, as you have r=caillon.

I've seen some requests for the 1.4 branch, but this is not appropriate for the
branch.
Comment 22 neil@parkwaycc.co.uk 2003-06-13 10:01:58 PDT
Fix checked in.
Comment 23 Vedran Miletic 2003-06-15 11:00:03 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030615 Mozilla
Firebird/0.6

Doesn't filter when I type something.
Comment 24 tyl2 2003-06-15 14:27:41 PDT
If someone can confirm that this works in Mozilla, perhaps someone can file a
Phoenix bug?

Filtering not working on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.4b) Gecko/20030614 Mozilla Firebird/0.6
Comment 25 Tim 2003-06-15 15:43:27 PDT
Verified in Mozilla trunk nightly 2003061505. Please open another bug for
firebird support. You can probably focus your attention on the notification part
in config.xul (currently, oncommand is used; it's possible that firebird still
wants callback).
Comment 26 djk 2003-06-16 10:41:39 PDT
FYI, Firebird support is bug 209503
Comment 27 Sander 2003-07-25 13:45:25 PDT
Could this be added to the 1.4 branch as well? Not going to request approval on
someone else's patch, but I think it would be nice to have...
Comment 28 Sander 2003-07-25 14:52:23 PDT
Comment on attachment 125317 [details] [diff] [review]
Un-bitrotted revised patch

Well, since Neil emailed me saying that he was up for it, requesting approval
for 1.4.x

Since 1.4 is the next stable long-lived branch, adding a very useful
enhancement like this that's proven its worth on the trunk seems like a good
idea to me.
Comment 29 Mike Kaply [:mkaply] 2003-08-05 15:20:47 PDT
Comment on attachment 125317 [details] [diff] [review]
Un-bitrotted revised patch

a=mkaply

I like this one.

Please make sure it works properly on the branch before checkin.
Comment 30 Mike Kaply [:mkaply] 2003-08-05 15:21:39 PDT
please add fixed1.4.1 keyword when this is checked in.
Comment 31 neil@parkwaycc.co.uk 2003-08-06 05:32:53 PDT
OK, so who's got a branch tree?
Comment 32 Tim 2003-08-06 10:05:26 PDT
I just tested it, and it works on latest 1.4.x branch. I tested attachment 125585 [details] [diff] [review].
Comment 33 Mike Kaply [:mkaply] 2003-08-06 22:20:27 PDT
Sorry guys, but I need a different change on the branch for this.

Can we hardcode the English so we don't affect a DTD file?

We want 1.4 localization packs to be compatible for 1.4.1.
Comment 34 Tim 2003-08-07 15:25:08 PDT
mkaply: would you be amenable to switching Clear All and Filter: to the
following (respectively)?
xpfe/components/console/resources/locale/en-US/console.dtd:<!ENTITY clear.label
    "Clear">
xpfe/components/search/resources/locale/en-US/find.dtd:<!ENTITY
search.button.label         "Search">

These DTDs appear to be in the default installation.  Or would you prefer simply
switching to hard-coded English?
Comment 35 Tim 2003-08-07 16:28:08 PDT
Created attachment 129393 [details] [diff] [review]
Hard-coded english version

This patch reverses the DTD changes from attachment 125585 [details] [diff] [review] and hard-codes the
English in the XUL. I'm putting this up first because it requires the least
work =). I won't do the DTD-using patch unless mkaply decides that's a better
solution.
Comment 36 Vaclav Dvorak 2004-03-27 19:57:58 PST
*** Bug 103911 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.