Last Comment Bug 103684 - RFE: Implement direct ordering of filters (insert new filter at the current position / above the selected filter)
: RFE: Implement direct ordering of filters (insert new filter at the current p...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- enhancement with 7 votes (vote)
: Thunderbird 15.0
Assigned To: :aceman
:
Mentors:
: 115155 358641 462779 679586 739984 755613 (view as bug list)
Depends on:
Blocks: 749096
  Show dependency treegraph
 
Reported: 2001-10-08 11:28 PDT by David A. Cobb
Modified: 2012-08-18 07:34 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch (4.02 KB, patch)
2012-03-31 10:44 PDT, :aceman
no flags Details | Diff | Review
patch v2 (4.18 KB, patch)
2012-04-10 13:42 PDT, :aceman
mconley: review+
bwinton: ui‑review+
iann_bugzilla: feedback+
Details | Diff | Review
Do it for SM too [Checked in: Comment 34] (1.31 KB, patch)
2012-04-23 18:45 PDT, Ian Neal
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Review
[checked in] patch v3 for TB (4.22 KB, patch)
2012-04-25 14:05 PDT, :aceman
acelists: review+
Details | Diff | Review

Description David A. Cobb 2001-10-08 11:28:53 PDT
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 2001-10-20 10:14:59 PDT
I like the idea of inserting after the currently selected one.  
Comment 2 Andrew M. Shaw 2002-03-22 09:38:58 PST
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 seth@sspitzer.org instead) 2003-05-08 10:35:06 PDT
mass re-assign.
Comment 4 Rod Carty 2003-06-09 22:19:08 PDT
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 2005-05-23 19:08:54 PDT
*** Bug 115155 has been marked as a duplicate of this bug. ***
Comment 6 (not reading, please use seth@sspitzer.org instead) 2007-06-21 14:47:01 PDT
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 7 Wayne Mery (:wsmwk, NI for questions) 2009-12-14 07:01:18 PST
*** Bug 462779 has been marked as a duplicate of this bug. ***
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2009-12-14 07:01:39 PST
*** Bug 358641 has been marked as a duplicate of this bug. ***
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2011-08-22 09:36:00 PDT
*** Bug 679586 has been marked as a duplicate of this bug. ***
Comment 10 :aceman 2012-03-29 05:57:28 PDT
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 11 :aceman 2012-03-29 05:57:48 PDT
*** Bug 739984 has been marked as a duplicate of this bug. ***
Comment 12 ffux 2012-03-29 06:14:02 PDT
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 2012-03-29 08:57:27 PDT
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 2012-03-29 10:52:39 PDT
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 2012-03-29 16:14:05 PDT
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 2012-03-30 21:11:24 PDT
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 2012-03-31 04:40:14 PDT
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 2012-03-31 10:44:19 PDT
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).
Comment 19 :aceman 2012-03-31 10:47:05 PDT
(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 2012-04-08 16:24:03 PDT
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 2012-04-10 13:42:53 PDT
Created attachment 613744 [details] [diff] [review]
patch v2

Good catches Ian, thanks!
Comment 22 Ian Neal 2012-04-10 16:02:05 PDT
Comment on attachment 613744 [details] [diff] [review]
patch v2

I still need to test against SM, so setting f?
Comment 23 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-04-17 11:52:58 PDT
Comment on attachment 613744 [details] [diff] [review]
patch v2

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

Thanks,
Blake.
Comment 24 :aceman 2012-04-18 02:42:35 PDT
Ian?
Comment 25 Ian Neal 2012-04-23 18:44:14 PDT
Comment on attachment 613744 [details] [diff] [review]
patch v2

Doesn't break SM for me -> f=me
Comment 26 Ian Neal 2012-04-23 18:45:14 PDT
Created attachment 617734 [details] [diff] [review]
Do it for SM too [Checked in: Comment 34]

Simple port for SM
Comment 27 Mike Conley (:mconley) - (Away until June 29th) 2012-04-25 13:35:27 PDT
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) - (Away until June 29th) 2012-04-25 13:55:31 PDT
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 2012-04-25 14:05:26 PDT
Created attachment 618426 [details] [diff] [review]
[checked in] patch v3 for TB

Yeah, as always, I bitrotted myself ;)
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-04-25 16:25:46 PDT
http://hg.mozilla.org/comm-central/rev/70892b85ee8a
Comment 31 :aceman 2012-04-26 13:16:05 PDT
Maybe I could also use list.currentIndex in TB :)
I'll do it in bug 749096.
Comment 32 Karsten Düsterloh 2012-04-26 14:22:34 PDT
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 2012-04-28 06:20:46 PDT
(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 2012-04-29 08:21:47 PDT
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
Comment 35 :aceman 2012-05-16 05:29:28 PDT
*** Bug 755613 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.