Closed
Bug 207840
Opened 21 years ago
Closed 21 years ago
about:config enhancement - filtering/searching preferences
Categories
(SeaMonkey :: Preferences, enhancement)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: timwatt, Assigned: neil)
References
Details
(Keywords: fixed1.4.1)
Attachments
(3 files, 4 obsolete files)
8.93 KB,
patch
|
caillon
:
review+
sspitzer
:
superreview+
mkaply
:
approval1.4.1+
|
Details | Diff | Splinter Review |
9.30 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
1.54 KB,
patch
|
Details | Diff | Splinter Review |
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.
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
A screenshot for those who want to see what it does without bothering with the patch.
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•21 years ago
|
||
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 ?
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.
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.
Assignee | ||
Comment 7•21 years ago
|
||
Assignee | ||
Comment 8•21 years ago
|
||
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 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).
Attachment #124664 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
Attachment #124665 -
Attachment is obsolete: true
Attachment #124715 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #124800 -
Flags: superreview?(sspitzer)
Attachment #124800 -
Flags: review?(caillon)
Reporter | ||
Comment 11•21 years ago
|
||
Neil is handling this.
Assignee: riceman+bmo → neil.parkwaycc.co.uk
Status: ASSIGNED → NEW
Reporter | ||
Comment 12•21 years ago
|
||
Comment on attachment 124800 [details] [diff] [review] Revised patch Also, can we get this in the on branch?
Attachment #124800 -
Flags: approval1.4?
Comment 13•21 years ago
|
||
Uselessly seconding the request for branch...
Comment 14•21 years ago
|
||
Comment on attachment 124800 [details] [diff] [review] Revised patch unsetting approval request since this doesn't yet have sufficient reviews.
Attachment #124800 -
Flags: approval1.4?
Assignee | ||
Comment 15•21 years ago
|
||
Comment on attachment 124800 [details] [diff] [review] Revised patch Sigh, bitrotted again; well unbitrotted really: you'll need to change callback="..." back to oncomand="..." :-)
Reporter | ||
Comment 16•21 years ago
|
||
s/callback/oncommand/; Untested by me.
Attachment #124800 -
Attachment is obsolete: true
Comment 17•21 years ago
|
||
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.
Attachment #125317 -
Flags: review+
Updated•21 years ago
|
Attachment #124800 -
Flags: review?(caillon)
Comment 18•21 years ago
|
||
Comment on attachment 124800 [details] [diff] [review] Revised patch clearing sr request, since this patch is obsolete
Attachment #124800 -
Flags: superreview?(sspitzer)
Assignee | ||
Comment 19•21 years ago
|
||
Attachment #125317 -
Attachment is obsolete: true
Assignee | ||
Comment 20•21 years ago
|
||
Comment on attachment 125585 [details] [diff] [review] Addressed caillon's comments Transferring caillon's review as per irc request
Attachment #125585 -
Flags: superreview?(sspitzer)
Attachment #125585 -
Flags: review+
Comment 21•21 years ago
|
||
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.
Attachment #125317 -
Attachment is obsolete: false
Attachment #125317 -
Flags: superreview+
Assignee | ||
Comment 22•21 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 23•21 years ago
|
||
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•21 years ago
|
||
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
Reporter | ||
Comment 25•21 years ago
|
||
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).
Status: RESOLVED → VERIFIED
Comment 26•21 years ago
|
||
FYI, Firebird support is bug 209503
Updated•21 years ago
|
Attachment #125585 -
Flags: superreview?(sspitzer)
Comment 27•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #125317 -
Flags: approval1.4.x?
Comment 29•21 years ago
|
||
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.
Attachment #125317 -
Flags: approval1.4.x? → approval1.4.x+
Comment 30•21 years ago
|
||
please add fixed1.4.1 keyword when this is checked in.
Flags: blocking1.4.x+
Assignee | ||
Comment 31•21 years ago
|
||
OK, so who's got a branch tree?
Reporter | ||
Comment 32•21 years ago
|
||
I just tested it, and it works on latest 1.4.x branch. I tested attachment 125585 [details] [diff] [review].
Keywords: fixed1.4.1
Comment 33•21 years ago
|
||
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.
Reporter | ||
Comment 34•21 years ago
|
||
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?
Reporter | ||
Comment 35•21 years ago
|
||
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•20 years ago
|
||
*** Bug 103911 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•