Open Bug 1163555 Opened 6 years ago Updated 8 hours ago

Allow 'Pinning' multiple 'Folder Views' to the top of the Folder Pane

Categories

(Thunderbird :: Folder and Message Lists, enhancement, P2)

enhancement

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: tanstaafl, Assigned: aleca)

References

Details

Attachments

(4 files, 9 obsolete files)

229.07 KB, image/png
Paenglab
: feedback+
mkmelin
: feedback+
Details
105.26 KB, image/png
Details
68.36 KB, image/png
Details
114.45 KB, patch
mkmelin
: feedback+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

Tried to find a way to add the 'Favorites' Unified Folder above, 'All Folders' to have a 'split' (for lack of a better word' view that always shows both the 'Favorites' at the top, above the 'All Folders', ala Outlook 2013...


Actual results:

No way to do this...


Expected results:

Would like to see a way to do this.

I would be fine if the choices available for the 'Split' view option were limited, as some of them probably don't make sense, but for sure, the 'Favorites' is the perfect candidate for one that many people might like to be able to have always available.
Component: Untriaged → Folder and Message Lists
Ok, adding some additional comments to some other related bugs I created, I'd like to suggest a term for this capability...

Lets call it 'pinning'.

So, this enhancement request is to provide a way to pin one or more of the Unified 'views' to the Folder Pane.

Also, if more than one view is pinned, there should be a way to define the order - ie, Favorites at the top, Unread below that, then the 'Unpinned' view(s) are shown at the bottom, and the user can still cycle through them, assuming they have the 'Folder Pane View Switcher' Addon installed - speaking of which - why was this feature removed? See bug 1164818 for bringing them back natively, but as a toggleable option under View > Folders.
Summary: Allow 'Split' view of multiple 'Unified Folders' → Allow 'Pinning' (and re-ordering) one or more 'Unified Folders' views to the top of the Folder Pane
Severity: normal → enhancement
Version: 38 → unspecified
Summary: Allow 'Pinning' (and re-ordering) one or more 'Unified Folders' views to the top of the Folder Pane → Allow 'Pinning' (and re-ordering) one or more 'Folder Views' to the top of the Folder Pane
Changed the Summary t0 reflect my corrected understanding of the Folder Views feature...

So, please replace the term 'Unified Folder(s)' in comment 0 to 'Folder View(s)'
And discussing a related feature, Jim pointed me to:

https://addons.mozilla.org/en-US/thunderbird/addon/additional-folder-views/

Which does precisely what I'm looking for here...

Too bad it was apparently abandoned long ago...
Duplicate of this bug: 1163562
Duplicate of this bug: 313342
Blocks: 1159713
Duplicate of this bug: 1574129
Assignee: nobody → alessandro
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2

It would be really awesome if this bug got some attention and was implemented!

I'll take a look at this and create some mock-ups so we can find a proper approach to tackle this implementation.

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.

Indeed, this will be very useful.

For what it's worth, I just add AAA or ** to the beginning of the folder name, and that forces it to the top.

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.

Status: NEW → ASSIGNED
Attached image Folder Pane toolbar.png

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.

Attachment #9178889 - Flags: feedback?(richard.marti)
Attachment #9178889 - Flags: feedback?(mkmelin+mozilla)

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

Attachment #9178889 - Flags: feedback?(mkmelin+mozilla) → feedback+

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.

Attachment #9178889 - Flags: feedback?(richard.marti) → feedback+
Summary: Allow 'Pinning' (and re-ordering) one or more 'Folder Views' to the top of the Folder Pane → Allow 'Pinning' multiple 'Folder Views' to the top of the Folder Pane

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.

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'

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.

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.

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

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?

Flags: needinfo?(mkmelin+mozilla)

I'd imagine we need one tree per view. The add-on listed above could perhaps give some inspiration [I didn't check].

Flags: needinfo?(mkmelin+mozilla)

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

See Also: → 244347

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:

  1. Ensure View > Folders > All
  2. right-click on your favorite Account1 in folder pane > Search messages (or Ctrl+Shift+F with that inbox selected)
  3. [x] Search subfolders
  4. [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.

Flags: needinfo?(alessandro)

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

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

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.

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

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.

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.

Flags: needinfo?(alessandro)

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 the gFolderTreeController 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?

Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bugzilla2007)

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

Flags: needinfo?(richard.marti)

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.

Flags: needinfo?(mkmelin+mozilla)

(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 the gFolderTreeController 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...

Flags: needinfo?(bugzilla2007)
Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

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.

Attachment #9187571 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9187571 [details] [diff] [review]
1163555-folder-views.diff

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

A bit too buggy to test much. If it's the "All Folders" separator, that looks pretty ok I think
Attachment #9187571 - Flags: feedback?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

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.
Attachment #9187571 - Attachment is obsolete: true
Attachment #9188191 - Flags: review?(mkmelin+mozilla)
Attachment #9188191 - Flags: feedback?(richard.marti)

Comment on attachment 9188191 [details] [diff] [review]
1163555-folder-views.diff

A ui-review is more appropriate.

Attachment #9188191 - Flags: feedback?(richard.marti) → ui-review?(richard.marti)

Heads up that this bitrotted after the bug 1676697 and bug 1677538 landed.

Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

unbitrotted

Attachment #9188191 - Attachment is obsolete: true
Attachment #9188191 - Flags: ui-review?(richard.marti)
Attachment #9188191 - Flags: review?(mkmelin+mozilla)
Attachment #9188354 - Flags: ui-review?(richard.marti)
Attachment #9188354 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9188354 [details] [diff] [review]
1163555-folder-views.diff

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

It seems a mean one changed primaryToolbar.css after you posted the patch. ;-)

This looks already promising. But some things that need thinking and maybe changing:
- When I hide a tree from the menu and re-enable it the closed/opened folders aren't stored.
- I think All and Unified shouldn't be shown together. Both show the same folders, only differently presented.
- 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.
- The TB UI is normally flat, should we use a gradient in this modeHeader? I think a flat appearance would be more consistent.
- 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.

With dark theme, at least on Windows, hovering the modeHeader shows the treechildren hover background colour.

ui-r+ for the first version but needs some improvements.

::: mail/themes/shared/mail/folderPane.css
@@ +52,5 @@
> +  list-style-image: none;
> +  padding: 0;
> +  margin: 0;
> +  width: 0!important;
> +  height: 0!important;

Please a space between 0 and !.

@@ +57,5 @@
> +}
> +
> +treechildren::-moz-tree-row(modeHeader) {
> +  border-top: 1px solid ThreeDShadow;
> +  border-bottom: 1px solid ThreeDShadow;

border-top / border-bottom can be exchanged to border-block.

On Windows you need a
  margin-inline: 0;
  border-inline-style: none;
to make the modeHeader connect to the tree border.
Attachment #9188354 - Flags: ui-review?(richard.marti) → ui-review+
  • 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.

Flags: needinfo?(mkmelin+mozilla)

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

Argh, really? They're stored for me...mhh

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.

Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

This should fix all the highlighted issues, and takes care of removing all the interactions from the modeHeader rows.

Attachment #9188354 - Attachment is obsolete: true
Attachment #9188354 - Flags: review?(mkmelin+mozilla)
Attachment #9188410 - Flags: ui-review?(richard.marti)
Attachment #9188410 - Flags: review?(mkmelin+mozilla)
Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

Updated patch after some troubleshooting with Richard.

Attachment #9188410 - Attachment is obsolete: true
Attachment #9188410 - Flags: ui-review?(richard.marti)
Attachment #9188410 - Flags: review?(mkmelin+mozilla)
Attachment #9188450 - Flags: ui-review?(richard.marti)
Attachment #9188450 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9188450 [details] [diff] [review]
1163555-folder-views.diff

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

Overall, works pretty well!

::: mail/base/content/folderPane.js
@@ +370,5 @@
> +  _activeModes: [],
> +  get activeModes() {
> +    if (!this._activeModes.length) {
> +      let modes = this._treeElement.getAttribute("mode").split(",");
> +      // Remove duplicate modes stored in the XUL.

just "Remove duplicate modes." perhaps. They come from the attribute, I wouldn't say they are stored in xul.

@@ +371,5 @@
> +  get activeModes() {
> +    if (!this._activeModes.length) {
> +      let modes = this._treeElement.getAttribute("mode").split(",");
> +      // Remove duplicate modes stored in the XUL.
> +      let uniqueModes = modes.filter((c, index) => {

I would reuse modes instead of creating a new vairable

@@ +377,5 @@
> +      });
> +
> +      // Skip non existing modes from the activeModes array. This can happen
> +      // when an extension is removed.
> +      for (let mode of uniqueModes) {

this._activeModes = modes.filter(mode => mode in this._modes);

@@ +413,5 @@
> +    }
> +
> +    // Remove modes that don't exist anymore. This might happen if an extension
> +    // is disabled since we don't require a full restart.
> +    for (let mode of this._activeModes) {

similarly just filtering would be shorter code

@@ +501,5 @@
> +      let label = document.createXULElement("toolbarbutton");
> +      label.setAttribute("type", "checkbox");
> +      label.setAttribute("value", mode);
> +      label.classList.add("subviewbutton", "subviewbutton-iconic");
> +      document.l10n.setAttributes(label, `show-${mode}-folders-label`);

Please list the values in a comments, so it's easier in the future to find where they are and that they are still used.

@@ +509,4 @@
>        }
> +
> +      label.addEventListener("command", () => {
> +        this.activeModes = mode;

I don't understand how this works. Shouldn't it just add to the active modes? Also would be nicer to grab the event.target value

@@ +998,5 @@
> +    let folder = view.getFolderAtCoords(event.clientX, event.clientY);
> +    if (
> +      view
> +        .getRowProperties(view.getIndexOfFolder(folder))
> +        .includes("modeHeader")

worth adding a comment for these

@@ +1868,5 @@
>  
> +        // Create the header only if multiple modes are currently displayed.
> +        if (gFolderTreeView.activeModes.length > 1) {
> +          let name = gFolderTreeView.messengerBundle.getString(
> +            "folderPaneModeHeader2_unread"

Would be good to move to fluent instead of adding more string to properties

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +570,5 @@
>  folderPaneModeHeader_all=All Folders
> +folderPaneModeHeader2_unread=Unread
> +folderPaneModeHeader2_favorite=Favorite
> +folderPaneModeHeader2_recent=Recent
> +folderPaneModeHeader2_smart=Unified

I think this should spell out "Unread Folders", "Favorite Folders" etc. Only the single world looks a bit mis-placed and it's not clear what it is.

::: mail/locales/en-US/messenger/messenger.ftl
@@ +37,5 @@
> +show-recent-folders-label =
> +    .label = Recent
> +    .accesskey = R
> +
> +toggle-compact-view-label =

Don't think this is used, and I don't see how it could fit in.
Attachment #9188450 - Flags: review?(mkmelin+mozilla) → feedback+

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.

Attachment #9188450 - Flags: ui-review?(richard.marti)
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.

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.

Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

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.

Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=95b1387f90572cea78ab5239ef1968d277cab355

Attachment #9188450 - Attachment is obsolete: true
Attachment #9188590 - Attachment is obsolete: true
Attachment #9189282 - Flags: ui-review?(richard.marti)
Attachment #9189282 - Flags: review?(mkmelin+mozilla)

Lots of bct4 and bct5 test failures.
I'll fix those.

Comment on attachment 9189282 [details] [diff] [review]
1163555-folder-views.diff

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

Looks good but still has the issue with the not remembering of the tree states and that the trees aren't sortable.
I hope you fix this in follow-up bug.

::: mail/themes/shared/mail/folderPane.css
@@ +58,5 @@
> +}
> +
> +treechildren::-moz-tree-row(modeHeader),
> +:root[lwt-tree] treechildren::-moz-tree-row(modeHeader, hover) {
> +  border-block: 1px solid ThreeDShadow;

Sorry that I come so late but with the dark theme is the border too shiny. Could you use instead `var(--sidebar-border-color, var(--splitter-color))` ?
Attachment #9189282 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

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

Attachment #9189282 - Attachment is obsolete: true
Attachment #9189282 - Flags: review?(mkmelin+mozilla)
Attachment #9189528 - Flags: ui-review+
Attachment #9189528 - Flags: review?(mkmelin+mozilla)

Ah, it seems I missed a couple of bct5 tests.

Attached patch 1163555-folder-views.diff (obsolete) — Splinter Review

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

Attachment #9189528 - Attachment is obsolete: true
Attachment #9189528 - Flags: review?(mkmelin+mozilla)
Attachment #9189545 - Flags: review?(mkmelin+mozilla)

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.

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

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.

Attachment #9189545 - Flags: review?(mkmelin+mozilla)

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.

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.

Attachment #9189545 - Attachment is obsolete: true
Attachment #9189923 - Flags: ui-review?(richard.marti)
Attachment #9189923 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9189923 [details] [diff] [review]
1163555-folder-views.diff

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

Works pretty ok. 
What's a bit confusing though is how the order of the visible modes is not consistent (always the last clicked first?)

::: mail/base/content/folderPane.js
@@ +379,5 @@
> +      // Skip non existing modes from the activeModes array. This can happen
> +      // when an extension is removed.
> +      this._activeModes = modes.filter(mode => mode in this._modes);
> +
> +      // If we end up with an empty array, add the default mode and udpate all

typo: update

@@ +1943,5 @@
> +          let name = gFolderTreeView.messengerBundle.getString(
> +            "folderPaneModeHeader_favorite"
> +          );
> +          let header = MailUtils.getOrCreateFolder(
> +            `mailbox://nobody@Local%20Folders/${name}`

and maybe worth adding a helper function for it?

@@ +2004,5 @@
> +          let name = gFolderTreeView.messengerBundle.getString(
> +            "folderPaneModeHeader_recent"
> +          );
> +          let header = MailUtils.getOrCreateFolder(
> +            `mailbox://nobody@Local%20Folders/${name}`

for clarity, should these contain something like "folderview-"?

@@ +2650,5 @@
>   * @param aFolderFilter  When showing children folders of this one,
>   *                       only show those that pass this filter function.
>   *                       If unset, show all subfolders.
>   */
> +function ftvItem(aFolder, aFolderFilter, mode = "all") {

please document, and add types to the existing doc

@@ +2922,5 @@
> +      case "folderNameCol":
> +        return this._folder.abbreviatedName;
> +      default:
> +        return "";
> +    }

I guess just return (aColName == "folderNameCol") ? this._folder.abbreviatedName : "";

::: mail/base/content/foldersummary.js
@@ +280,5 @@
> +      if (
> +        !msgFolder ||
> +        gFolderTreeView
> +          .getRowProperties(gFolderTreeView.getIndexOfFolder(msgFolder))
> +          .includes("modeHeader")

should this have a proper helper, in the view or so, isFolderHeader() perhaps?

::: mail/components/customizableui/content/panelUI.inc.xhtml
@@ +708,2 @@
>                         name="viewmessages"
> +                       oncommand="gFolderTreeView.activeModes = this.getAttribute('value');"/>

would be good to add a function like folderViewMenuOnCommand(event) to handle these

::: mail/locales/en-US/messenger/messenger.ftl
@@ +13,5 @@
> +folder-pane-header-label = Folders
> +
> +## Folder Toolbar Header Popup
> +
> +hide-toolbar-label =

Shouldn't put -label in the name, and please use less generic keys so we don't get collisions as easily. Like folder-toolbar-hide-toolbar-toolbarbutton

::: mail/test/browser/folder-tree-modes/browser_modeSwitching.js
@@ +74,5 @@
> +  Assert.ok(tree.activeModes.includes(aMode));
> +
> +  // We need to open the menu because only then the right mode is set in them.
> +  if (["linux", "win"].includes(AppConstants.platform)) {
> +    // On OS X the main menu seems not accessible for clicking from mozmill.

from tests

::: mail/test/browser/shared-modules/FolderDisplayHelpers.jsm
@@ +1367,5 @@
>    if (aController == null) {
>      aController = mc;
>    }
> +  if (!aController.folderTreeView.activeModes.includes(aMode)) {
> +    throw new Error(`The folder mode should be "${aMode}" is not visible`);

some grammar problem here
Attachment #9189923 - Flags: review?(mkmelin+mozilla) → feedback+

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.

Attachment #9189923 - Flags: ui-review?(richard.marti)
You need to log in before you can comment on or make changes to this bug.