546722 - add ability to "pin" folders in the MoveTo->Recent folder list

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, enhancement)

Description

[Tony Hansen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.21022)
Build Identifier: 

I use a fair number of folders often. But once in a while I do some major house cleaning that tends to zap some of the folders out of the Recent list. It then seems to take a long time for the folders that I really do use on a frequent basis from getting back onto the list. Being able to pin certain folders to that list would help alleviate such problems.

Reproducible: Always

Comment 1

[Nathan Tuggy (:tuggyne)]

Wouldn't this fit better as an additional Move To Favorite submenu? As such, I think it would be good to implement as an addon first, per current Thunderbird feature policy. (For future reference, https://addons.mozilla.org/en-US/thunderbird/addon/1624 + https://addons.mozilla.org/en-US/thunderbird/addon/10524 look like they'd make it easy if mashed-together, or just modify the first one; also, the latter extension, which works with 3.0, might be a useful workaround.)

Comment 2

[Tony Hansen]

A Move to Favorite submenu would meet the need, if it's easy to set up. I'll take a look at the add-ons you mention.

Comment 3

[:aceman]

I think this should be doable. Bwinton, would you accept this?
It would add a new item into the folder picker, similar to the "Recent" item, showing all the folders having the "favorite" flag.

Comment 4

[:aceman]

I can build it on the cleaned up Recent menu code from bug 809066 and then it can even share a lot of code. Both submenus can have largely the same code, just the function doing recursive folder traversal would be different.

Comment 5

[:aceman]

Attachment: proposal screenshot

This is how it could look like.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 679163 [details]
proposal screenshot

Yeah, that seems like a good way of doing it.

I think we shouldn't show the "Favorite" option if the user doesn't have any favourite folders, but you might have already been planning that.  :)

Thanks,
Blake.

Comment 7

[:aceman]

Yeah, but to find out if there are any favourites I'd need to go through all folders. Of course I can do that, but I wanted to optimize this to go through them only IF the submenu is opened.

Also for clarification, I think the Favorites submenu can be there whenever Recent submenu is wanted by the caller. I don't think we need a new attribute for it.

Comment 8

[Mihai]

Is anyone still working on this? I'm interested and willing to help.

Comment 9

[:aceman]

Sure, go for it :)
Similarly to the "Recent" folder list, there could be another list (submenu) listing folders having the Ci.nsMsgFolderFlags.Favorite folder flag.

Comment 10

[Mihai]

Attachment: MoveToFavorites_v0.1.patch

Here's a working patch. I don't normally share incomplete work, but since we're going to collaborate, here it is.

Support for dynamic updates is needed next.

Comment 11

[Mihai]

Attachment: MoveToFavorites_v0.2.patch

Here's the version that supports dynamic updates.

The last thing that I would like to have is for the _MAXFAVORITES constant to be user definable.

PS: Feedback, anyone?

Comment 12

[:aceman]

Comment on attachment 9031689 [details] [diff] [review]
MoveToFavorites_v0.2.patch

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

Hi, thank you for the start.
The patch seems to work fine so far, the Favorites submenu opens,

The patch is missing the hg headers.
Also, the patch is using Windows line endings (CRLF), which makes it not apply. You must use the Linux ones (LF).

::: mail/base/content/mainNavigationToolbox.inc
@@ +763,5 @@
>                           showFileHereLabel="true"
>                           showRecent="true"
>                           recentLabel="&moveCopyMsgRecentMenu.label;"
> +                         recentAccessKey="&moveCopyMsgRecentMenu.accesskey;"
> +                         showFavorites="true"

Is there a case where we would NOT want to show Favorites when Recent are also shown? If not, we wouldn't need the new "showFavorites" attribute.

::: mailnews/base/content/folderWidgets.xml
@@ +189,5 @@
> +      
> +      <!--
> +         - The maximum number of entries in the "Favorites" menu
> +        -->
> +      <field name="_MAXFAVORITES">20</field>

Why is this needed? When would we not want to display all the favorites?

@@ +238,5 @@
>          OnItemIntPropertyChanged: function(aItem, aProperty, aOld, aNew) {
> +          if (aItem instanceof Ci.nsIMsgFolder) {
> +            if (aProperty == "FolderFlag" &&
> +                ((aOld & Ci.nsMsgFolderFlags.Favorite) != 
> +                 (aNew & Ci.nsMsgFolderFlags.Favorite))) { 

Trailing whitespace.

@@ +660,5 @@
>            sep.setAttribute("generated", "true");
>            this.appendChild(sep);
>          ]]></body>
>        </method>
> +      

Trailing whitespace.

@@ +718,5 @@
> +          var popup = document.createElement("menupopup");
> +          popup.setAttribute("class", this.getAttribute("class"));
> +          popup.setAttribute("generated", "true");
> +          menu.appendChild(popup);
> +          

Trailing whitespace.

::: mailnews/base/util/folderUtils.jsm
@@ +220,5 @@
> +   *
> +   * @param aFolder  The folder to check for recency.
> +   */
> +  function addIfFavorite(aFolder) {
> +    if ((aFolder.flags & Ci.nsMsgFolderFlags.Favorite) && favoriteFolders.length < aMaxHits) {

Please use aFolder.getFlag(i.nsMsgFolderFlags.Favorite), but if we do not need aMaxHits this whole function is unneded.

::: suite/locales/en-US/chrome/mailnews/messenger.dtd
@@ +483,5 @@
>  <!ENTITY contextCopyMsgMenu.accesskey "C">
>  <!ENTITY contextMoveCopyMsgRecentMenu.label "Recent">
>  <!ENTITY contextMoveCopyMsgRecentMenu.accesskey "R">
> +<!ENTITY contextMoveCopyMsgFavoritesMenu.label "Favorites">
> +<!ENTITY contextMoveCopyMsgFavoritesMenu.accesskey "F">

Somehow this hunk does not apply, please check you have the file up to date.

Comment 13

[:aceman]

Guys, do you remember why we have the cludge of passing the recentLabel and recentAccessKey attributes from the caller?
Why can't the folderpicker get the strings from folderWidgets.properties ?
Do we expect the label be different in different contexts? But the content of the submenu does not change in different contexts.

Comment 14

[Mihai]

Hi,

> thank you for the start.
Welcome. IMO it's 90% done.

> The patch is missing the hg headers.
I have generated it using the Hg Workbench from TortoiseHg. Is it usable that way for you?

> Also, the patch is using Windows line endings (CRLF), which makes it not apply. You must use the Linux ones (LF).
I'm currently on Windows, so that explains it. I'll see how to reconfigure it.

> Is there a case where we would NOT want to show Favorites when Recent are also shown? 
Definitely. They could both become user-definable someday. Also, I would otherwise find it unintuitive for one to imply the other. Let's keep both flags please.

> Why is this needed? When would we not want to display all the favorites?
Not for my use case, but I figured some would have a ton of favorites defined for the corresponding folders view, but don't care about the context sub-menus here filling the screen. Thoughts?

> Please use aFolder.getFlag(i.nsMsgFolderFlags.Favorite), but if we do not need aMaxHits this whole function is unneded.
Thanks for the tip.

Comment 15

[Mihai]

> @@ +238,5 @@
> Trailing whitespace.
Good eye. Corrected.

> @@ +660,5 @@
>            sep.setAttribute("generated", "true");
Not seeing it & not my code. It's from _buildRecentMenu.

> @@ +718,5 @@
> +          var popup = document.createElement("menupopup");
> +          popup.setAttribute("class", this.getAttribute("class"));
Not seeing it. On what line exactly?

Comment 16

[Mihai]

Attachment: MoveToFavorites_v0.3.patch

Here's the updated patch based on your feedback, except the items that need clarification / discussion. Use this one for future reviews.

Comment 17

[alta88]

(In reply to :aceman from comment #13)
> Guys, do you remember why we have the cludge of passing the recentLabel and
> recentAccessKey attributes from the caller?
> Why can't the folderpicker get the strings from folderWidgets.properties ?
> Do we expect the label be different in different contexts? But the content
> of the submenu does not change in different contexts.

It seems to just be a place to cache the string using dtd rather than using a string property when dynamically creating a menu; I don't recall anything about it specifically and it seems old.

Comment 18

[Mihai]

On a related front, can someone please help me identify the source control tag for Thunderbird 64.0 beta4? Further details on why this is no obvious here: https://support.mozilla.org/en-US/questions/1243594

TIA.

Comment 19

[Mihai]

Attachment: MoveToFavorites_v0.4.patch

Here's the version with no limit on the no. of favorite folders to display.

Comment 20

[:aceman]

(In reply to Mihai from comment #14)
> > The patch is missing the hg headers.
> I have generated it using the Hg Workbench from TortoiseHg. Is it usable
> that way for you?

It is strange, that the header isn't there created in the patch. But in the worst case we can add it when landing the patch.
 
> > Also, the patch is using Windows line endings (CRLF), which makes it not apply. You must use the Linux ones (LF).
> I'm currently on Windows, so that explains it. I'll see how to reconfigure
> it.

It's that the source file use LF, so you must keep that.
 
> > Is there a case where we would NOT want to show Favorites when Recent are also shown? 
> Definitely. They could both become user-definable someday. Also, I would
> otherwise find it unintuitive for one to imply the other. Let's keep both
> flags please.

OK.
 
> > Why is this needed? When would we not want to display all the favorites?
> Not for my use case, but I figured some would have a ton of favorites
> defined for the corresponding folders view, but don't care about the context
> sub-menus here filling the screen. Thoughts?

Yeah, that would be useful. One possibility is to build (find the favorite folders) only when the user hovers on the Favorites menuitem and intends to open the submenu. We have bug 809066 for analysing the possibilities here.

(In reply to Mihai from comment #15)
> > @@ +718,5 @@
> > +          var popup = document.createElement("menupopup");
> > +          popup.setAttribute("class", this.getAttribute("class"));
> Not seeing it. On what line exactly?

You can see all the surplus whitespace using the "Splinter review" tool on the attachment here on bugzilla. E.g. for the latest attachment it gives this:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=546722&attachment=9032630

Comment 21

[Mihai]

Attachment: MoveToFavorites_v0.5.patch

> It's that the source file use LF, so you must keep that.
The latest patches that I have uploaded are fine in this respect, as I have generated them with "hg diff".

> You can see all the surplus whitespace using the "Splinter review" tool on the
attachment here on bugzilla.
Thanks, there were three whitespace lines - removed now in the this patch.

Can you please see my question above about the source-code tags? Maybe you can shed some light there.

Otherwise, for my needs, I'm done. Please feel free to take over the patch.

Comment 22

[Mihai]

> Can you please see my question above about the source-code tags? Maybe you can shed some light there.
Ignore this question. I just got the answer from Jorg K.

Comment 23

[:aceman]

Comment on attachment 9032868 [details] [diff] [review]
MoveToFavorites_v0.5.patch

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

Thanks you, I think this is ready now. I noticed there is a separator line after Recent and also after Favorites. I'll fix that as the patch needs refreshing anyway to add the hg headers.

Comment 24

[:aceman]

Attachment: 546722.patch v0.6

Unbitrotted patch, added hg headers, removed the duplicate separator line and added the showFavorites to the Go->Folder menu and also appmenu Messages->Move to/Copy to as it is in the main menu.

Comment 25

[:aceman]

Yes, the new function basically duplicates existing code in buildRecentMenu, but I will clean that up as part of bug 809066 where changes are needed in that area.

Comment 26

[:aceman]

Attachment: 546722.patch v0.6.1

Good that I ran tests (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c695b8664824d390ab2edc7853c5cc1ef0d3bd39), the separator fix wasn't right.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e63b335d0e43
show Favourite folders as a separate list in the folder picker. r=aceman,ui-r=bwinton

Comment 28

[Mihai]

Thanks for committing it and making the adjustments.

Comment 29

[Mihai]

@:aceman, thanks for the assignment.