Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Filters
--
enhancement
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: David A. Cobb, Assigned: aceman)

Tracking

Trunk
Thunderbird 15.0

SeaMonkey Tracking Flags

(seamonkey2.12 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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).

Updated

16 years ago
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

16 years ago
I like the idea of inserting after the currently selected one.  
Target Milestone: --- → mozilla1.0.1

Comment 2

16 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.
mass re-assign.
Assignee: naving → sspitzer

Comment 4

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

Comment 5

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

Updated

10 years ago
OS: Windows 98 → All
QA Contact: laurel → filters
Hardware: PC → All
Product: Core → MailNews Core
Duplicate of this bug: 462779
Duplicate of this bug: 358641
Duplicate of this bug: 679586
(Assignee)

Comment 10

5 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 → ---
(Assignee)

Updated

5 years ago
Duplicate of this bug: 739984

Comment 12

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
Created attachment 611175 [details] [diff] [review]
patch

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

5 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

5 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

5 years ago
Created attachment 613744 [details] [diff] [review]
patch v2

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

5 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 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

5 years ago
Ian?

Comment 25

5 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+

Comment 26

5 years ago
Created attachment 617734 [details] [diff] [review]
Do it for SM too [Checked in: Comment 34]

Simple port for SM
Attachment #617734 - Flags: review?(mnyromyr)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 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]
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

5 years ago
Created attachment 618426 [details] [diff] [review]
[checked in] patch v3 for TB

Yeah, as always, I bitrotted myself ;)
Attachment #613744 - Attachment is obsolete: true
Attachment #618426 - Flags: review+
http://hg.mozilla.org/comm-central/rev/70892b85ee8a
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
(Assignee)

Comment 31

5 years ago
Maybe I could also use list.currentIndex in TB :)
I'll do it in bug 749096.
Blocks: 749096

Comment 32

5 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

5 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

5 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]

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-seamonkey2.12: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 755613
You need to log in before you can comment on or make changes to this bug.