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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Phil Lacy]

drag and drop external message files into the thread pane

Comment 1

[Phil Lacy]

Attachment: drag and drop external files to threadpane

much handier and will be used more.
Drag and drop to the open threadpane.

Comment 2

[Phil Lacy]

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

will fix indents and resubmit. this patch is good for testing though

Comment 3

[Phil Lacy]

Attachment: drag and drop external files to threadpane

Fixed some indents.

Comment 4

[Phil Lacy]

Attachment: drag and drop external files to threadpane

Missed a space before {

Comment 5

[Magnus Melin [:mkmelin]]

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

If i drop on an empty folder pane i keep getting

Error: dt.mozGetDataAt("application/x-moz-file", 0).QueryInterface is not a function
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1139


>-                    <treechildren ondraggesture="threadPaneOnDragStart(event);"/>
>+                    <treechildren ondraggesture="threadPaneOnDragStart(event);"
>+                                  ondragover="threadPaneOnDragOver(event);"
>+                                  ondrop="threadPaneOnDrop(event);"/>

These method names should be capitalized (please also fix the existing threadPaneOnDragStart).



>+      if (len > 4 && extFile.leafName.substr(len-4).toLowerCase() == '.eml')

len - 4 and please use double quotes around .eml (single quotes only when necessary)

Comment 6

[Magnus Melin [:mkmelin]]

I meant "on an empty *thread* pane, of course.

Comment 7

[Phil Lacy]

Attachment: fix some review remarks

I can't seem to get your error. But I did other fixes.

Note I added a test to each file for .eml on the drop.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 386158 [details] [diff] [review]
fix some review remarks

Sorry, still the same exception when the folder pane is emtpy.

>+  let dt = aEvent.dataTransfer;
>+  if (Array.indexOf(dt.mozTypesAt(0), "application/x-moz-file") != -1) {
>+    let extFile = dt.mozGetDataAt("application/x-moz-file", 0)
>+                    .QueryInterface(Components.interfaces.nsILocalFile);

Should this be nsIFile too. Or is there a reason it's nsILocalFile?

Comment 9

[Phil Lacy]

Sorry, can't dupe the error using latest build with only this patch added. I had other patches in place that seemed to be irrelevant so I unloaded those and still can't get an error.

This is starting with new empty folder in gmail IMAP build af of 6/30/09.
This is same code as 171907. Magnus try dnd a eml file to the folderpane of an empty IMAP folder as per 171907 fix.

Comment 10

[Phil Lacy]

Attachment: change to nsIFile and diff to tip

Magnus,
changed nsILocalFile to nsIFile but also  I notice this patch had a 'fuzz' factor but applied ok when I brought it to the tip.  Maybe there's an chance the diff wasn't good the way I had it before.

Comment 11

[Magnus Melin [:mkmelin]]

Tried it some more, and I still get the error quite a lot. Some of the moves do succeed though, so it's a bit odd. 

Error: dt.mozGetDataAt("application/x-moz-file", 0).QueryInterface is not a function
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1157
Error: dt.mozGetDataAt("application/x-moz-file", 0) is null
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1156

In the native console i get
(thunderbird-bin:10170): Gtk-CRITICAL **: gtk_drag_get_data: assertion `GTK_IS_WIDGET (widget)' failed

... but i'm not sure that's the cause or just some symptom.

Comment 12

[Magnus Melin [:mkmelin]]

Btw, read-only folders, (e.g. news), and dropping on the thread pane of saved searches don't seem to be handled.

Comment 13

[Phil Lacy]

should the d&d of files be limited to local store only?  It appears that would be best at this point.  Also is there more limits that should be placed on the drop?  Drag and drop to the folder tree had various limits in place that I didn't have to worry about, and now I believe I'll need some help on what kind of limits need to be in this drop action.

Comment 14

[Phil Lacy]

Attachment: can't drop or rss or folders that can't file messages

works good in windows. this may satisfy review comments regarding folders that should be forbidden.

Comment 15

[Phil Lacy]

Attachment: change previous patch to allow drop file to folder of server type rss

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 389802 [details] [diff] [review]
change previous patch to allow drop file to folder of server type rss

I get the usual 
Error: dt.mozGetDataAt("application/x-moz-file", 0) is null
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1139

Error: dt.mozGetDataAt("application/x-moz-file", 0).QueryInterface is not a function
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1140

and one i don't recall seeing before:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.isFile]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: ThreadPaneOnDragOver :: line 1141"  data: no]

>+    if (!gFolderDisplay.displayedFolder.canFileMessages)
>+      return;

Remove the spaces from the empty line ^^

It does work occasionally though.

I don't know why it's not working for me. You can try having someone else reviewing it. If it works on other platforms I think it's ok to take this - the exceptions doesn't cause much harm (except for that the drop doesn't work) afaikt.

Comment 17

[Phil Lacy]

Attachment: remove excess spaces and moved a test up in a function

Need to find a Mac tester and maybe bienvenu can confirm it is ok on windows.

I removed the nit on spaces, but also moved up or earlier in the function, a test for canFileMessages.

Comment 18

[Phil Lacy]

Comment on attachment 390758 [details] [diff] [review]
remove excess spaces and moved a test up in a function

could you review this on MAC platform?

Comment 19

[Phil Lacy]

Comment on attachment 390758 [details] [diff] [review]
remove excess spaces and moved a test up in a function

could you review this on windows and confirm it works. linux seems to have problems so I better have it confirmed on windows and/or vista if possible

Comment 20

[David :Bienvenu]

Comment on attachment 390758 [details] [diff] [review]
remove excess spaces and moved a test up in a function

works on windows, thx for the patch.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 390758 [details] [diff] [review]
remove excess spaces and moved a test up in a function

This works on Mac as well (no errors in the console).

>+  let dt = aEvent.dataTransfer;
>+  if (Array.indexOf(dt.mozTypesAt(0), "application/x-moz-file") != -1) {
>+    let extFile = dt.mozGetDataAt("application/x-moz-file", 0)
>+                    .QueryInterface(Components.interfaces.nsIFile);

I can't see anything wrong with these lines but for some reason mozGetDataAt is sometimes returning null, and sometimes returning something like a string of unprintable characters.

Can you raise a core bug with the symptoms and errors we get on Linux please?

Otherwise this looks and works fine. Thanks for the patch. r=Standard8 with the core bug filed.

Comment 22

[Phil Lacy]

(thunderbird-bin:10170): Gtk-CRITICAL **: gtk_drag_get_data: assertion
`GTK_IS_WIDGET (widget)' failed

I marked checkin needed. Tokoe tried this and confirmed this backend linux problem. I also asked him to file a bug when this gets checked in.

Comment 23

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/585420c2f9d4

Comment 24

[Tobias Koenig]

Added new bug report #507607 that addresses the Linux problem