multiple drags outside moz app to shell or external program fails

RESOLVED FIXED in mozilla1.9.3a2

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: philbaseless-firefox, Assigned: khuey)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9.3a2
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Blocks: 270292
(Reporter)

Updated

10 years ago
Duplicate of this bug: 524020
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.
Status: NEW → ASSIGNED
Component: Drag and Drop → Widget: Win32
QA Contact: drag-drop → win32
Assignee: nobody → me
Posted patch Patch Draft 1 (obsolete) — Splinter Review
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.
Been busy with exams and getting my computer fixed and stuff.  Hope to have a patch w/tests for review sometime next week.
(Reporter)

Updated

10 years ago
Duplicate of this bug: 533415
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #419086 - Flags: review?(roc)
Comment on attachment 419086 [details] [diff] [review]
Patch

This is bitrotted.
Attachment #419086 - Attachment is obsolete: true
Attachment #419086 - Flags: review?(roc)
Posted patch Unbitrotted patch (obsolete) — Splinter Review
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.
Attachment #423151 - Flags: review?(roc)
Comment on attachment 423151 [details] [diff] [review]
Unbitrotted patch

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

Heh that's fine.
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.
+  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.
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.
Posted patch Patch (obsolete) — Splinter Review
It's so much nicer to look at without all that cruft
Attachment #423151 - Attachment is obsolete: true
Attachment #424400 - Flags: review?(jmathies)
Attachment #423151 - Flags: review?(jmathies)
Posted patch PatchSplinter Review
Helps if I qrefresh first.
Attachment #424400 - Attachment is obsolete: true
Attachment #424401 - Flags: review?(jmathies)
Attachment #424400 - Flags: review?(jmathies)

Updated

9 years ago
Attachment #424401 - Flags: review?(jmathies) → review+
http://hg.mozilla.org/mozilla-central/rev/a7037263dd9b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2

Comment 17

9 years ago
Posted patch mingw fixSplinter Review
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.

Updated

9 years ago
Attachment #428184 - Flags: review?(jmathies)

Comment 18

9 years ago
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?
(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.

Updated

9 years ago
Attachment #428184 - Flags: review?(jmathies) → review+

Updated

9 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.