Last Comment Bug 349547 - Should not be able to open the same draft multiple times
: Should not be able to open the same draft multiple times
Status: RESOLVED FIXED
[tb-papercut]
: dataloss
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 All
: -- critical with 3 votes (vote)
: Thunderbird 18.0
Assigned To: michelr
:
:
Mentors:
: 550022 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-21 11:31 PDT by Jon B
Modified: 2012-11-10 11:08 PST (History)
14 users (show)
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix that handles saving draft (saves changes the internal message key) (5.42 KB, patch)
2012-09-03 04:12 PDT, michelr
mkmelin+mozilla: review-
Details | Diff | Splinter Review
simplified version of previous patch : just check draftId on opened windows (2.07 KB, patch)
2012-09-11 16:41 PDT, michelr
no flags Details | Diff | Splinter Review
use of a regex ; comments formatted (2.33 KB, patch)
2012-09-14 02:26 PDT, michelr
mkmelin+mozilla: review+
Details | Diff | Splinter Review
cleaning from mkmelin review (2.39 KB, patch)
2012-10-01 04:48 PDT, michelr
renon: review+
Details | Diff | Splinter Review
proposed mozmill test (3.63 KB, patch)
2012-10-11 11:21 PDT, Magnus Melin
mconley: review-
Details | Diff | Splinter Review
proposed mozmill test, v2 (4.16 KB, patch)
2012-10-19 12:45 PDT, Magnus Melin
mconley: review+
Details | Diff | Splinter Review
proposed mozmill test, v3 (6.13 KB, patch)
2012-10-27 13:28 PDT, Magnus Melin
mconley: review+
Details | Diff | Splinter Review

Description Jon B 2006-08-21 11:31:58 PDT
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 2006-09-12 14:27:26 PDT
upgrade to major because it causes loss of work
Comment 2 Jon B 2007-08-14 08:08:46 PDT
Still a problem in 2.0.0.6
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2008-12-14 12:28:38 PST
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] 2008-12-14 13:13:18 PST
agreed, we should probably always bring the open draft to the front instead of opening up a new window.
Comment 5 Wayne Mery (:wsmwk, NI for questions) 2008-12-14 15:08:07 PST
confirming
Comment 6 Jeff Beckley 2008-12-15 01:06:49 PST
(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 7 Wayne Mery (:wsmwk, NI for questions) 2011-02-16 12:44:08 PST
*** Bug 550022 has been marked as a duplicate of this bug. ***
Comment 8 Neville Hillyer 2011-02-16 13:29:23 PST
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, NI for questions) 2011-02-16 13:40:22 PST
edge case, but dataloss is dataloss
Comment 10 Neville Hillyer 2011-02-17 02:03:58 PST
Thanks Wayne.

Is there a way to update version and platform?
Comment 11 Neville Hillyer 2011-02-17 03:41:06 PST
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 2012-08-07 10:24:46 PDT
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://firstname%2Elastname@pop.isp.fr/Drafts#296929?fetchCompleteMessage=true
+				// instead of :
+				//    draft : mailbox-message://firstname.lastname@pop.isp.fr/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 2012-08-07 23:23:58 PDT
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] 2012-08-08 00:57:11 PDT
(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 2012-09-02 05:41:44 PDT
(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 2012-09-03 01:10:41 PDT
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 2012-09-03 04:12:56 PDT
Created attachment 657815 [details] [diff] [review]
fix that handles saving draft (saves changes the internal message key)
Comment 18 Ludovic Hirlimann [:Usul] 2012-09-03 05:35:40 PDT
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 2012-09-03 05:58:18 PDT
(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 2012-09-11 12:55:16 PDT
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 2012-09-11 16:41:58 PDT
Created attachment 660254 [details] [diff] [review]
simplified version of previous patch : just check draftId on opened windows
Comment 22 Magnus Melin 2012-09-13 13:26:30 PDT
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 2012-09-14 02:26:33 PDT
Created attachment 661167 [details] [diff] [review]
use of a regex ; comments formatted
Comment 24 Magnus Melin 2012-09-16 11:50:41 PDT
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://john%2Edoe@pop.isp.com/Drafts#123456?fetchCompleteMessage=true
> + *    ex : mailbox-message://john%2Edoe@pop.isp.com/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 2012-09-29 15:25:00 PDT
Simple question : can I add 'checkin-needed' keyword, just like I did for the bug 739311 ? Thanks
Comment 26 Kent James (:rkent) 2012-09-30 05:59:16 PDT
(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 2012-10-01 04:48:00 PDT
Created attachment 666500 [details] [diff] [review]
cleaning from mkmelin review

carrying forward mkmelin r+
Comment 28 Neville Hillyer 2012-10-01 06:29:39 PDT
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] 2012-10-01 16:18:10 PDT
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 2012-10-03 07:58:07 PDT
(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 2012-10-11 11:21:50 PDT
Created attachment 670469 [details] [diff] [review]
proposed mozmill test

Yes it should have a test, i'll give you this one
Comment 32 Mike Conley (:mconley) 2012-10-16 11:42:12 PDT
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 2012-10-19 12:45:42 PDT
Created attachment 673374 [details] [diff] [review]
proposed mozmill test, v2

Addressing review comments.
Comment 34 Mike Conley (:mconley) 2012-10-22 12:46:11 PDT
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 2012-10-25 11:55:42 PDT
Tests checked in: http://hg.mozilla.org/comm-central/rev/8b167c627851
Comment 36 Mark Banner (:standard8, limited time in Dec) 2012-10-26 04:59:45 PDT
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 38 Magnus Melin 2012-10-27 13:28:31 PDT
Created attachment 675888 [details] [diff] [review]
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) 2012-11-07 18:07:21 PST
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 2012-11-10 11:08:58 PST
(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

Note You need to log in before you can comment on or make changes to this bug.