95.02 KB, image/jpeg
38.52 KB, image/png
6.04 KB, patch
|Details | Diff | Splinter Review|
2.70 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.)
"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.
(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.
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.
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.
(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?
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.
Just checking to see if this is still a concern. I tested the latest 18.104.22.168 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.
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.
Just to keep this bug alive: Any work on this issue?
(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?
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.
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"?
What about using the pickers used in Account manager / copies&folders (in the "other" fields)? Maybe I could copy that.
(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.
(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).
OK, I have a WIP patch ready, will upload it soon.
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 on attachment 673376 [details] [diff] [review] patch Seems good on Mac, and Windows 7. ui-r=me. Thanks, Blake.
Created attachment 674014 [details] [diff] [review] patch v2
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.
Sorry I do not understand what is the problem. Can you attach a screenshot?
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.
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 on attachment 674370 [details] [diff] [review] patch v3 Thanks for the catch, Ian.
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.
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'.
(It is not this.getAttribute('value'), but: var actionTarget = document.getAnonymousNodes(this); var actionItem = document.getAnonymousNodes(actionTarget); actionItem.value;)
Created attachment 675664 [details] [diff] [review] patch v4
(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 on attachment 675664 [details] [diff] [review] patch v4 OK this looks good. Thanks!
Yes, the patch v4 prefers 'value' and suppresses 'uri' so that it is more visible that 'value' is important and used.
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?
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.
I'll be happy to! But... I don't know how... is the process documented somewhere? Thanks again aceman!
Set the approval‑comm‑beta? flag on the 'patch v4' attachment.
Comment on attachment 675664 [details] [diff] [review] patch v4 Review of attachment 675664 [details] [diff] [review]: ----------------------------------------------------------------- approval‑comm‑beta?
Not sure I did that right... ? :(
Not right. But what is 'SOGo' ?
(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.
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.
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.
(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.
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?
(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.
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.
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.
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.
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 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 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 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 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.