TYMED_ISTREAM should be used for CFSTR_FILECONTENTS (PR #25776)
Stefan Ziegler
When we drop files to a HTML webpage in Chrome that has a dropzone which handles "drop" events, then files are dropped twice in chrome and Edge. Firefox behaves differently, there only one file is dropped. Debugging showed that Chrome internally calls QueryGetData twice, first with tymed=TYMED_HGLOBAL and second with tymed=TYMED_HGLOBAL|TYMED_ISTREAM|TYMED_ISTORAGE. When we allow both, then two files (the same) are available in event.dataTransfer.files. When we only allow one, then only one file is available.
MS does not recommend TYMED_HGLOBAL for virtual files. So, an easy solution to this is to check for TYMED_ISTREAM in QueryGetData. I have tested with several other drop targets like Windows Explorer, Outlook, Firefox, Edge, Chrome, ... and the behavior is OK.
In my opinion, this is a Chrome issue. However, since wxWidgets should support virtual files in the future, and TYMED_HGLOBAL is less efficient for larger files, this fix and the limitation to TYMED_ISTREAM are acceptable.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25776
Commit Summary
- 3563a91 TYMED_ISTREAM should be used for CFSTR_FILECONTENTS
File Changes
(1 file)
- M src/msw/ole/dataobj.cpp (11)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25776.patch
- https://github.com/wxWidgets/wxWidgets/pull/25776.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25776)I'm worried by what happens if someone only calls QueryGetData() once with TYMED_HGLOBAL — if we refuse to return the data in this case, it would break drag and drop in this case.
So it looks like it would be safer (if less efficient) to only return data once, for the first to call, if 2 calls are made in quick succession. But I'm not sure if we can define what "quick" really means...
Also, you say that Chrome calls QueryGetData() twice, but I assume it also calls GetData() twice too, right? If so, it definitely looks like a bug, I wonder if it could be worth reporting it to Chrome, especially if you have a simple reproducer.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Ziegler
sz5000 left a comment (wxWidgets/wxWidgets#25776)I wouldn't want to restrict CFSTR_FILECONTENTS to TYMED_ISTREAM either. Small files can be transferred via global memory. This would be problematic for larger files. I don't think the idea of a "quick" query is right.
Unfortunately, due to the abstraction in wxWidgets, you can't access the actual OLE calls and override them to react based on FORMATETC and STGMEDIUM. But fortunately, you can adapt the wxWidgets code yourself to find a solution to the problem.
I'll see if I can find the corresponding code in Chrome to report an issue there.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25776)I wouldn't want to restrict CFSTR_FILECONTENTS to TYMED_ISTREAM either.
Sorry, now I'm really confused. Isn't this exactly what this PR does? AFAICS it forces the use of TYMED_ISTREAM for this format.
Unfortunately, due to the abstraction in wxWidgets, you can't access the actual OLE calls and override them to react based on FORMATETC and STGMEDIUM.
I can't think of a good way to allow customizing the medium type used for this. But if you can come up with something, I'd be glad to add it, instead of hardcoding any relationships between the format and the medium.
But fortunately, you can adapt the wxWidgets code yourself to find a solution to the problem.
Yes, open source is great like this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25776)I really don't know what to do about this one. I understand that the problem is real but I'm just not confident enough that this change is not going to break something else when fixing it.
If you can provide some way to make the behaviour optional, i.e. to customize the medium used, it would be ideal. If not, please explain why this does not break the case of transferring data via memory.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #25776.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25776)Sorry, I'm not going to apply this for now as I'm not confident enough about the effects. If you can add some way to configure the behaviour here, it would be welcome. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.