513464 - multiple drags outside moz app to shell or external program fails

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Phil Lacy]

dragging multiple items such as attachments and messages in TB to desktop fails.

This is a placeholder bug waiting for front-end code to actually implement multi drag so as to confirm this bug.
Attachments do not have front end code in place for multiple drags but messages will if bug 227305 lands. That bug will be set for 'depends on' for now.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm going to use this bug to handle the nsDataObjCollection changes split off from Bug 338478.  There's a very rough implementation over there in the obsolete patches.  I'll have a refactored version up here in a few days.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch Draft 1

Implements nsDataObjCollection in a way that allows more than one nsITransferable to be transferred at the same time.  This still needs some work, notably it needs

1) The code in question in Bug 528731 to make drag images
2) To make sure that a given set of nsITransferables can be dragged and dropped from one Gecko window to the other and recreated. (i.e. a drop happening between Gecko windows should act exactly the same as a drop inside one Gecko window at the js level.  This may or may not already be the case, I haven't tested).
3) Tests

Roc, we can test bug 338478 and the ability to drop multiple files here with an explorer window, but that really doesn't catch everything else.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Been busy with exams and getting my computer fixed and stuff.  Hope to have a patch w/tests for review sometime next week.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Implements drag and drop of multiple nsITransferables between Gecko windows and the Win32 Shell/Other Apps.  Should be fairly straightforward.  There is a lot of string handling involved to make sure that we put the formats togheter properly.  Most of the other formats (besides the text ones and CF_HDROP) don't support multiple objects so we call GetFirstSupporting which passes the call to the first supporting nsDataObj child.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 419086 [details] [diff] [review]
Patch

This is bitrotted.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Unbitrotted patch

Roc, if you're not going to have time to review this let me know and I'll ask jmathies or one of the other peers for review.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 423151 [details] [diff] [review]
Unbitrotted patch

Sorry Kyle :-(.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #9)
> (From update of attachment 423151 [details] [diff] [review])
> Sorry Kyle :-(.

Heh that's fine.

Comment 11

[Jim Mathies [:jimm]]

yay, more drag and drop updates and cleanup. I'll apply this and review tomorrow afternoon, got a couple oopp related patches to finish up first.

Comment 12

[Jim Mathies [:jimm]]

+  case CF_BITMAP:
+  case CF_DIB:
+  case CF_METAFILEPICT:
+    return GetFirstSupporting(pFE, pSTM);
+  default:
+    if (pFE->cfFormat == fileDescriptorFlavorA ||
+        pFE->cfFormat == fileDescriptorFlavorW)
+      return GetFileDescriptors(pFE, pSTM);
+    if (pFE->cfFormat == fileFlavor)
+      return GetFileContents(pFE, pSTM);
   }
-
-	return ResultFromScode(DATA_E_FORMATETC);
+  return GetFirstSupporting(pFE, pSTM);

Can we simplify this by falling through for the formats that need do go to GetFirstSupporting? (maybe add a comment explaining what GetFirstSupporting does.) Lets bracket those if statements as well. 

+void nsDataObjCollection::SetTransferable(nsITransferable * aTransferable)

Do we need this anymore?

// These two lists are the mapping to and from data flavors and FEs
// Later, OLE will tell us it's needs a certain type of FORMATETC
// (text, unicode, etc) so we will look up data flavor that
// corresponds to the FE and then ask the transferable for that type of data
// Don't add the same FORMATETC repeatedly though, also, we don't need
// the data flavor (that gets handled by the nsDataObj)

nit, clean up this comment. (Also, this references a transferable, but we don't hold one anymore here?)

If we don't need info about transferables, we can snip the header include.

//-----------------------------------------------------
// Registers a the DataFlavor/FE pair
//-----------------------------------------------------

Not your issue but lets nix this awful commenting and just go with simple "// ..." comments on these methods.

+    realbuffer = (PRUnichar*)((char*)GlobalLock(hGlobalMemory) + buffersize);
+    realbuffer--; // Overwrite the preceding null
+    memcpy(realbuffer, filename.get(), alloclen);

We should check for a failure here with GlobalLock just to be safe. Looks like you're checking it in some cases and ignoring it in others.

-		virtual HRESULT SetMetafilePict(FORMATETC&  FE, STGMEDIUM&  STM);
+    virtual HRESULT GetFile(LPFORMATETC pFE, LPSTGMEDIUM pSTM);

Might as well clean up the rest of this header unless you're planning on doing that in another bug.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I hadn't intended to clean up the whole file in this bug, but you're right, lets go all in and get it done here.  Patch later today.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

It's so much nicer to look at without all that cruft

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Helps if I qrefresh first.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/a7037263dd9b

Comment 17

[Jacek Caban]

Attachment: mingw fix

This patch broke mingw compilation. It causes link failure due to duplicated IID_IDataObjectCollection in object files. It revealed a problem that was already existing in nsIDataObjectCollection.h. DEFINE_GUID is used to declare IID and initguid.h is always included. This results in exporting IID_nsIDataObjectColletion from all files using this header.

We could improve initguid.h trick, but it's usually a bad idea to use it from header files. It results in defining all DEFINE_GUID to generate an instance of IID instead of declaration, even in public header files. It's an unwanted result, because all files including it will have to be aware of that. In my proposed patch I get rid of DEFINE_GUID macro.

Comment 18

[Philip Chee]

Quick question. I'm working on a front end SeaMonkey bug to allow multiple DnD of mail messages (Bug 537448). I can DnD multiple files into Total Commander (an alternative file manager for WinXP) but not into a native windows explorer window, nor on to the Windows desktop - single files work obviously. Anybody know why?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #18)
> Quick question. I'm working on a front end SeaMonkey bug to allow multiple DnD
> of mail messages (Bug 537448). I can DnD multiple files into Total Commander
> (an alternative file manager for WinXP) but not into a native windows explorer
> window, nor on to the Windows desktop - single files work obviously. Anybody
> know why?

Could be a lot of different reasons, though looking at your patch in that bug I don't see anything obvious.  Let's move this conversation over there.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/5cc86c09dbd3