Last Comment Bug 315367 - Filters' Folder picker ['Copy To' and 'Move To'] is unusable with large numbers of folders
: Filters' Folder picker ['Copy To' and 'Move To'] is unusable with large numbe...
Status: RESOLVED FIXED
[filter-mgmt]
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- major with 4 votes (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
: 466247 (view as bug list)
Depends on:
Blocks: 808524 808974
  Show dependency treegraph
 
Reported: 2005-11-07 01:37 PST by Jay Rossiter
Modified: 2012-11-13 04:21 PST (History)
16 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Which folder is which? (95.02 KB, image/jpeg)
2005-12-22 01:13 PST, Marcus Münch
no flags Details
patch (3.96 KB, patch)
2012-10-19 12:46 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (3.94 KB, patch)
2012-10-22 14:21 PDT, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
Screenshot of narrow picker (38.52 KB, image/png)
2012-10-23 13:07 PDT, Ian Neal
no flags Details
patch v3 (4.07 KB, patch)
2012-10-23 13:49 PDT, :aceman
iann_bugzilla: review+
rkent: review-
Details | Diff | Splinter Review
patch v4 (6.04 KB, patch)
2012-10-26 13:15 PDT, :aceman
rkent: review+
standard8: approval‑comm‑beta-
Details | Diff | Splinter Review
reduced patch for TB17 (2.70 KB, patch)
2012-11-05 12:19 PST, :aceman
bwinton: ui‑review-
rkent: feedback-
Details | Diff | Splinter Review

Description Jay Rossiter 2005-11-07 01:37:34 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

The pre-expanded folder selection dropdown added in Thunderbird 1.5 is unusable when dealing with large numbers of folders, or across multiple accounts.  It is incredibly difficult to locate a single folder when looking at a list of hundreds.

In short:  The old folder selection was much more user friendly

Reproducible: Always

Steps to Reproduce:
Create a mail folder structure that contains several (10) top-level folders with multiple folder trees underneath.  (Mailing Lists -> Tech -> HTML, etc.)
Select a message
Right click on email address
Create Filter from Message
Attempt to select a folder from the middle of the folder structure.
Actual Results:  
The UI is user-hostile; difficult to navigate accurately, impossible to add folders to the tree.  (See bug 308660)


Marked as Major because for users with large numbers of folders filter creation is unusable.
Comment 1 Marcus Münch 2005-12-22 01:13:05 PST
Created attachment 206579 [details]
Which folder is which?

It is not only the amount of folders. If you use a folder structure which has a depth of 4 or more, you have no chance to decide, which folder is the one you want to select (see attached screenshot, done with TB 1.5 RC2 german).
And: Since 1.5RC1 (did not use earlier versions of 1.5) I am missing the possibility to create a new folder while creating a filter rule.
So the folder handling within filters has gone really bad.
Comment 2 Mike Cowperthwaite 2006-01-06 11:59:10 PST
I'm not a fan of the new picker either.

The width problem from comment 1 might be mitigated a bit by widening the filter-rules dialog -- the picker width expands along with the dialog, to a limit.

Moving this to Mail backend, following bug 302120.  (The plan is to use this 
new picker in all places where a folder is selected, not just filters.)
Comment 3 Marcus Münch 2006-01-06 12:10:21 PST
"The width problem from comment 1 might be mitigated a bit by widening the
filter-rules dialog -- the picker width expands along with the dialog, to a
limit."

This does not work for me on Windows XP. When I change the width of the dialog, the width of the picker does not change.
Comment 4 Mike Cowperthwaite 2006-01-06 13:07:39 PST
(In reply to comment #3)
> When I change the width of the dialog,
> the width of the picker does not change.

That may be an issue with the theme you're using.
Comment 5 Jay Rossiter 2006-01-06 14:40:31 PST
Seeing this new folder picker added to everything might just start my mind towards moving back to another email client.  It's completely unusable.  Having the entire folder structure pre-expanded is ridiculous - why not just force everyone to have their folders expanded in the mail window as well?  It's the same logic.  

If there are folders I rarely use (such as for family members, mailing lists with infrequent posts, etc.), I sure as hell don't want to have to sift through them all just to find the folder I'm looking for.
Comment 6 Marcus Münch 2006-01-07 00:05:40 PST
To comment 4: The size was an issue of the theme (Thunderstripe 1.5c), but even with the default theme the width of the folderpicker as shown in the screenshot is not sizeable under Windows XP. I did not check other platforms so far.

But I agree with comment 5, preexpanding all folders, in all accounts, is really not a good idea! When you want to create a new filter for the account that is the last in a large list, you have to croll through all folders of all accounts above, which can be a lot. This maybe a small step for the programmer, but is really a big step backwards in useability. On Mac I might also decide to switch to Apple Mail, on Linux to Evolution, but on Windows I see no real alternative. 
Please change this back to the old folder picker, which was really fast to use.
Comment 7 eric 2006-01-07 13:40:23 PST
(In reply to comment #6)
I would prefer the old picker as well. This picker is really tedious and I lose information when names are long or depth gets past several. This picker was not well thought out. Why remove a good thing? 
Comment 8 R. Landfill 2006-02-21 08:51:17 PST
The new folder picker is terrible. At least be able to set some sort of option to go back to the old, un-expanded version. 
Comment 9 Paul Swan 2008-02-25 15:00:46 PST
Just checking to see if this is still a concern.  I tested the latest 2.0.0.13 nightly as well as a Thunderbird 3 build, and this still appears to be the case with the folder picker.  It makes setting up filter rules quite difficult when dealing with an install that has a large number of folders.
Comment 10 Jay Rossiter 2009-02-21 16:49:10 PST
I haven't even thought about this in quite a while since I switched to IMAP and procmail rules, but yes... it's definitely still a usability issue in my mind.  

The folder picker for my setup is ten "pages" (via pagedown) long.
Comment 11 Marcus Münch 2009-04-08 06:02:04 PDT
Just to keep this bug alive: Any work on this issue?
Comment 12 Jens Müller (:tessarakt) 2011-02-27 04:36:54 PST
(In reply to comment #6)
> When you want to create a new filter for the account
> that is the last in a large list, you have to croll through all folders of all
> accounts above, which can be a lot.

Well, IMO finding a folder in the middle of the list is even worse. Just scrolling to the end is easy compared to that ...

If there are other folder pickers which can be used instead, why not just switch to them to have an easy solution for this annoying UI bug?
Comment 13 Jens Müller (:tessarakt) 2011-02-27 04:39:42 PST
Can someone please update the summary to include the word "filter" in there? Just to distinguish from the Move/Copy to folder picker in a message's context menu.
Comment 14 Wayne Mery (:wsmwk, NI for questions) 2011-10-24 14:46:08 PDT
since you all understand the history here -- which bug # caused this regression?

And, I may be naive, but comment 1/attachment 206579 [details] (which needs a separate bug unless we revert to the old picker, or similar flyout model) seems a FAR more severe problem than comment 0.


(In reply to Jens Müller from comment #12)that ...
> 
> If there are other folder pickers which can be used instead, why not just
> switch to them to have an easy solution for this annoying UI bug?

What about the model used in "recent folders"?
Comment 15 Ludovic Hirlimann [:Usul] 2011-11-02 06:20:11 PDT
*** Bug 466247 has been marked as a duplicate of this bug. ***
Comment 16 :aceman 2012-04-17 03:32:42 PDT
What about using the pickers used in Account manager / copies&folders (in the "other" fields)? Maybe I could copy that.
Comment 17 Jens Müller (:tessarakt) 2012-04-17 03:53:40 PDT
(In reply to :aceman from comment #16)
> What about using the pickers used in Account manager / copies&folders (in
> the "other" fields)? Maybe I could copy that.

Looks similar or even the same as the one in the "Copy to"/"Move to" menus. IMO a good solution.
Comment 18 Axel Grude [:realRaven] 2012-04-17 11:27:29 PDT
(In reply to Jens Müller from comment #17)
> (In reply to :aceman from comment #16)
> > What about using the pickers used in Account manager / copies&folders (in
> > the "other" fields)? Maybe I could copy that.
> 
> Looks similar or even the same as the one in the "Copy to"/"Move to" menus.
> IMO a good solution.

Yes! I think a collapsible treeview would be much much better design than a flat list. From 150 folders upwards the move To dropdown becomes fairly painful, to the point where it discourages me from creating filters. (In fact I based a whole functionality in QuickFolders on a workaround - a wizard that pre-populates the New Filter dialog based on a mail-drag-and-drop operation and a tempolate pick. Since the user and the moved email have given pretty much all the information including target folder, subject, senders etc. it is easy to build fairly complex filter logic on these 2 simple actions).
Comment 19 :aceman 2012-10-17 06:40:41 PDT
OK, I have a WIP patch ready, will upload it soon.
Comment 20 :aceman 2012-10-19 12:46:46 PDT
Created attachment 673376 [details] [diff] [review]
patch

Bwinton, please see if the styling of the picker is OK on all platforms. It seems fine on Linux (all levels of the menu are the same solid grey color).

I add new string into msgFolderPickerOverlay.dtd for the "recent" label. Do we actually want this item in the picker?

I find it strange that the recentLabel must be passed as attribute to the folder picker element:
http://mxr.mozilla.org/comm-central/search?string=recentLabel&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Do we expect it to have different values depending on context? (It does not in en-US). If not, why does folderWidgets.xml not import msgFolderWidgetsOverlay.dtd and fetch the entity itself?
Comment 21 Blake Winton (:bwinton) (:☕️) 2012-10-22 13:10:47 PDT
Comment on attachment 673376 [details] [diff] [review]
patch

Seems good on Mac, and Windows 7.  ui-r=me.

Thanks,
Blake.
Comment 22 Ian Neal 2012-10-22 14:01:19 PDT
Comment on attachment 673376 [details] [diff] [review]
patch

You bitrotted yourself with Bug 777287
Comment 23 :aceman 2012-10-22 14:21:40 PDT
Created attachment 674014 [details] [diff] [review]
patch v2
Comment 24 Ian Neal 2012-10-23 10:53:20 PDT
Comment on attachment 674014 [details] [diff] [review]
patch v2

Prior to this patch the second drop down would have the first item in the menu pre-selected (even if it was just the default account name). Now there is just a very narrow (height-wise) picker which probably doesn't look very good to the average user.
This picker should at least be of normal height. Whether it should display the default account name or just be blank I leave up to ui-review.
Comment 25 :aceman 2012-10-23 11:50:09 PDT
Sorry I do not understand what is the problem. Can you attach a screenshot?
Comment 26 Ian Neal 2012-10-23 13:07:22 PDT
Created attachment 674344 [details]
Screenshot of narrow picker
Comment 27 :aceman 2012-10-23 13:33:07 PDT
The initial widget should look (and be initialized) the same way as before the patch. Only when the menu is expanded the new picker is shown.

I see what you do now. You create a new filter. I confirm I see the problem there. It worked fine for me when editing existing filters.
I'll look into that.
Comment 28 :aceman 2012-10-23 13:49:58 PDT
Created attachment 674370 [details] [diff] [review]
patch v3

This should be better. Preselect the folder from the gFilterList.
Leaving that value in the picker will cause an error dialog when attempting to save the filter. As it was before.
Comment 29 :aceman 2012-10-23 14:45:47 PDT
Comment on attachment 674370 [details] [diff] [review]
patch v3

Thanks for the catch, Ian.
Comment 30 Kent James (:rkent) 2012-10-24 22:57:16 PDT
Comment on attachment 674370 [details] [diff] [review]
patch v3

This change breaks the saving of folder when used by a custom filter action. You can see this by loading the FiltaQuilla addon, enabling the Move Later filter action, then trying to set the folder. It changes, but does not get saved.

I seem to be the last person to change some of these lines, to support custom filter actions, and the change was to add the code that set the 'value' attribute. That was a few years ago, I really don't remember what the issues were anymore, but somehow I recall that custom filter actions rely on the 'value' to get a consistently usable string for purposes of setting action values.

I'll be traveling until 2012-11-01 BTW and will be mostly unavailable during that time.
Comment 31 :aceman 2012-10-24 23:44:56 PDT
I understand. See the "saveToFilter" method.
If the filter is nsMsgFilterAction.CopyToFolder or nsMsgFilterAction.MoveToFolder then it gets the real value from the "uri" attribute (which the new picker fills in so I did not notice any problem).
In the nsMsgFilterAction.Custom case it gets the value to save from this.getAttribute('value'). In my patch 'value' was not synchronized to 'uri' as it was before.

Would you be OK with removing the 'uri' usage and also use 'value' in the CopyToFolder and MoveToFolder case? Notice in "initWithAction" that the folder URI is already put into the 'value' first. If we were saving from 'value' already, I would spot the problem as Move/CopyToFolder would not work.

Or alternatively we could keep using 'uri' (and ignore 'value' even more in case of Move/CopyToFolder) but you would need to change FiltaQuilla, special case 'move later' to use 'uri', not 'value'.
Comment 32 :aceman 2012-10-25 00:20:12 PDT
(It is not this.getAttribute('value'), but:
var actionTarget = document.getAnonymousNodes(this)[1];
var actionItem = document.getAnonymousNodes(actionTarget);
actionItem[0].value;)
Comment 33 :aceman 2012-10-26 13:15:30 PDT
Created attachment 675664 [details] [diff] [review]
patch v4
Comment 34 Kent James (:rkent) 2012-11-01 09:31:24 PDT
(In reply to :aceman from comment #31)

OK I'm back now after my trip. Looking at the patch, but here responding to this comment:

> Or alternatively we could keep using 'uri' (and ignore 'value' even more in
> case of Move/CopyToFolder) but you would need to change FiltaQuilla, special
> case 'move later' to use 'uri', not 'value'.

Although FiltaQuilla is the primary (but not the only) user of custom actions, the issue is not FiltaQuilla, but the intended API for the custom action. The intended API is that the action value is a string that is obtained from the "value" attribute. So I think you should continue to support that in changes in the filter code. 

In the original patch for custom actions, the existing
Comment 35 Kent James (:rkent) 2012-11-01 10:32:15 PDT
Comment on attachment 675664 [details] [diff] [review]
patch v4

OK this looks good. Thanks!
Comment 36 :aceman 2012-11-02 00:38:26 PDT
Yes, the patch v4 prefers 'value' and suppresses 'uri' so that it is more visible that 'value' is important and used.
Comment 37 Ryan VanderMeulen [:RyanVM] 2012-11-02 13:01:24 PDT
https://hg.mozilla.org/comm-central/rev/22d170a632be
Comment 38 Charles 2012-11-03 06:57:56 PDT
First, I'm really glad to see this fixed, so many thanks aceman!

But... I'm sad to see that it isn't going to make it into ESR 17 (we are forced to stick with ESR since we will be using SOGo soon and SOGo only supports ESR)...

Are these chances really so invasive that they cannot be added to 17?
Comment 39 :aceman 2012-11-03 14:32:09 PDT
I think the changes are very small. But there is a new string so pushing it to beta may be a problem. But you can try nominating it.
Comment 40 Charles 2012-11-04 06:31:11 PST
I'll be happy to! But... I don't know how... is the process documented somewhere?

Thanks again aceman!
Comment 41 :aceman 2012-11-04 06:34:22 PST
Set the approval‑comm‑beta? flag on the 'patch v4' attachment.
Comment 42 Charles 2012-11-04 06:41:56 PST
Comment on attachment 675664 [details] [diff] [review]
patch v4

Review of attachment 675664 [details] [diff] [review]:
-----------------------------------------------------------------

approval‑comm‑beta?
Comment 43 Charles 2012-11-04 06:42:37 PST
Not sure I did that right... ? :(
Comment 44 :aceman 2012-11-04 07:09:58 PST
Not right.
But what is 'SOGo' ?
Comment 45 Tony Mechelynck [:tonymec] 2012-11-04 10:55:43 PST
(In reply to Charles from comment #43)
> Not sure I did that right... ? :(

To do it right, open the "Details" view of the attachment, unroll the "approval-comm-beta" and/or "approval-comm-aurora" rolldown, and set it to "?". Optionally (but recommended) enter the Bugzilla email-address-of-record of the person who is to decide whether or not to grant approval, and add him (or her) to the Cc list (if not already listed).

To find out who is best placed to grant (or deny, as the case may be) approval, see the appropriate subpage of https://wiki.mozilla.org/Modules — in this case probably the "MailNews Core" subpage.
Comment 46 Kent James (:rkent) 2012-11-04 16:37:26 PST
You could do a version of the patch that uses "Recent" from one of the existing locations for folder menus and avoid the string change if this was to land in beta.
Comment 47 :aceman 2012-11-04 23:51:28 PST
Thanks, that is a possibility.

Does this now allow to remove the "folderTargetPopup" class and binding as found here: http://mxr.mozilla.org/comm-central/search?string=folderTargetPopup&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central ? It seems unused in SM and TB.
Comment 48 neil@parkwaycc.co.uk 2012-11-05 01:44:33 PST
(In reply to aceman from comment #47)
> Does this now allow to remove the "folderTargetPopup" class and binding
Yes, they were only added by bug 294094 because that was the only way at the time to reliably show the folders in a listitem.

Note that you are not properly setting the class on the popups, they should be class="menulist-menupopup" like they are for the Copies & Folders menulists.
Comment 49 :aceman 2012-11-05 03:36:14 PST
Thanks, I make a new bug for it.
Thanks for the class="menulist-menupopup", I asked about the styling in comment 20 :) At least on windows this will make all the submenus the same white background as the first level (accounts). Only the Recent submenu is still grey. Is that intentional or a bug?
Comment 50 neil@parkwaycc.co.uk 2012-11-05 04:00:02 PST
(In reply to aceman from comment #49)
> Thanks for the class="menulist-menupopup", I asked about the styling in
> comment 20 :) At least on windows this will make all the submenus the same
> white background as the first level (accounts).
Not just the background, IIRC the padding is different too.

> Only the Recent submenu is still grey. Is that intentional or a bug?
I don't actually know how the class is applied to the menupopups. I just know that the copies & folders ones have them and yours don't.
Comment 51 :aceman 2012-11-05 04:05:17 PST
It seems the submenus get the class set from the parent menu:
http://hg.mozilla.org/comm-central/file/71adceeda938/mailnews/base/content/folderWidgets.xml#l442

But that does not happen for the Recent submenu.
Comment 52 neil@parkwaycc.co.uk 2012-11-05 04:17:00 PST
Indeed, the Recent menupopup is created 130 lines later, and doesn't bother to copy its parent's class, presumably because nobody needed it to before.
Comment 53 Tony Mechelynck [:tonymec] 2012-11-05 07:06:05 PST
ISTR that one of popup and menupopup is deprecated (and gets transparent background) and the other not (and gets opaque background) but I can't repember which is which. Don't know if relevant.
Comment 54 :aceman 2012-11-05 12:19:38 PST
Created attachment 678419 [details] [diff] [review]
reduced patch for TB17

What about a reduced version with less changes and without the Recent menu? It is not strictly necessary, just convenient.
Comment 55 Kent James (:rkent) 2012-11-05 12:59:13 PST
Comment on attachment 678419 [details] [diff] [review]
reduced patch for TB17

I think I mislead you about my thoughts with my comment 46. I was just mentioning what was possible, not what I thought was wise.

I am not the release driver, but I would be very reluctant if I were to accept a patch like this 2 weeks before an important release. There should be a very high bar for a fix to be accepted this late, and I do not think this bug is important enough to meet that bar. If I were to accept it, I would want the patch to be as close as possible to what landed on trunk, and not the reduced patch that you have here. So I would prefer the original patch, but just eliminate the string change by using an existing string.

But this is now in the realm of release management, not code review, so I'm going to just give my opinion as feedback-  Feel free to ask Standard8 for an opinion, I'm happy to do whatever he wants.

If the fix is desirable for some group of users, this could be accomplished through an extension at lower risk.
Comment 56 Blake Winton (:bwinton) (:☕️) 2012-11-12 08:15:33 PST
Comment on attachment 678419 [details] [diff] [review]
reduced patch for TB17

Yeah, I think I agree with Kent.  If you think this should get in a newer release, let's nominate the patch for approval, and then talk to Standard8 about what we should add/remove from it to make it acceptable.

Thanks,
Blake.
Comment 57 :aceman 2012-11-12 10:26:47 PST
Comment on attachment 675664 [details] [diff] [review]
patch v4

[Approval Request Comment]

This patch would be ready for TB17, except for the string change.
I see 2 options:
1. Import whole messenger.dtd into the FilterDialog.xul to get the "Recent" string. But I find this ugly.
2. Drop the "Recent" submenu from the picker. But I think it is useful in this specific dialog.

So I don't know which one is worse.
Comment 58 Mark Banner (:standard8) 2012-11-13 04:21:41 PST
Comment on attachment 675664 [details] [diff] [review]
patch v4

Realistically, this is just too late. We're less than a few hours away from building the final beta, and whilst I realise this can be a significant bug for people, the age of the issue, amount of reports etc, all mean that I don't think we would hold the release for this bug.

I'd also be concerned about breaking extensions at this stage - I don't know if any of them play around with the interfaces on that binding, but if they do, we may be breaking them very late in the game.

If we're able to have a patch that doesn't change strings and doesn't break extensions, we may be able to consider it for a post-17 release.

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