Allow 'Pinning' multiple 'Folder Views' to the top of the Folder Pane
Categories
(Thunderbird :: Folder and Message Lists, enhancement, P2)
Tracking
(thunderbird_esr78 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | wontfix |
People
(Reporter: tanstaafl, Assigned: aleca)
References
Details
Attachments
(6 files, 17 obsolete files)
Updated•10 years ago
|
Updated•10 years ago
|
Updated•4 years ago
|
It would be really awesome if this bug got some attention and was implemented!
Assignee | ||
Comment 8•4 years ago
|
||
I'll take a look at this and create some mock-ups so we can find a proper approach to tackle this implementation.
Comment 9•4 years ago
|
||
FWIW, oulook has this (but maybe limited to the favorites view?) - https://cdn.extendoffice.com/images/stories/outlook-tutorials/outlook-folder-pane/doc-outlook-folder-pane-1.png
I think we could also allow our other folder views (see View | Folders), but favourites is likely the most useful one to have in combination with all. That has been a major limitation in the folder views usage. I can never stay in anything except for All, since ever so often I need to find a folder that is not in the specific view. Even if I'd like to mostly look at 5-10 folders for most of the time.
Assignee | ||
Comment 10•4 years ago
|
||
Indeed, this will be very useful.
Comment 11•4 years ago
|
||
For what it's worth, I just add AAA or ** to the beginning of the folder name, and that forces it to the top.
Comment 12•4 years ago
|
||
I think that's what this is trying to avoid - I have seen too much of that ;)
Also, doesn't work if you have say 8 Inboxes at different servers you need to keep an eye on.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 13•4 years ago
|
||
Here's a mock-up to properly prototype this implementation as there are a lot of things to consider before start coding.
Folder pane toolbar
Since we're not using a radio button selection, the pane toolbar doesn't work anymore as it's currently only a menulist which shows the current folder list (All, Favorite, etc.).
I propose to replace it with a simple header container with a option button that opens a popup.
Folder popup
This popup would behave like the main App Menu with the ability to toggle the checkboxes to show/hide different folder lists. The popup shouldn't close when the click happens inside.
I'd like to propose also the unification of the various "Compact" lists into a single toggle which controls all lists at once to simplify a lot our options.
The first menuitem allows users to hide this header area directly from here, without accessing the App Menu.
We should also find a way to allow users to show this header area from this section, but that's for another bug.
How to handle multiple lists at once
If a user decides to show multiple lists, we need some "header dividers" to properly group the different views.
I have a couple of proposals here in case we want to enable the ability to collapse those lists as well.
I personally prefer the example without the chevrons and indentation as it looks cleaner, but I think the ability to collapse those lists might be necessary when implementing the drag&drop to reorder them.
The final example shows a couple of lists with the Folder Pane toolbar hidden.
Drag & Drop to reorder
I will ignore this for now as this patch will be pretty complex already with these changes.
I will create a follow up bug once this lands.
Comment 14•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #13)
Created attachment 9178889 [details]
I propose to replace it with a simple header container with a option button that opens a popup.
Sounds good.
I'd like to propose also the unification of the various "Compact" lists into a single toggle which controls all lists at once to simplify a lot our options.
Personally I'd drop the compact mode altogether. There are really few reasons to have it (collapsed the folders take the same amount of space,minus the one row for which account they are under).
I have a couple of proposals here in case we want to enable the ability to collapse those lists as well.
Maybe we don't need the collapsing.
I personally prefer the example without the chevrons and indentation as it looks cleaner
Agreed.
The final example shows a couple of lists with the Folder Pane toolbar hidden.
Ideally we'd not need this toolbar at all, and find a better way for users to discover how to work with the modes. But I have no suggestions.
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Comment on attachment 9178889 [details]
Folder Pane toolbar.png
Looks good for me. I'm also for without the chevrons and indentation
.
Earlier, the Folder pane toolbar was a real toolbar with the possibility to add buttons etc. but this was removed. Maybe we should remove the toolbar in the name and choose another description...but what, this is the question.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 16•4 years ago
|
||
Personally I'd drop the compact mode altogether. There are really few reasons to have it
Sounds good to me.
Ideally we'd not need this toolbar at all, and find a better way for users to discover how to work with the modes. But I have no suggestions.
Earlier, the Folder pane toolbar was a real toolbar with the possibility to add buttons etc. but this was removed. Maybe we should remove the toolbar in the name and choose another description...but what, this is the question.
We could call it "Folder Header".
I think, at least for now, I'll stick with this paradigm to access and edit the folder pane views.
I'll take some time to explore other approaches, but we could also tackle it in a follow up bug.
Comment 17•4 years ago
|
||
I was the original reporter of this issue/idea, 15 years ago (bug # 313342). So cool to see it coming back to life now there is time / capacity!
I like these new ideas, but have recently wondered if it makes sense to broaden the idea beyond just Folders, and ask:
- what types off 'message views' (a collections of messages, analogous to a folder) can best help users deal with email / messaging management?
- how can we let users customize 'message views' as 'custom Folders' ?
- how can we let users 'organize' custom folders into useful groups (e.g. "Work" folders, "Personal" folders, "Project 1" folders, etc.)
When looked at this way a 'Folder' is just one type of 'message view' (based on the folder a message is actually in). But there are many other useful views, such as these examples:
- all inbox mail from all of my mail accounts
- all 'drafts' sorted by outbound mail account
- all 'unanswered mail' but only from Inboxes on my work account(s)
- all mail I've received from 'testuser@gmail.com' in any of my accounts / local folders
- all mail I've received from 'testuser@gmail.com' in any of my accounts / local / remote folders - plus messages from anyone else who has 'replied' to these messages from 'testuser'
Assignee | ||
Comment 18•4 years ago
|
||
Hi Ian, glad to see you still here after many years, and I'm happy to be fulfilling such old requirements :D
Your questions are very interesting and definitely worth exploring in future bugs.
I'd say, for now, we can "offer" the user some default views, which we can manage and control easily.
The only current customization for those lists is adding or removing a folder to the list of favorite.
Once this lands, and with that the ability to have multiple lists in the same tree pane without affecting performance, we can explore more advanced features like:
- Enabling the creation of "Groups/Containers" as parent lists.
- Reorder these lists with a drag&drop.
- Moving folders from a list to another with a drag&drop.
Creating custom lists and extending the capacity of organizing your folders is definitely something we should slowly implement.
Comment 19•4 years ago
|
||
I have no personal objections to dropping compact - I don't use it. (and in look at it I would probably hate it).
But we have any data that suggests how much it is used or whether some users dislike it? If not I would suggest deferring such a decision until we have telemetry.
Comment 20•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #18)
Hi Ian, glad to see you still here after many years, and I'm happy to be fulfilling such old requirements :D
Your questions are very interesting and definitely worth exploring in future bugs.
I'd say, for now, we can "offer" the user some default views, which we can manage and control easily.
The only current customization for those lists is adding or removing a folder to the list of favorite.Once this lands, and with that the ability to have multiple lists in the same tree pane without affecting performance, we can explore more advanced features like:
- Enabling the creation of "Groups/Containers" as parent lists.
- Reorder these lists with a drag&drop.
- Moving folders from a list to another with a drag&drop.
Creating custom lists and extending the capacity of organizing your folders is definitely something we should slowly implement.
Sounds like a good plan - real progress is better than grand ideas.
FYI just checked and my first bugzilla report was bug # 13610 - 21 years old and still open. Now I feel really old ;-)
Assignee | ||
Comment 21•4 years ago
|
||
All right, this is tricky and it requires a lot of reworking of the gFolderTreeView
.
Currently, we allow only 1 "mode" at a time, and whenever the user changes the mode, the entire tree is rebuilt.
How do we want to handle multiple views?
Should we generate separated tree
elements based on the amount of views the user activates?
Or should we only have 1 tree
element, and add items to it with a "header" separator to distinguish the different views?
Comment 22•4 years ago
|
||
I'd imagine we need one tree per view. The add-on listed above could perhaps give some inspiration [I didn't check].
Comment 23•4 years ago
|
||
This is interesting, and brave in many ways, given that we're struggling even to make accounts re-orderable within one view...
Bug 244347 - Cannot change sort order of accounts
Comment 24•4 years ago
|
||
Alex, FYI!
(In reply to Ian Graham from comment #17)
When looked at this way a 'Folder' is just one type of 'message view' (based on the folder a message is actually in). But there are many other useful views, such as these examples:
Hey Ian, are you aware that Thunderbird already offers most of this in some way?
- all inbox mail from all of my mail accounts
- all 'drafts' sorted by outbound mail account
View > Folders > Unified does just that.
- all 'unanswered mail' but only from Inboxes on my work account(s)
- all mail I've received from 'testuser@gmail.com' in any of my accounts / local folders
- all mail I've received from 'testuser@gmail.com' in any of my accounts / local / remote folders - plus messages from anyone else who has 'replied' to these messages from 'testuser'
For all of these, you can create "Saved Search" folders which is exactly your idea of "message views". You can search for the needle in the hay-stack, cross-folder, even cross-account, and your saved search folder will automatically be populated with all matching messages as they occur. Only it's so hard to discover that newcomers may not even know about it... and the real search power is buried several levels deep (so deep that even I thought it's no longer available). If we could make that more accessible, and maybe give the user more freedom as to where to place those "message view" folders, that might be a great way of making powerful message views available without re-inventing the wheel.
(For a start, it would be cool if I could just rename a saved search folder...)
Try this:
- Ensure View > Folders > All
- right-click on your favorite Account1 in folder pane > Search messages (or Ctrl+Shift+F with that inbox selected)
- [x] Search subfolders
- [Save as Search Folder] (that should really have ellipsis ... in the label!)
--> This will open the "New Saved Search" dialog!
And voilà! Here's a whole new world, the most precise and powerful cross-folder, cross-account search that Thunderbird has to offer (except for fulltext, you'll want Gloda global search for that, but that's way less precise, and not scalable, storable, or attachable to the UI in any way atm).
I have no idea why this nifty power feature isn't more exposed...
It's not actually hard to operate, and even average users may find this useful to sift out important messages without having to move them physically with filters or manually. The obvious advantage is that all folders/accounts will keep their original messages, but you'll still see exactly what you're looking for from all those folders/accounts in one convenient combined view.
Comment 25•4 years ago
|
||
(In reply to Thomas D. (:thomas8) from comment #24)
Created attachment 9180373 [details]
Screenshot 2: Creating a customized message view (aka "Saved Search Folder"): Meet TB's most powerful, precise and versatile search, deeply hidden under the hood
As seen in the screenshot, there's a Choose
button to choose random folders from any accounts to be searched for your combined message view.
Caveat: There's a UI bug in the "Select Folder(s)" dialog: checking or unchecking an account-level folder has no effect, so you have to select the actual content folders like Inbox etc. for your cross-folder/cross-account search to work.
Comment 26•4 years ago
|
||
So here's a cross-account message view ("Saved Search Folder", existing feature) in action:
john.doe@example.com is actually a fake account without any messages, and yet we're seeing messages matching the search criteria gathered from other selected accounts/folders in the search folder's message list.
Obviously, it would be great if such cross-account message views could also be created in an independent location as opposed to inside a certain account, which sort of defeats the idea (depending on user's needs).
(In reply to Thomas D. (:thomas8) from comment #24)
Created attachment 9180373 [details]
Screenshot 2: Creating a customized message view (aka "Saved Search Folder"): Meet TB's most powerful, precise and versatile search, deeply hidden under the hood
Comment 27•4 years ago
|
||
Saved searches are not the same thing. The are message centric but folder-centricity is needed for many workflows. E.g. I have many folders I read, but only at my convenience. Then there are folders I read directly as the mails come in.
Comment 28•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #27)
Saved searches are not the same thing. The are message centric but folder-centricity is needed for many workflows. E.g. I have many folders I read, but only at my convenience. Then there are folders I read directly as the mails come in.
Maybe not exactly the same thing, but very similar. Saved search can easily be used in a folder-centric way to (mix and) mirror folders. You can just create a saved search for all messages of your favorite folder or several folders by just choosing "match all messages" and the respective folders in the saved search setup. So already now, you can mirror any folder of any account as a virtual subfolder of any account. Which achieves almost exactly the "pin-folder-to-top" feature discussed here. Of course, as I mentioned, it would need some polishing and more discoverability.
Comment 29•4 years ago
|
||
I don't know why anyone would like to do that. It sounds confusing and for multi-account setups the virtual folder would still be under the wrong account. Also, then there is no easy possibility to file into subfolders as you go.
Assignee | ||
Comment 30•4 years ago
|
||
Which achieves almost exactly the "pin-folder-to-top" feature discussed here.
That's not what we're dealing with here.
In this bug we're implementing a way to allow users to view multiple "Folder Views" at once. So if they want to see the "Favorite Folders" and "Compact Unread Folders" list at the same time.
We're not implementing any pinning or creation of custom folders.
These are interesting ideas, and as I wrote in Comment 18, it something we will consider in the future.
Assignee | ||
Comment 31•4 years ago
|
||
I'm still working on this as I'm encountering various issues that might affect the way we approach this.
Before moving too forward, I think a bit more of brainstorming to find the optimal approach should be done.
Here's a bunch of questions and issues I stumbled upon while working on this implementation.
Multiple Views Solution
Upon looking at this section, and the way the listed add-on was handling the implementation, going the route of having multiple views seems to be the only acceptable one.
Multiple views means having completely separated XUL tree
elements we can show/hide accordingly.
We reached this solution because a single tree
doesn't offer the flexibility we need, due to the current limitations:
- We can't have identical folders repeated in the same
tree
element (eg. the same account inbox listed in both recent and favourite lists). - We can't create "headers" to separate the different lists (eg. All Folders, Favourite, etc., like in the mock-up).
- We can't easily prevent a drag&drop action between folders in different lists without stumbling upon multiple issues.
Using a multiple view solution with completely separated tree
elements, will remove the issues listed above.
These are the questions/issues I'm encountering with this approach:
- Having all the currently supported modes as standalone
tree
XUL elements in the XHTML file means all of those will be loaded on startup, impacting performance. I think I can solve this by tweaking the load method to not run if the element is hidden. - Our current implementation is limited to 1 folder
tree
element in the entire application, which is referenced by its id "folderTree". Having multiple folder tree views will force us to rewrite/update a lot of parts and all the methods touching thegFolderTreeController
or directly using the "folderTree" id as a selector. This is gonna be pretty big. - How do we handle scrollbars? Should all the view be in the same scrollable area or each view should have its own scrollbar? (I suggest a single scroll area)
- Do we allow manually changing the height of each view with a splitter, or each view will grow in height to show the entire content? (I suggest to go for the latter, as it's easier and similar to what other apps do)
Thoughts, suggestions, or recommended approaches?
Comment 32•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #31)
- How do we handle scrollbars? Should all the view be in the same scrollable area or each view should have its own scrollbar? (I suggest a single scroll area)
I'm also for a single scrollbar if this is possible. See also my answer below.
- Do we allow manually changing the height of each view with a splitter, or each view will grow in height to show the entire content? (I suggest to go for the latter, as it's easier and similar to what other apps do)
Do you mean that the focused tree will grow, when there is not enough space for all contents, or scroll into view? With only one scrollbar it's not possible to use a splitter as hidden content can't be scrolled into view without a second scrollbar. If we have two big trees, both are expanded and the scrollbar scrolls over the total height.
Maybe we should make it possible to set one tree (like the Favourite tree in your Folder Pane toolbar.png) to be unscrollable. But this would only work for smaller trees that don't overflow the view.
Comment 33•4 years ago
|
||
Yes definitely just one scrollbar. Not sure where there would be a splitter except for between the modes, but I think making sizes of the different modes resizeable would likely be quite a mess.
Comment 34•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #31)
- Having all the currently supported modes as standalone
tree
XUL elements in the XHTML file means all of those will be loaded on startup, impacting performance. I think I can solve this by tweaking the load method to not run if the element is hidden.
Sounds like a must. Already now TB startup isn't very fast.
- Our current implementation is limited to 1 folder
tree
element in the entire application, which is referenced by its id "folderTree". Having multiple folder tree views will force us to rewrite/update a lot of parts and all the methods touching thegFolderTreeController
or directly using the "folderTree" id as a selector. This is gonna be pretty big.
Sounds like you can end up recoding stuff all over Thunderbird. Good luck!
- How do we handle scrollbars? Should all the view be in the same scrollable area or each view should have its own scrollbar? (I suggest a single scroll area)
- Do we allow manually changing the height of each view with a splitter, or each view will grow in height to show the entire content? (I suggest to go for the latter, as it's easier and similar to what other apps do)
These two items look related. So probably the choice is more like this:
- resizable views with splitter, each view with its own scrollbar (and perhaps an additional overall scrollbar).
- stacked collapsible views without splitter, one scrollbar for the entire stack
I find that a bit hard to imagine in the abstract, but for starters, one overall scrollbar with collapsible views does seem less complicated.
However, I have no idea if that scales and works out for users with long views, because any long view may occupy most of the vertical space which kinda defeats the purpose of "multi-view".
I also find Richard's idea of pinning (smaller) view(s) on top quite interesting. However, what if that view is longer, maybe covering the whole vertical space? Would seem that pinning does require size limits (or manual resizing) and inner scroll bars.
Not sure about the cost-benefit ratio of this project, especially as we're still using XUL <trees> which are bound to go away but here we are designing complex solutions all around them...
Assignee | ||
Comment 35•4 years ago
|
||
Early working prototype that is driving me slightly crazy.
I decided to pursue the approach of using a single tree after trying other approaches.
These are my findings:
- The
tree
element uses its own scrollbar, pretty hard to control or disable. - The
tree
item uses the entire eight of its parent container, so if we wanted to use multiple trees, we would need to have vboxes for each of them, separated by a splitter. - Having multiple containers with multiple scrollbars is a usability nightmare.
- Having to handle multiple
tree
IDs throughout the entire application is an even worse nightmare.
I think we can discard the approach above.
Using a single Tree
With this WIP patch you can toggle on and off multiple modes at once.
Each mode will have its own "Header" row, with a different style to separate the modes.
The header is not visible if you only have 1 mode active.
These are the issues I'm having and bugs I need to address:
- The header row is not super customizable in terms of height or padding, I wish the tree was more flexible but I think that's all we can do.
- The header rows shouldn't be clickable, as there's nothing to load obviously. How can I disable those row items?
- Every time a mode is toggled, the persistent Open and Closed state of the folder goes bananas. I'll try to fix it.
- The view order is dictated by the order in which you toggle the modes. We should have a fixed order and later on allow users to drag and drop the headers to reorder the lists.
I'm sure there are other thousands of bugs I haven't considered due to this approach, but we can slowly deal with those as we progress.
In conclusion
Even if frustrating and hard, I think this approach is doable, with some more rewrite and code clean up.
I saw that m-c is using tables in some areas with long lists, and I wonder if we should maybe try to completely rebuild the Folder Tree and move to tables, which could give us the flexibility we need, but that means rebuilding this essential area entirely from scratch, and I'm not sure I feel super comfortable doing it.
Comment 36•4 years ago
|
||
Assignee | ||
Comment 37•4 years ago
|
||
Do you know how/if can I disable the header rows from receiving any sort of events? Clicks, drags, drops, etc?
I wonder if there's an attribute I could use, or if we currently have a flag I could assign to create proper conditions for all those interactions.
Assignee | ||
Comment 38•4 years ago
|
||
This seems to be working, but I'm sure I missed some edge cases which might break everything.
Two things are currently missing:
- Disable any interaction with the mode headers.
- Properly handle what might happen when a folder is dragged and dropped between different modes.
Assignee | ||
Comment 39•4 years ago
|
||
Comment on attachment 9188191 [details] [diff] [review]
1163555-folder-views.diff
A ui-review is more appropriate.
Assignee | ||
Comment 40•4 years ago
|
||
Heads up that this bitrotted after the bug 1676697 and bug 1677538 landed.
Assignee | ||
Comment 41•4 years ago
|
||
unbitrotted
Comment 42•4 years ago
|
||
Assignee | ||
Comment 43•4 years ago
|
||
- When I hide a tree from the menu and re-enable it the closed/opened folders aren't stored.
We deliberately don't persist the mode of "unread", "favorite", and "recent".
It's always been like that, I guess because those lists are a bit volatile and keeping track of everything at every startup is a bit demanding.
- I think All and Unified shouldn't be shown together. Both show the same folders, only differently presented.
If you show Unified, you can click on All and hide that. I think we should leave the flexibility to show what users want, as it would add more complexity for us to deal with excluding one mode if another mode is visible.
- The different trees should be sortable and stay on their position after hiding/showing. Now they appear always on the top and you have to hide/show the trees until they are sorted like you want.
Yup, that's a temporary issue I'd like to deal with in a follow up bug to not make this patch too big.
- The TB UI is normally flat, should we use a gradient in this modeHeader? I think a flat appearance would be more consistent.
I had a flat background for the modeHeader, but it was blending a bit too much with the rest, and not properly separating the views. That gradient is the same currently used in the Today Pane header. I'll play a bit more with the flat colour trying to find a good balance.
- Not sure if the
Hide Header
in the Folder pane header popup is clear. I thought I can hide the modeHeaders and show only a line to separate the trees.
Ah, bad wording from me. I guess we could call it "Hide Toolbar" even if it's not a real toolbar.
I wouldn't implement the option to hide the modeHeaders as that would require a bit of a hack in CSS to hide the text and leave a centered border. I guess we could do it, but maybe in a follow up bug.
With dark theme, at least on Windows, hovering the modeHeader shows the treechildren hover background colour.
Ah, good catch, I'll fix that and all the other CSS issues.
Thanks for the review.
P.S. for Magnus, I found a way to prevent any interaction on those header treeitems. A less buggy patch is coming.
Comment 44•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #43)
- When I hide a tree from the menu and re-enable it the closed/opened folders aren't stored.
We deliberately don't persist the mode of "unread", "favorite", and "recent".
It's always been like that, I guess because those lists are a bit volatile and keeping track of everything at every startup is a bit demanding.
I meant especially "All" and "Unified" and they also don't store them. Without patch they do.
Assignee | ||
Comment 45•4 years ago
|
||
Argh, really? They're stored for me...mhh
Assignee | ||
Comment 46•4 years ago
|
||
I meant especially "All" and "Unified" and they also don't store them. Without patch they do.
I think I know the problem.
We only store the open/close state when we close the application. So if you toggle them on an off they will always reset until you close thunderbird in the state you want.
Assignee | ||
Comment 47•4 years ago
|
||
This should fix all the highlighted issues, and takes care of removing all the interactions from the modeHeader rows.
Assignee | ||
Comment 48•4 years ago
|
||
Updated patch after some troubleshooting with Richard.
Comment 49•4 years ago
|
||
Comment 50•4 years ago
|
||
Comment on attachment 9188450 [details] [diff] [review]
1163555-folder-views.diff
Alessandro, you missed my changes in folderPane.css to fix the hover issue on dark theme under Windows.
I'll upload your patch with this changes.
What do you think about using background-color: hsla(0, 0%, 50%, 0.1);
instead of var(--toolbar-bgcolor)
? Or a alpha of 0.05.
The not remembering of the closed folders does still happen. Maybe for a follow-up bug.
Comment 51•4 years ago
|
||
Assignee | ||
Comment 52•4 years ago
|
||
label.addEventListener("command", () => { this.activeModes = mode;
I don't understand how this works. Shouldn't it just add to the active modes?
Since the menu item is a toggable checkbox, I'm using the setter to add or remove the clicked mode, so on click I'm directly passing the mode to the setter instead of having a checked/unchecked condition in the listener.
Assignee | ||
Comment 53•4 years ago
|
||
Apologies for the delay on this, I found a couple of edge cases I just solved, and I'm also updating the tests to cover everything, which turned to be a bit tricky.
Assignee | ||
Comment 54•4 years ago
|
||
Patch updated with the following changes:
- No new strings added to properties or dtd file.
- Prevent an empty Folder Pane if the user deselects all the modes.
- Implement tests to cover all modes and multiple scenarios.
Assignee | ||
Comment 55•4 years ago
|
||
Lots of bct4
and bct5
test failures.
I'll fix those.
Comment 56•4 years ago
|
||
Assignee | ||
Comment 57•4 years ago
|
||
Lots of tests covering the folder pane modes, which is great as I mostly had to simply update the old variable names used in the tests to let them all pass.
Try run in progress: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=fde8ba66ffc2eb389511be64251eceb902ac30a3
Assignee | ||
Comment 58•4 years ago
|
||
Ah, it seems I missed a couple of bct5 tests.
Assignee | ||
Comment 59•4 years ago
|
||
Some tests were failing only when running in pair due to some missing cleanups during shutdown.
Let's see how this goes: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=4c366d2eb89abb9e33ab7b999f9383258940854a
Assignee | ||
Comment 60•4 years ago
|
||
Try run seems good, other than that bct2 policy failure which I can't reproduce locally.
I doubt this patch caused it.
I think I figured why the open state is not maintained, but I prefer to do it in a follow up bug as it's tightly coupled with the ability to sort the display modes.
Assignee | ||
Comment 61•4 years ago
|
||
This is the culprit of the issue of all the folders.
I don't quite understand why this condition was written in this way
https://searchfox.org/comm-central/rev/f6b563aaf622ff6fe9836cd8e6ffe9b2851a63b6/mail/base/content/folderPane.js#1587-1589
Assignee | ||
Comment 62•4 years ago
|
||
Comment on attachment 9189545 [details] [diff] [review]
1163555-folder-views.diff
Removing the review as I'm updating this patch to improve it.
I figured what was the issue in the restoring of the open state and I understood the code I shared above.
I fixed the open/close state when dealing with multiple modes, and I also put some conditions in place to avoid looping through the _rowMap
when not necessary.
I found a small bug when showing all + unified folders on startup.
I'm fixing it.
Assignee | ||
Comment 63•4 years ago
|
||
We might also have some performance issues here as the _restoreOpenStates()
loops through the tree _rowMap
recursively every time a folder is opened.
Considering that we're running that method for every currently active mode, we're looping a lot.
Assignee | ||
Comment 64•4 years ago
|
||
Open state
This should fix the open/close state not being restored properly.
If you tested one of the previous patch you might had your folderTree.json filled with all the folders.
You can clear the open object to start with a clean slate (eg. "open":{}
).
This is only necessary if you tested a previous patch from this bug, regular users won't need to do anything.
Performance
I implemented some conditions to not run the loop if the mode doesn't match or if the current row doesn't have any children (apparently we were toggling the row also for folders without any child item).
I'm not sure this will be enough.
Do we have any performance test in place to cover this area at startup? I'm especially worried about users with many opened subfolders while using multiple views.
Outstanding issue
The Unified (smart) mode doesn't load properly on startup and I can't figure out why.
Comment 65•4 years ago
|
||
Comment 66•4 years ago
|
||
Comment on attachment 9189923 [details] [diff] [review]
1163555-folder-views.diff
The UI looks good. With, like Magnus wrote, still the sort issue. But you wanted to fix this in a follow-up.
(In reply to Alessandro Castellani (:aleca) from comment #64)
Open state
This should fix the open/close state not being restored properly.
If you tested one of the previous patch you might had your folderTree.json filled with all the folders.
You can clear the open object to start with a clean slate (eg."open":{}
).
This is only necessary if you tested a previous patch from this bug, regular users won't need to do anything.
This still doesn't work for me, also with clearing folderTree.json like you suggested. I'd say it's now worse as the "Unread" and "Favorite" trees also don't remember the state when hiding/showing them. With earlier patches this worked.
Outstanding issue
The Unified (smart) mode doesn't load properly on startup and I can't figure out why.
That would be very bad for me when it lands like this as this is my default tree.
Assignee | ||
Comment 67•4 years ago
|
||
for clarity, should these contain something like "folderview-"?
What do you mean?
Comment 68•4 years ago
|
||
These are "fake" folders, now with urls like mailbox:// .... Recent", right? I suggested adding folderview- to that url. But I looking at this again, I guess that would cause complications so in that case not needed.
Assignee | ||
Comment 69•4 years ago
|
||
I found another problem with this implementation.
The "Mode Headers" (the fake local folders I'm creating to use a dividers between different modes) are registered as actual local folders.
So whenever a mode is activated, that folder is actually created, and during the next restart, that folder will appear in the local folders children tree items. Adding more and more if the user enables and disables the view multiple times.
I tried to delete the those fake folders on shutdown or when a mode is disabled, but I get prompted with a confirmation dialog.
How can we solve this?
Should we create those mode dividers in another way?
Does our folder tree accept "non-folders" as tree items?
Assignee | ||
Comment 70•4 years ago
|
||
I think I found the solution.
I'm able to generate an ftvItem
that doesn't expect or depends on a folder. By doing so i can remove the necessity of creating fake local folders.
The problem is we have a lot of this._rowMap[row]._folder
calls in the code that assume each row is a folder.
I'll need to update a lot of things and make them all use the getFTVItemForIndex()
method so I can create a condition for the header modes.
I'm starting to see the light at the end of the tunnel.
Apologies for the delay and the multiple patches on this, but it's more complicated than it seemed originally.
Comment 71•4 years ago
|
||
Virtual folders (saved searches) could have some resemblance, so perhaps worth looking at what we do for those.
Assignee | ||
Comment 72•4 years ago
|
||
All right, I found a solution that it's fast and works without problems.
The ftvItemheader
doesn't have a _folder
attribute, therefore I'm ignoring those items for any interaction. Pretty straightforward.
I've also figured the open/close state issue. Now I'm saving the state for each mode, so even if you hide/show a mode multiple times, it should always remember the previously opened state.
I was thinking to implement a right click > move up/down
context menu for the Mode Headers, in order to add an initial sorting option, and remembering that sorting in the folderPane.js.
Should I do it in this patch or deal with it in a follow up?
By default, the new modes will always be added above existing modes.
Assignee | ||
Comment 73•4 years ago
|
||
This one should fix all the open sate problems.
I haven't implemented the sorting or fixed ordering of the modes as per what I wrote in the comment above.
Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1e0c321cd91bcc41384966116935907854e3fc5b
Edit:
Some bct5 failures, I'll fix them.
Comment 74•4 years ago
|
||
Comment on attachment 9191213 [details] [diff] [review]
1163555-folder-views.diff
The collapsed folders are working now, great.
But unfortunately the modeHeader aren't readable when a dark LW-theme is active because of the color: var(--toolbar-color);
. Is this needed or could we simply use the default tree text colour?
Also are the modeHeader selectable and with the dark or light theme they get the selected treechildren colours.
Assignee | ||
Comment 75•4 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #74)
The collapsed folders are working now, great.
Hurray!
But unfortunately the modeHeader aren't readable when a dark LW-theme is active because of the
color: var(--toolbar-color);
. Is this needed or could we simply use the default tree text colour?
Uh, it worked last time I checked, weird.
Yeah, the color can inherit the default tree color.
Also are the modeHeader selectable and with the dark or light theme they get the selected treechildren colours.
The modeHeaders are not selectable, for now, I will remove those interactions in dark mode.
Thanks
Comment 76•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #75)
The modeHeaders are not selectable, for now, I will remove those interactions in dark mode.
This happens on light themes too.
Assignee | ||
Comment 77•4 years ago
|
||
The screenshot posted by the reporter on bug 1680947 shows that he's using the "Compact Favorite Mode".
I wonder if we shouldn't be so quick in deprecating the compact mode and reconsider its benefits.
Assignee | ||
Comment 78•4 years ago
|
||
Comment on attachment 9191213 [details] [diff] [review]
1163555-folder-views.diff
Removing the review request as I'm fixing tests and identifying other bugs.
Assignee | ||
Comment 79•4 years ago
|
||
I found a bunch of other issues when interacting with the various Folder features when viewing multiple modes at once.
I'm writing them here mostly as a sanity check:
- Creating a new folder doesn't properly reload the tree, sometimes showing the folder only in one mode and not the other.
- Deleting a folder deletes only the selected folder, but not if present in other currently visible modes (eg. if the deleted folder is also listed as favorite, it remains in that list).
- Emptying the Trash will focus on the parent folder of the wrong mode, toggling it open if closed.
- Emptying the Trash "removes" the selected Trash folder from the current mode.
I assume all these are downfalls of the folderPane code and related nsIMsgFolder C++ code assuming only one mode is always visible.
This needs more work, luckily our tests cover pretty much everything.
Assignee | ||
Comment 80•4 years ago
|
||
Thanks to the tests failing I was able to fix a lot of things and improve some areas.
The overall issue of adding, removing, and editing folders when multiple views are visible still persists and I'm working on it.
By forcing a rebuild of the tree it can be solved, but that doesn't look like a viable option as it might impact performance for large number of folders, and also loses the focus on the parent or new folder, which is not great.
I'm simply uploading the patch if anyone wants to look at it or test it.
Assignee | ||
Comment 81•4 years ago
|
||
Assignee | ||
Comment 82•4 years ago
|
||
I further updated this patch to re-enable the "compact" mode as I stumbled upon a couple of users that use that variation.
It's a very simple addition that doesn't create extra modes, but adapts the generated map if the compact option is toggled.
Since we only have a compact variation for unread
and favorite
, I don't see it as a problem having one option to rule them all.
I added the extra option in all the menus, and made the app menu items clickable without closing the menu.
Comment 83•4 years ago
|
||
Comment on attachment 9193127 [details] [diff] [review]
1163555-folder-views.diff
Looks good for me, thanks.
Comment 84•4 years ago
|
||
Comment 85•4 years ago
|
||
Unbitrotted.
Assignee | ||
Comment 86•4 years ago
|
||
The for for the "mode headers" (e.g. Unified Folders) looks like it's slightly smaller than the other cells, which is a bit odd.
Sorry, I don't think I understand. Are you talking about a for loop
or it's a UI issue? Can you share a screenshot?
The compact mode de-toggling doesn't work: once set you can't untick it.
Mhh, I can't reproduce this issue.
I'm still not sure we want to keep that - it's really a per-mode setting too so the UI doesn't exactly do what it says now.
it would be interested to see if we could have some data from telemetry to see how many users rely on a compact mode, if any.
I personally saw a couple of users using the Favorite compact mode to list only the inbox of multiple accounts without the parent > child structure.
I think that's one of the various example in which a compact mode can be quite handy.
I agree that the current solution is not optimal as by being a per-mode setting doesn't make sense presenting it as a "global" setting.
I'll try another solution.
Unbitrotted.
Thank you so much!
Assignee | ||
Comment 87•4 years ago
|
||
Here's a solution that might work.
Disable the Compact mode menu item if the currently active modes don't have a compact variation.
With this change we can visually communicate when the Compact mode actually does something to the UI.
Another addition I'd like to make is the disabling of the All Folders toggle if that's the only currently active mode. Even if is not possible to uncheck that mode if it's the only one, giving a visual feedback to the user is better, and preventing a click altogether prevents us from running unnecessary checks.
Comment 88•4 years ago
|
||
Compare the font of the inbox and the "Unread Folders" The o's doesn't look to be the same size (even when not bold).
I can still reproduce the not being able to de-toggle compact mode.
Now I also notice in the console (I think it's for an account that only has an inbox, never connected):
JavaScript error: chrome://messenger/content/folderPane.js, line 3769: TypeError: can't access property "sort", aFtvItems is null
... coming from the row sortFolderItems(parent._children);
where _children is undefined
Comment 89•4 years ago
|
||
What's "hide header"?
Assignee | ||
Comment 90•4 years ago
|
||
Now I also notice in the console (I think it's for an account that only has an inbox, never connected):
JavaScript error: chrome://messenger/content/folderPane.js, line 3769: TypeError: can't access property "sort", aFtvItems is null
... coming from the row sortFolderItems(parent._children); where _children is undefined
Ah, thanks, that's why I couldn't reproduce the error.
What's "hide header"?
Sorry, mock-up mistake, that should be the "Hide Toolbar" menuitem.
Assignee | ||
Comment 91•4 years ago
|
||
Compare the font of the inbox and the "Unread Folders" The o's doesn't look to be the same size (even when not bold).
Ah, I see.
Yes, that's on purpose as the Mode Headers have a slightly smaller font size (0.9em
) to better distinguish them from the clickable ones.
They have a smaller font also to decrease importance.
Assignee | ||
Comment 92•4 years ago
|
||
Patch updated with the requested fixes and implemented the disabling of the "compact" and "all folders" items when necessary.
Comment 93•4 years ago
|
||
Assignee | ||
Comment 94•4 years ago
|
||
It's a bit weird though, to have a "header" that has smaller font than the "content". Usually it's the other way around. Could we make them the same font size?
But that's not really a "header" in the context of a written document.
That's more a separator field.
I agree that it would look better if we had the ability to manage the spacing of those items, but since we're locked by the styling limitations of the XUL tree, having the same font size, or even larger, looks very weird.
You can quickly check by editing the font-size CSS attribute for this declaration treechildren::-moz-tree-cell-text(modeHeader) {
.
To me it looks odd at the same size, but let me know how that feels for you.
Richard, what do you think?
Assignee | ||
Comment 95•4 years ago
|
||
Maybe we should consider also showing the header if "All folders" is not in the set?
If been a small problem that users can end up with a blank folder pane (for favourite and unread esp.) and don't understand what happened.
Mh, that's a good point, I'll do that.
Didn't notice this before, but wouldn't it make more sense to fix the ftvItemHeader constructor it could take mode as first argument and then name would be optional and created like a above if not passed in.
Indeed, that makes sense. I think the createModeHeader()
method originally had more complexity during first iterations, but now is not really helpful as helper method.
Also we should capitalize F, to FtvItemHeader (the existing code is odd and wrong, classes should use capitalization for names)
Agree. I'll update also the other classes in this file to keep it consistent.
Comment 96•4 years ago
|
||
With the same size they look too much like normal treechildren. I like them more with 0.9em. They are still emphasised through the bold font but differ enough to not look like normal treechildren.
Assignee | ||
Comment 97•4 years ago
|
||
Patch updated and new try run launched to test if the recent changes caused test failures: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a4c20f92b283d2d0134607f1e9e2a90d9513f4c6
Comment 98•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 99•4 years ago
|
||
Seems to work alright, but there's some test failures.
Ah, fixing those now.
I still don't agree about the header font size, but meh :/
Hopefully we will have some capacity in the near future to replace the tree
element with something more HTML-y to get better control on those UI elements.
I'm gonna keep this feature enabled once this lands in order to use it and get used to it visually. I'll ask the same to our community and gather feedback to see how other react to the implemented styling.
folder &&
why was this needed? I think better to fix the one caller not to call it if there's no folder.
Good catch. That's unnecessary.
oncommand="gFolderTreeView.toggleCompactMode(this.getAttribute('checked'));"/>
The seems wrong: menuitem checked="false" will lead to sending "false". But "false" is true
I thought that too when I was writing it but it works.
The menuitem
returns ""
when the click un-checks it, making it a falsy attribute.
It looks a bit sketchy, but I found this solution to work for our situation, where we need to react and update this checkbox for 2 different elements, a menuitem
and a toolbarbutton
.
Thank you so much for the r+
I'll fix the tests and be sure to get a successful try-run.
I'm sure some regressions and edge cases will come out from this, but hopefully no major breakage.
Assignee | ||
Comment 100•4 years ago
|
||
Patch updated with tests passing locally.
Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=495f7118c99bd827b1c99aaa4ad02d19cde4919b
I might have also fixed the intermittent bug 1620869, but let's wait and see.
Assignee | ||
Updated•4 years ago
|
Comment 101•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/e9d4c8baa1bd
Allow showing multiple Folder Views in the Folder Pane. r=mkmelin, ui-r=Paenglab
Comment 104•4 years ago
|
||
Comment on attachment 9196441 [details] [diff] [review]
1163555-folder-views.diff
@@ -592,17 +590,19 @@ var gFolderTreeView = {
* view. Defaults to false.
* @returns true if the folder selection was successful, false if it failed
* (probably because the folder isn't in the view at all)
*/
selectFolder(aFolder, aForceSelect = false) {
// "this" inside the nested function refers to the function...
// Also note that openIfNot is recursive.
let tree = this;
- let folderTreeMode = this._modes[this._mode];
+ let mode = this.getModeForIndex(this.getIndexOfFolder(aFolder));
+ let folderTreeMode = this._modes[mode];
If you read the comment for getIndexOfFolder()
you will see it can return null, and getModeForIndex()
does not handle that. Tabs as folders manifest this. Please follow up, thanks.
Assignee | ||
Comment 105•4 years ago
|
||
Thanks for the heads up, my bad I missed this.
Description
•