Closed Bug 537448 Opened 10 years ago Closed 10 years ago

Port Bug 227305 (Support drag-drop single message to desktop / file-system window)

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

[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]
Attached patch Patch v1.0 (obsolete) — Splinter Review
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).
Attachment #419748 - Flags: superreview?(neil)
Attachment #419748 - Flags: review?(mnyromyr)
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)"?
Attached patch Patch v1.1 fix nits. (obsolete) — Splinter Review
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.
Attachment #419748 - Attachment is obsolete: true
Attachment #419829 - Flags: superreview?(neil)
Attachment #419829 - Flags: review?(mnyromyr)
Attachment #419748 - Flags: superreview?(neil)
Attachment #419748 - Flags: review?(mnyromyr)
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
Attached patch Patch v1.2 More nits fixed. (obsolete) — Splinter Review
> (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.
Attachment #419829 - Attachment is obsolete: true
Attachment #419909 - Flags: superreview?(neil)
Attachment #419909 - Flags: review?(mnyromyr)
Attachment #419829 - Flags: superreview?(neil)
Attachment #419829 - Flags: review?(mnyromyr)
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 +)
Attachment #419909 - Flags: superreview?(neil) → superreview+
Attached patch Patch v1.3 (sr=neil) (obsolete) — Splinter Review
> (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.
Attachment #419909 - Attachment is obsolete: true
Attachment #420044 - Flags: superreview+
Attachment #420044 - Flags: review?(mnyromyr)
Attachment #419909 - Flags: review?(mnyromyr)
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.
Attachment #420044 - Flags: approval-seamonkey2.0.3?
Attachment #420044 - Flags: approval-seamonkey2.0.3? → 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?
Attachment #420044 - Flags: review?(mnyromyr) → review-
> 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.
Attached patch Patch v1.4 (sr=neil) (obsolete) — Splinter Review
>> - 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.
Attachment #420044 - Attachment is obsolete: true
Attachment #420906 - Flags: superreview+
Attachment #420906 - Flags: review?(iann_bugzilla)
Attachment #420906 - Flags: approval-seamonkey2.0.3?
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.
Attachment #420906 - Attachment is obsolete: true
Attachment #421001 - Flags: superreview+
Attachment #421001 - Flags: review?(iann_bugzilla)
Attachment #420906 - Flags: review?(iann_bugzilla)
Attachment #420906 - Flags: approval-seamonkey2.0.3?
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.
Attachment #421001 - Flags: review?(iann_bugzilla) → review+
>>--- 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.
Attachment #421001 - Attachment is obsolete: true
Attachment #423767 - Flags: review?(iann_bugzilla)
(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.
Attached patch Patch v1.7 fix nits. (obsolete) — Splinter Review
>>> +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.
Attachment #423767 - Attachment is obsolete: true
Attachment #425415 - Flags: superreview?(neil)
Attachment #425415 - Flags: review?(neil)
Attachment #423767 - Flags: review?(iann_bugzilla)
Attachment #420044 - Flags: approval-seamonkey2.0.3+
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).
Attachment #425415 - Flags: review?(neil) → review+
Attachment #425415 - Flags: superreview?(neil) → superreview+
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.
Attachment #425415 - Attachment is obsolete: true
Attachment #427940 - Flags: superreview+
Attachment #427940 - Flags: review+
Keywords: checkin-needed
Duplicate of this bug: 59622
Blocks: 155643
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.
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 555972
This probably caused bug 556887. Philip, can you take a look?
Depends on: 556887
(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.
You need to log in before you can comment on or make changes to this bug.