See bug 380372 for more info...

This also affects editing a message as new, in some cases. We could just reload the saved message into the editor, which would fix everything up, but it could be quite slow, especially in the imap case. We'd like auto-save to be as inconspicuous as possible. I'm not sure how we can fix draft saving to let the compose window know about the correct mime part numbers.

I poked around the message editing and saving code for quite a while and this could be very tricky. I thought I could come up with some mapping between mime parts numbers and cid's, and see if that changes from the time we bring the message into the editor, to when we save as draft, but we regenerate cid's every time we save a message, so we can't use the cids. I had thought about figuring out the mime part numbers by changing nsMsgSendPart to calculate and store the part number, and number the child parts correctly as we build up the tree of parts. But we still need some way of figuring out what the old parts were to see if part1.1.x is different. Perhaps we could set an attribute on the image elements when we load the message that is the part number, and then update that when we save the message. Or, perhaps just set an attribute when we save the message, and use that in the ResetEmbeddedUri's code.

Setting attribute on image node probably will fail when user edit HTML. I mean existing Insert/HTML option, or any extension that allows HTML editing. I think that this parts numbers sould me generated by part of code which actually handle MIME encoding and generate all MIME parts. For example it may generate some mapping table, then some higher level code will use it to update all image URI in edited message. Generated table with mapping may be: key - URI in message, which have been turned into MIME part. value - string with part number (ex 1.3, or 1.1.2). or - whole replacement URI, with part number too Maybe nsMsgComposeAndSend::ProcessMultipartRelated can generate this maping table? Or even it may update HTML... It seems it already do some temporary updates to message DOM.

The only place I can see to figure out the part numbers is in the SendPart handling, as I said earlier. You can't figure them out from the editor. What I might have to do is grab the new cid from the nsMsgAttachmentHandler when each send part is created, and store that in the send part as well (or create a hash table mapping between cid and mime part number stored in the send part, and hang that hash table off the mMsgSend nsMsgComposeAndSend object), and when resetembeddeduri's is called, look up the mime part number from the cid.

This is what I had in mind. It seems to be working fine for me. I'll generate a try server build with it, though I haven't had much luck with try server for the past month or so. The basic approach is that whenever we count multipartrelated attachments, we remember the index in the editor's embedded object list of each domElement as a an attribute called nodeIndex. This gets called right before we save a message as a draft so it's up to date info, and handles new images, deleting old images, etc, before saving. Then, when building up the mime multipart related message to save, we retrieve the node index from the dom node, and stick it in the nsMsgAttachmentData for the attachment. This gets transferred to the nsMsgAttachmentHandler for the attachment. Next, when we're building up the nsMsgSendParts for the message, we store the mime part number in each nsMsgSendPart. In nsMsgComposeAndSend::PreProcessPart, we store the mapping between the dom node index for the nsMsgAttachmentHandler and the corresponding nsMsgSendPart part number in the nsMsgSend object. Finally, after the draft is saved, we use the mapping to find the up to date part number for the embedded image and update the spec to use the new part number.

win32 try server build here - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-03a02437b65e/try-comm-central-win32-debug/thunderbird-10.0a1.en-US.win32.zip

That try build appears to be a debug ? build 28.8 MB looking for MSVCR80D.dll (which I don't have on this test system.) It may run on another system I have with the free MS visual studio tools loaded. I'll try there later.

Well, although I was able to get that build to run, it is way too crashy to do any meaningful testing.

Comment on attachment 568580 [details] [diff] [review] proposed fix >+ m_attachments[i].mNodeIndex = attachment.mNodeIndex; As far as I can tell, attachment.mNodeIndex is always the same as locCount at this point. I say that because I crashed the code here: + if (ma->mNodeIndex != -1) + m_partNumbers.InsertElementAt(ma->mNodeIndex, part->m_partNum); when testing on a message with a) two attached images b) both images inserted twice in the message.

(In reply to neil@parkwaycc.co.uk from comment #9) > Comment on attachment 568580 [details] [diff] [review] [diff] [details] [review] > proposed fix > > >+ m_attachments[i].mNodeIndex = attachment.mNodeIndex; > As far as I can tell, attachment.mNodeIndex is always the same as locCount > at this point. I say that because I crashed the code here: > + if (ma->mNodeIndex != -1) > + m_partNumbers.InsertElementAt(ma->mNodeIndex, part->m_partNum); > when testing on a message with a) two attached images b) both images > inserted twice in the message. do you mean a message with two images as attachments, and then both those images inserted inline twice? Or just a message with two images inserted twice? I tried the latter w/o a problem; I'll try the former. - David

simple steps to reproduce the crash would be helpful. I'm not seeing it here.

Joe, I don't know why the try server build would be debug, but that might account for the crashiness you saw.

Joe, yeah, I gave you a link to the debug build, not realizing we do both debug and opt builds now, and the opt build seems to have not been generated. We have a new try server infrastructure now, and I'm just getting used to it.

Ooops, I was running the wrong build - Neil, I do see the crash. Will investigate.

OK, I can fix the crash, but the larger issue is that when we duplicate inline images, we only generate one cid part, but the code that's avoiding the duplicate parts doesn't communicate this downstream. I think I can remember it in the attachment data such that the duplicate parts know to use the part number of the original part. Re locCount always being the same as nodeIndex, I believe if this code is hit: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#1570 the counts can get off.

wow, this code is fun to work with. This fixes the crash, and fixes the duplicate images problem.

win32 release build w/ the latest patch: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-4ae823da8af6/try-comm-central-win32/thunderbird-10.0a1.en-US.win32.installer.exe

This might be a better link to the try builds: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-4ae823da8af6/ I prefer the zip builds for testing myself Tested the following scenarios: Inserted images before, after and in between existing images Tested with multiple insertions of a single attachment (Important for limiting message size for multiple references..thanks for noticing that Niel) All of the above seem to work fine. Probably beyond the scope of this bug , there remains problems with attachment content other than images. Linked text attachments for one. Steps to reproduce that condition: Highlight some text in an html composition Insert>>Link and embed some content (maybe a sound file) Notice that link attachment does not get converted to the Draft uri when saved Don't get me wrong here, this fix is a great step in the right direction and much appreciated. But it seems there is more to be done.

I also run couple of tests, and all seems to work corectly. Good work :-) As I understand, this fix will be in TB 10 final version?

(In reply to Arivald from comment #19) > I also run couple of tests, and all seems to work corectly. Good work :-) > > As I understand, this fix will be in TB 10 final version? If it gets reviewed and landed before the next aurora branch, then, yes, TB 10, otherwise, TB 11.

Comment on attachment 569391 [details] [diff] [review] fix crash, handle duplicate images >+ // look for earlier part with the same content id. If we find it, >+ // need to remember the mapping between our node index and the >+ // part num of the earlier part. >+ for (PRUint32 j = 0; j < i; j++) >+ if (!m_attachments[j].m_contentId.IsEmpty() && >+ m_attachments[j].m_contentId.Equals(m_attachments[i].m_contentId)) >+ { >+ PRInt32 nodeIndex = m_attachments[i].mNodeIndex; >+ if (nodeIndex != -1) >+ { >+ if (nodeIndex >= (PRInt32) m_partNumbers.Length()) >+ m_partNumbers.SetLength(nodeIndex + 1); >+ if (m_attachments[j].mNodeIndex != -1) >+ m_partNumbers[nodeIndex] = m_partNumbers[m_attachments[j].mNodeIndex]; >+ } >+ } This part is very confusingly written. At the very least you should check that m_attachments[i].mNodeIndex != -1 before doing anything else, and it's probably worth checking m_attachments[j].mNodeIndex != -1 before the contentId. >+ if (ma->mNodeIndex >= m_partNumbers.Length()) >+ m_partNumbers.SetLength(ma->mNodeIndex + 1); We know how many embedded objects there are so can't we preallocate the array? >+ if (node) >+ { >+ nsString nodeIndex; >+ nodeIndex.AppendInt(i); >+ domElement->SetAttribute(NS_LITERAL_STRING("nodeIndex"), nodeIndex); >+ } > bool acceptObject = false; > rv = GetEmbeddedObjectInfo(node, &attachment, &acceptObject); > if (NS_SUCCEEDED(rv) && acceptObject) > count ++; > else > mEmbeddedObjectList->DeleteElementAt(i); So, as you mentioned in your comment, this line means that the embedded object list gets out of sync. I think it would make sense to remove this, because: > bool acceptObject = false; > rv = GetEmbeddedObjectInfo(node, &attachment, &acceptObject); > NS_ENSURE_SUCCESS(rv, NS_ERROR_MIME_MPART_ATTACHMENT_ERROR); >+ m_attachments[i].mNodeIndex = attachment.mNodeIndex; > > if (!acceptObject) > continue; a) we check that the object is acceptable anyway, and b) we can then simplify m_attachments[i].mNodeIndex = locIndex; c) we can remove the mNodeIndex field on the attachment object. The only place to be careful is to move the i++ from inside the for to the end of the block so that it doesn't happen for the !acceptObject case. > NS_IMETHODIMP >+nsMsgComposeAndSend::GetPartForDomIndex(PRInt32 aDomIndex, nsACString &aPartNum) >+{ >+ aPartNum = m_partNumbers[aDomIndex]; m_partNumbers.SafeElementAt(aDomIndex, EmptyCString()); or NS_ENSURE_ARG_RANGE perhaps?

I have one concern about "nodeindex" attribute. It is not very good that this is added permamently to message. I think it should be removed at some point, when it is no longer needed. Also it should be renamed, to _moz_nodeindex, like already used _moz_quote.

this addresses the simple comments; I'll address the rest later today.

(In reply to neil@parkwaycc.co.uk from comment #21) > So, as you mentioned in your comment, this line means that the embedded > object list gets out of sync. I think it would make sense to remove this, > because: > >> a) we check that the object is acceptable anyway, and > b) we can then simplify m_attachments[i].mNodeIndex = locIndex; > c) we can remove the mNodeIndex field on the attachment object. > The only place to be careful is to move the i++ from inside the for to the > end of the block so that it doesn't happen for the !acceptObject case. > It would be great if that approach works - I think it means that I can get rid of the nodeIndex on the dom node as well. I think you mean locCount, not locIndex... I'll give it a try.

Comment on attachment 570853 [details] [diff] [review] address rest of comments Time to link libxul again... >+ // look for earlier part with the same content id. If we find it, >+ // need to remember the mapping between our node index and the >+ // part num of the earlier part. >+ for (PRUint32 j = 0; j < i; j++) >+ { >+ PRInt32 nodeIndex = m_attachments[i].mNodeIndex; >+ if (nodeIndex != -1 && (m_attachments[j].mNodeIndex != -1) && >+ !m_attachments[j].m_contentId.IsEmpty() && >+ m_attachments[j].m_contentId.Equals(m_attachments[i].m_contentId)) Nit: I would move the nodeIndex != -1 check out of the loop. Also I'm still not sure why you check for m_attachments[j].m_contentId.IsEmpty() - if you're worried about comparing two attachments with empty content IDs then you might as well check for m_attachments[i].m_contentId.IsEmpty() up front as well. >- j++; >+ j++; Nit: trailing space crept in here.

sadly, I found a case that doesn't work with my newest patch (removing an old inline image and adding the same one later in the message, iirc, or vice versa). I'll see if that worked with my previous patch before I changed the way we handled the embeddedobject list.

this seems to be working better - it does a better job of ignoring the attachments we used to delete.

fixed on trunk http://hg.mozilla.org/comm-central/rev/1dd58de982ee

I tested this fix for as many combinations of image changes that I could think of including drag and drop of existing images to change their order. All works fine. Thanks a lot for sticking with this David. I can now use auto-save for the very first time.

Joe, thx to you and Arivald for you help with this.

Arivald (Bug 754655 Comment 19) suspects that this might be related with or causing Bug 754655: > We did not test for multiple images with same URL, probably renumbered is only > first one. Bug 754655 uncovers that for multiple identical inline images, part number is missing for all but 1st image, hence subsequent images break. David?