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

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[David A. Cobb]

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).

Comment 1

[scottputterman]

I like the idea of inserting after the currently selected one.  

Comment 2

[Andrew M. Shaw]

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.

Comment 3

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 4

[Rod Carty]

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.

Comment 5

[Mike Cowperthwaite]

*** Bug 115155 has been marked as a duplicate of this bug. ***

Comment 6

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 10

[:aceman]

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.

Comment 12

[ffux]

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

Comment 13

[:aceman]

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

[Frederick Sieber]

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

[Brian Hauer]

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

[John David Galt]

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.

Comment 17

[:aceman]

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.

Comment 18

[:aceman]

Attachment: 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).

Comment 19

[:aceman]

(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 ;)

Comment 20

[Ian Neal]

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.

Comment 21

[:aceman]

Attachment: patch v2

Good catches Ian, thanks!

Comment 22

[Ian Neal]

Comment on attachment 613744 [details] [diff] [review]
patch v2

I still need to test against SM, so setting f?

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 613744 [details] [diff] [review]
patch v2

Yeah, I like the way this works.  ui-r=me!  :)

Thanks,
Blake.

Comment 24

[:aceman]

Ian?

Comment 25

[Ian Neal]

Comment on attachment 613744 [details] [diff] [review]
patch v2

Doesn't break SM for me -> f=me

Comment 26

[Ian Neal]

Attachment: Do it for SM too [Checked in: Comment 34]

Simple port for SM

Comment 27

[Mike Conley (: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!

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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

Comment 29

[:aceman]

Attachment: [checked in] patch v3 for TB

Yeah, as always, I bitrotted myself ;)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/70892b85ee8a

Comment 31

[:aceman]

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

Comment 32

[Karsten Düsterloh]

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.

Comment 33

[Ian Neal]

(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

[Ian Neal]

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