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)
MailNews Core
Filters
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)
1.31 KB,
patch
|
mnyromyr
:
review+
mnyromyr
:
superreview+
|
Details | Diff | Splinter Review |
4.22 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•23 years ago
|
||
I like the idea of inserting after the currently selected one.
Target Milestone: --- → mozilla1.0.1
Comment 2•23 years ago
|
||
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.
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.
Updated•20 years ago
|
Product: MailNews → Core
Comment 5•19 years ago
|
||
*** Bug 115155 has been marked as a duplicate of this bug. ***
Comment 6•17 years ago
|
||
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
Updated•17 years ago
|
OS: Windows 98 → All
QA Contact: laurel → filters
Hardware: PC → All
Updated•16 years ago
|
Product: Core → MailNews Core
Assignee | ||
Comment 10•13 years ago
|
||
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 → ---
Comment 12•13 years ago
|
||
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
Assignee | ||
Comment 13•13 years ago
|
||
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.
Comment 14•13 years ago
|
||
This is VERY related (almost identical) to bug 256582. Enjoy the discussions there. IF there are enough discussions, maybe someone will actually DO something.
Comment 15•13 years ago
|
||
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!
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
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)
Assignee | ||
Comment 19•13 years ago
|
||
(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 20•13 years ago
|
||
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.
Assignee | ||
Comment 21•13 years ago
|
||
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 22•13 years ago
|
||
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 23•12 years ago
|
||
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+
Assignee | ||
Comment 24•12 years ago
|
||
Ian?
Comment 25•12 years ago
|
||
Comment on attachment 613744 [details] [diff] [review]
patch v2
Doesn't break SM for me -> f=me
Attachment #613744 -
Flags: feedback?(iann_bugzilla) → feedback+
Attachment #613744 -
Flags: review?(mconley)
Comment 27•12 years ago
|
||
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]
Comment 28•12 years ago
|
||
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
Assignee | ||
Comment 29•12 years ago
|
||
Yeah, as always, I bitrotted myself ;)
Attachment #613744 -
Attachment is obsolete: true
Attachment #618426 -
Flags: review+
Comment 30•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #618426 -
Attachment description: patch v3 for TB → [checked in] patch v3 for TB
Assignee | ||
Comment 31•12 years ago
|
||
Maybe I could also use list.currentIndex in TB :)
I'll do it in bug 749096.
Blocks: 749096
Comment 32•12 years ago
|
||
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+
Comment 33•12 years ago
|
||
(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 34•12 years ago
|
||
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
status-seamonkey2.12:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•