Closed Bug 440366 Opened 14 years ago Closed 13 years ago

all keywords/tags of a mail will be deleted if I cut off (not delete) attachments

Categories

(Thunderbird :: General, defect, P1)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: xem021-misc, Assigned: Bienvenu)

Details

(Keywords: dataloss)

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: DE Thunderbird Version 2.0.0.14 (20080421)

all keywords of a mail are no longer present after I cut off (not delete) attachments

Reproducible: Always

Steps to Reproduce:
1. add keyword to mail
2. cut off attachment of this mail

Actual Results:  
added keyword is no longer present

Expected Results:  
added keyword is still present
confirm, happens for message in imap or local
1. tag
2. delete attachment

David do you remember seeing this?  sounds like a dupe but I'm not finding one - it's not bug 370440
Wayne, yikes. I have not heard of this bug before. I'll look into it.

confirming based on your confirm comment :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: all keywords of a mail will be deleted if I cut off (not delete) attachments → all keywords/tags of a mail will be deleted if I cut off (not delete) attachments
I am seeing the same thing.  When I detach (or remove) an attachment from a message with only one attachment, the tags for that mail message are being removed.  
This is happening for TB on Windows and Linux.

Please note: 
- I've tagged one message with three tags (keyboard input 1, 2, 3).  After 
  detaching the attachment the first two tags were removed, the third one was 
  still available.  
- When I'm using just one tag, that tag is removed.
- When I'm tagging (3, 2, 1) the third tag only is preserved
- When I'm tagging (7, 8, 9) all tags are removed.
didn't test, but perhaps affects other things too - star, ...
Flags: blocking-thunderbird3?
Keywords: dataloss
marking blocking
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Assignee: nobody → bienvenu
Priority: -- → P1
Target Milestone: --- → Thunderbird 3.0b1
taking - I was able to reproduce this.
Status: NEW → ASSIGNED
Attached patch one possible approach (obsolete) — Splinter Review
I started down this path, which is to make detaching attachments use CopyFileMessage with an original message header, so the copy action can delete the original message, and propagate the various flags naturally. I got this working for local messages, with some scary changes, and then tested IMAP, which didn't work. I'll spend a little time looking at why IMAP broke, but I suspect a much less risky approach is going to be to make the detach code propagate properties from the old header to the new header. It would be a really nice simplification if I could get the copy service to do the work for me, but changing the core copy service is scary.
IMAP CopyFileMessage doesn't delete the original message, even if msgToReplace is set - I could fix that, but I'm going to go for the safe approach for b1.
Attached patch proposed fix (obsolete) — Splinter Review
It occurred to me that it might be useful for other callers of CopyFileMessage to be able to set keywords (e.g., if an extension wanted to create a message from a file, and set tags on that message), so I bit the bullet and changed all the callers and impls to pass in an optional keyword string. This way, we can set the keyword directly when we append the msg to the imap server, in the imap case.

I had to rearrange some of the notification code so that I could set the keywords on the local messages after we've released the semaphore. This allowed me to get the test I added to pass, but it should help other listeners should there be any.
Attachment #334965 - Attachment is obsolete: true
Attachment #334987 - Flags: superreview?(neil)
Attachment #334987 - Flags: review?(neil)
Assignee: bienvenu → nobody
Status: ASSIGNED → NEW
messed up assignment.  undoing.
Assignee: nobody → bienvenu
Seeing if Standard8 has time for this review - I'd like to get this in for b1 since it is dataloss of a sort.
Attachment #334987 - Attachment is obsolete: true
Attachment #335572 - Flags: superreview?(neil)
Attachment #335572 - Flags: review?(bugzilla)
Attachment #334987 - Flags: superreview?(neil)
Attachment #334987 - Flags: review?(neil)
Comment on attachment 335572 [details] [diff] [review]
missed some test changes in my previous patch

I think there must be something missing from this patch, my build stopped at nsMessenger.cpp with these errors:
nsMessenger.cpp(1637) : error C2660: 'nsIMsgCopyService::CopyFileMessage' : function does not take 8 arguments
nsMessenger.cpp(2570) : error C2660: 'nsIMsgCopyService::CopyFileMessage' : function does not take 8 arguments

>   rv = copyRequest->Init(nsCopyFileMessageType, fileSupport, dstFolder,
>                          isDraft, aMsgFlags, listener, window, PR_FALSE);
>   if (NS_FAILED(rv)) goto done;
>+  copyRequest->m_newMsgKeywords = aNewMsgKeywords;
IMHO it would be cleaner to add a parameter to Init.

>   copyService.CopyFileMessage(bugmail1, gLocalInboxFolder, null, false, 0,
>-                              copyListener, null);
>+                              "", copyListener, null);
There are a number of changes like this, but that extra parameter is an const nsACString&, not a const char*, so this should be EmptyCString(), right? (I've seen VC allow something similar for NS_NewURI when it shouldn't.)

>+      if (aKeywords  && (supportedFlags & kImapMsgSupportUserFlag))
Nit: you have two spaces before &&

sr=me once this compiles ;-)
Attachment #335572 - Flags: superreview?(neil) → superreview+
the nsIMsgCopyService.idl changes were missing from the patch somehow...

>   copyService.CopyFileMessage(bugmail1, gLocalInboxFolder, null, false, 0,
>-                              copyListener, null);
>+                              "", copyListener, null);
>There are a number of changes like this, but that extra parameter is an const
>nsACString&, not a const char*, so this should be EmptyCString(), right? (I've
>seen VC allow something similar for NS_NewURI when it shouldn't.)

these are js changes so "" is OK, isn't it? All the c++ calls use EmptyCString().

I'll fix the Init call and attach a patch that should build :-)
Attached patch address Neil's comments... (obsolete) — Splinter Review
carrying forward Neil's sr, after addressing comments, requesting r from Standard8 for new patch.
Attachment #335572 - Attachment is obsolete: true
Attachment #335882 - Flags: superreview+
Attachment #335882 - Flags: review?(bugzilla)
Attachment #335572 - Flags: review?(bugzilla)
Comment on attachment 335882 [details] [diff] [review]
address Neil's comments...

I just tried this on an IMAP message that I'd just sent myself and tagged with the default tags (probably keys 1 and 2). When I detached the attachment, the tags weren't kept.

This did work on local folders though.


>diff --git a/mailnews/base/public/nsIMsgCopyService.idl b/mailnews/base/public/nsIMsgCopyService.idl

This file needs a uuid bump.

> NS_IMETHODIMP
> nsMsgLocalMailFolder::OnCopyCompleted(nsISupports *srcSupport, PRBool moveCopySucceeded)
> {
>   if (mCopyState && mCopyState->m_notifyFolderLoaded)
>     NotifyFolderEvent(mFolderLoadedAtom);
> 
>-  delete mCopyState;
>-  mCopyState = nsnull;
>   (void) RefreshSizeOnDisk();
>   // we are the destination folder for a move/copy
>+  PRBool haveSemaphore;
>+  nsresult rv = TestSemaphore(static_cast<nsIMsgLocalMailFolder*>(this), &haveSemaphore);
>+  if(NS_SUCCEEDED(rv) && haveSemaphore)

nit: not you fault, but as you're touching it, please insert a space between the if and the (

>+// Test of setting keywords with CopyFileMessage
>+
>+const copyService = Cc["@mozilla.org/messenger/messagecopyservice;1"]
>+                      .getService(Ci.nsIMsgCopyService);
>+const bugmail11 = do_get_file("../mailnews/test/data/bugmail11");
>+                     
>+// main test
>+
>+var hdrs = [];
>+
>+// tag used with test messages
>+var tag1 = "istag";
>+
>+function run_test()
>+{
>+
>+  loadLocalMailAccount();
>+
>+  // bugmail11 has no keywords.    
>+  copyService.CopyFileMessage(bugmail11, gLocalInboxFolder, null, false, 0, tag1,
>+                              copyListener, null);

This should have a call to do_test_pending() before it...(I'm assuming this call is likely to be async).

>+  return true;
>+}
>+
>+// nsIMsgCopyServiceListener implementation
>+var copyListener = 
>+{
>+  OnStartCopy: function() {},
>+  OnProgress: function(aProgress, aProgressMax) {},
>+  SetMessageKey: function(aKey)
>+  {
>+    hdrs.push(gLocalInboxFolder.GetMessageHeader(aKey));
>+  },
>+  SetMessageId: function(aMessageId) {},
>+  OnStopCopy: function(aStatus)
>+  {
>+    var copiedMessage = gLocalInboxFolder.GetMessageHeader(hdrs[0]);
>+    do_check_eq(copiedMessage.getStringProperty("keywords"), tag1);

Now that you've finished the test, you need a do_test_finished();
My guess is that your server doesn't support user-defined keywords.  I can deal with that, but I'd rather do it in a follow on...
Attachment #335882 - Attachment is obsolete: true
Attachment #335901 - Flags: review?(bugzilla)
Attachment #335882 - Flags: review?(bugzilla)
Mark sent me a protocol log that showed his server does NOT support user-defined keywords, so the behavior he's seeing is expected.
Whiteboard: waiting standard8 review
Attachment #335901 - Flags: review?(bugzilla) → review+
fix checked in
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: waiting standard8 review
OS: Windows XP → All
Hardware: PC → All
Will be this fix available for Thunderbird 2.* ?
You need to log in before you can comment on or make changes to this bug.