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

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

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.

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.

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.

This is how it could look like.

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.

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.

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

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.

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.

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

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.

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.

> @@ +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?

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

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

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.

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

(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

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

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

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.

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.

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

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

Thanks for committing it and making the adjustments.