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

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: tony, Assigned: mihaicodrean)

Tracking

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

9 years ago
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.)
Reporter

Comment 2

9 years ago
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

7 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bwinton)

Comment 4

7 years ago
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.
Depends on: 809066
Version: unspecified → Trunk

Comment 5

7 years ago
Posted image proposal screenshot
This is how it could look like.
Assignee: nobody → acelists
Attachment #679163 - Flags: ui-review?(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.
Attachment #679163 - Flags: ui-review?(bwinton) → ui-review+
Flags: needinfo?(bwinton)

Comment 7

7 years ago
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.
Assignee

Comment 8

6 months ago
Is anyone still working on this? I'm interested and willing to help.
Flags: needinfo?(acelists)

Comment 9

6 months ago
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.
Flags: needinfo?(acelists)
Assignee

Comment 10

5 months ago
Posted patch MoveToFavorites_v0.1.patch (obsolete) — Splinter Review
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.
Assignee

Comment 11

5 months ago
Posted patch MoveToFavorites_v0.2.patch (obsolete) — Splinter Review
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?
Assignee

Updated

5 months ago
Attachment #9030755 - Attachment is obsolete: true

Comment 12

5 months ago
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.
Attachment #9031689 - Flags: feedback+

Comment 13

5 months ago
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.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(alta88)
Assignee

Comment 14

5 months ago
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.
Assignee

Comment 15

5 months ago
> @@ +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?
Assignee

Comment 16

5 months ago
Posted patch MoveToFavorites_v0.3.patch (obsolete) — Splinter Review
Here's the updated patch based on your feedback, except the items that need clarification / discussion. Use this one for future reviews.
Attachment #9031689 - Attachment is obsolete: true

Comment 17

5 months ago
(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.
Flags: needinfo?(alta88)
Assignee

Comment 18

5 months ago
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.
Flags: needinfo?(mkmelin+mozilla)
Assignee

Comment 19

5 months ago
Posted patch MoveToFavorites_v0.4.patch (obsolete) — Splinter Review
Here's the version with no limit on the no. of favorite folders to display.
Attachment #9031829 - Attachment is obsolete: true

Comment 20

5 months ago
(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
Assignee

Comment 21

5 months ago
Posted patch MoveToFavorites_v0.5.patch (obsolete) — Splinter Review
> 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.
Attachment #9032630 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Assignee

Comment 22

5 months ago
> 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

5 months ago
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.
Attachment #9032868 - Flags: review+

Comment 24

5 months ago
Posted patch 546722.patch v0.6 (obsolete) — Splinter Review
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.
Attachment #9032868 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #9034564 - Flags: review+

Comment 25

5 months ago
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 26

5 months ago
Good that I ran tests (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c695b8664824d390ab2edc7853c5cc1ef0d3bd39), the separator fix wasn't right.
Attachment #9034564 - Attachment is obsolete: true
Attachment #9034568 - Flags: review+

Comment 27

5 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 months ago
Target Milestone: --- → Thunderbird 66.0
Assignee

Comment 28

5 months ago
Thanks for committing it and making the adjustments.

Updated

4 months ago
Assignee: acelists → mihaicodrean
Assignee

Comment 29

4 months ago

@:aceman, thanks for the assignment.

You need to log in before you can comment on or make changes to this bug.