349547 - Should not be able to open the same draft multiple times

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jon B]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

It should not be possible to open a draft for editing multiple times.  Only one compose window should be allowed for a draft.  If the draft is opened again, it should just make the already-open window active.  

On the other hand, the Copy To... command doesn't work for Drafts.  The Copy To command should be the way to create a copy of a draft, which is rare, but needed sometimes.

Reproducible: Always

Steps to Reproduce:
1. Compose an email
2. Save to drafts
3. Open the email from your Drafts folder by double-clicking
4. Edit it but don't save
5. Open the email from your Drafts folder again - it opens a second copy in a new window that doesn't include the changes you made in step 3.  
6. Press send in the latest window
Actual Results:  
The changes you made in step 3 are lost; not included in the sent version

Expected Results:  
When you double-click the message in the Drafts folder a second time, it should not open a new window; it should just bring the window that is already open to the foreground

It should of course, be possible to create a second copy of a draft if desired, but this is a rare circumstance, and is covered by the Copy To... menu selection.  (Which, apparently, doesn't work...)  :-)

Comment 1

[Jon B]

upgrade to major because it causes loss of work

Comment 2

[Jon B]

Still a problem in 2.0.0.6

Comment 3

[Wayne Mery (:wsmwk)]

Does Eudora do this?

There's some rationale to this, though I'd think it's a rare occurrence for the average user, and would have saved me some pain a few times.  And anyone who truly, intentionally wants to open multiple compose windows from the save "draft" can always do it with "edit as new"

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

agreed, we should probably always bring the open draft to the front instead of opening up a new window.

Comment 5

[Wayne Mery (:wsmwk)]

confirming

Comment 6

[Jeff Beckley]

(In reply to comment #3)
> Does Eudora do this?

Classic Eudora only opens one copy of a message in a separate window.  That's not true for just comp messages, but received messages as well.


> There's some rationale to this, though I'd think it's a rare occurrence for the
> average user, and would have saved me some pain a few times.  And anyone who
> truly, intentionally wants to open multiple compose windows from the save
> "draft" can always do it with "edit as new"

Getting multiple copies of drafts by the simple double-click method is bad UI.  Giving the user the ability to do it with "Edit As New" is fine.

Comment 8

[Neville Hillyer]

I agree that my 550022 bug  appears to be a duplicate of this well described much earlier bug. The bug appears to affect all platforms and all versions of TB.

Since this bug results in data loss I hope somebody will work on it soon.

Comment 9

[Wayne Mery (:wsmwk)]

edge case, but dataloss is dataloss

Comment 10

[Neville Hillyer]

Thanks Wayne.

Is there a way to update version and platform?

Comment 11

[Neville Hillyer]

Since my 'duplicate' 550022 bug was about TB 3 on a G4 with OS X  perhaps it is reasonable to assume that the bug relates to:

OS: - All
Hardware: - All
TB version: - All

Does the above 'Platform: - x86 All' imply all OSs on x86 hardware? If so might it be better to remove 'x86'?

Comment 12

[michelr]

Hi, I made some tests on thisbug and I have a hack to show you :
it's just few lines of js inserted at the begining of ComposeMessage() 
(file chrome/messenger/content/messenger/mailCommands.js in omni.ja)
The idea is to look for a window with a reference to the same message URI.
I use it on my thunderbird and it's ok.
As I made those tests in my copy of sources, I don't know if my partch is correctly formatted.
Here it is, as text :


--- omni.ja_bak_FILES/chrome/messenger/content/messenger/mailCommands.js
+++ mailCommands_mrc.js
@@ -124,6 +124,33 @@
 // type is a nsIMsgCompType and format is a nsIMsgCompFormat
 function ComposeMessage(type, format, folder, messageArray)
 {
+	// start of workaround for bug 349547
+	// we test only for one DRAFT
+	if (type == Components.interfaces.nsIMsgCompType.Draft && messageArray && messageArray.length == 1) {
+		// we'll search this uri in the opened windows
+		var messageUri = decodeURIComponent(messageArray[0]).toLowerCase();
+		var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator);
+		var wenum = wm.getEnumerator("");
+		while(wenum.hasMoreElements()) {
+			var w = wenum.getNext();
+			// with DOMInspector, i found that the uri is stored in the field 'document.defaultView.gEditingDraft'
+			if (w.document.defaultView && w.document.defaultView.gEditingDraft) {
+				var w_uri = decodeURIComponent(w.document.defaultView.gEditingDraft).toLowerCase();
+				// if we don't decode, we have  :
+				//    draft : mailbox-message://[e-mail]/Drafts#296929?fetchCompleteMessage=true
+				// instead of :
+				//    draft : mailbox-message://[e-mail]/Drafts#296929?fetchCompleteMessage=true
+				if (w_uri.indexOf(messageUri) == 0) {
+					// found ! just focus it...
+					w.focus();
+					// ...and nothing to do anymore
+					return;
+				}
+			}
+		}
+	}
+	// end of workaround for bug 349547
+
   var msgComposeType = Components.interfaces.nsIMsgCompType;
   var identity = null;
   var newsgroup = null;

Comment 13

[Magnus Melin [:mkmelin]]

Why the decodings? Can't you just compare w_uri == messageArray[0] 
Also, you can use Services.wm.getEnumerator("") instead of getting the wm the long way.

If you haven't checked out the source using hg, try it - it pays off quickly...
https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Getting_comm-central

Comment 14

[Ludovic Hirlimann [:Usul]]

(In reply to Magnus Melin from comment #13)
> Why the decodings? Can't you just compare w_uri == messageArray[0] 
> Also, you can use Services.wm.getEnumerator("") instead of getting the wm
> the long way.
> 
> If you haven't checked out the source using hg, try it - it pays off
> quickly...
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/
> Getting_comm-central

Also read this befor attaching your patch https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in it will help us get the patch in easier.

Comment 15

[michelr]

(In reply to Magnus Melin from comment #13)
> Why the decodings? Can't you just compare w_uri == messageArray[0] 

I started tests on Ubuntu and no need of decodings. When I tested on Mac, there was some encoding differences --> I had to force decode to have correct results.

> Also, you can use Services.wm.getEnumerator("") instead of getting the wm
> the long way.
thanks for the info : I'll integrate it

However, I found a use case where that patch doesn't work :
- from the main window, edit a draft message : a new window opens : OK
- from the main window, edit again the message : the opened window becomes front : OK
- make some modifications in the message and save (and keep the window opened)
- from the main window, edit again the message : a new window opens : FAIL

This is because of saving draft : 
- the editing window loose its current reference to the message ('gEditingDraft' becomes 'false')
- the saved message has a new ID

I can understand the second point (assigning a new ID), but the first point is really surprising and INMHO is a bug.
With grep, I tried to find the line of code that erases field 'gEditingDraft', but found nothing. Any clues ?

Can't we create a simpler way to link a message and the editing window ?
For example, the window may have a field with the pure ID of the message. The code that saves a message would just hav to update that field.

Comment 16

[Magnus Melin [:mkmelin]]

Tried using gMsgCompose.compFields.draftId instead of gEditingDraft?

We probably should !! gEditingDraft to boolean here... http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2308

Comment 17

[michelr]

Attachment: fix that handles saving draft (saves changes the internal message key)

Comment 18

[Ludovic Hirlimann [:Usul]]

Comment on attachment 657815 [details] [diff] [review]
fix that handles saving draft (saves changes the internal message key)

You need to request review to a specific person if you want the review to take place. Asking Magnus to have a look.

Comment 19

[michelr]

(In reply to Ludovic Hirlimann [:Usul] from comment #18)
> 
> You need to request review to a specific person if you want the review to
> take place. Asking Magnus to have a look.

ok : I downloaded the 'getReviewer.py' script for my next patches. thanks also for review of  bug 73931.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 657815 [details] [diff] [review]
fix that handles saving draft (saves changes the internal message key)

Review of attachment 657815 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Michel! It's working nicely for me. There's a few things i'd like changed though. 

Sorry for the delay!

::: mail/base/content/mailCommands.js
@@ +127,5 @@
>  // type is a nsIMsgCompType and format is a nsIMsgCompFormat
>  function ComposeMessage(type, format, folder, messageArray)
>  {
> +  // start of workaround for bug 349547
> +  // we test only for one DRAFT

This isn't really a workaround, it's the fix. Please let the comments reflect that. We usually don't talk about bug numbers when you could just as easily just writ what it's about. 

So, change it to 
// Check if the draft is already open in another window. If it is, just focus that window.

@@ +135,5 @@
> +    // ie : #key
> +    var pos = messageArray[0].indexOf('#')+1;
> +    var messageUri = messageArray[0].substr(pos); // TODO : should access 'getDraftFullKey()'
> +    var wenum = Services.wm.getEnumerator("");
> +    var w_key = '';

just declare variables where you use them, it's way easier to read. 
and please use 'let' instead of 'var' for new code where applicable.

@@ +136,5 @@
> +    var pos = messageArray[0].indexOf('#')+1;
> +    var messageUri = messageArray[0].substr(pos); // TODO : should access 'getDraftFullKey()'
> +    var wenum = Services.wm.getEnumerator("");
> +    var w_key = '';
> +    while(wenum.hasMoreElements()) {

space after while

::: mail/components/compose/content/MsgComposeCommands.js
@@ +105,5 @@
>  var gAutoSaveInterval;
>  var gAutoSaveTimeout;
>  var gAutoSaveKickedIn;
>  var gEditingDraft;
> +var gEditingDraftFullKey;

I don't think you need to store this, i'd prefer what the patch did earlier, and just compare with gMsgCompose.compFields.draftId.

Comment 21

[michelr]

Attachment: simplified version of previous patch : just check draftId on opened windows

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 660254 [details] [diff] [review]
simplified version of previous patch : just check draftId on opened windows

Review of attachment 660254 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few more nits. 
Would be nice to have a mozmill test for this. I can offer some assistance if you want.

::: mail/base/content/mailCommands.js
@@ +123,5 @@
>      }
>    }
>  }
>  
> +function getDraftKey(value) {

The convention here is to Capitalize function names. Also, please make the name more descriptive. It's also good to add doxygen/javadoc style comment like 

/**
 * Figure out the message key from the message uri.
 */
GetMsgKeyFromURI(uri) {

@@ +131,5 @@
> +  let key = value.substr(pos);
> +  pos = key.indexOf('?');
> +  if (pos > 0)
> +    key = key.substr(0, pos);
> +  return key;

This whole function could be easier done with a regexp, 

let match = /.+#(\d+)/.exec(uri);
return (match) ? match[1] : null;

@@ +138,4 @@
>  // type is a nsIMsgCompType and format is a nsIMsgCompFormat
>  function ComposeMessage(type, format, folder, messageArray)
>  {
> +  // check if the draft is already open in another window. If it is, just focus the window

Capitalize sentence, and end it with a dot.

Comment 23

[michelr]

Attachment: use of a regex ; comments formatted

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 661167 [details] [diff] [review]
use of a regex ; comments formatted

Review of attachment 661167 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=mkmelin with one last nit

::: mail/base/content/mailCommands.js
@@ +128,5 @@
> + * we keep only the part after '#' and before an optional '?'
> + * ie : email/folder#key?params
> + * model : protocol://email/folder#key?params --> key
> + *    ex : mailbox-message://[e-mail]/Drafts#123456?fetchCompleteMessage=true
> + *    ex : mailbox-message://[e-mail]/Drafts#12345

The function documentation should say what the function does. The below comments are implementation details and belong inside the function.

Comment 25

[michelr]

Simple question : can I add 'checkin-needed' keyword, just like I did for the bug 739311 ? Thanks

Comment 26

[Kent James (:rkent)]

(In reply to michelr from comment #25)
> Simple question : can I add 'checkin-needed' keyword, just like I did for
> the bug 739311 ? Thanks

mkmelin gave you a conditional r+ that assumes that you fix the nit in his last comment. So what you need to do is one more patch, that has that nit fixed, just select review + yourself with the added comment "carrying forward mkmelin r+". Then you can add checkin-needed to that patch.

Thanks for working on this, by the way!

Comment 27

[michelr]

Attachment: cleaning from mkmelin review

carrying forward mkmelin r+

Comment 28

[Neville Hillyer]

I was concerned that the latest SeaMonkey may also exhibit this bug so I had a quick look and as far as I can tell it allows more than one window per draft but I could not get it to destroy data in the way that Thunderbird did on my Mac. (My 550022 bug was marked as a duplicate of this bug.) SeaMonkey appears to detect a conflict and save more than one draft if necessary.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3beee18b2751

Thanks for the patch, Michel. One request - your commit message should be an overall description of what the patch is doing, not what changed in the last version of it. Thanks!

Also, should this have a test?

Comment 30

[michelr]

(In reply to Ryan VanderMeulen from comment #29)
> https://hg.mozilla.org/comm-central/rev/3beee18b2751
> 
> Thanks for the patch, Michel. One request - your commit message should be an
> overall description of what the patch is doing, not what changed in the last
> version of it. Thanks!
ok
 
> Also, should this have a test?

I tested manually on Ubuntu and MacOS (by applying the patch in the omni.jar file)
But I have no knowledge how to create automated tests.

Comment 31

[Magnus Melin [:mkmelin]]

Attachment: proposed mozmill test

Yes it should have a test, i'll give you this one

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 670469 [details] [diff] [review]
proposed mozmill test

Review of attachment 670469 [details] [diff] [review]:
-----------------------------------------------------------------

Magnus:

This looks really good.

Just a few suggestions, but nothing major - see below,

-Mike

::: mail/test/mozmill/composition/test-drafts.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests draft related functionality:
> + * - that we don't allow opening multipe copies of a draft.

type: "multipe" -> "multiple"

@@ +23,5 @@
> +  collector.getModule("folder-display-helpers").installInto(module);
> +  collector.getModule("compose-helpers").installInto(module);
> +  collector.getModule("window-helpers").installInto(module);
> +
> +  if (!MailServices.accounts.localFoldersServer.rootFolder.containsChildNamed("Drafts")) {

I'd prefer to have this broken up like this:

if (!MailServices.accounts
                 .localFolderServer
                 .rootFolder
                 .containsChildNamed("Drafts")) {
  // ...
}

@@ +26,5 @@
> +
> +  if (!MailServices.accounts.localFoldersServer.rootFolder.containsChildNamed("Drafts")) {
> +    create_folder("Drafts", [Ci.nsMsgFolderFlags.Drafts]);
> +  }
> +  draftsFolder = MailServices.accounts.localFoldersServer.rootFolder

Same as above -

MailServices.accounts
            .localFoldersServer
            .rootFolder
            .getChildNamed("Drafts")

@@ +42,5 @@
> +  mc.click(mc.eid("editMessageButton"));
> +  let cwc = wait_for_compose_window();
> +
> +  mc.click(mc.eid("editMessageButton")); // click edit in main win again
> +  

Nit: whitespace

@@ +46,5 @@
> +  
> +  mc.sleep(1000); // wait a sec to see if it caused a new window
> +
> +  assert_true(Services.ww.activeWindow == cwc.window,
> +    "the original draft composition window should have got focus (again)");

Could we also do a quick check to ensure that the number of compose windows has not changed? Just to make the test a little stronger.

::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
@@ +388,5 @@
>   */
>  
>  /**
>   * Create a folder and rebuild the folder tree view.
> + * @param aFolderName  A folder name with no support for hierarchy at this time. 

Nit - whitespace

Comment 33

[Magnus Melin [:mkmelin]]

Attachment: proposed mozmill test, v2

Addressing review comments.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 673374 [details] [diff] [review]
proposed mozmill test, v2

Review of attachment 673374 [details] [diff] [review]:
-----------------------------------------------------------------

I'm OK with this. Thanks Magnus!

Comment 35

[Magnus Melin [:mkmelin]]

Tests checked in: http://hg.mozilla.org/comm-central/rev/8b167c627851

Comment 36

[Mark Banner (:standard8)]

Unfortunately I had to back this out due to mozmill bustage on Windows & Mac:

https://hg.mozilla.org/comm-central/rev/d514587bff1e

Test Failure: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBHdr.getStringReference]
++DOMWINDOW == 88 (0x148d6f910) [serial = 330] [outer = 0x110d06fa0]
++DOMWINDOW == 89 (0x14b46bad0) [serial = 331] [outer = 0x110d06fa0]
--DOMWINDOW == 88 (0x14cbeca00) [serial = 327] [outer = 0x0] [url = chrome://global/content/commonDialog.xul]
WARNING: Subdocument container has no frame: file ../../../../mozilla/layout/base/nsDocumentViewer.cpp, line 2385
++DOMWINDOW == 89 (0x147dc55f0) [serial = 332] [outer = 0x110d06fa0]
++DOCSHELL 0x14ae3f0d0 == 43 [id = 142]
++DOMWINDOW == 90 (0x13dff88b0) [serial = 333] [outer = 0x0]
++DOMWINDOW == 91 (0x13cc16340) [serial = 334] [outer = 0x13dff8830]
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/composition/test-forward-headers.js | test-forward-headers.js::test_forward_inline

Test Failure: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBHdr.getStringReference]
++DOMWINDOW == 95 (0x14cb8f360) [serial = 345] [outer = 0x110d06fa0]
++DOMWINDOW == 96 (0x146f07be0) [serial = 346] [outer = 0x110d06fa0]
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/composition/test-forward-headers.js | test-forward-headers.js::test_forward_as_attachments

Comment 37

[Magnus Melin [:mkmelin]]

The errors were: https://tbpl.mozilla.org/php/getParsedLog.php?id=16487740&tree=Thunderbird-Trunk

Comment 38

[Magnus Melin [:mkmelin]]

Attachment: proposed mozmill test, v3

So the other test started testing on the message this test left in Drafts. 
Adding press_delete(mc); and fixing references checking on those other tests...

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 675888 [details] [diff] [review]
proposed mozmill test, v3

Review of attachment 675888 [details] [diff] [review]:
-----------------------------------------------------------------

Last two nits, and I think it's good (I assume you've run it on try as well).  Thanks Magnus!

::: mail/test/mozmill/composition/test-drafts.js
@@ +13,5 @@
> +
> +const RELATIVE_ROOT = "../shared-modules";
> +const MODULE_REQUIRES = ["folder-display-helpers", "compose-helpers", "window-helpers"];
> +
> +Cu.import("resource:///modules/Services.jsm");

This doesn't look right - should it be resource://gre/modules/Services.jsm?

@@ +35,5 @@
> +                             .rootFolder
> +                             .getChildNamed("Drafts");
> +}
> +
> +/** Tests that we only open one compose window for one instance of draft. */

Please use

/**
 * Test description
 */

Comment 40

[Magnus Melin [:mkmelin]]

(In reply to Mike Conley (:mconley) from comment #39)
> > +Cu.import("resource:///modules/Services.jsm");
> 
> This doesn't look right - should it be resource://gre/modules/Services.jsm?

yes, must have copied it from one of the places it's wrong (there's a couple of them in the tree). strangely Services does work anyways...

https://hg.mozilla.org/comm-central/rev/949f03209968