The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
MailNews: Message Display
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Phil Lacy, Assigned: Phil Lacy)

Tracking

({fixed-seamonkey2.0.1})

unspecified
seamonkey2.0
fixed-seamonkey2.0.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

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

drag and drop external message files into the thread pane
(Assignee)

Comment 1

8 years ago
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.
Attachment #385594 - Flags: review?(mnyromyr)
(Assignee)

Comment 2

8 years ago
Created attachment 385596 [details] [diff] [review]
drag and drop external files to threadpane

see other description.  Last was a bad patch.
Attachment #385594 - Attachment is obsolete: true
Attachment #385596 - Flags: review?(mnyromyr)
Attachment #385594 - Flags: review?(mnyromyr)
(Assignee)

Comment 3

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

>+    if (extFile.isFile()){

need space before {
(Assignee)

Updated

8 years ago
No longer depends on: 500564
(Assignee)

Comment 4

8 years ago
Created attachment 386172 [details] [diff] [review]
fix nits and add test for ".eml" before drop and changed func name
Attachment #385596 - Attachment is obsolete: true
Attachment #386172 - Flags: review?(mnyromyr)
Attachment #385596 - Flags: review?(mnyromyr)

Comment 5

8 years ago
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!
Attachment #386172 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 6

8 years ago
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.
Attachment #386172 - Attachment is obsolete: true
Attachment #387373 - Flags: review?(mnyromyr)

Comment 7

8 years ago
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. :(
Attachment #387373 - Flags: review?(mnyromyr) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Updated

8 years ago
Keywords: checkin-needed

Comment 8

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

You need SR for shared code in /mailnews.
Attachment #387373 - Flags: superreview?(bienvenu)
(Assignee)

Comment 9

8 years ago
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
Attachment #387373 - Flags: superreview?(bienvenu)
(Assignee)

Comment 10

8 years ago
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.
Attachment #387373 - Attachment is obsolete: true
Attachment #389418 - Flags: review?(mnyromyr)

Updated

8 years ago
Attachment #389418 - Flags: review?(mnyromyr) → review+
(Assignee)

Updated

8 years ago
Attachment #389418 - Flags: superreview?(bienvenu)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED

Comment 11

8 years ago
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.
Attachment #389418 - Flags: superreview?(bienvenu) → superreview-
(Assignee)

Comment 12

8 years ago
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.
Attachment #389418 - Attachment is obsolete: true
Attachment #393112 - Flags: superreview?(bienvenu)
(Assignee)

Comment 13

8 years ago
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
Blocks: 499878
(Assignee)

Updated

8 years ago
No longer blocks: 499878
Depends on: 499878

Comment 14

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

I'm not the right person for SM reviews.
Attachment #393112 - Flags: superreview?(bienvenu) → superreview?(neil)
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?
Attachment #393112 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 16

8 years ago
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.
Attachment #393112 - Attachment is obsolete: true
Attachment #405784 - Flags: review?(neil)
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())
Attachment #405784 - Flags: review?(neil) → review+
(Assignee)

Comment 18

8 years ago
Created attachment 405787 [details] [diff] [review]
combined tests
Attachment #405784 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
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.
Attachment #405787 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 20

8 years ago
Need approval-sm2.0 before checkin.
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.0
(Assignee)

Updated

8 years ago
Attachment #406798 - Flags: approval-seamonkey2.0?

Comment 21

8 years ago
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.
Attachment #406798 - Flags: approval-seamonkey2.0?
Attachment #406798 - Flags: approval-seamonkey2.0.1+
Attachment #406798 - Flags: approval-seamonkey2.0-
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 22

8 years ago
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
Attachment #406798 - Attachment description: for checkin → for checkin [Checkin: Comment 22]

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed → fixed-seamonkey2.0.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.