For some reason I hadn't noticed this before. Should be from bug 1231592. Now when I move a message to another folder it's immediately removed and expunged. I'm using the mark-as-deleted imap model, so that's why it's showing, and it's unexpected. And mail.server.default.forceSelect is false by default, so the behaviour should not have changed. Expected result: should not immediately expunge a message after move.

Hmm, I'm surprised. We took great care only to expunge if the server doesn't support MOVE: https://hg.mozilla.org/comm-central/rev/4fd458dcdd4a95a833e577ed0ef2e2b0d176b85e#l7.64 Says: !kHasMoveCapability https://hg.mozilla.org/comm-central/rev/4fd458dcdd4a95a833e577ed0ef2e2b0d176b85e#l7.85 Which server are you using, Magnus? Gmail? that should support move that there shouldn't be any difference. What am I missing? Gene?

It's an Exchange server actually. From https://hg.mozilla.org/comm-central/rev/4fd458dcdd4a95a833e577ed0ef2e2b0d176b85e it looks like you call UidExpunge() when gExpungeAfterDelete is false. That's probably why. Earlier we did nothing.

Sure, we call UidExpunge(messageIdString), so just expunging the message that got moved on a server that didn't support MOVE, so had to be copied and might as well be expunged from the original source. Does that Exchange server really not have MOVE capability? You saw that all this is controlled by: if (GetServerStateParser().LastCommandSuccessful() && (m_imapAction == nsIImapUrl::nsImapOnlineMove) && !(GetServerStateParser().ServerIsAOLServer() || GetServerStateParser().GetCapabilityFlag() & kHasMoveCapability)) So if the server offers MOVE, that UidExpunge(messageIdString) won't run. Sure, for non-MOVE servers the behaviour has changed, but is it really not better now? You *move* something, so why keep a superseded copy in the old located? Try on Gmail, which I understand supports MOVE, and you won't see that.

gmail's even worse. If you mark a message as deleted then move to another folder and then come back, the message goes away. On charter, I can move marked as deleted messages OK but haven't yet tried what I did on gmail. I will keep looking...

Seems OK on charter (no MOVE support). Seem OK on yahoo (has MOVE support). Just bad on gmail. I think the problem with gmail is that there are two different "capability" responses. The first one does *not* include MOVE but the 2nd one does. I *ass*umed that that the MOVE cap. would still be stored up in the 2nd capability response but apparently it isn't. Will continue looking...

Well, even on google I am not doing a call to UidExpunge() when moving. So it it must be seeing MOVE cap and doing a "real" move. However, something is still badly broken when trying to move or copy messages marked as deleted on gmail. Maybe it's the same problem as exchange which I don't have. Is there a "public" exchange imap server I can try?

(In reply to Magnus Melin from comment #0) > For some reason I hadn't noticed this before. Should be from bug 1231592. > > Now when I move a message to another folder it's immediately removed and > expunged. I'm using the mark-as-deleted imap model, so that's why it's > showing, and it's unexpected. > > And mail.server.default.forceSelect is false by default, so the behaviour > should not have changed. > > > Expected result: should not immediately expunge a message after move. I assume this means it is expunged from the destination folder? The server should expunge it from the source folder when a MOVE command occurs. Right?

I asked that already ;-) (In reply to Jorg K (GMT+2) from comment #3) > Sure, we call UidExpunge(messageIdString), so just expunging the message > that got moved on a server that didn't support MOVE, so had to be copied and > might as well be expunged from the original source. > Sure, for non-MOVE servers the behaviour has changed, but is it really not > better now? You *move* something, so why keep a superseded copy in the old > location? So I'm not convinced that the new behaviour is worse then the old. You could just get Magnus to dump out whether the server has MOVE capability, but I guess from what we're seeing we can derive that it doesn't. (In reply to gene smith from comment #6) > ... on google I am not doing a call to UidExpunge() when moving. So it it must be > seeing MOVE cap[ability] and doing a "real" move. > However, something is still badly broken when trying to move or copy messages > marked as deleted on gmail. Why not focus on the bug here and do the Gmail stuff in another bug. So you're moving messages which have been marked as deleted with undesired effects? Note that there are a few problems with Gmail support, see for example bug 618553.

Hmm, what is the Gmail problem exactly, I'm doing this: 1) "Just mark it as deleted" in account settings. 2) Delete a bunch of message in folder A, they are now struck out. 3) Move them to folder B. 4) They disappear from folder A, good. 5) They appear in folder B as non-deleted, questionable, but not lethal. 6) Going back to folder A they don't reappear, good.

(In reply to Jorg K (GMT+2) from comment #9) > Hmm, what is the Gmail problem exactly, I'm doing this: > 1) "Just mark it as deleted" in account settings. > 2) Delete a bunch of message in folder A, they are now struck out. > 3) Move them to folder B. > 4) They disappear from folder A, good. > 5) They appear in folder B as non-deleted, questionable, but not lethal. > 6) Going back to folder A they don't reappear, good. At step 5 I don't see the moved messages at all! Is your folder B "All mail"? Mine's not. I remember now when working with gmail when testing it was "different". Looking at the imap log, when you "delete" something, say in Inbox, tb marks it as /deleted. However, when the next fetch occurs on the folder, the /deleted message doesn't appear in the list sent back. However, it is still in the "All Mail" folder and not mark deleted, so even if expunged from other folders it is still really there. The only way you can really delete an email is move it to Trash and then empty Trash (or mark message in trash as deleted and then compact trash if in that mode). So, in conclusion, I don't think there is not anything that can be done about gmail behavior. Also, I think you are saying that Magnus's only complaint is that the source message is expunged on a move but it displays OK in the destination (struck out). If so, then that is how it should work.

You have to be careful. Gmail uses labels,not IMAP folders, so the delete is simply removing the "delete" label from the email that is already resident in the all mail folder (the gmail archive of "all mail") In gmail all mail is in the "all mail" folder. that same mail can be in as many folders as it has labels attached. Once the labels are all removed the mail still resides in the "all mail folder", the archive. My understanding may be faulty, but only when a delete is issued against the email in the "all mail" folder is it deleted. But we then place it in the deleted folder and undo the delete in practice, because that adds it back to the all mail folder. So delete really does not work. Or didn't. unless you held shift to bypass the delete folder.

Yes, I see that all gmail messages, Inbox, Sent, etc, are dumped into "All mail". What I observer is that I can truly delete emails only if I MOVE them from "All mail" to Trash. I'm still testing in "mark as deleted" mode and this leaves deleted emails X'd out in "All mail" and now present in Trash. Then in trash, I have to delete them again (they become X'd out) and then compact trash for them to go away in trash. In "all mail" they are still X'd out. I then compact "all mail" and they go away there too. If I restart tb and look again in "all mail" the delete messages are still gone. If I look on gmail app on tablet they also appear to be gone. So unless something is fooling me, the mails are actually deleted. Doing shift-delete on an email in "all mail" X's it out but doesn't put it in trash. So a compact on "all mail" brings it right back and removes the X, so it doesn't get actually deleted.

Sorry, I had it in my head that Magnus was moving an email that had already been X'd out. Anyhow, signed up for an outlook.com account and it uses exchange server according to imap.log. Its CAPABILITY does not include MOVE and it supports UIDPLUS. So outlook does the simulated MOVE ending with UidExpunge() and what Magnus now sees is the new "expected". I see the same behavior with Yahoo (that supports MOVE) and with charter (that simulates move just like outlook -- no MOVE, yes UIDPLUS). Gmail also supports MOVE but it does not always expunge the MOVEed messages, depending on source folder, so you sometimes see X'd out messages remaining after they are moved.

Well, sorry, I'm wrong again in Comment 13. Gmail always expunges the moved emails in any source folder when I move emails NOT marked as deleted. I only see problems, as I point out in Comment 10, when I try to move messages that are already marked as deleted, i.e., X'd out. But Jorg says in Comment 9 he doesn't see this problem...? Then again, I don't think this is what Magnus was trying to do.

I'll get back to the details later. But just to clarify, I'm moving a read, but not deleted message to another folder - and that message is then "instantly" removed from the source folder.

Yes, we understood that. But why is that bad? We all agree that the behaviour has changed, but why leave the X-ed message in the source folder? Anyway, this behaviour can be restored easily by checking "Just mark it as deleted" before doing the UidExpunge(messageIdString).

(In reply to gene smith from comment #4) > gmail's even worse. If you mark a message as deleted then move to another > folder and then come back, the message goes away. I didn't understand what you mean exactly here, hence my comment #9. (In reply to gene smith from comment #10) > At step 5 I don't see the moved messages at all! Is your folder B "All > mail"? Mine's not. I was moving messages between "normal folders". > So, in conclusion, I don't think there is not anything that can be done > about gmail behavior. OK. Yes, let's not mix different (non-)issues in this bug. Let's close the discussion about Gmail. > Also, I think you are saying that Magnus's only > complaint is that the source message is expunged on a move ... Yes, that is his complaint. > If so, then that is how it should work. I agree that if we can, and we can, we should *not* leave an X-ed message in the source folder. So we have two options: 1) Magnus is wrong and the changed behaviour is better -> WONTFIX. 2) Magnus is right, so we do what I said in comment #16. In any case, bug 1231592, didn't "break" anything. It changed the behaviour for non-MOVE servers in a well-understood way and we can either accept or revert that.

CAPABILITY IMAP4 IMAP4rev1 AUTH=PLAIN AUTH=NTLM AUTH=GSSAPI SASL-IR UIDPLUS ID UNSELECT CHILDREN IDLE NAMESPACE LITERAL+ ... so no MOVE support. (In reply to Jorg K (GMT+2) from comment #16) > Yes, we understood that. But why is that bad? We all agree that the > behaviour has changed, but why leave the X-ed message in the source folder? That's the point of using the mark-as-deleted model. You get to delete/move messages quickly, and still have a super safe and easy way to get the message back if you changed your mind, or like it happened to me, worked too quickly and moved the message, but it had gone to the wrong folder so it was hard to find. I do see it's simulating a MOVE. Just not sure I like it.

(In reply to Magnus Melin from comment #18) > but it had gone to the wrong folder so it was hard to find. That happens with local message, too ;-) In fact, undo works for IMAP, doesn't it? And if you don't go and find the message, it will live in the wrong location forever. > I do see it's simulating a MOVE. Just not sure I like it. Well, someone has to make a decision. WONTFIX or do comment #16?

(In reply to Jorg K (GMT+2) from comment #19) > And if you don't go and find the message, it will live in the wrong location > forever. Yes but it's easier to find if you know the subject so you can just search for it! > > I do see it's simulating a MOVE. Just not sure I like it. > Well, someone has to make a decision. WONTFIX or do comment #16? Let's see how easy it is to get used to it first.

On charter account (no MOVE support) and still in "just leave marked as deleted" mode, if you move messages to another folder the move can be reversed with menu Edit | Undo (or ctrl-z). It leaves the messages in the errant destination folder X'd out but they completely vanish if you compact the errant destination folder.

(In reply to Jorg K (GMT+2) from comment #8) > Why not focus on the bug here and do the Gmail stuff in another bug. Ok, here's my observation on outlook.com (exchange) and gmail: Bug 1353221

(In reply to Magnus Melin from comment #0) > Now when I move a message to another folder it's immediately removed and > expunged. Not exactly. RFC 6851 states that this is the intended behavior for the IMAP Move command: | [...] the original message is removed from the source mailbox, and | it appears to the client as a single action. This has the same | effect for each message as this sequence: | | 1. [UID] COPY | 2. [UID] STORE +FLAGS.SILENT \DELETED | 3. UID EXPUNGE > I'm using the mark-as-deleted imap model, so that's why it's showing, and it's unexpected. > Expected result: should not immediately expunge a message after move. This would mean that the Move command can not be used in the case of mark-as-deleted.

The behavior is correct (per spec) for a server supporting IMAP MOVE. The question is if that should behavior should be simulated for a server not supporting MOVE.

This change should *not* be part of the beta releases and should not be in future releases (after esr52 I think). This change should only be in future esr52 "dot" releases since it avoids the behavior change pointed out by this bug 1352859. This allows the other charter changes regarding force_select to be in future ers52 releases. (Sorry, I don't know the details of tb release roadmap so I might be using the wrong terms here.) So the beta releases and future releases will have the behavior change/improvement (by *not* including this patch) while only the current releases (esr52?) will have this patch plus the other force_select changes related to charter.

Comment on attachment 8914622 [details] [diff] [review] 1352859-review-changes0.patch - ESR only Magnus, this bug has been very quiet and since you haven't made up your mind what you want. Gene would like to see bug 1231592 and bug 1360117 uplifted to the ESR release to improve the situation for Charter users, and this bug here is stopping us. Here we have a suggestion to maintain the old behaviour for ESR while the new behaviour has been shipping for ages now without a single complaint I'm aware of. So a possible plan could be: WONTFIX this bug going forward, but accept the patch for the ESR.

Comment on attachment 8914622 [details] [diff] [review] 1352859-review-changes0.patch - ESR only Review of attachment 8914622 [details] [diff] [review]: ----------------------------------------------------------------- I don't think it's a good idea to diverge backwards for ESR and trunk. As for this actual bug, I'd suggest not doing the extra expunge for imap mark-as-deleted model users. It really is fairly annoying and unexpected, and I haven't gotten used to it after all this time using trunk. Then of course, ideally get the server fixed as it's clearly a bug.

https://bugzilla.mozilla.org/show_bug.cgi?id=1231592#c50 (In reply to Magnus Melin from comment #27) > Comment on attachment 8914622 [details] [diff] [review] > 1352859-review-changes0.patch - ESR only > > Review of attachment 8914622 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think it's a good idea to diverge backwards for ESR and trunk. > > As for this actual bug, I'd suggest not doing the extra expunge for imap > mark-as-deleted model users. It really is fairly annoying and unexpected, > and I haven't gotten used to it after all this time using trunk. > The following patch will just skip the uid expunge if the mark-as-deleted model is in effect. The other models will still do uid expunge if capability UIDPLUS is supported. This results in no behavior change for a user while still working around the openwave imap server bug(s) -- see more below regarding the bugs. Also, this can remove the divergence between ESR and trunk. > Then of course, ideally get the server fixed as it's clearly a bug. I looked again to find out what the server bug really is. With the default delete model this is what's happening. In inbox, delete an email. It gets copied to Deleted folder and marked as \deleted in inbox. More specifically and for example, the deleted email has UID 2187 in inbox and inbox has UIDNEXT 2190. So UID 2187 gets marked \deleted in inbox and is still present there. In Deleted folder the email has UID 1001 after being copied there. In TB, the deleted message appears gone from Inbox and now is present in Deleted, as expected. Then with Deleted folder selected in tb, the message is then copied back to Inbox. The destination UID in inbox is determined by the server. With Deleted folder selected, all tb does is issue the UID copy command: UID COPY 1001 "Inbox". What the server does next is the problem. It copies message 1001 in Deleted to 2187 in Inbox but does not clear the \deleted flag on 2187. So when Inbox is selected in tb, the message just copied back does not appear in the Inbox list, although it is still actually present in Inbox. There are two possible server bugs here: 1. When the server copies the message back to Inbox it doesn't clear the /deleted flag. The imap rfc says flags should be preserved on a copy so this is wrong. 2. When the message is copied back to Inbox, the server determines that the same message is still present at uid 2187 and marked \deleted, so it seems to copy the message back over itself but leaves the \deleted flag set. In this situation, other servers I have checked copy the message to the UIDNEXT of inbox, 2190 and just abandon UID 2187 leaving it marked \deleted and a candidate for expungment. UID 2190 would be a new message in Inbox with \deleted flag not set so the message would be visible. In what looks like an attempt to optimize storage by not storing duplicate messages, the server causes a bug. I compared TB with the windows-live email client and that client has no problem with this. But the reason is that when folder selection is exited with window-live, an imap CLOSE is done for the exited folder. The imap close causes an effective imap expunge which removes *all* emails marked as \deleted, something that should be avoided. So the server then copies to the UIDNEXT of Inbox when a previously deleted email is copied from Deleted folder back to Inbox since the original UID, e.g., 2187 in the example above, has been expunged (completely removed) from the folder. Note: Reuse of UIDs marked as \deleted is not explicitly forbidden by the RFC (at least I don't see it) but it is not really discussed either. Additional discussion regarding the openwave imap server bug(s) can be found in mailing list thread here: http://mailman13.u.washington.edu/pipermail/imap-protocol/2017-October/002604.html

Comment on attachment 8916485 [details] [diff] [review] 1352859-review-changes1.patch Not much of a code change here, so I let Magnus go first to try it out. The comment is a little on the wordy side, and we try to avoid bug numbers in the code apart from "special" occasions. I'd put: // and don't want them to disappear. + // Only do UidExpunge() when user selected delete method is "Move + // it to this folder" or "Remove it immediately", not when the + // delete method is "Just mark it as deleted".

This is an alternate patch proposal to make the behavior of "Just mark it as delete" mode consistent between whether only COPY is supported or if MOVE is supported by the server. Now in both cases when a message is moved in tb the messages in the source folder will remain in place but crossed out. In the patch you will notice some code marked with "temp test gds". The code supports an AOL server that does "xaol-move" instead of normal imap "move". So I obtained an AOL imap account to test it out. It seem that now the AOL server supports imap MOVE and XAOL-MOVE. However the code does not detect the current AOL server as an AOL server so only normal imap MOVE occurs. (Tb code detects AOL server by the XAOL-OPTIONS capability that doesn't appear in the current AOL server's capability list.) So to detect the current AOL server I added a temporary capability flag for XAOL-MOVE, so I was able to test the actual imap XAOL-MOVE happening, it it seems to do exactly the same thing as "MOVE". If it's decided to use this patch instead of the previous patch, I will remove the temporary test code. From the patch: 1352859: User move always just X's out messages when "mark as deleted" set. This introduces a behavior change in that servers that support imap MOVE will now do a COPY when "Just mark it as deleted" is set and the user does a move. The move user command will now always leave the source message(s) listed but X'd out regardless of whether imap MOVE or only COPY is supported when delete mode is "mark as deleted". When delete mode is "mark as deleted", this does no uid expunge after the server does imap COPY for a user's move command, so source messages(s) remain listed but X'd out. So unlike before, behavior is consistent for message moves in "mark as deleted" mode regardless of MOVE or only COPY capability.

Comment on attachment 8917253 [details] [diff] [review] 1352859-review-changes2.patch Oops, this doesn't do expunge when not "just mark as deleted". So I'm obsoleting the patch and I'll try again.

Comment on attachment 8917253 [details] [diff] [review] 1352859-review-changes2.patch Look again and I think its OK. So removing the obsolete flag. Sorry.

Well, found a bug in the test code for the patch. Any server that supports capability COMPRESS=DEFLATE was detected as supporting XAOL-MOVE. E.g., gmail was sent xaol-move and returned an error. Ok now.

Just to be clear, "Just mark it as deleted" mode, with or without my proposed patches, has a problem with servers that auto-expunge when a message is marked as deleted (i.e., the \deleted flag is stored). When a message is deleted in tb the message will disappear, either immediately or after the folder is re-selected in tb and not just become crossed out. The known servers that have this non-standard feature are: imap.gmail.com imap-mail.outlook.com However, this non-standard but default behavior can be switched off at gmail webmail page for your account under Setting/Forwarding and POP/IMAP: When I mark a message in IMAP as deleted: [ ] Auto-Expunge on - Immediately update the server. (default) [X] Auto-Expunge off - Wait for the client to update the server. <-- Set this to work with "Just mark it as deleted" There does not seem to be a way to switch this off for outlook. This issue is discussed further in Bug 1353221.

Comment on attachment 8916485 [details] [diff] [review] 1352859-review-changes1.patch Review of attachment 8916485 [details] [diff] [review]: ----------------------------------------------------------------- This version looks good to me. r=mkmelin One nit: in the commit comment, please add "Bug" before the bug number.

Comment on attachment 8917592 [details] [diff] [review] 1352859-review-changes3.patch Review of attachment 8917592 [details] [diff] [review]: ----------------------------------------------------------------- I think this is perhaps over complicating things a bit. But maybe a follwoup to fix, or maybe remove the AOL detection?

(In reply to Magnus Melin from comment #36) > One nit: in the commit comment, please add "Bug" before the bug number. I'll fix that when checking it in. I'll also adjust the comment as detailed in comment #30. Gene, do you prefer option 1 or option 2? Or do option 2 in a follow-up?

Jorg, I would say go with option 1, 1352859-review-changes1.patch since it avoids the behavior change. Option 2 was just a preemptive patch to ensure consistent, but changed, move behavior; but it turned out more complicated than I originally expected. So if you can fix up the comment and check it in and I don't have to do anymore on this bug, that would be great!

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/56ff0ad0e707 Avoid behavior change when delete model is "Just mark it as deleted". r=jorgk,mkmelin

Jorg, In and around this comment https://bugzilla.mozilla.org/show_bug.cgi?id=1360117#c72 you mentioned the possibility of moving the patch in this bug and the patch in Bug 1360117 into an earlier release (tb 52 ESR?) since the patch in this bug now eliminates any behavior change caused by the other patch. Is this still possible or is v58, as shown above, the earliest possible target? Just thought I'd ask about this but don't want to get you fired :).

Comment on attachment 8918680 [details] [diff] [review] 1352859-review-changes1.patch - nits fixed I'll uplift this into TB 57 beta and it can go out into TB 52.4.1 together with its friends after passing some time in beta. Sounds good?

Thanks Jorg, sounds good!

Comment on attachment 8918680 [details] [diff] [review] 1352859-review-changes1.patch - nits fixed Goes with bug 1231592 and bug 1360117.

TB 52.5 ESR (should be tracking 57+): https://hg.mozilla.org/releases/comm-esr52/rev/02c07340adbad00ac66bd90d5fe26ffb25d8a875