500917 - drag & drop an .eml file into a mailbox folder via thread pane drop

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Phil Lacy]

+++ This bug was initially created as a clone of Bug #500564 +++

drag and drop external message files into the thread pane

Comment 1

[Phil Lacy]

Attachment: drag and drop external files to threadpane

not sure about the naming of functions.  these are same as shredder patch.

Comment 2

[Phil Lacy]

Attachment: drag and drop external files to threadpane

see other description.  Last was a bad patch.

Comment 3

[Phil Lacy]

Comment on attachment 385596 [details] [diff] [review]
drag and drop external files to threadpane

>+    if (extFile.isFile()){

need space before {

Comment 4

[Phil Lacy]

Attachment: fix nits and add test for ".eml" before drop and changed func name

Comment 5

[Karsten Düsterloh]

Comment on attachment 386172 [details] [diff] [review]
fix nits and add test for ".eml" before drop and changed func name

Works great, "even" with Linux and IMAP.
Just some nits, mostly similar to those in 499878...

First of all, please follow the prevalent bracing style in messengerdnd.js, which is "{ go on their own line, vertically aligned with }".

>diff --git a/mailnews/base/resources/content/messengerdnd.js b/mailnews/base/resources/content/messengerdnd.js
>+function DragOverThreadPane(aEvent) {
>+  let ds = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                     .getService(Components.interfaces.nsIDragService)
>+                     .getCurrentSession();
>+  ds.canDrop = false;
>+  let dt = aEvent.dataTransfer;
>+  if (Array.indexOf(dt.mozTypesAt(0), "application/x-moz-file") != -1) {

MIME type should be "message/rfc822".

>+        cs.CopyFileMessage(extFile, gMsgFolderSelected, null, false, 1, "", null,
>+                           msgWindow);

This looks odd, just put the msgWindow one line up, despite the line length.


Thanks for tackling this!

Comment 6

[Phil Lacy]

Attachment: fix per review and changed flavor data interface

thanks for review.

we need the x-moz-file flavor for transfer if files per our nsiTransferable.idl.

for some reason I thought I needed nsILocalFile functionality but nsIFile is official transfer flavor data so I changed it to that.

Comment 7

[Karsten Düsterloh]

Comment on attachment 387373 [details] [diff] [review]
fix per review and changed flavor data interface

> we need the x-moz-file flavor for transfer if files per our
> nsiTransferable.idl.

Yeah, I realized that after commenting. Sorry about that. :(

Comment 8

[Karsten Düsterloh]

Comment on attachment 387373 [details] [diff] [review]
fix per review and changed flavor data interface

You need SR for shared code in /mailnews.

Comment 9

[Phil Lacy]

Comment on attachment 387373 [details] [diff] [review]
fix per review and changed flavor data interface

Karsten can you look at the problems with this patch on TB before sr.
Bug 500564

Comment 10

[Phil Lacy]

Attachment: forbid drop on certain folders

this patch prevents or forbids drop of the file on 'rss' feed folder thread panes and also those that canfilemessage is false.

Also patched to new location in c-c hg tree.

Comment 11

[David :Bienvenu]

Comment on attachment 389418 [details] [diff] [review]
forbid drop on certain folders

I feel like I reviewed this before, but in any case, you should be able to drop into rss folders since they're just local folders.

Comment 12

[Phil Lacy]

Attachment: drop file on rss folder is ok

this is the SeaMonkey version of a similiar patch reviewed and checked-in in TB.

Comment 13

[Phil Lacy]

I set this to depend on Bug 499878 hoping that gets check in first and avoid the bit rot if this had been a diff to the tip. Hope that's kosher

Comment 14

[David :Bienvenu]

Comment on attachment 393112 [details] [diff] [review]
drop file on rss folder is ok

I'm not the right person for SM reviews.

Comment 15

[[e-mail]]

Comment on attachment 393112 [details] [diff] [review]
drop file on rss folder is ok

>+  let ds = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                     .getService(Components.interfaces.nsIDragService)
>+                     .getCurrentSession();
>+  ds.canDrop = false;
Isn't this the default? Anyway, I understand we should be using the new API for this where possible (see below).

>+  let dt = aEvent.dataTransfer;
Nit: should set the .effectAllowed to "copy"

>+    if (!gMsgFolderSelected.canFileMessages)
>+      return;
Should do this check earlier.

>+      let len = extFile.leafName.length;
>+      if (len > 4 && extFile.leafName.substr(len - 4).toLowerCase() == ".eml")
if (/\.eml$/.test(extFile.leafName)) [both times of course]

>+        ds.canDrop = true;
dev.m.o says event.preventDefault(); is the new API for this.

Note: it's slightly confusing that you can drop a group of files if the first one is a .eml file but other files get imported too, but if the first one is not a .eml file then the drop is not permitted. Check all the files maybe?

Comment 16

[Phil Lacy]

Attachment: change per review with exceptions

rerequest review due to following
>Nit: should set the .effectAllowed to "copy"
didn't add this for a few reasons.
TB2 doesn't and our patch in TB3 doesn't bar move symbol (copy still occurs either way).
But more important, due to some other defect, user can ctrl-drag for copy but once in pane it has no effect and releasing or pressing ctrl just cycles '+' symbol and it drops/copies source either way.

Comment 17

[[e-mail]]

Comment on attachment 405784 [details] [diff] [review]
change per review with exceptions

>+      if (extFile.isFile())
>+        if (/\.eml$/.test(extFile.leafName))

>+    if (extFile.isFile())
>+      if (/\.eml$/.test(extFile.leafName))

Nit: These could be combined into a single test using &&
if (/\.eml$/.test(extFile.leafName) && extFile.isFile())

Comment 18

[Phil Lacy]

Attachment: combined tests

Comment 19

[Phil Lacy]

Attachment: for checkin [Checkin: Comment 22]

I added effectAllowed="copy". In windows it doesn't do anything.
Maybe it works in mac or unix so added per our api docs for future implementation on windows.

Comment 20

[Magnus Melin [:mkmelin]]

Need approval-sm2.0 before checkin.

Comment 21

[Robert Kaiser]

Comment on attachment 406798 [details] [diff] [review]
for checkin [Checkin: Comment 22]

1) Please "forward" the review/superreview flags in the future, by setting them yourself on patches that were changed according to review comments, and noting e.g. "forward r+sr=Neil" so that someone like me, who is only coming in here for approval, can easily see what's up.

2) Please land on comm-central before landing on 1.9.1, comm-central doesn't need approval now.

3) 2.0 final is done, approving for 2.0.1, which is what current comm-1.9.1 is headed for.

Comment 22

[Ian Neal]

Comment on attachment 406798 [details] [diff] [review]
for checkin [Checkin: Comment 22]

http://hg.mozilla.org/comm-central/rev/eca2aaf19ffa
http://hg.mozilla.org/releases/comm-1.9.1/rev/9feb8020af55