1718839 - Archive message action does not work

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[lemon_juice]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 SeaMonkey/2.53.7.1

Steps to reproduce:

In SeaMonkey 2.53.8 moving messages to archive does not work (Message > Archive or Shift+A). I noticed this problem after upgrading from 2.53.6 to 2.53.8. This doesn't work both for IMAP and POP3 accounts. My folder settings are default: archiving should move the message to the Archive folder on the account.

After some unsuccessful testing I reverted to 2.53.7.1 and archiving started to work again.

Comment 1

[Frank-Rainer Grahl (:frg)]

Error: An error occurred executing the cmd_archive command: ReferenceError: notificationService is not defined
Source File: chrome://global/content/globalOverlay.js
Line: 95

Comment 2

[Frank-Rainer Grahl (:frg)]

Timestamp: 01.07.2021, 14:05:59
Error: ReferenceError: EnsureFolderIndex is not defined
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 1271

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1718839-notificationservice-2539.patch

Fix for first issue

[Approval Request Comment]
Regression caused by (bug #): Bug 1694760
User impact if declined: no archiving
Testing completed (on m-c, etc.): 2.53.9b1pre
Risk to taking this patch (and alternatives if risky): trivial
String changes made by this patch: --

Comment 4

[Ian Neal]

Comment on attachment 9229516 [details] [diff] [review]
1718839-notificationservice-2539.patch

[Triage Comment]
Sorry, my bad r/a=me

Comment 5

[Ian Neal]

Attachment: WIP-1718839-EnsureFolderIndex-v1_1-2539.patch

I noticed whilst testing another bug. If the Archive folder isn't showing (e.g. if the parent of the Archive folder isn't expanded), it doesn't seem to properly create the relevant folder (you have to close and reopen the mail window). Perhaps we need to do an earlier:
EnsureFolderIndex(gFolderTreeView, dstFolder.parent); or
EnsureFolderIndex(gFolderTreeView, archiveFolder.parent);

Comment 6

[Dmitry Butskoy (:buc)]

Yet another if (!index) { nested block with msgFolder.parent.parent seems help... :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/25efd35b0559
Replace undefined notificationService with correct Mailservices define. r=IanN

Comment 11

[Rainer Bielefeld]

WFM with installation of unofficial (by wg9s) De SeaMonkey 2.53.9 beta 1 Mozilla/5.0 (NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Build 20210707210003 (Newly created User Profile, Default Classic Theme) on German WIN7 64bit. Result of the push or something magic?

Comment 12

[Ian Neal]

Attachment: 1718839-EnsureFolderIndex-v1_2-2539.patch

This version fixes the opening of the folder, but there is still the issue when copying from NNTP that it doesn't seem to release the folder it is copying too until the application is restarted.

Comment 13

[Dmitry Butskoy (:buc)]

The problem for the news archiving is that news folders should have "canDeleteMessages" flag set to false, but it is not actually set anywhere.
(You can even try to Delete a message from the news folder, which even shows some success).

There is no "Move To" item in the context menu for news, because other check is perfomed for this. But all the other places use check for "canDeleteMessages", including when archiving.

Currently this flag is set for true for IMAP readonly folders only, and is not set for news folders.

So when you try to do "Archive" for a news message, the code actually tries to perform MOVE, not COPY. Then probably some code components start to wait for the finish of the moving, which it is never finished since the actual removing from the news is impossible. This leads to a permanent lock.

So, this one fixes archiving from the news folder:

--- comm/suite/mailnews/content/mailWindowOverlay.js.orig       2021-05-25 21:36:20.000000000 +0300
+++ comm/suite/mailnews/content/mailWindowOverlay.js    2021-07-13 18:13:27.630261297 +0300
@@ -1273,7 +1273,7 @@ BatchMessageMover.prototype =
       // If the source folder doesn't support deleting messages, we
       // make archive a copy, not a move.
       MailServices.copy.CopyMessages(srcFolder, moveArray, dstFolder,
-                                     srcFolder.canDeleteMessages, this,
+                                     (srcFolder.canDeleteMessages && !gFolderDisplay.selectedMessageIsNews), this,
                                      msgWindow, true);
       return; // continues with OnStopCopy
     }

But a better way is to set "canDeleteMessages" for the news folders properly.

TB affected as well.

Comment 14

[Dmitry Butskoy (:buc)]

s/Currently this flag is set for true for IMAP readonly/Currently this flag is set to "false" for IMAP readonly/
typos...

Comment 15

[Dmitry Butskoy (:buc)]

The default "true" for this flag is set in nsMsgDBFolder::GetCanDeleteMessages(), and only imap has its own. So probably such a method should be add somewhere in mailnews/news/src/nsNewsFolder.cpp and always (?) return "false".

Comment 16

[Ian Neal]

Attachment: 1166225-news-getcandeletemessages-2539.patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: archiving doesn't work on news servers
Testing completed (on m-c, etc.): 2.53.9
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

This is 2.53.x version of patch from Bug 1166225

Comment 17

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9231045 [details] [diff] [review]
1166225-news-getcandeletemessages-2539.patch

Isn't the cure worse that the issue with the patch applied? Previously "Delete Message" worked. With the patch I can no longer remove newsgroup messages and see no easy way to do so other than probably expire them folder wide. I would rather make "copy or move fully work or use the mentioned workaround.

If you use Cancel Messages on messages not from you a similar "hang" still occurs. Not sure if related.

Comment 18

[Dmitry Butskoy (:buc)]

Other news clients -- fe. Evolution -- allows to "delete" messages in the same manner as it is in SM for now. (And even allows to "move").

So the work-around from bug #1718839 comment #13 should go further then.

Comment 19

[Ian Neal]

Attachment: 1166225-news-copynotmove-2539.patch

As adding canDeleteMessages to news breaks other stuff, let's go this way instead.

Comment 20

[Dmitry Butskoy (:buc)]

(In reply to Frank-Rainer Grahl (:frg) from comment #17)

If you use Cancel Messages on messages not from you a similar "hang" still occurs. Not sure if related.

Seems just a permanetly busy throbber occurs.

The following fixes this:

--- comm/mailnews/news/src/nsNNTPProtocol.cpp.orig      2021-07-14 02:06:22.964490563 +0300
+++ comm/mailnews/news/src/nsNNTPProtocol.cpp   2021-07-14 02:06:47.736331827 +0300
@@ -3665,13 +3665,7 @@ nsresult nsNNTPProtocol::DoCancel()
       rv = dialog->Alert(nullptr, alertText.get());
       // XXX:  todo, check rv?

-      /* After the cancel is disallowed, Make the status update to be the same as though the
-         cancel was allowed, otherwise, the newsgroup is not able to take further requests as
-         reported here */
       status = MK_NNTP_CANCEL_DISALLOWED;
-      m_nextState = NNTP_RESPONSE;
-      m_nextStateAfterResponse = NNTP_SEND_POST_DATA_RESPONSE;
-      SetFlag(NNTP_PAUSE_FOR_READ);
       failure = true;
       goto FAIL;
     }

The idea is to interprete the case "try to cancel, but not allowed" in the same way as the case "start to cancel, but then changed their mind" -- see here.

The code removed appeared a long time ago (was inherited from CVS in 2008). No idea why it was added and when, but probably it is no more needed nowadays?

Comment 21

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #19)

Created attachment 9231074 [details] [diff] [review]
1166225-news-copynotmove-2539.patch

As adding canDeleteMessages to news breaks other stuff, let's go this way instead.

Thanks, works as expected.

Comment 22

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9231074 [details] [diff] [review]
1166225-news-copynotmove-2539.patch

LGTM Maybe a 100% solution which also deletes the message will be found in Bug 1166225 but till then good enough. Probably best to implement move. Move works for rss folders and there it makes even less sense.

r/a+ for 2.53 and 2.57.

Comment 23

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9230635 [details] [diff] [review]
1718839-EnsureFolderIndex-v1_2-2539.patch

LGTM

Comment 24

[Dmitry Butskoy (:buc)]

If you drug-and-drop some news message into a local folder, the behavior will be the same as with Archive before the patch.

There is no "Move To" in the context menu for news (and user can do copy only), but a move by "drug-and-drop" is allowed.

Comment 25

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #19)

Created attachment 9231074 [details] [diff] [review]
1166225-news-copynotmove-2539.patch

Besides the "copynotmove" check for Archive item, we should also implement the similar "copynotmove" check for general drag-and-drop case.
(The general case for the context menu is handled in a different way, as was mentioned before).

So this change fix the issue:

If you drug-and-drop some news message into a local folder, the behavior will be the same as with Archive before the patch.

--- comm/suite/mailnews/content/folderPane.js.orig      2021-05-25 21:36:20.000000000 +0300
+++ comm/suite/mailnews/content/folderPane.js   2021-07-14 20:12:59.771070262 +0300
@@ -627,7 +627,8 @@ let gFolderTreeView = {
       let isMove = Cc["@mozilla.org/widget/dragservice;1"]
                       .getService(Ci.nsIDragService).getCurrentSession()
                       .dragAction == Ci.nsIDragService.DRAGDROP_ACTION_MOVE;
-      if (!sourceFolder.canDeleteMessages)
+      let isNews = sourceFolder.flags & Ci.nsMsgFolderFlags.Newsgroup;
+      if (!sourceFolder.canDeleteMessages || isNews)
         isMove = false;

       Services.prefs.setCharPref("mail.last_msg_movecopy_target_uri",

Comment 26

[Ian Neal]

Attachment: 1166225-news-copynotmove-v1_1-2539.patch

Changes since last version:

• Fixes the drag and drop issue mentioned by buc;
• Disables move option in Message menu when it is a news message.

Comment 27

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9231246 [details] [diff] [review]
1166225-news-copynotmove-v1_1-2539.patch

LGTM. Because this is essentially a suite bug right now putting it in comm-central with commit message Bug 1718839 ... and refer to Bug 1166225 in the commit on a separate line:

Bug 1718839 - Archive function for NNTP news post renders target Archive folder unusable until restart. r=frg a=frg

See Bug 1166225 which tracks this for Thunderbird.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/486d408a5fc6
Replace obsolete EnsureFolderIndex in suite. r=IanN
https://hg.mozilla.org/comm-central/rev/6d2ee1b035b0
Archive function for NNTP news post renders target Archive folder unusable until restart. r=frg

Comment 30

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/1d0f8622c9c0a080fee650591b7c2ac1da18a812
Replace undefined notificationService with correct Mailservices define. r=IanN a=IanN

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/2df5aed92ccd5d2710e65212d99601c638dc8244
Replace obsolete EnsureFolderIndex in suite. r=IanN a=IanN

Archive function for NNTP news post renders target Archive folder unusable until restart. r=frg a=frg
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/477bfe19a99bfbade62089b038868038cdd73ca1

Comment 31

[Aarron Pina]

Ok, so what does this all mean to the layman?
How do I fix this? I see a bunch of code at the top of the page under attachments - is this the solution to the problem? How do I make this all work?

Comment 32

[Frank-Rainer Grahl (:frg)]

How do I make this all work?

Update to 2.53.8.1

Comment 33

[Aarron Pina]

(In reply to Frank-Rainer Grahl (:frg) from comment #32)

How do I make this all work?

Update to 2.53.8.1

Thank you. Done and done.