Closed Bug 103684 Opened 23 years ago Closed 12 years ago

RFE: Implement direct ordering of filters (insert new filter at the current position / above the selected filter)

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

Tracking

(seamonkey2.12 fixed)

RESOLVED FIXED
Thunderbird 15.0
Tracking Status
seamonkey2.12 --- fixed

People

(Reporter: superbiskit, Assigned: aceman)

References

Details

Attachments

(2 files, 2 obsolete files)

It is convenient to hhave a defined ordering to the filters. Personally, I keep them alphabetic by title so I can find one. The present implementation adds a new filter at the top of the list. To move it to its proper position one must click <Move Down> "N/2" times. The display is scrolled from the new position to the top for each move; the scroll is O(N). Thus the effort to add ONE filter is O(N^2). The aggregate effort to build an ordered list of N filters is O(N^3). For me N~200 so this is very time consuming. RECOMMENDED CHANGE: (1) Easy - eliminate scrolling the list for each move. This would at least reduce each addition to O(N). (2) The Right Way (TM) - On <New>, if a filter is selected add the new filter immediately following the selected filter. This would make the addition of each filter O(const) and the TOTAL only O(N).
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
I like the idea of inserting after the currently selected one.
Target Milestone: --- → mozilla1.0.1
This absolutely should be done. The proposal is how it is implemented in NS 4. The current situation prevents me from moving to Mozilla: I need filters that don't take all day (and wear out my finger) to administer.
mass re-assign.
Assignee: naving → sspitzer
A related problem is when I delete or rename a filter the context goes back to the top of the list rather than staying where I was. (The renamed filter stays in place though.) This means if I am deleting or renaming several filters I have to keep scrolling back down to where I was.
Product: MailNews → Core
*** Bug 115155 has been marked as a duplicate of this bug. ***
sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
OS: Windows 98 → All
QA Contact: laurel → filters
Hardware: PC → All
Product: Core → MailNews Core
I've done some things with the focus/selection in the filter list dialog. I wonder how hard this could be. I'll check it out.
Assignee: nobody → acelists
Summary: RFE: Implement direct ordering of filters → RFE: Implement direct ordering of filters (insert new filter at the current position / above the selected filter)
Target Milestone: mozilla1.0.1 → ---
Great to see this one getting some attention. It's old enough to be wearing long pants now. ;) A possibly annex issue that I also filed a bug on and may be worth addressing while you have the hood open, is the possibility to put an auto repeat on the move buttons. I currently have about 100 filters and moving one even half that amount is a real PITA with the current interface. Either autorepeat on the up/down buttons and / or some key combo to move the selected filter within the list. My guess is this would be pretty trivial to program once someone is working on this part of the code. Thx
Autorepeat could be hard, but I think there is a request for adding a "Move up by 10" button, you surely find the bug. That seems much more doable.
This is VERY related (almost identical) to bug 256582. Enjoy the discussions there. IF there are enough discussions, maybe someone will actually DO something.
When I created 739984, I of course looked for an existing bug that already captured the same sentiment. I am surprised I didn't find this, but I suppose I wasn't using the same terminology. What's certainly -not- surprising is that the need was in fact identified many years ago. Early thank-you to whomever out there can take this one on and close it!
I would like to see this bug broadened (or a new one created) to include the entire problem in bug 373075 comment 7. The filter interface ought to be made to work decently for those of us who use hundreds of filters. I suggest at least adding "Page Up" and "Page Down" buttons, cut/copy/paste buttons (preferably using the same keyboard shortcuts as whatever operating system you are on), and a search function. Ideally, drag-and-drop of filters should also be allowed. I cross-posted this from bug 373075, which appears also to be motivated by the same general problem situation.
There already are bugs open for all the issues you just raised. Please join them. See e.g. bug 668995, bug 450302, bug 214548. No need to broaden this bug.
Attached patch patch (obsolete) — Splinter Review
Looks like this patch could do it. The code is in Thunderbird part and sends the position into the /mailnews core filter creation code. I tried to make it universal so that if the core does not get any position sent, it uses the first one as it is today. Ian, please check this doesn't break Seamonkey. Alternatively, if SM wants to pick this up too, tell me what to do (but I can't test it).
Attachment #611175 - Flags: ui-review?(bwinton)
Attachment #611175 - Flags: feedback?(iann_bugzilla)
(In reply to ffux from comment #12) > I currently have about 100 filters and moving one even half that amount is a > real PITA with the current interface. > Either autorepeat on the up/down buttons and / or some key combo to move the > selected filter within the list. There is bug 668995 for moving in larger steps. I think I could fix that but I didn't get approval yet. Maybe if you do some noise there ;)
Status: NEW → ASSIGNED
Comment on attachment 611175 [details] [diff] [review] patch >+++ b/mail/base/content/FilterListDialog.js > function onNewFilter(emailAddress) > { >+ let list = document.getElementById("filterList"); >+ let filterNodes = list.childNodes; >+ let selectedFilter = currentFilter(); >+ // if no filter is selected use the first position, starting at 0 >+ let position = 0; The only play where we need to one less is when passing as an arg, so just -1 there? let position = 1; >+ if (selectedFilter) { >+ for (let i = 1; i < filterNodes.length; i++) { >+ if (filterNodes[i]._filter == selectedFilter) { >+ // filterNodes are offset by 1 compared to positions in gCurrentFilterList >+ position = i - 1; position = i; >+ break; >+ } >+ } >+ } >+ >+ let args = {filterList: gCurrentFilterList, filterPosition: position}; let args = {filterList: gCurrentFilterList, filterPosition: position -1}; >+ // select the new filter, it is at the position of previous selection > list.clearSelection(); >+ list.addItemToSelection(list.childNodes[position + 1]); list.addItemToSelection(list.childNodes[position]); > updateViewPosition(1); Doesn't this need to be changed too? Not had chance to test on SM yet.
Attached patch patch v2 (obsolete) — Splinter Review
Good catches Ian, thanks!
Attachment #611175 - Attachment is obsolete: true
Attachment #613744 - Flags: ui-review?(bwinton)
Attachment #611175 - Flags: ui-review?(bwinton)
Attachment #611175 - Flags: feedback?(iann_bugzilla)
Comment on attachment 613744 [details] [diff] [review] patch v2 I still need to test against SM, so setting f?
Attachment #613744 - Flags: feedback?(iann_bugzilla)
Comment on attachment 613744 [details] [diff] [review] patch v2 Yeah, I like the way this works. ui-r=me! :) Thanks, Blake.
Attachment #613744 - Flags: ui-review?(bwinton) → ui-review+
Ian?
Comment on attachment 613744 [details] [diff] [review] patch v2 Doesn't break SM for me -> f=me
Attachment #613744 - Flags: feedback?(iann_bugzilla) → feedback+
Simple port for SM
Attachment #617734 - Flags: review?(mnyromyr)
Attachment #613744 - Flags: review?(mconley)
Comment on attachment 613744 [details] [diff] [review] patch v2 Review of attachment 613744 [details] [diff] [review]: ----------------------------------------------------------------- I see nothing wrong with this. Great job, aceman!
Attachment #613744 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Whiteboard: [it is OK to check-in the TB patch first, but leave the bug open until SM patch is checked-in]
aceman: Oh - I forgot - I experienced a tiny bit of bitrot when applying your patch. Nothing too big, but I think your patch needs to be updated. -Mike
Yeah, as always, I bitrotted myself ;)
Attachment #613744 - Attachment is obsolete: true
Attachment #618426 - Flags: review+
Keywords: checkin-needed
Whiteboard: [it is OK to check-in the TB patch first, but leave the bug open until SM patch is checked-in]
Target Milestone: --- → Thunderbird 15.0
Attachment #618426 - Attachment description: patch v3 for TB → [checked in] patch v3 for TB
Maybe I could also use list.currentIndex in TB :) I'll do it in bug 749096.
Blocks: 749096
Comment on attachment 617734 [details] [diff] [review] Do it for SM too [Checked in: Comment 34] Just some nits. >+ var position = gFilterTree.currentIndex; >+ position = position > 0 ? position : 0; You could just say var position = Math.max(gFilterTree.currentIndex, 0); Or at least only assign a new valeue if it changes. >+ var args = {filterList: curFilterList, >+ filterPosition: position, refresh: false}; No need to wrap here. r/moa=me with that.
Attachment #617734 - Flags: superreview+
Attachment #617734 - Flags: review?(mnyromyr)
Attachment #617734 - Flags: review+
(In reply to Karsten Düsterloh from comment #32) > Comment on attachment 617734 [details] [diff] [review] > Do it for SM too > > Just some nits. > > >+ var position = gFilterTree.currentIndex; > >+ position = position > 0 ? position : 0; > > You could just say > var position = Math.max(gFilterTree.currentIndex, 0); > Or at least only assign a new valeue if it changes. Will do the Math.max > > >+ var args = {filterList: curFilterList, > >+ filterPosition: position, refresh: false}; > > No need to wrap here. It's over 80 characters, so there is a need.
Comment on attachment 617734 [details] [diff] [review] Do it for SM too [Checked in: Comment 34] Checked in with Math.max: http://hg.mozilla.org/comm-central/rev/21373a678b46
Attachment #617734 - Attachment description: Do it for SM too → Do it for SM too [Checked in: Comment 34]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: