[quote] AFAIK Thunderbird is not able to drag and drop messages outside of the application. It should be possible to drop emails into the filesystem or into text editors with predictable results. [/quote]

Basically just a cut and paste port from Thunderbird Bug 227305. After this patch there is only one consumer for BeginDragTree(). Should I move it into BeginDragFolderTree()? Works smoothly on comm-central but there are some peculiarities on comm-191 where some applications do not accept drops from SeaMonkey (WinXP explorer windows however are fine).

Comment on attachment 419748 [details] [diff] [review] Patch v1.0 >+ var ios = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); Not used? >+ var msgUrls = {}; Not used? >+ for (let i in messages) { Please use a method that does not enumerate custom Array.prototype properties. >+ messenger.messageServiceFromURI(messages[i]) >+ .GetUrlForUri(messages[i], msgUrls, null); >+ let subject = messenger.messageServiceFromURI(messages[i]) >+ .messageURIToMsgHdr(messages[i]).mime2DecodedSubject; Nit: calling messageServiceFromURI twice. >+ var uniqueFileName = suggestUniqueFileName(subject.substr(0,124), ".eml", >+ fileNames); >+ fileNames[i] = uniqueFileName; >+ aEvent.dataTransfer.mozSetDataAt("text/x-moz-message", messages[i], i); >+ aEvent.dataTransfer.mozSetDataAt("text/x-moz-url",msgUrls.value.spec, i); Nit: spaces after commas, also limit it to 120 just in case. >+ suggestion = GenerateValidFilename(base, type); GenerateValidFilename is deprecated, please use validateFileName and you only need to call it once. >+ for (let i = 0; i < existingNames.length; i++) { >+ if (existingNames[i] == suggestion) { Use indexOf >+ base = identifier + suffix; Use (2), (3) etc. >- let dt = aEvent.dataTransfer; >+ var dt = aEvent.dataTransfer; Even I'm not that mean!

>>+ var msgUrls = {}; > Not used? It looks used. Unless you mean to move this declaration into the for loop. >>+ base = identifier + suffix; > Use (2), (3) etc. I'm sorry but I haven't a clue what your comment means. How about a dummies guide to what those bracketed numbers mean?

Neil: I also plan to port Thunderbird Bug 58140 (save multiple messages as individual files in directory) and I it occurs to me that I could reuse suggestUniqueFileName() there. If this is the case I should move it to a more accessible location. Would utilityOverlay.js be suitable?

(In reply to comment #3) > >>+ var msgUrls = {}; > > Not used? > It looks used. Sorry, I wasn't expecting an outparam. >>>+ base = identifier + suffix; >> Use (2), (3) etc. > I'm sorry but I haven't a clue what your comment means. How about a dummies > guide to what those bracketed numbers mean? They're alternate suffixes (instead of 1, 2 etc.) Perhaps I should say "(2)"?

Changes since v1.0: 1. Moved suggestUniqueFileName() to utilityOverlay.js so that it can be reused elsewhere. 2. Fixed the following: >(From update of attachment 419748 [details] [diff] [review]) >>+ var ios = Components.classes["@mozilla.org/network/io-service;1"] >>+ .getService(Components.interfaces.nsIIOService); > Not used? Removed. >>+ for (let i in messages) { > Please use a method that does not enumerate custom Array.prototype properties. Fixed. >>+ messenger.messageServiceFromURI(messages[i]) >>+ .GetUrlForUri(messages[i], msgUrls, null); >>+ let subject = messenger.messageServiceFromURI(messages[i]) >>+ .messageURIToMsgHdr(messages[i]).mime2DecodedSubject; > Nit: calling messageServiceFromURI twice. Fixed. >>+ var uniqueFileName = suggestUniqueFileName(subject.substr(0,124), ".eml", >>+ fileNames); >>+ fileNames[i] = uniqueFileName; >>+ aEvent.dataTransfer.mozSetDataAt("text/x-moz-message", messages[i], i); >>+ aEvent.dataTransfer.mozSetDataAt("text/x-moz-url",msgUrls.value.spec, i); > Nit: spaces after commas, also limit it to 120 just in case. Fixed. > >+ suggestion = GenerateValidFilename(base, type); > GenerateValidFilename is deprecated, please use validateFileName and you only > need to call it once. Fixed. >>+ for (let i = 0; i < existingNames.length; i++) { >>+ if (existingNames[i] == suggestion) { > Use indexOf Fixed. >>+ base = identifier + suffix; > Use (2), (3) etc. Fixed.

Comment on attachment 419829 [details] [diff] [review] Patch v1.1 fix nits. >+ do { >+ exists = false; >+ suggestion = base + type; >+ if (existingNames.indexOf(suggestion) != -1) >+ { >+ base = identifier + "(" + suffix + ")"; >+ suffix++; >+ exists = true; >+ } >+ } while (exists); >+ return suggestion; Might be easier to do either a) while (existingName.indexOf(base + type) != -1) b) if (existingName.indexOf(suggestion) == -1) return suggestion; >+ let uniqueFileName = suggestUniqueFileName(subject.substr(0,120), ".eml", Nit: 0, 120

> (From update of attachment 419829 [details] [diff] [review]) >>+ do { >>+ exists = false; >>+ suggestion = base + type; >>+ if (existingNames.indexOf(suggestion) != -1) >>+ { >>+ base = identifier + "(" + suffix + ")"; >>+ suffix++; >>+ exists = true; >>+ } >>+ } while (exists); >>+ return suggestion; > Might be easier to do either > a) while (existingName.indexOf(base + type) != -1) > b) if (existingName.indexOf(suggestion) == -1) > return suggestion; Fixed using suggestion (a). >>+ let uniqueFileName = suggestUniqueFileName(subject.substr(0,120), ".eml", > Nit: 0, 120 Fixed. Plus removed the base and exists variables.

Comment on attachment 419909 [details] [diff] [review] Patch v1.2 More nits fixed. >+ while(existingNames.indexOf(suggestion) != -1) Nice. But nit: space between while and ( >+ suggestion = identifier + "(" + suffix + ")" + type; >+ suffix++; Nit: swap these two so that suffixes start at 2. (Or use + ++suffix +)

> (From update of attachment 419909 [details] [diff] [review]) >>+ while(existingNames.indexOf(suggestion) != -1) > Nice. But nit: space between while and ( Fixed. >>+ suggestion = identifier + "(" + suffix + ")" + type; >>+ suffix++; > Nit: swap these two so that suffixes start at 2. (Or use + ++suffix +) Fixed. Although I personally think it would be more logical to start at (1) - think of the filenames starting at base 0 rather than base 1.

Comment on attachment 420044 [details] [diff] [review] Patch v1.3 (sr=neil) This patch also includes: [Bug 524852] drag and drop message to local file system: file with 0 bytes [Bug 532779] Unable to open Attached Message Part Both of which have just been backported (including the changes to the backend) to Thunderbird 3.0.x. Now that the backend changes have made it to 191 I am requesting for approval‑seamonkey2.0.3.

Comment on attachment 420044 [details] [diff] [review] Patch v1.3 (sr=neil) This does neither work on Linux (Kubuntu 8.04.3 LTS) trunk nor branch. Although I can drag messages from the threadpane onto the Desktop or into an editor (kate), the resulting file consists of a single character, regardless of the message I choose. It doesn't even matter if your patch is applied or not. (Dragging into Dolphin file manager fails completely, jftr.) No related errors in EC. Even tried a new profile, to no avail. >diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js >+/** >+ * example use: >+ * suggestUniqueFileName("testname", ".txt", ["testname", "testname1"]) >+ * returns "testname2.txt" >+ * does not check file system for existing files >+ * >+ * @param identifier base name for generating unique filenames. >+ * >+ * @param type extension name to use for the generated filename. >+ * >+ * @param existingNames array of names in use. >+ * >+ * @return suggested filename as a string. >+ */ >+function suggestUniqueFileName(identifier, type, existingNames) >+{ >+ var suffix = 1; >+ identifier = validateFileName(identifier); >+ var suggestion = identifier + type; >+ while (existingNames.indexOf(suggestion) != -1) >+ { >+ suffix++; >+ suggestion = identifier + "(" + suffix + ")" + type; >+ } >+ return suggestion; >+} This function is quite a mess: - The comments don't match what the function actually does, eg. the suggestion assignment contains brackets! - Calling the function with the example data will result in testname.txt! - The parameters "identifier" and "type" are poorly named and violate the parameter naming guidelines. How about "aBaseName" and "aExtension"? Under which OS did you test this patch?

> This does neither work on Linux (Kubuntu 8.04.3 LTS) trunk nor branch. Although > I can drag messages from the threadpane onto the Desktop or into an editor > (kate), the resulting file consists of a single character, regardless of the > message I choose. It doesn't even matter if your patch is applied or not. > (Dragging into Dolphin file manager fails completely, jftr.) > No related errors in EC. > Even tried a new profile, to no avail. See https://bugzilla.mozilla.org/show_bug.cgi?id=227305#c92 "This does one message at a time for windows only. Linux backend fix is in the works but MAC is unknown at this time." Also see Linux/GTK Bug 377621 (Drag and Drop attachments to desktop or folders doesn't work) > Under which OS did you test this patch? WindowsXP of course! >>+function suggestUniqueFileName(identifier, type, existingNames) > > This function is quite a mess: > - The comments don't match what the function actually does, eg. the suggestion > assignment contains brackets! > - Calling the function with the example data will result in testname.txt! Last minute changes requested by Neil. I forgot to update the comments. > - The parameters "identifier" and "type" are poorly named and violate the > parameter naming guidelines. How about "aBaseName" and "aExtension"? Ok. Will fix this in the next patch.

>> - The comments don't match what the function actually does, eg. the suggestion >> assignment contains brackets! >> - Calling the function with the example data will result in testname.txt! > Last minute changes requested by Neil. I forgot to update the comments. Comments updated. >> - The parameters "identifier" and "type" are poorly named and violate the >> parameter naming guidelines. How about "aBaseName" and "aExtension"? > > Ok. Will fix this in the next patch. Fixed. Moving review request to IanN for testing on Windows.

>> - The comments don't match what the function actually does, eg. the suggestion >> assignment contains brackets! >> - Calling the function with the example data will result in testname.txt! > Last minute changes requested by Neil. I forgot to update the comments. Comments updated. >> - The parameters "identifier" and "type" are poorly named and violate the >> parameter naming guidelines. How about "aBaseName" and "aExtension"? > > Ok. Will fix this in the next patch. Fixed.

Comment on attachment 420906 [details] [diff] [review] Patch v1.4 (sr=neil) >+/** >+ * example use: >+ * suggestUniqueFileName("testname", ".txt", ["testname", "testname(2)"]) >+ * returns "testname(3).txt" This is still wrong. The function checks for something + extension in aExistingNames, which will always fail here, hence the result is still "testname.txt".

> This is still wrong. > The function checks for something + extension in aExistingNames, which will > always fail here, hence the result is still "testname.txt". Sigh. I hope I got it right this time.

Ping IanN?

Comment on attachment 421001 [details] [diff] [review] Patch v1.5 fix comment again (sr=neil) >--- a/suite/mailnews/messengerdnd.js >@@ -465,50 +465,74 @@ function BeginDragFolderTree(event) > { > var startIndex = {}; > var endIndex = {}; > folderTree.view.selection.getRangeAt(i, startIndex, endIndex); > for (var j = startIndex.value; j <= endIndex.value; j++) > folderArray.push(GetFolderResource(folderTree, j).Value); > } > >- return BeginDragTree(event, folderTree, folderArray, flavor); >+ return BeginDragTree(event, folderArray, flavor); As this appears to be the only caller of BeginDragTree why not inline it? Apart from that everything looks good.

>>--- a/suite/mailnews/messengerdnd.js >>@@ -465,50 +465,74 @@ function BeginDragFolderTree(event) > As this appears to be the only caller of BeginDragTree why not inline it? Fixed. I also took the opportunity to sync with the Thunderbird equivalent |gFolderTreeView._onDragStart()|. Because of this, asking for another review. > -function BeginDragFolderTree(event) > +function BeginDragFolderTree(aEvent) > + var folderTree = GetFolderTree(); > + var row = folderTree.treeBoxObject.getRowAt(event.clientX, event.clientY); > + if (row == -1) > + return false; Thunderbird doesn't do this. Do we still need this? > -function BeginDragFolderTree(event) > +function BeginDragFolderTree(aEvent) > { > - debugDump("BeginDragFolderTree\n"); > > -function BeginDragThreadPane(event) > +function BeginDragThreadPane(aEvent) > { > - debugDump("BeginDragThreadPane\n"); Removed useless dump statements like Thunderbird.

(In reply to comment #20) >> +function BeginDragFolderTree(aEvent) >> + var folderTree = GetFolderTree(); >> + var row = folderTree.treeBoxObject.getRowAt(event.clientX, event.clientY); >> + if (row == -1) >> + return false; >Thunderbird doesn't do this. Do we still need this? Not now that we don't get the flavour from the dragged row any more, no.

Comment on attachment 423767 [details] [diff] [review] Patch v1.6 Inline BeginDragFolderTree() >+ //A message can be dragged from one window and dropped on another window >+ //therefore setNextMessageAfterDelete() here >+ //no major disadvantage even if it is a copy operation Nit: space between // and comment, plus comment could do with some punctuation. >+ // dragging multiple messages to desktop does not >+ // currently work, pending core fixes for >+ // multiple-drop-on-desktop support. (bug 513464) Nit: This comment could probably fit on 2 rather than 3 lines. I don't have a windows build env, so cannot test - patching jar files no longer seems to work for me.

>>> +function BeginDragFolderTree(aEvent) >>> + var folderTree = GetFolderTree(); >>> + var row = folderTree.treeBoxObject.getRowAt(event.clientX, event.clientY); >>> + if (row == -1) >>> + return false; >>Thunderbird doesn't do this. Do we still need this? > Not now that we don't get the flavour from the dragged row any more, no. Removed. >>+ //A message can be dragged from one window and dropped on another window >>+ //therefore setNextMessageAfterDelete() here >>+ //no major disadvantage even if it is a copy operation > Nit: space between // and comment, plus comment could do with some punctuation. Fixed. >>+ // dragging multiple messages to desktop does not >>+ // currently work, pending core fixes for >>+ // multiple-drop-on-desktop support. (bug 513464) > Nit: This comment could probably fit on 2 rather than 3 lines. Fixed. > I don't have a windows build env, so cannot test - patching jar files no longer > seems to work for me. Bummer. Did you set |nglayout.debug.disable_xul_cache| to true? Changing the reviewer yet again.

Comment on attachment 425415 [details] [diff] [review] Patch v1.7 fix nits. r=me on this (though suffix = 2 and suffix++ after suggestion would read easier to me).

Comment on attachment 425415 [details] [diff] [review] Patch v1.7 fix nits. >+ var folders = GetSelectedMsgFolders(); >+ folders = folders.filter(function(f) { return !f.isServer; }); Nit: need to return here if only a server was dragged.

>>+ var folders = GetSelectedMsgFolders(); >>+ folders = folders.filter(function(f) { return !f.isServer; }); > Nit: need to return here if only a server was dragged. Hmm? Like this? + if (!folders.length) + return false;

>>+ var folders = GetSelectedMsgFolders(); >>+ folders = folders.filter(function(f) { return !f.isServer; }); > Nit: need to return here if only a server was dragged. Hmm? Like this? + if (!folders.length) + return false; Neil says this looks vaguely ok over IRC.

Philip, is there anywhere I can get a build of Seamonkey with this patch without doing it myself? (tryserver or something?) If not, I can build it myself but I won't get around to doing that until later this week.

Looking at my patch in Bug 513464, I do see a possible bug with the "x-moz-file-promise" format code that I wrote. I'll try to test this soon.

Thanks Kyle, I've asked KaiRo about tryservers, I don't know myself if we have any.

Oh Thunderbird/Shredder nightlies should have the equivalent patch in it already.

Awesome I'll experiment with that.

yes on latest TB, I can drag multiple messages to an editor program but not to the file system shell

Pushed to c-c: http://hg.mozilla.org/comm-central/rev/3d75fae6a1ed

This probably caused bug 556887. Philip, can you take a look?

(In reply to comment #34) > yes on latest TB, I can drag multiple messages to an editor program but not to > the file system shell This still holds true for SM (haven't check TB). The code checked in for this bug contains a comment referring to bug 513464. That's RESOLVED FIXED now, though, so I expect no progress there. We should probably file a follow-up and note it in bug 513464 so that we don't have to change the comment in the code. Philip?

> We should probably file a follow-up and > note it in bug 513464 so that we don't have to change the comment in the code. Check with TB first?

(In reply to comment #38) > > We should probably file a follow-up and > > note it in bug 513464 so that we don't have to change the comment in the code. > Check with TB first? Same with latest trunk Shredder on WinXP, i.e. dragging of single messages works, for multiple the drop is not allowed.