It seems that bug 288702 has broken some of the original behavior. Some were fixed in the meantime, so in bug 292568 (overwriting only for drafts & templates, see bug 292568 comment 11) and with bug 317396 (overwriting only for drafts, see bug 317396 comment 9). BUT this is still a different behaviour as Mozilla 1.7/TB 1.0 and as expected by many users if someone does a "Edit As New" (see bug 292568 comment 14, bug 288062 comment 11). The command is "Edit (Message) As New" not simply Edit ... Expected Result: Edit a draft message as new and later saving should NOT overwrite the original message in the drafts folder.

"loss of data" -> Severity = Critical

This is a horrible, horrible bug. This bug destroys data! It makes "Edit as New" completely meaningless when applied to drafts. Every month I lose time because of this bug. Please, please, please fix it.

In the same vein, when you "edit a draft" in the drafts folder of an account not the default, the from address dropdown is reset to the default. When you save, the message is saved in the drafts folder of the default address and deleted from the drafts folder of the original non-default address.

this seems really bad - nominating wanted. workaround is to use template - does not happen with template, only draft seen with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090515 Shredder/3.0b3pre

Would be good to fix for tb3.

Adding my voice to the need to fix this . . I'm sure I'm not the only one to have a really bad data-loss experience with it ... Typical scenario Need to write similar message to two people . Write message A - save it Drafts -> select message -> Edit message as New Save it Hit send Message A has disappeared - all the work that went into it lost, even worse, I probably miss-remember it as having been sent. Using Templates isn't an answer to this, this is a data-loss bug where the expected behavior doesn't happen, and the result is loss of data.

OS Vista Thunderbird version 24.5.0 Request two options with a saved draft email. 1. Edit draft This opens in a new Write message for editing the original draft and when I save it as a draft, it overwrites the orginal. 2. Edit as new message This opens in a new Write message for editing, but when I save as draft, it does not overwrite the orignal instead it saves it as a new draft message. currently, I have to either open draft using edit as a new message option and open a new Write message, then copy paste all the saved draft content into new Write message and then edit the section required - then save the new Write message as the second edited draft. note these two drafts have very similar content but go to different people. So I need both as separate drafts. OR I have to open the original draft as edit as a new message, edit content and save the orginal draft as a template and save in that folder. You cannot move it from template to drafts as it still overwrites. This is a workarounnd but not a satisfactory solution.

I'm using Thunderbird 24.6.0 on Windows 7 Home Premium Service Pack 1 (64-bit). Having saved a draft message, I expect that double-clicking it would open the draft to allow me to work on it further, while Edit as New should create a new message from the saved draft, which can also be saved as a new draft and subsequently sent completely independently of the original draft. It seems that Edit as New doesn't currently work like this - and I think it should!

OMG! *#!#+§$%&!? Please CC me to such bad bugs. It might give me a heart attack, but otherwise it's better to look the monster into the eye and face the truth. That monster which grew in full view of its official caretakers, and in fact they were always too few to ever gain full control and really "take care", clean, nourish, and grow the animal into something beautiful and domesticated. So as the beast became more ugly over time from systematic neglection, surprise - the crowds in their masses somehow never got warm with such a thing, whose inner beauty, strength and full potential was hard to see from a rough outside. Worse, so many specific appeals from the crowds how to domesticate the thing and how to increase the attraction just went with the wind unheard. So in something like a self-fulfilling prophecy, the top caretakers decided that well, if people are not interested in this creature as we offer it now, with that little care we're doing, why should we care? Let's release the beast into the wilderness, perhaps it'll find new friends and caretakers, whilst we can focus on the brave new world of mobile everything. So there we are now, a small circle of volunteer friends taking care of the beast, trying to heal the wounds of time, polishing and adding here and there, and shaking our heads over and over again over the unbelievable neglection this once beautiful creature had to suffer over time. Imagine the majesty and beauty this could have achieved, with an investment of more manpower, more care, more listening, more sincerity, more focus on UX, stringent bug management, and more direction...

Email sent ...

Perhaps WADA can analyse what is required to fix this?

(In reply to Thomas D. from comment #13) > Perhaps WADA can analyse what is required to fix this? It's pretty simple that "Edit As New of draft mail in Drafts folder" of "Mozilla Mail&New.Thunderbird code" did do/does do same thing as "Edit Draft", isn't it? I think following ; "Edit As New" menu was added in addition to Edit Draft, Reply, Forward etc. "Draft Edit" routine does do same thing as "Edit Draft" when "Draft Edit" routine is invoked via "Edit As New" menu. (i.e. "Edit As New" menu is not for drfaft mail. "Edit As New" is for creation os new mail from existent mail.) It's all up to Thunderbird developers. If "Edit As New" is meaningless in "Draft Edit" routine, when Drafts folder, disable "Edit As New" menu in Commnd Controler etc. If "Edit As New of draft mail" is mandatory & important feature, support "Edit As New of draft mail" appropriatelyy, in order to avoid user's confusion and misleding.and unwanted bug open.

(In reply to WADA from comment #14) > (In reply to Thomas D. from comment #13) > > Perhaps WADA can analyse what is required to fix this? > > It's pretty simple that "Edit As New of draft mail in Drafts folder" of > "Mozilla Mail&New.Thunderbird code" did do/does do same thing as "Edit > Draft", isn't it? Maybe, although internally we call... > 1869 composeMsgByType(Components.interfaces.nsIMsgCompType.Template); ...which is .Template so it should *not* usually behave like "Edit draft". "Edit draft" and "Edit as new message" are two clearly distinct features. Btw, "Edit draft" command (the most common action on drafts!) does not even exist in the menus, which looks like another bug, in terms of ux-consistency and probably accessability. > It's all up to Thunderbird developers. Which is now a team of volunteers including you and me... Given that this already has wanted-thunderbird+ from :mkmelin (comment 6) and strong support from :wsmwk (comment 5), and my strong support (comment 11), plus several supportive user stories (again from known figures), I don't think we need to wait for anyone else to decide on the desired UX of this. > If "Edit As New of draft mail" is mandatory & important feature, support > "Edit As New of draft mail" appropriatelyy, > in order to avoid user's confusion and misleding.and unwanted bug open. I can't see any reason (apart from manpower bottlenecks) against this feature, and we shouldn't disable commands and create exceptions without an actual design need. User stories on this bug show that users like this feature and intuitively use it for their own workflows, which are just broken by this bug. As described in comment 9, it makes perfect sense to start out from a base draft and then create clone copies of that for different recipients. Which avoids the mental and practical overhead of creating a dedicated template for that, which is probably neither intuitive nor desirable for the scenarios of this bug. Even if I start a draft and at some point along the way I want to try out re-arranging or modifying the draft while perserving a copy of the status quo, this looks like an intuitive way of creating a fresh copy of an existing draft. So as far as the UX of this is concerned, in view of the above support, I'll just make the call and confirm this is wanted (by virtue of being one of TB's key UX contributors) :)

WADA, could you comment where along the code path we could change things (and how) so that it doesn't overwrite the original draft? OK, here's the workflow for this: http://mxr.mozilla.org/comm-central/search?string=MsgEditMessageAsNew%28%29 http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725 (and some other starting points in xul) 725 <menuitem id="mailContext-editAsNew" 726 label="&contextEditMsgAsNew.label;" 727 accesskey="&contextEditMsgAsNew.accesskey;" 728 oncommand="MsgEditMessageAsNew();"/> <menuitem id="mailContext-editAsNew" label="Edit As New Message" accesskey="E" oncommand="MsgEditMessageAsNew();"/> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1867 1867 function MsgEditMessageAsNew() 1868 { 1870 } http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1540 1540 function composeMsgByType(aCompType, aEvent) { 1541 // If we're the hidden window, then we're not going to have a gFolderDisplay 1542 // to work out existing folders, so just use null. 1543 let msgFolder = gFolderDisplay ? GetFirstSelectedMsgFolder() : null; 1544 let msgUris = gFolderDisplay ? gFolderDisplay.selectedMessageUris : null; 1545 1546 if (aEvent && aEvent.shiftKey) { 1547 ComposeMessage(aCompType, 1548 Components.interfaces.nsIMsgCompFormat.OppositeOfDefault, 1549 msgFolder, msgUris); 1550 } 1551 else { 1552 ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.Default, 1553 msgFolder, msgUris); 1554 } 1555 } http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#154 153 // type is a nsIMsgCompType and format is a nsIMsgCompFormat 154 function ComposeMessage(type, format, folder, messageArray) 155 { [... too long to paste here, but if I read this correctly, we end up here:] 262 let hdrIdentity = getIdentityForHeader(hdr, type); 263 MailServices.compose.OpenComposeWindow(null, hdr, messageUri, type, 264 format, hdrIdentity, msgWindow); 265 } 266 } Then, correct me if I'm wrong, we get here: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#501 499 500 NS_IMETHODIMP 501 nsMsgComposeService::OpenComposeWindow(const char *msgComposeWindowURL, nsIMsgDBHdr *origMsgHdr, const char *originalMsgURI, 502 MSG_ComposeType type, MSG_ComposeFormat format, nsIMsgIdentity * aIdentity, nsIMsgWindow *aMsgWindow) 503 { 504 nsresult rv; 505 506 nsCOMPtr<nsIMsgIdentity> identity = aIdentity; 507 if (!identity) 508 GetDefaultIdentity(getter_AddRefs(identity)); 509 510 /* Actually, the only way to implement forward inline is to simulate a template message. 511 Maybe one day when we will have more time we can change that 512 */ 513 if (type == nsIMsgCompType::ForwardInline || type == nsIMsgCompType::Draft || type == nsIMsgCompType::Template 514 || type == nsIMsgCompType::ReplyWithTemplate || type == nsIMsgCompType::Redirect) 515 { 516 nsAutoCString uriToOpen(originalMsgURI); 517 uriToOpen += (uriToOpen.FindChar('?') == kNotFound) ? '?' : '&'; 518 uriToOpen.Append("fetchCompleteMessage=true"); 519 if (type == nsIMsgCompType::Redirect) 520 uriToOpen.Append("&redirect=true"); 521 522 aMsgWindow->SetCharsetOverride(true); 523 return LoadDraftOrTemplate(uriToOpen, type == nsIMsgCompType::ForwardInline || type == nsIMsgCompType::Draft ? 524 nsMimeOutput::nsMimeMessageDraftOrTemplate : nsMimeOutput::nsMimeMessageEditorTemplate, 525 identity, originalMsgURI, origMsgHdr, type == nsIMsgCompType::ForwardInline, 526 format == nsIMsgCompFormat::OppositeOfDefault, aMsgWindow); 527 } http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1487 1487 /** 1488 * LoadDraftOrTemplate 1489 * Helper routine used to run msgURI through libmime in order to fetch the contents for a 1490 * draft or template. 1491 */ 1492 nsresult 1493 nsMsgComposeService::LoadDraftOrTemplate(const nsACString& aMsgURI, nsMimeOutputType aOutType, 1494 nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI, 1495 nsIMsgDBHdr * aOrigMsgHdr, 1496 bool aForwardInline, 1497 bool overrideComposeFormat, 1498 nsIMsgWindow *aMsgWindow) 1499 { 1500 return RunMessageThroughMimeDraft(aMsgURI, aOutType, aIdentity, 1501 aOriginalMsgURI, aOrigMsgHdr, 1502 aForwardInline, EmptyString(), 1503 overrideComposeFormat, aMsgWindow); 1504 } 1505 1506 /** 1507 * Run the aMsgURI message through libmime. We set various attributes of the 1508 * nsIMimeStreamConverter so mimedrft.cpp will know what to do with the message 1509 * when its done streaming. Usually that will be opening a compose window 1510 * with the contents of the message, but if forwardTo is non-empty, mimedrft.cpp 1511 * will forward the contents directly. 1512 * 1513 * @param aMsgURI URI to stream, which is the msgUri + any extra terms, e.g., 1514 * "redirect=true". 1515 * @param aOutType nsMimeOutput::nsMimeMessageDraftOrTemplate or 1516 * nsMimeOutput::nsMimeMessageEditorTemplate 1517 * @param aIdentity identity to use for the new message 1518 * @param aOriginalMsgURI msgURI w/o any extra terms 1519 * @param aOrigMsgHdr nsIMsgDBHdr corresponding to aOriginalMsgURI 1520 * @param aForwardInline true if doing a forward inline 1521 * @param aForwardTo e-mail address to forward msg to. This is used for 1522 * forward inline message filter actions. 1523 * @param aOverrideComposeFormat True if the user had shift key down when 1524 doing a command that opens the compose window, 1525 * which means we switch the compose window used 1526 * from the default. 1527 * @param aMsgWindow msgWindow to pass into DisplayMessage. 1528 */ 1529 nsresult 1530 nsMsgComposeService::RunMessageThroughMimeDraft( 1531 const nsACString& aMsgURI, nsMimeOutputType aOutType, 1532 nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI, 1533 nsIMsgDBHdr * aOrigMsgHdr, 1534 bool aForwardInline, 1535 const nsAString &aForwardTo, 1536 bool aOverrideComposeFormat, 1537 nsIMsgWindow *aMsgWindow) 1538 { 1539 nsCOMPtr <nsIMsgMessageService> messageService; 1540 nsresult rv = GetMessageServiceFromURI(aMsgURI, getter_AddRefs(messageService)); 1541 NS_ENSURE_SUCCESS(rv, rv); 1542 1543 // Create a mime parser (nsIMimeStreamConverter)to do the conversion. 1544 nsCOMPtr<nsIMimeStreamConverter> mimeConverter = 1545 do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); 1546 NS_ENSURE_SUCCESS(rv, rv); 1547 1548 mimeConverter->SetMimeOutputType(aOutType); // Set the type of output for libmime 1549 mimeConverter->SetForwardInline(aForwardInline); 1550 if (!aForwardTo.IsEmpty()) 1551 { 1552 mimeConverter->SetForwardInlineFilter(true); 1553 mimeConverter->SetForwardToAddress(aForwardTo); 1554 } 1555 mimeConverter->SetOverrideComposeFormat(aOverrideComposeFormat); 1556 mimeConverter->SetIdentity(aIdentity); 1557 mimeConverter->SetOriginalMsgURI(aOriginalMsgURI); 1558 mimeConverter->SetOrigMsgHdr(aOrigMsgHdr); 1559 1560 nsCOMPtr<nsIURI> url; 1561 bool fileUrl = StringBeginsWith(aMsgURI, NS_LITERAL_CSTRING("file:")); 1562 nsCString mailboxUri(aMsgURI); 1563 if (fileUrl) 1564 { 1565 // We loaded a .eml file from a file: url. Construct equivalent mailbox url. 1566 mailboxUri.Replace(0, 5, NS_LITERAL_CSTRING("mailbox:")); 1567 mailboxUri.Append(NS_LITERAL_CSTRING("&number=0")); 1568 // Need this to prevent nsMsgCompose::TagEmbeddedObjects from setting 1569 // inline images as moz-do-not-send. 1570 mimeConverter->SetOriginalMsgURI(mailboxUri.get()); 1571 } 1572 if (fileUrl || PromiseFlatCString(aMsgURI).Find("&type=application/x-message-display") >= 0) 1573 rv = NS_NewURI(getter_AddRefs(url), mailboxUri); 1574 else 1575 rv = messageService->GetUrlForUri(PromiseFlatCString(aMsgURI).get(), getter_AddRefs(url), aMsgWindow); 1576 NS_ENSURE_SUCCESS(rv, rv); [...] 1607 // Now, just plug the two together and get the hell out of the way! 1608 nsCOMPtr<nsIStreamListener> streamListener = do_QueryInterface(mimeConverter); 1609 return messageService->DisplayMessage(PromiseFlatCString(aMsgURI).get(), streamListener, 1610 aMsgWindow, nullptr, mailCharset.get(), nullptr);; 1611 } So I think that's where we finally display the "new" msg composition window.

(In reply to Thomas D. from comment #16) Grrrr, when we finally get comment editing in bugzilla... :( Accidentally cut instead of copied line 1869 when writing comment 15, so ftr, here it is: > http://mxr.mozilla.org/comm-central/source/mail/base/content/ > mailWindowOverlay.js#1867 > 1867 function MsgEditMessageAsNew() > 1868 { 1869 composeMsgByType(Components.interfaces.nsIMsgCompType.Template); > 1870 }

(In reply to Thomas D. from comment #16) > WADA, could you comment where along the code path we could change things > (and how) so that it doesn't overwrite the original draft? homas D., please don't ask me any more, because I believe that "disable 'Edit As New' menu for Drafts" is sufficient and feasible, as I already stated in my previous comment #14.

I agree with you Thomas D. "Edit as New" is deceptive. Its bad wording that causes data loss if you expect it to work in the same way as "Edit as New" does everywhere else. Sure - disabling the menu item is better than leaving it there. Changing its name to "Edit Draft" would also be acceptable elimination of the data-loss potential. But **MUCH** better is what Thomas is suggesting, i.e. to make it actually edit a new copy just as happens everywhere else in TB.

I sometimes need to send two similar messages at the same time. My approach is to write the first message and save it as a draft. Then use "Edit as New" to create a copy of the draft, make the necessary changes and save that as a draft, so that I have two messages ready to send at the same time. Only I discovered the hard way that it doesn't work like that, and my second edited draft overwrites the original draft. I would really like to see "Edit as New" implemented correctly. If this cannot be done, then it should definitely be disabled/removed from the menus. I have lost quite a lot of edits this way in the past.

For the reference, I am attaching a patch to fix this issue. I have not investigated much further though. An xpcshell test is necessary.

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=e8c446fdeac7 Thomas and others, could you please confirm following binary can work fine as you expect? http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/hiikezoe@mozilla-japan.org-e8c446fdeac7/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22 > Fix with a mozmill test "Double click of mail at Templates" is assigned to "Edit As New"(no "Edit" in Templates). What is currently aassigned to "Double click of mail at Drafts"? If it's "Edit As New" currently, it should be chaned to "Edit" if the behaviour change of "Edit As New at Drafts" will be made.

(In reply to WADA from comment #23) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #22 > > Fix with a mozmill test > > "Double click of mail at Templates" is assigned to "Edit As New"(no "Edit" > in Templates). > What is currently aassigned to "Double click of mail at Drafts"? > If it's "Edit As New" currently, it should be chaned to "Edit" if the > behaviour change of "Edit As New at Drafts" will be made. The double click behavior is "Edit" on drafts folder with attachment 8530495 [details] [diff] [review]. attachment 8530495 [details] [diff] [review] does not change UI/UX at all, so I think the current behavior on drafts folder is "Edit". I am not a person who decides UI/UX, so I can't judge which behavior is more proper in drafts folder though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24) > (In reply to WADA from comment #23) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #22 > > > Fix with a mozmill test > > > > "Double click of mail at Templates" is assigned to "Edit As New"(no "Edit" > > in Templates). > > What is currently aassigned to "Double click of mail at Drafts"? > > If it's "Edit As New" currently, it should be chaned to "Edit" if the > > behaviour change of "Edit As New at Drafts" will be made. > > The double click behavior is "Edit" on drafts folder with attachment 8530495 [details] [diff] [review] > [©] [details] [diff] [review]. > attachment 8530495 [details] [diff] [review] does not change UI/UX at > all, so I think the current behavior on drafts folder is "Edit". > > I am not a person who decides UI/UX, so I can't judge which behavior is more > proper in drafts folder though. As a decisive UX person, I confirm that the current behaviour of "double-click on draft = Edit (original) draft" is intentional and obviously the correct default action. "Edit as new message" (clone the current draft msg to create and work on the copy) is a separate feature offered on context menu, the functionality of which will be fixed here. "Edit draft" (the default action) is currently missing on context menu, and should be added (we also offer default actions on context menus of other things like inbox msgs, AB contact cards etc.). Hiro, can you add this command to your patch if it's not there yet, or you prefer a separate bug for that? I'll test the trybuild a bit later when I have time. Pls needinfo me for such requests. Thanks!

(In reply to Thomas D. from comment #25) > "Edit draft" (the default action) is currently missing on context menu, and > should be added (we also offer default actions on context menus of other > things like inbox msgs, AB contact cards etc.). Ah, I did not notice that. > Hiro, can you add this command to your patch if it's not there yet, or you prefer a separate bug > for that? I'd prefer to open a new bug for that. > I'll test the trybuild a bit later when I have time. Pls needinfo me for > such requests. Thanks! I am sorry for my lazy!

(In reply to Thomas D. from comment #25) > "Edit draft" (the default action) is currently missing on context menu, (snip) "Edit" button is shown at Message Header Box when mail is mail in Drafts folder. This is done by ; > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3029 > setDraftEditMessage: function() > if (msgHdr.folder.isSpecialFolder(nsMsgFolderFlags.Drafts, true)) > { > let buttons = [{ ..., ..., ..., > callback: function(aNotification, aButton) { > MsgComposeDraftMessage(); > return true; // keep notification open > } > }] ; MsgComposeDraftMessage() is sibling of MsgEditMessageAsNew(), MsgForwardAsAttachment(event), MsgForwardAsInline(event) etc. which is defined in mailWindowOverlay.js. > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1862 To do same thing at menu, following is needed. 1. Add "Edit Draft" to "Message" of main menu and Context menu of mail, as currently done on "Edit As New". Detailed definition at one of 2 is sufficient, if other watches primary menuitem item or toolbarbutton. "common command use in menuitem/toolbarbutton definition" is also possible. 2. If "command" is utilized, in Command Controller, enable/disable the menuitem according to context, status etc., as currently done on Forward, Forward As, Edit As New etc. 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context menu if folder is not Drafts, instead of simply disabling(gray out at menu). In this case, control in onmenupopup is additionlly needed. > EditAsNew relevant definitions for menu > http://mxr.mozilla.org/comm-central/search?string=editasnew > Edit As New in main menu > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2687 > Edit As New in context menu > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725 I think following are different jobs and (b) is better done in separated bug. (b) is never fault of patch for this bug :-) (a) "Edit As New" should be "Edit As New" at anywhere. "Edit As New" shouldn't be "Edit draft". Change "Edit As New of draft mail" from same one as "Edit draft" to same one as "Edit New of Template or ordinal mail". (b) If "Edit As New" is "Edit As New" at anywhere as expected, we loose "Edit draft" via. menu. Add equivalence to "Edit button at Message Header Box of draft mail" in main menu and/or context menu.

(In reply to WADA from comment #27) Thank you WADA for this helpful comment, with which I agree! :) > To do same thing at menu, following is needed. > 1. Add "Edit Draft" to "Message" of main menu and Context menu of mail, as > currently done on "Edit As New". > Detailed definition at one of 2 is sufficient, if other watches primary > menuitem item or toolbarbutton. > "common command use in menuitem/toolbarbutton definition" is also > possible. Yes, central command controller would be best, than let the UI observe that. > 2. If "command" is utilized, in Command Controller, enable/disable the > menuitem according to context, status etc., > as currently done on Forward, Forward As, Edit As New etc. > 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context > menu if folder is not Drafts, > instead of simply disabling(gray out at menu). +1, hiding this command for non-draft folders lgtm. > In this case, control in onmenupopup is additionlly needed. Yeah, most likely. I wonder if that could be streamlined using something like a "hidden" attribute on the command controller which is then auto-forwarded to UI elements observing that command? > > EditAsNew relevant definitions for menu > > http://mxr.mozilla.org/comm-central/search?string=editasnew > > Edit As New in main menu > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2687 > > Edit As New in context menu > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725 > > I think following are different jobs and (b) is better done in separated > bug. (b) is never fault of patch for this bug :-) > (a) "Edit As New" should be "Edit As New" at anywhere. "Edit As New" > shouldn't be "Edit draft". > Change "Edit As New of draft mail" from same one as "Edit draft" to > same one as "Edit New of Template or ordinal mail". Rephrase for clarity: (a) "Edit As New" should mean/do "Edit as New" anywhere in the UI, because "Edit As New" cannot and should never mean "Edit this draft" Consequently, in this bug we are changing "Edit As New" for message in draft folder from current wrong behaviour ("Edit draft") to correct behaviour, which is same as "Edit As New Msg" of msg in Template folder or any other ordinary (non-draft) msg. > (b) If "Edit As New" is "Edit As New" at anywhere as expected, we loose > "Edit draft" via. menu. (b) After this bug, when "Edit As New Msg" means/does "Edit as New Msg" anywhere in the UI as expected, we'll have lost "Edit draft" functionality via menu for draft msgs, which might be considered a regression of this bug. So I think before landing this bug, we should fix that: > Add equivalence to "Edit button at Message Header Box of draft mail" > in main menu and/or context menu. Iow (in a new bug) add "Edit draft" command to main menu's message menu and context menu in drafts folder (we currently *do* offer default actions in context menus, which imo makes sense, because it assists discovery and understanding, and also helps when you are tired, more control-oriented, or just insecure, and thus prefer explicit context menu actions over trusting that double click or Enter will do the right thing for you).

needinfo myself, see comment 22 (test try build)

Bug 1106412 - Menu items for "Edit draft" functionality missing after bug 321355 fixes "Edit as New Message" to do just that (In reply to Thomas D. from comment #25) > "Edit draft" (the default action) is currently missing on context menu, and > should be added (we also offer default actions on context menus of other > things like inbox msgs, AB contact cards etc.). (In reply to WADA from comment #27) > (b) ... Add equivalence to "Edit button at Message Header Box of draft mail" > in main menu and/or context menu. (In reply to Thomas D. from comment #28) > Iow (in a new bug) add "Edit draft" command to main menu's message menu and > context menu in drafts folder (we currently *do* offer default actions in > context menus...

(In reply to Thomas D. from comment #28) > > 2. If "command" is utilized, in Command Controller, enable/disable the > > menuitem according to context, status etc., > > as currently done on Forward, Forward As, Edit As New etc. > > 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context > > menu if folder is not Drafts, > > instead of simply disabling(gray out at menu). > > +1, hiding this command for non-draft folders lgtm. > > > In this case, control in onmenupopup is additionlly needed. > > Yeah, most likely. I wonder if that could be streamlined using something like a "hidden" attribute on the command controller > which is then auto-forwarded to UI elements observing that command? Example of this kind of menu control : Context menu of message at Thread Pane -> "Tag". Because "Tag added to mail in Tb" is not solid element in Tb(user can add/delete/alter it any time), shown row in dropdown menu of "Context menu of message at Thread Pane" -> "Tag" is dynamically generated upon onmenupopup. If required action is simply "Show or Hide a menuitem", needless to say, "display: visible/none of CSS" etc. can be freely used.

Thanks WADA! Ideally, such comments should now go into Bug 1106412 ;) (In reply to WADA from comment #32) > If required action is simply "Show or Hide a menuitem", needless to say, > "display: visible/none of CSS" etc. can be freely used.

Comment on attachment 8530495 [details] [diff] [review] Fix with a mozmill test Review of attachment 8530495 [details] [diff] [review]: ----------------------------------------------------------------- It looks one liner patch. > - fields->SetDraftId(mdd->url_name); > After it, msgComposeType = Redirect or Template is set, then CreateTheComposeWindow() is called. By this change, I think "delete of original draft upon first draft save/send" won't be executed when "Edit draft". Please read bug 1106412 comment #3. If it's right, where/how should fields->SetDraftId(mdd->url_name) be set by whom when "Edit draft"?

Sorry, removed line by patch was https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1455 instead of https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1487 Sorry for my confusion. If cmposition-type=Draft, draftID is still set, so "delete edited draft" is executed as expected upon first save/send.

When "Edit As New of non-Drafts folder", gMsgCompose.compFields.draftID=original mail, gMsgCompose.deleteDraft=false was set, and original mail(in draftID) is not deleted upon "Save as draft". However, when "Edit As New of a draft mail in Drafts folder", gMsgCompose.compFields.draftID=edited draft, window.deleteDraft=false was set, but edited draft=original=draftID was deleted upon save. If draftID is set, and if Drafts folder, Tb always deletes draftID regardless of deleteDraft upon draft save/send? Why "edited mail by Edit As New is deleted if Drafts" but "edited mail by Edit As New is not deleted if non-Drafts" is perhaps following : > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3674 > After end of SaveAsDrft/SaveAsTemplate/QueueForLater/DeliverBackground, RemoveCurrentDraftMessage() is called. > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728 > if draftID is not set, RemoveCurrentDraftMessage does do nothing. > if draftID is set && folderFlags has nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage deletes draftID. > even if draftID is set, if folderFlags doesn't have nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage does do nothing. Small mismatch on "delete original or not" is seen in composer component modules. Someone deletes original if draftID is set && deleteDraft=true regardless of folder type, (I think this is concept/design in composer since initial, because deleteDraft is set only when composition-type=Draft) but other someone deletes original if draftID is set && folder is Drafts folder regardless of deleteDraft. This mismatch maybe is difference between "process during mail composition" and "post save/send process". "post save/send process", RemoveCurrentDraftMessage(), looks mainly coded for IMAP. Because UID of newly saved draft mail can not known upon draft save execution(append) unless UIDPLUS is supported, UID of newly saved draft mail should be obtained by SEARCH command. And delete of old draft mail == "uid store UIDofOldDraft +Flags(\Deleted)" should be executed independently if imap. But it maybe fault in OnStopCopy() and RemoveCurrentDraftMessage() : - OnStopCopy() sets deleteDraft=true before first call of RemoveCurrentDraftMessage(). - RemoveCurrentDraftMessage() desn't check gMsgCompose.deleteDraft. - RemoveCurrentDraftMessage() deletes message of draftID only when draftID is set && the draftID is in Drafts folder. - In RemoveCurrentDraftMessage(), if msgDBHdr for draftID is not found after save, Tb assumes that it's IMAP and Drafts folder is not opened. However, "msgDBHdr for draftID is not found" can occur if following conditions are satisfied. Edit As New of mail in local mail folder && edited mail is deleted/Compacted or Offset of edited mail is changed by Compact && it's after first save. Is this a reason why phenomenon of "old draft is not deleted" on IMAP Drafts sometimes? If "append to IMAP Drafts for first draft save" is executed and mail data is accepted by server, and if above condition happens, and if IMAP Drafts folder access fails upon getting UID of newly saved draft mail, draftID may not be updated by UID of the newly saved draft mail. => After next draft save, "msgDBHdr for draftID is not found" occurs => "Delete previous draft" is skipped Ikezoe-san, is my guess right? Ikezoe-san, can you check change like next without removing mimedrft.cpp#l1455? (A) Set deleteDraft=true after first RemoveCurrentDraftMessage call, in case of "deleteDraft=false for edited mail" > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3646 > nsMsgComposeSendListener::OnStopCopy(nsresult aStatus) > 3676 // We should only close the window if we are done. Things like templates > 3677 // and drafts aren't done so their windows should stay open > 3678 if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft || > 3679 mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate) > 3680 { > 3681 msgCompose->NotifyStateListeners(nsIMsgComposeNotificationType::SaveInFolderDone, aStatus); > 3682 // Remove the current draft msg when saving as draft/template is done. > 3683 - msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call > 3684 RemoveCurrentDraftMessage(msgCompose, true); > + msgCompose->SetDeleteDraft(true); <== Should be set after first beforeRemoveCurrentDraftMessage call > 3685 } > 3686 else > 3687 { > 3688 // Remove (possible) draft if we're in send later mode > 3689 if (mDeliverMode == nsIMsgSend::nsMsgQueueForLater || > 3690 mDeliverMode == nsIMsgSend::nsMsgDeliverBackground) > 3691 { > 3692 - msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call > 3693 RemoveCurrentDraftMessage(msgCompose, true); > + msgCompose->SetDeleteDraft(true); <== Should be set after first beforeRemoveCurrentDraftMessage call > 3694 } > 3695 msgCompose->CloseWindow(true); > 3696 } (B) Check deleteDraft in RemoveCurrentDraftMessage, according to design of deleteDraft > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728 > nsMsgComposeSendListener::RemoveCurrentDraftMessage(nsIMsgCompose *compObj, bool calledByCopy) > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3746 > + bool deleteDraft=false; > // See http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1383 > + rv = compObj->GetDeleteDraft(getter_AddRefs(deleteDraft)); > + NS_ASSERTION(NS_SUCCEEDED(rv), "RemoveCurrentDraftMessage can't get deleteDraft"); > + if (NS_FAILED(rv)) > + return rv; > 3746 // Skip if no draft id (probably a new draft msg). > 3747 if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty()) > => if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty() && deleteDraft ) > ); > 3761 // only do this if it's a drafts or templates folder. <= Comment should be changed > 3762 if (folderFlags & nsMsgFolderFlags::Drafts) > 3763 { > => Delete of message set in draftID is dione in this block. This is reason why problem hppens on Drafts only. > After removing mimedrft.cpp#l1455, this check is not needed, but the is no need to remove this check. composition-type=Template is "Never delete original(edited) mail" mode(i.e. original is never draft mail even in Drafts folder), and composition-type=Draft is "Surely delete original(edited) mail" mode, even if gMsgCompose.deleteDraft=false is always set when composition-type=Template and even if gMsgCompose.deleteDraft=true is always set when composition-type=Draft. If "original mail" is needed, it's always saved in gMsgCompose.originalMsgURI. So, even if above change works and is correct action, I believe "removing mimedrft.cpp#l1455" is correct and mandatory action. Ikezoe-san, do you think change of OnStopCopy() and RemoveCurrentDraftMessage() is needed? Or "removing mimedrft.cpp#l1455" is sufficient, so such change is merely wasting of resources?

Question about "OnStopCopy() /RemoveCurrentDraftMessage() by Send/Manul-Save and Auto-Save at same time". Because Auto-Save is timer-pop event, it can happen even if Tb tries to serialize Send, Manul-Save, and Auto-Save. IIRC, Tb tries to stop Auto-Save when Send is requested by setting flag in gMsgCompose.something. but it doesn't seem to work perfectly. So, "Auto-Save just after Send request", "Send just after Auto-Save is kicked" can occur. and non-deleted draft mail cn happen. Ikezoe-san, is there easy way to resolve problem of "non-deleted draft mail due to Auto-Save" by small change of OnStopCopy() and/or RemoveCurrentDraftMessage() code?

FYI. I've opened Bug 1108441 for issue of Comment #36. See Bug 638390 for problem in "delete editing draft mail" which is relevant to AutoSave. AutoSave relevant issue should be processed in that bug instead of this bug. Sorry for excess comments to this bug.

Thomas, can you summarize if there is anything to do in this bug? Does the patch work?

Comment on attachment 8517970 [details] [diff] [review] do_not_overwrite_draft_message_when_editing_as_new.diff Yes, it needs a determined developer to fix and land this :) Probably it's ready to land as-is. But I'm not entirely sure, too much hightech discussion here... The desired UX is simple - "Edit as New" must do just that, namely to create and edit a duplicate copy (this bug), and for draft folders, we need a separate "Edit" command to edit the original draft (Bug 1106412). I think this was wrongly obsoleted, because this one-liner seems to fix the main half of this bug per Wada's comment 34. The other patch attachment #8530495 [details] [diff] [review] is tests-only (probably the one-liner was forgotten). It would be good to land this together with Bug 1106412 which has WIP patches, otherwise we're making things worse the other way round, esp. for accessibility-dependent users.

Per comments on bug 1106412, this should ideally involve creating cleaner code with central <command> which the other UI elements can then listen to / observe. But perhaps for this bug it is not necessary or we can do it later in another bug. For bug 110641 it's probably necessary because we're adding a missing command (I think).

Looks like Hiro is not working on this any more. Aceman, could you take over? I tried the code patch and it works: Use an existing draft, edit it, save. Now creates a new draft. I will attach a patch with a proper header. Could you take care of the test, please ;-)

Comment on attachment 8517970 [details] [diff] [review] do_not_overwrite_draft_message_when_editing_as_new.diff This is not needed since it is contained in the other patch. It works though. Sorry, I won't attach anything since the other patch contains everything.

OK, I've refreshed the patch and I've run the two tests: mozmake SOLO_TEST=composition/test-drafts.js mozmill-one (added new test) mozmake SOLO_TEST=composition/test-charset-edit.js mozmill-one (needed to change since we need to "Edit draft" instead of "Edit as new message". Looks good to me. Since it's not my own work, I can give it f+. Let's get it landed after 1.5 years of rotting. After all, it has six votes and is deemed critical. Thanks to Thomas for the wake-up call.

Comment on attachment 8726810 [details] [diff] [review] Fix with a mozmill test (refreshed) Review of attachment 8726810 [details] [diff] [review]: ----------------------------------------------------------------- So this seems to work and I r+ the mozmill test. I find it strange that something in mime decides the outcome of this operation. I'd expect the frontend code to say if it wants edit the draft or make a copy. So I'd like jcranmer to review the mime part. I don't know why the removed line was even there and if it does not break something else now. ::: mail/test/mozmill/composition/test-charset-edit.js @@ +114,5 @@ > > // Edit the original message. Charset should be windows-1252 now. > msg = select_click_row(0); > plan_for_new_window("msgcompose"); > + mc.click(mc.eid("msgNotificationBar", {tagName: "button", label: "Edit"})); When you use the notification bar, you need to wait for it until it slides in so you can click it. Without it, it may randomly fail (and it does for me locally). Put something like 'wait_for_notification_to_show(mc, "msgNotificationBar", "draftMsgContent");' before the plan_for_new_window(). You can see it in test_open_draft_again(). @@ +155,5 @@ > > // Edit the original message. Charset should be UTF-8 now. > msg = select_click_row(0); > plan_for_new_window("msgcompose"); > + mc.click(mc.eid("msgNotificationBar", {tagName: "button", label: "Edit"})); Same here. ::: mail/test/mozmill/composition/test-drafts.js @@ +160,5 @@ > assert_format_value("format_both", Ci.nsIMsgCompSendFormat.Both); > > close_compose_window(cwc); > > press_delete(mc); // clean up the created draft Please modify the draft in this test (test_open_draft_again) and then at the end check there is only one draft in the folder. That should be the difference to the new test below when you open the draft via the menu (not the Edit button).

Aceman, thanks for the r+, can you please check again. Joshua, please review the - fields->SetDraftId(mdd->url_name); Looking for GetDraftId, there are five calls in nsMsgCompose.cpp. Most have a comment like "if no draft ID, it's a new draft".

Comment on attachment 8726937 [details] [diff] [review] Fix with a mozmill test (v2) Magnus, are you able to review the mime change so we don't have to wait for Joshua. Maybe we can even land this before the next branch date.

Comment on attachment 8726937 [details] [diff] [review] Fix with a mozmill test (v2) Review of attachment 8726937 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the test with the fix below. ::: mail/test/mozmill/composition/test-charset-edit.js @@ +115,5 @@ > // Edit the original message. Charset should be windows-1252 now. > msg = select_click_row(0); > + > + // Wait for the notification with the Edit button. > + wait_for_notification_to_show(mc, "msgNotificationBar", "draftMsgContent"); The test fails as the function is not defined. You need to import notificationbox-helpers in MODULE_REQUIRES.

Sorry, I ran test-drafts.js again manually, but not test-charset-edit.js, grrr ;-( Now it works. Carrying forward Aceman's r+ for the test and requesting review of the mime change - fields->SetDraftId(mdd->url_name); via the superreview field.

Comment on attachment 8727043 [details] [diff] [review] Fix with a mozmill test (v2a). Review of attachment 8727043 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin

Oh, please put r=aceman,mkmelin.

Comment on attachment 8727043 [details] [diff] [review] Fix with a mozmill test (v2a). [Approval Request Comment] Regression caused by (bug #): No regression. User impact if declined: Will work badly as before, see below. Testing completed (on c-c, etc.): Manual and with Mozmill test. Risk to taking this patch (and alternatives if risky): Not risky. This has been a long-standing issue and annoyance deemed critical from 2005 with six votes. It's a one line code change.

approval-comm-release is for Seamonkey builds. We will be using approval-comm-esr45 for further uplifts to Thunderbird.

Comment on attachment 8727043 [details] [diff] [review] Fix with a mozmill test (v2a). I know. Sorry, I didn't see the flag. (Will fix the other bugs too.)

I'll land it on C-C myself using a transplant from Aurora.

Comment on attachment 8727043 [details] [diff] [review] Fix with a mozmill test (v2a). http://hg.mozilla.org/releases/comm-beta/rev/e7e79f9aaf79 http://hg.mozilla.org/releases/comm-esr45/rev/60c360c6b4aa

I had to adapt the mozmill test (which is not something I really do well) to uplift this, since another patch that also modified test-drafts.js conflicted. Now that test is failing (see treeherder on comm-esr45 or comm-beta). Could you investigate, and see the issue is my test adaptation or something else?

Can you see if maybe bug 1106412 landed first? It should land after the bug here. Or maybe that bug 1202165 is not on the branches. Awyway, why are you removing the loop with cwins2 ? That wasn't in the original patch. And the failure mentions that variable.

Bug 1202165 was not in the branches, and is marked as WONTFIX for the branches. Can you prepare an adapted mozmill test that we can land? I'm not a mozmill kind of guy.

I think you need to put this block back, which you removed in the landed patch: - let cwins2 = 0; - let e2 = Services.wm.getEnumerator("msgcompose"); - while (e2.hasMoreElements()) { - e2.getNext(); - cwins2++; - }

https://hg.mozilla.org/releases/comm-beta/rev/fb34bc19b35f - Backed out wrong merge https://hg.mozilla.org/releases/comm-beta/rev/3a386f9a75b5 - landed again. Please uplift to ESR45. Aceman is right, I put that back. Sorry, you were writing comments while I was doing the work and not watching bugmail.

http://hg.mozilla.org/releases/comm-esr45/rev/d6f4c7a7ce1f http://hg.mozilla.org/releases/comm-esr45/rev/48f502911bac Thanks for the help!

The beta looks a lot greener than the ESR45, but I guess that's life.