Closed Bug 1831759 Opened 2 years ago Closed 1 years ago

Add support for folder move and copy from folder context menu

Categories

(Thunderbird :: Folder and Message Lists, enhancement)

enhancement

Tracking

(thunderbird_esr102 wontfix)

RESOLVED FIXED
115 Branch
Tracking Status
thunderbird_esr102 --- wontfix

People

(Reporter: gds, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Whiteboard: [snnot3p])

Attachments

(2 files, 4 obsolete files)

Attached patch WIP--add-move-copy-to-folder-context (obsolete) (deleted) — Splinter Review

Currently the only way to move or copy a complete folder is drag and drop. (There is no hint that I've seen in TB that this feature even exists.) This enhancement adds a "Move To" and "Copy To" to the right-click context menu for folders.

This is similar to bug 404955 but does not include implementing Ctrl+C/X/V keys to do the copy, move and paste. Instead, it just uses the same expanding/cascading folder menu that appears when messages are copied or moved using message context menu.

The WIP patch currently assumes the changes from bug 1828372 are in place which would allow "move" of folders across servers but only the messages are actually moved.

Attached file WIP: add-move-copy-to-folder-context-v1.diff (obsolete) (deleted) —

I updated the patch so it no longer depends on changes at bug 1828372 and it can be applied to daily directly.

The only problem I see is that when I copy a folder using context menu to Local Folders or to a POP3 account, the right click context menu doesn't appear if I try to bring it up and I see this printed in linux console:

[Parent 274168, Main Thread] WARNING: Refusing to show duplicate popup: file /home/gene/mozilla2/layout/xul/nsXULPopupManager.cpp:1862

If I comment out the return on error at line 1863 of the above file, the right click produces the context menu with no problem after a copy to Local or POP3.
I don't see this problem when the destination is an imap folder, only when Local/POP3.
Another way to "clear" the failure to see the context menu is to click any menu item like "Help". Then the right-click menu appears again even without a changes in nsXULPopupManager.cpp.

Note: I don't see a problem if I do a drag of a folder to Local/POP3, only if I use the new 'Copy Folder" context menu. (Of course, using D&D there is no context or expanding folder menu that pops up.)

Attachment #9332038 - Attachment is obsolete: true
No longer depends on: 1828372

Awesome, thanks Gene!

Would you be able to file a separate patch for the string changes, and add them to a suitable .ftl file or create one if there's one?
I see that most of the existing translations are there, but afasik we are no longer accepting additions to .dtd files.
We're already 4 days past the string freeze, but if a couple of new and important strings need to land, the sooner the better. This feature would be a significant improvement also wrt accessibility.

Maybe Rob can advise also wrt which (new?) .ftl file these would go into.

Wrt triage: While this isn't directly necessitated by or a specific improvement to sn-3p changes, maybe worth tracking as supernova3p anyway as it affects that space and code directly. Also useful for ensuring this new feature gets mentioned in release documentation if we can land this in time.

Flags: needinfo?(rob)
Whiteboard: [Supernova3p]

Right, the .properties string is fine (albeit confusing). The DTD strings should go into mail/locales/en-US/messenger/messenger.ftl. There's some other menus in about3Pane.xhtml doing that.
Gene -- you're able to submit patches to Phabricator, right? It's hard to review in Bugzilla these days.

Flags: needinfo?(rob)

(In reply to Thomas D. (:thomas8) from comment #2)

Awesome, thanks Gene!

Would you be able to file a separate patch for the string changes, and add them to a suitable .ftl file or create one if there's one?
I see that most of the existing translations are there, but afasik we are no longer accepting additions to .dtd files.
We're already 4 days past the string freeze, but if a couple of new and important strings need to land, the sooner the better. This feature would be a significant improvement also wrt accessibility.

Actualy we're good, [string freeze was changed to the 18th](https://thunderbird.topicbox.com/groups/planning/Tb5916ae842b041c2-M2462b30949d94ad0a5b924e6/reminder-thunderbird-115-string-freeze-may-18, so there are a couple days left.

Flags: needinfo?(gds)

Working on another reported bug right now: Bug 1831969. Will work on those strings ASAP.
Also, will submit the patch as "WIP:" in moz-phab ASAP and a non-WIP one with just the strings (before 18th).

Flags: needinfo?(gds)

I've decide to just use the existing "Copy To" and "Move To" strings, currently used for message transfers, instead of adding new "Copy Folder" and "Move Folder" strings to the .ftl.
I'll also share the existing "Copy to <destination folder> again" and "Move to <destination folder> again" strings currently only used for message transfers. However, I haven't implemented this yet and not sure it's really needed for folder transfers.

So the only new strings are the "confusing" strings added to mail/locales/en-US/chrome/messenger/messenger.properties.
These are only needed because I don't know how to make the folder selection pop-up menu only show the currently selected folder when "Move To" is clicked. And I don't know how to make it not show the currently selected folder when "Copy To" is selected. This is to enforce the same rules that drag and drop of a folder currently enforces:

  • Within a server/account, can only move a folder, you can't copy it.
  • Across servers/accounts, you can only copy a folder, you can't move it.

Yes, this confusing!
Note also, my proposed changes in Bug 1828372 will allow a move of only the messages across servers when "Move To" is selected so the proposed added string "moveFolderInterServer" will not be used if/when that change is accepted.

I'll submit the patch for just the strings first.

Depends on D178144 (doesn't actually depend on this)

Assignee: nobody → gds
Status: NEW → ASSIGNED

Problem with WIP patch of comment 8:

This works as I would expect except for one thing: If I copy (or move) a folder into Local Folders and try to open a context menu on any other folder at any place in the folder pane, nothing pops up. To get another context menu on a folder I have to open a top level menu item (like "Help") and close it or open a context menu on a message and close it. Only then can I open a new context menu on a folder.

Copying into imap folders doesn't have this problem. It only happens for local folders, including folders under POP3 accounts.

I described this in comment 1. I've been unable to find a fix for this.

Edit: Geoff/darktrojan discovered that wrapping the copyFolder() call in a setTimeout() fixes the problem, but not sure why.

Attachment #9334178 - Attachment is obsolete: true
Attachment #9334180 - Attachment is obsolete: true

(In reply to Thomas D. (:thomas8) from comment #2)

Would you be able to file a separate patch for the string changes, and add them to a suitable .ftl file or create one if there's one?

I did this in comment 7 but Rob commented on the submission "Please land the string changes with the UI code that uses them. (not separately)" so I just went ahead and submitted the whole thing as-is in comment 10.
Hopefully, this is what is wanted.

FYI, for readers not subscribed to https://bugzilla.mozilla.org/attachment.cgi?id=9334667, rjl said the strings were OK but couldn't review patch. So I added mkmelin and aleca as reviewers there.
(Not sure if this is the correct procedure to just get the strings approved on the stated deadline, 5-18, as requested in comment 4 and earlier.)

Edit 6-2-23: Now just silently switching from "move to copy" or from "copy to move" when appropriate rather than showing warning (Magnus' suggestion). So adding new strings is no longer an issue.

Attachment #9334667 - Attachment description: Bug 1831759 -- Adds folder move/copy context menus. r=rjl → Bug 1831759 -- Adds folder move/copy context menus. r=darktrojan,mkmelin
Attachment #9332966 - Attachment is obsolete: true
Target Milestone: --- → 115 Branch

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/eeafb6188146
Adds folder move/copy context menus. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 1 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b8c51cf12d2b
Fix test failure and avoid showing "move" context menu on smart tag folders. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 1 years ago1 years ago
Resolution: --- → FIXED

I've noticed an inconsistency with this change.

If you try to drag a folder within the same account/server and hold down ctrl key to cause a "copy" instead of the default "move", the + symbol doesn't appear and no transfer occurs at all on drop. (This is the original behavior before this patch landed.)

If you right click a folder and use the new "copy to" option and try to copy a folder to another folder in the same account/server, we now, with the landed patch, silently do a "move" of the folder instead.

To resolve this there are options:

  1. Do nothing when right-click "copy to" is directed to the same account/server instead of changing the operation to "move to".
  2. Or, change the folder drag with ctrl held down so it also does a "move" when dropped onto a folder on the same account/server.
  3. Or, change both drag with ctrl pressed and right-click "copy to" to allow actual copy of folder within the same account/server. I can't find documentation or bug reports on why this is not allowed in the UI but the backend allows it and unit tests actually tests that folders can be copied within the same account/server.

The change to implement any of these is fairly simple. I implemented option 3 and don't see any big problems when I allow copy of folders inside the same account/server. The only issue I see is that if the copy is done twice (e.g., using the right-click "again" option) you can end up with duplicate messages inside the destination folder; however, some servers like gmail and intermail auto-expunge the duplicates so you never see them.

Blocks: 1838844
No longer blocks: sn-folderpane
Whiteboard: [Supernova3p] → [snnot3p]
Blocks: 1842514
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: