Closed Bug 1718839 Opened 3 months ago Closed 2 months ago

Archive message action does not work

Categories

(SeaMonkey :: MailNews: General, defect)

defect

Tracking

(seamonkey2.53+ fixed)

RESOLVED FIXED
seamonkey 2.89
Tracking Status
seamonkey2.53 + fixed

People

(Reporter: michal-ok, Assigned: frg)

References

(Regression)

Details

(Keywords: regression, Whiteboard: SM2.53.8.1)

Attachments

(3 files, 3 obsolete files)

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.

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Regressed by: 1694760
Version: SeaMonkey 2.53 Branch → Trunk

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

Regressed by: 507601

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

Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #9229516 - Flags: review?(iann_bugzilla)
Attachment #9229516 - Flags: approval-comm-release?
Attachment #9229516 - Flags: approval-comm-esr60?

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

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

Attachment #9229516 - Flags: review?(iann_bugzilla)
Attachment #9229516 - Flags: review+
Attachment #9229516 - Flags: approval-comm-release?
Attachment #9229516 - Flags: approval-comm-release+
Attachment #9229516 - Flags: approval-comm-esr60?
Attachment #9229516 - Flags: approval-comm-esr60+

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

Attachment #9229660 - Flags: feedback?(frgrahl)

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

Keywords: leave-open
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/25efd35b0559
Replace undefined notificationService with correct Mailservices define. r=IanN
Duplicate of this bug: 1719564
Duplicate of this bug: 1719819
Duplicate of this bug: 1719980

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?

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.

Attachment #9229660 - Attachment is obsolete: true
Attachment #9229660 - Flags: feedback?(frgrahl)
Attachment #9230635 - Flags: review?(frgrahl)
Attachment #9230635 - Flags: approval-comm-release?
Attachment #9230635 - Flags: approval-comm-esr60?

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.

Flags: needinfo?(iann_bugzilla)

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

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

Depends on: 1166225

[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

Flags: needinfo?(iann_bugzilla)
Attachment #9231045 - Flags: review?(frgrahl)
Attachment #9231045 - Flags: approval-comm-release?
Attachment #9231045 - Flags: approval-comm-esr60?

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.

Flags: needinfo?(iann_bugzilla)

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.

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

Attachment #9231045 - Attachment is obsolete: true
Attachment #9231045 - Flags: review?(frgrahl)
Attachment #9231045 - Flags: approval-comm-release?
Attachment #9231045 - Flags: approval-comm-esr60?
Flags: needinfo?(iann_bugzilla)
Attachment #9231074 - Flags: review?(frgrahl)
Attachment #9231074 - Flags: approval-comm-release?
Attachment #9231074 - Flags: approval-comm-esr60?

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

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

Attachment #9231074 - Flags: review?(frgrahl)
Attachment #9231074 - Flags: review+
Attachment #9231074 - Flags: approval-comm-release?
Attachment #9231074 - Flags: approval-comm-release+
Attachment #9231074 - Flags: approval-comm-esr60?
Attachment #9231074 - Flags: approval-comm-esr60+

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

LGTM

Attachment #9230635 - Flags: review?(frgrahl)
Attachment #9230635 - Flags: review+
Attachment #9230635 - Flags: approval-comm-release?
Attachment #9230635 - Flags: approval-comm-release+
Attachment #9230635 - Flags: approval-comm-esr60?
Attachment #9230635 - Flags: approval-comm-esr60+

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.

(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",
Flags: needinfo?(iann_bugzilla)

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.
Attachment #9231074 - Attachment is obsolete: true
Flags: needinfo?(iann_bugzilla)
Attachment #9231246 - Flags: review?(frgrahl)
Attachment #9231246 - Flags: approval-comm-release?
Attachment #9231246 - Flags: approval-comm-esr60?

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.

Attachment #9231246 - Flags: review?(frgrahl)
Attachment #9231246 - Flags: review+
Attachment #9231246 - Flags: approval-comm-release?
Attachment #9231246 - Flags: approval-comm-release+
Attachment #9231246 - Flags: approval-comm-esr60?
Attachment #9231246 - Flags: approval-comm-esr60+
Whiteboard: SM2.53.8.1
Blocks: 1720686
Pushed by frgrahl@gmx.net:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Future
Duplicate of this bug: 1721366

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

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?

How do I make this all work?

Update to 2.53.8.1

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

Target Milestone: Future → seamonkey 2.89
You need to log in before you can comment on or make changes to this bug.