Closed
Bug 349547
Opened 18 years ago
Closed 12 years ago
Should not be able to open the same draft multiple times
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 18.0
People
(Reporter: 4aeikob02, Assigned: renon)
References
Details
(Keywords: dataloss, Whiteboard: [tb-papercut])
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
renon
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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...) :-)
upgrade to major because it causes loss of work
Severity: normal → major
Updated•16 years ago
|
Assignee: mscott → nobody
Comment 3•16 years ago
|
||
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•16 years ago
|
||
agreed, we should probably always bring the open draft to the front instead of opening up a new window.
Comment 5•16 years ago
|
||
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-thunderbird3?
Comment 6•16 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.
Comment 8•14 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.
Comment 9•14 years ago
|
||
edge case, but dataloss is dataloss
Severity: major → critical
Keywords: dataloss
Comment 10•14 years ago
|
||
Thanks Wayne.
Is there a way to update version and platform?
Updated•14 years ago
|
OS: Windows XP → All
Version: 2.0 → Trunk
Comment 11•14 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•12 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•12 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
Comment 14•12 years ago
|
||
(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•12 years ago
|
Whiteboard: [tb-papercut]
Assignee | ||
Comment 15•12 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•12 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•12 years ago
|
||
Comment 18•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Attachment #657815 -
Attachment is obsolete: true
Attachment #660254 -
Flags: review?(mkmelin+mozilla)
Comment 22•12 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•12 years ago
|
||
Attachment #660254 -
Attachment is obsolete: true
Attachment #660254 -
Flags: review?(mkmelin+mozilla)
Attachment #661167 -
Flags: review?(mkmelin+mozilla)
Comment 24•12 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•12 years ago
|
||
Simple question : can I add 'checkin-needed' keyword, just like I did for the bug 739311 ? Thanks
Comment 26•12 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•12 years ago
|
||
carrying forward mkmelin r+
Attachment #661167 -
Attachment is obsolete: true
Attachment #666500 -
Flags: review+
Keywords: checkin-needed
Comment 28•12 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.
Comment 29•12 years ago
|
||
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
Closed: 12 years ago
Flags: wanted-thunderbird3? → in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
Assignee | ||
Comment 30•12 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•12 years ago
|
||
Yes it should have a test, i'll give you this one
Attachment #670469 -
Flags: review?(mconley)
Comment 32•12 years ago
|
||
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•12 years ago
|
||
Addressing review comments.
Attachment #670469 -
Attachment is obsolete: true
Attachment #673374 -
Flags: review?(mconley)
Comment 34•12 years ago
|
||
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•12 years ago
|
||
Tests checked in: http://hg.mozilla.org/comm-central/rev/8b167c627851
Flags: in-testsuite? → in-testsuite+
Comment 36•12 years ago
|
||
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•12 years ago
|
||
Comment 38•12 years ago
|
||
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 39•12 years ago
|
||
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•12 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
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•