Closed Bug 1163555 Opened 10 years ago Closed 4 years ago

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

Categories

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

enhancement

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
86 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: tanstaafl, Assigned: aleca)

References

Details

Attachments

(6 files, 17 obsolete files)

(deleted), image/png
Paenglab
: feedback+
mkmelin
: feedback+
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), patch
aleca
: review+
aleca
: ui-review+
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...
Blocks: 1159713
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 (deleted) —

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

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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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.

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

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)

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

What do you mean?

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.

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?

Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)

Virtual folders (saved searches) could have some resemblance, so perhaps worth looking at what we do for those.

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.

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

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.

Attachment #9189923 - Attachment is obsolete: true
Attachment #9191213 - Flags: ui-review?(richard.marti)
Attachment #9191213 - Flags: review?(mkmelin+mozilla)

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.

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

(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

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

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.

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

Removing the review request as I'm fixing tests and identifying other bugs.

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

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.

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

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.

Attachment #9191213 - Attachment is obsolete: true
Attached patch 1163555-folder-views.diff (obsolete) (deleted) — Splinter Review
Attachment #9192300 - Attachment is obsolete: true
Attachment #9192801 - Flags: review?(mkmelin+mozilla)
Attached patch 1163555-folder-views.diff (obsolete) (deleted) — Splinter Review

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.

Attachment #9192801 - Attachment is obsolete: true
Attachment #9192801 - Flags: review?(mkmelin+mozilla)
Attachment #9193127 - Flags: ui-review?(richard.marti)
Attachment #9193127 - Flags: review?(mkmelin+mozilla)

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

Looks good for me, thanks.

Attachment #9193127 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9193127 [details] [diff] [review] 1163555-folder-views.diff Review of attachment 9193127 [details] [diff] [review]: ----------------------------------------------------------------- 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. The compact mode de-toggling doesn't work: once set you can't untick it. 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. ::: mail/base/content/folderPane.js @@ +375,5 @@ > + modes = modes.filter((c, index) => { > + return modes.indexOf(c) === index; > + }); > + > + // Skip non existing modes from the activeModes array. This can happen Exclude non-existing @@ +720,5 @@ > let endIndex = {}; > selection.getRangeAt(i, startIndex, endIndex); > for (let j = startIndex.value; j <= endIndex.value; j++) { > + let folder = gFolderTreeView.getFolderForIndex(j); > + if (folder) { What is the non-folder case? Please add a comment about it. @@ +1734,5 @@ > /** > * Add the item from the persistent list, meaning the item should > * be persisted as open (expanded) in the tree. > * > + * @param {nsIMsgFolder|null} item - The folder item if it exists. @param {?nsIMsgFolder} @@ +2851,5 @@ > +/** > + * The ftvItem constructor for the fodler row. > + * > + * @param {nsIMsgFolder} aFolder - The folder attached to this row in the tree. > + * @param {Function|null} aFolderFilter - When showing children folders of this this is actually not a nullable parameter, it's an optional one - if it's passed in it's expected to be a function. So @param {Function} [aFolderFilter] - ::: mail/base/content/mainNavigationToolbox.inc.xhtml @@ +440,5 @@ > <menuitem id="menu_allFolders" value="all" > label="&allFolders.label;" > accesskey="&allFolders.accesskey;" > + type="checkbox" name="viewmessages" > + oncommand="gFolderTreeView.activeModes = this.value;"/> would be preferable to get away from inline scripts, so one step in that direction would be to pass the event oncommand="gFolderTreeView.setFolderMode(event);" + then use event.target to set activeModes. Later it's then easier to remove oncommand, by just finding the elements hooking up event listening.
Attachment #9193127 - Flags: review?(mkmelin+mozilla) → feedback+
Attached file 1163555-folder-views.diff (unbitrotted) (obsolete) (deleted) —

Unbitrotted.

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!

Attached image folders-mode-compact.png (deleted) —

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.

Attached image unreadfolders.png (deleted) —

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

What's "hide header"?

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.

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.

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

Patch updated with the requested fixes and implemented the disabling of the "compact" and "all folders" items when necessary.

Attachment #9193127 - Attachment is obsolete: true
Attachment #9194985 - Attachment is obsolete: true
Attachment #9195757 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9195757 [details] [diff] [review] 1163555-folder-views.diff Review of attachment 9195757 [details] [diff] [review]: ----------------------------------------------------------------- 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? The "compact view" toggle in the app menu is working. It's the normal menu for which it doesn't work. ::: mail/base/content/folderPane.js @@ +2047,5 @@ > folder.addServerName = true; > } > sortFolderItems(map); > + > + // Create the header only if multiple modes are currently displayed. 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. @@ +2838,5 @@ > + * @returns {ftvItemHeader} > + */ > + createModeHeader(mode) { > + let name = this.messengerBundle.getString(`folderPaneModeHeader_${mode}`); > + return new ftvItemHeader(name, mode); 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. Also we should capitalize F, to FtvItemHeader (the existing code is odd and wrong, classes should use capitalization for names) @@ +3814,2 @@ > * > + * @param {?Array<ftvItem>} aFtvItems - The array of ftvItems to sort. I find this easier to read: @param {?fvtItem[]} @@ +3816,4 @@ > */ > function sortFolderItems(aFtvItems) { > + // Interrupt if no array has been passed. This might happen if an account was > + // configured but it never properly connected. that's my theory, maybe "e.g." would be in order
Attachment #9195757 - Flags: review?(mkmelin+mozilla) → feedback+

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?

Flags: needinfo?(richard.marti)

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.

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.

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

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

Attachment #9195757 - Attachment is obsolete: true
Attachment #9195973 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9195973 [details] [diff] [review] 1163555-folder-views.diff Review of attachment 9195973 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work alright, but there's some test failures. I still don't agree about the header font size, but meh :/ ::: mail/base/content/folderPane.js @@ +1431,5 @@ > * @param flag folder flag to create smart folders for > * @param folderName name to give smart folder > * @param position optional place to put folder item in map. If not specified, > * folder item will be appended at the end of map. > + * @returns The smart folder's FtvItem if one was added, null otherwise. @returns {?FtvItem} The smart.... @@ +1760,2 @@ > * > + * @param {nsIMsgFolder|null} item - The folder item if it exists. would prefer the notation @param {?nsIMsgFolder} @@ +1791,2 @@ > */ > + _persistItemOpen(item, index) { please document index ::: mail/base/content/foldersummary.js @@ +331,5 @@ > // server names by themselves and do not have server name already appended > // in their label. > let folderIndex = gFolderTreeView.getIndexOfFolder(folder); > if ( > + folder && why was this needed? I think better to fix the one caller not to call it if there's no folder. ::: mail/base/content/mainNavigationToolbox.inc.xhtml @@ +467,4 @@ > label="&compactVersion.label;" > accesskey="&compactVersion.accesskey;" > + type="checkbox" name="viewmessages" > + oncommand="gFolderTreeView.toggleCompactMode(this.getAttribute('checked'));"/> The seems wrong: menuitem checked="false" will lead to sending "false". But "false" is true ::: mail/components/customizableui/content/panelUI.inc.xhtml @@ +712,3 @@ > name="viewmessages" > + closemenu="none" > + oncommand="PanelUI.folderViewMenuOnCommand(event)"/> maybe we want to have the trailing ;
Attachment #9195973 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 86 Branch

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.

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

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.

Attachment #9195973 - Attachment is obsolete: true
Attachment #9196441 - Flags: ui-review+
Attachment #9196441 - Flags: review+
Blocks: 1686144

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1686306
Regressions: 1686352
Regressions: 1686562
Regressions: 1689553
Regressions: 1690630
Regressions: 1691395
Blocks: 1692766
Regressions: 1692764
Regressions: 1696965

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.

Flags: needinfo?(alessandro)
Blocks: 1699206

Thanks for the heads up, my bad I missed this.

Flags: needinfo?(alessandro)
Regressions: 1769221
Regressions: 1734192
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: