Strange. I see the invocation of DiscardNewMessage() in otherwise perfect download. I wonder if there is some optimization to remove the last message if that was moved to other folder from Inbox by calling DiscardNewMessage() (Is there some magic expected of DidscardNewMessage() to truncate the Inbox folder?) I sent a series of messages in the following manner: test data for messages have been created. ``` /var/tmp/3C is sent ... will stay in Inbox /var/tmp/2C is sent ... will stay in Inbox /var/tmp/1C is sent ... will stay in Inbox /var/tmp/3D is sent ... moved to folder-D by a message filter. /var/tmp/2D is sent ditto /var/tmp/1D is sent ditto /var/tmp/3E is sent ... moved to folder-E by a message filter. /var/tmp/2E is sent ditto /var/tmp/1E is sent ditto /var/tmp/3F is sent ... moved to folder-D by a message filter. /var/tmp/2F is sent ditto /var/tmp/1F is sent dottp ``` I counted the number of times DiscardNewMessage() are called and I think they are 9 times and you can see they are the number of messages that are moved from the end of Inbox mbox file by the filter. Oh, well, that could be due to the locally added patch to try to enable the compaction for moved message. But I am not sure if that is the correct approach. In the job,: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=dc2f2003fb46537698936c69d20c096a6e9040d2 the patch is https://hg.mozilla.org/try-comm-central/rev/6b255fadebd5af1fda4c86331997e5c935f31f4d In the file, comm/mailnews/search/src/nsMsgFilterService.cpp, filter move shares the code to copy operation. BUT it forgets to set the removal information for the messages copied successfully. The logic of the code, esp. flow is a bit hard to understand. Here is my comment in the patch: ``` + + // mark messages as expunged if fall through from MOVE + // operation Copied verbatim from DELETE + + // XXX BIG QUESTION. + // Not sure if the copy operation just done has successfully + // ended or not. We need to mark the messages as expunged + // ONLY WHEN the COPY succeeds. Anyone care to check the + // logic/flow here? If there are two async operations + // issued in tandem, does the paramters in the following + // line properly refers to the original value as intended? + // The logical flow ought to be + // initiate copy + // -> copy ends successfully + // -> ONLY THEN we mark the original messages as expunged. + + // The following seems insufficient for now? + if (actionType == nsMsgFilterAction::MoveToFolder) { + rv = curFolder->DeleteMessages(m_searchHitHdrs, m_msgWindow, false, + false, nullptr, false /*allow Undo*/); + BREAK_ACTION_IF_FAILURE(rv, "Deleting messages after move failed"); + ``` So this may be invoking DiscardNewMessage() for the just downloaded but moved message. I am not sure if it is working or not at the moment (probably not compaction does not seem to work after the download. Oh, wait, if the message is deleted and not added because of DiscardNewMessage(), then compaction won't be necessary. Neat. But is this what is expected of my patch here to set the messages as expunged by calling DeleteMessages() ? They seem to only mark the messages as expunged. As a matter of fact, the file is still big. I found the messages moved to the other folders are still inside Inbox. So it is buggy, but there seems to be a pattern. ) Is my attempted patch the correct one? Obviously not quite. I will show the incorrect behavior. I wonder if anyone can shed light on this behavior.
Bug 1890253 Comment 1 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Strange. I see the invocation of DiscardNewMessage() in otherwise perfect download. I wonder if there is some optimization to remove the last message if that was moved to other folder from Inbox by calling DiscardNewMessage() (Is there some magic expected of DidscardNewMessage() to truncate the Inbox folder?) I sent a series of messages in the following manner: test data for messages have been created. ``` /var/tmp/3C is sent ... will stay in Inbox /var/tmp/2C is sent ... will stay in Inbox /var/tmp/1C is sent ... will stay in Inbox /var/tmp/3D is sent ... moved to folder-D by a message filter. /var/tmp/2D is sent ditto /var/tmp/1D is sent ditto /var/tmp/3E is sent ... moved to folder-E by a message filter. /var/tmp/2E is sent ditto /var/tmp/1E is sent ditto /var/tmp/3F is sent ... moved to folder-F by a message filter. /var/tmp/2F is sent ditto /var/tmp/1F is sent dottp ``` I counted the number of times DiscardNewMessage() are called and I think they are 9 times and you can see they are the number of messages that are moved from the end of Inbox mbox file by the filter. Oh, well, that could be due to the locally added patch to try to enable the compaction for moved message. But I am not sure if that is the correct approach. In the job,: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=dc2f2003fb46537698936c69d20c096a6e9040d2 the patch is https://hg.mozilla.org/try-comm-central/rev/6b255fadebd5af1fda4c86331997e5c935f31f4d In the file, comm/mailnews/search/src/nsMsgFilterService.cpp, filter move shares the code to copy operation. BUT it forgets to set the removal information for the messages copied successfully. The logic of the code, esp. flow is a bit hard to understand. Here is my comment in the patch: ``` + + // mark messages as expunged if fall through from MOVE + // operation Copied verbatim from DELETE + + // XXX BIG QUESTION. + // Not sure if the copy operation just done has successfully + // ended or not. We need to mark the messages as expunged + // ONLY WHEN the COPY succeeds. Anyone care to check the + // logic/flow here? If there are two async operations + // issued in tandem, does the paramters in the following + // line properly refers to the original value as intended? + // The logical flow ought to be + // initiate copy + // -> copy ends successfully + // -> ONLY THEN we mark the original messages as expunged. + + // The following seems insufficient for now? + if (actionType == nsMsgFilterAction::MoveToFolder) { + rv = curFolder->DeleteMessages(m_searchHitHdrs, m_msgWindow, false, + false, nullptr, false /*allow Undo*/); + BREAK_ACTION_IF_FAILURE(rv, "Deleting messages after move failed"); + ``` So this may be invoking DiscardNewMessage() for the just downloaded but moved message. I am not sure if it is working or not at the moment (probably not compaction does not seem to work after the download. Oh, wait, if the message is deleted and not added because of DiscardNewMessage(), then compaction won't be necessary. Neat. But is this what is expected of my patch here to set the messages as expunged by calling DeleteMessages() ? They seem to only mark the messages as expunged. As a matter of fact, the file is still big. I found the messages moved to the other folders are still inside Inbox. So it is buggy, but there seems to be a pattern. ) Is my attempted patch the correct one? Obviously not quite. I will show the incorrect behavior. I wonder if anyone can shed light on this behavior. Edit: Typo about folder-F fixed.