Last Comment Bug 500917 - drag & drop an .eml file into a mailbox folder via thread pane drop
: drag & drop an .eml file into a mailbox folder via thread pane drop
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: seamonkey2.0
Assigned To: Phil Lacy
:
Mentors:
Depends on: 499878
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-27 07:52 PDT by Phil Lacy
Modified: 2009-11-02 15:39 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
drag and drop external files to threadpane (3.56 KB, patch)
2009-06-27 11:12 PDT, Phil Lacy
no flags Details | Diff | Review
drag and drop external files to threadpane (3.03 KB, patch)
2009-06-27 11:16 PDT, Phil Lacy
no flags Details | Diff | Review
fix nits and add test for ".eml" before drop and changed func name (3.16 KB, patch)
2009-06-30 18:36 PDT, Phil Lacy
mnyromyr: review-
Details | Diff | Review
fix per review and changed flavor data interface (3.15 KB, patch)
2009-07-07 21:21 PDT, Phil Lacy
mnyromyr: review+
Details | Diff | Review
forbid drop on certain folders (3.22 KB, patch)
2009-07-19 21:48 PDT, Phil Lacy
mnyromyr: review+
mozilla: superreview-
Details | Diff | Review
drop file on rss folder is ok (3.17 KB, patch)
2009-08-06 19:48 PDT, Phil Lacy
neil: superreview+
Details | Diff | Review
change per review with exceptions (2.92 KB, patch)
2009-10-11 11:46 PDT, Phil Lacy
neil: review+
Details | Diff | Review
combined tests (2.89 KB, patch)
2009-10-11 12:26 PDT, Phil Lacy
no flags Details | Diff | Review
for checkin [Checkin: Comment 22] (2.92 KB, patch)
2009-10-16 15:51 PDT, Phil Lacy
kairo: approval‑seamonkey2.0-
kairo: approval‑seamonkey2.0.1+
Details | Diff | Review

Description Phil Lacy 2009-06-27 07:52:52 PDT
+++ 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 2009-06-27 11:12:55 PDT
Created attachment 385594 [details] [diff] [review]
drag and drop external files to threadpane

not sure about the naming of functions.  these are same as shredder patch.
Comment 2 Phil Lacy 2009-06-27 11:16:41 PDT
Created attachment 385596 [details] [diff] [review]
drag and drop external files to threadpane

see other description.  Last was a bad patch.
Comment 3 Phil Lacy 2009-06-27 19:12:46 PDT
Comment on attachment 385596 [details] [diff] [review]
drag and drop external files to threadpane

>+    if (extFile.isFile()){

need space before {
Comment 4 Phil Lacy 2009-06-30 18:36:10 PDT
Created attachment 386172 [details] [diff] [review]
fix nits and add test for ".eml" before drop and changed func name
Comment 5 Karsten Düsterloh 2009-07-07 15:19:35 PDT
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 2009-07-07 21:21:25 PDT
Created attachment 387373 [details] [diff] [review]
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 2009-07-08 16:04:32 PDT
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 2009-07-08 22:51:54 PDT
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 2009-07-14 18:46:57 PDT
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 2009-07-19 21:48:03 PDT
Created attachment 389418 [details] [diff] [review]
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 2009-08-05 20:34:53 PDT
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 2009-08-06 19:48:07 PDT
Created attachment 393112 [details] [diff] [review]
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 2009-08-06 19:53:21 PDT
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 2009-08-06 21:05:24 PDT
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 neil@parkwaycc.co.uk 2009-08-10 09:35:52 PDT
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 2009-10-11 11:46:39 PDT
Created attachment 405784 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-10-11 12:07:16 PDT
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 2009-10-11 12:26:28 PDT
Created attachment 405787 [details] [diff] [review]
combined tests
Comment 19 Phil Lacy 2009-10-16 15:51:55 PDT
Created attachment 406798 [details] [diff] [review]
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 2009-10-17 08:17:55 PDT
Need approval-sm2.0 before checkin.
Comment 21 Robert Kaiser (not working on stability any more) 2009-10-25 04:44:29 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.