Should not be able to open the same draft multiple times

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Message Compose Window
--
critical
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Jon B, Assigned: michelr)

Tracking

({dataloss})

Trunk
Thunderbird 18.0
x86
All
dataloss
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb-papercut])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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...)  :-)
(Reporter)

Comment 1

11 years ago
upgrade to major because it causes loss of work
Severity: normal → major
(Reporter)

Comment 2

10 years ago
Still a problem in 2.0.0.6
Version: unspecified → 2.0

Updated

9 years ago
Assignee: mscott → nobody
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"
agreed, we should probably always bring the open draft to the front instead of opening up a new window.
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-thunderbird3?

Comment 6

9 years ago
(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.
Duplicate of this bug: 550022

Comment 8

7 years ago
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.
edge case, but dataloss is dataloss
Severity: major → critical
Keywords: dataloss

Comment 10

7 years ago
Thanks Wayne.

Is there a way to update version and platform?
OS: Windows XP → All
Version: 2.0 → Trunk

Comment 11

7 years ago
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'?
(Assignee)

Comment 12

5 years ago
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

5 years ago
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
(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.
Assignee: nobody → michel.renon
Status: NEW → ASSIGNED

Updated

5 years ago
Whiteboard: [tb-papercut]
(Assignee)

Comment 15

5 years ago
(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

5 years ago
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
(Assignee)

Comment 17

5 years ago
Created attachment 657815 [details] [diff] [review]
fix that handles saving draft (saves changes the internal message key)
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.
Attachment #657815 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 19

5 years ago
(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

5 years ago
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.
Attachment #657815 - Flags: review?(mkmelin+mozilla) → review-
(Assignee)

Comment 21

5 years ago
Created attachment 660254 [details] [diff] [review]
simplified version of previous patch : just check draftId on opened windows
Attachment #657815 - Attachment is obsolete: true
Attachment #660254 - Flags: review?(mkmelin+mozilla)

Comment 22

5 years ago
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.
(Assignee)

Comment 23

5 years ago
Created attachment 661167 [details] [diff] [review]
use of a regex ; comments formatted
Attachment #660254 - Attachment is obsolete: true
Attachment #660254 - Flags: review?(mkmelin+mozilla)
Attachment #661167 - Flags: review?(mkmelin+mozilla)

Comment 24

5 years ago
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.
Attachment #661167 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 25

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

Comment 26

5 years ago
(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!
(Assignee)

Comment 27

5 years ago
Created attachment 666500 [details] [diff] [review]
cleaning from mkmelin review

carrying forward mkmelin r+
Attachment #661167 - Attachment is obsolete: true
Attachment #666500 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 28

5 years ago
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.
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: wanted-thunderbird3? → in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
(Assignee)

Comment 30

5 years ago
(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

5 years ago
Created attachment 670469 [details] [diff] [review]
proposed mozmill test

Yes it should have a test, i'll give you this one
Attachment #670469 - Flags: review?(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
Attachment #670469 - Flags: review?(mconley) → review-

Comment 33

5 years ago
Created attachment 673374 [details] [diff] [review]
proposed mozmill test, v2

Addressing review comments.
Attachment #670469 - Attachment is obsolete: true
Attachment #673374 - Flags: review?(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!
Attachment #673374 - Flags: review?(mconley) → review+

Comment 35

5 years ago
Tests checked in: http://hg.mozilla.org/comm-central/rev/8b167c627851
Flags: in-testsuite? → in-testsuite+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 37

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

Comment 38

5 years ago
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...
Attachment #673374 - Attachment is obsolete: true
Attachment #675888 - Flags: review?(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
 */
Attachment #675888 - Flags: review?(mconley) → review+

Comment 40

5 years ago
(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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.