Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drag&Drop support #1043

Merged
merged 5 commits into from
Nov 26, 2023
Merged

Drag&Drop support #1043

merged 5 commits into from
Nov 26, 2023

Conversation

Yanrishatum
Copy link
Contributor

Resolve #1040

This is a draft implementation of Drag&Drop support with unified API. Needs review and feedback on what can be changed and/or improved. @ncannasse @jefvel

Right now:

  • The base API for Drag&Drop events.
  • JS implementation for said API (without Node)

API is async by default because JS, but I want to provide synchronous API for sys target (and possibly node).
I didn't event test it yet, as it's just a draft for API, as main implementation details are less important at this point (especially since I'll need to provide implementation of the SDL/DX as a separate PR to HL).

@Yanrishatum
Copy link
Contributor Author

After some more digging turns out SDL does not support ongoing drag&drop operation. I'll keep the ongoing D&D support for potentially future support of it, but right now it'll work only with JS. And for DX getting ongoing drag&drop is also a lot of awkward C++ code that interfaces with winapi so I'm unlikely to implement it and just use WM_DROPFILES (same as SDL).

@ncannasse
Copy link
Member

ncannasse commented Feb 6, 2022

A few comments :

  • I think we should only deal with file/bytes drops : no String file data (we can convert it in the background) and no mime type. That would mean removing "kind" / "type" / "mime".
  • in order to keep the api cross platform I would remove the "sync" methods.
  • file name should be read-only in the interface
  • "unknown" files should be entirely ignored (what are these anyway ?)
  • (convention) SupportsOngoingEvent should be SUPPORTS_ONGOING_EVENTS (static are uppercased)
  • I think we can remove the DropMove operation, since we have EMove already. DropStart/End should be named DropEnter/Leave

@Yanrishatum
Copy link
Contributor Author

I think we should only deal with file/bytes drops : no String file data (we can convert it in the background) and no mime type. That would mean removing "kind" / "type" / "mime".

Sure, I based it off JS drag&drop support (but frankly SDL and DX also have a distinction between files and text) and decided to do exhaustive API first. As I mentioned - it's more of a draft that needs feedback (especially since at least SDL support for it have to be merged in HL first anyway).

in order to keep the api cross platform I would remove the "sync" methods.

This one I don't really agree on, since in most cases when I process a drag&drop I would prefer to obtain data immediately instead of introducing extra-continuation method (and awkward context transfer) or lambdas. But since file name is actually the path to the file on sys, it can be removed safely so user can just grab the path and use sys.io.File directly.

file name should be read-only in the interface
SupportsOngoingEvent should be SUPPORTS_ONGOING_EVENTS

Will do.

"unknown" files should be entirely ignored (what are these anyway ?)

API was written prior more deeper dig into what data types can be drag/dropped across APIs, hence I went safe route of unknowns. If we treat everything as "file", it can be outright removed anyway.

I think we can remove the DropMove operation, since we have EMove already. DropStart/End should be named DropEnter/Leave

In JS, DropMove is a separate event and I didn't verify that mouse events are fired properly during the action, on top of that in same JS - it already reports on what data is being dropped. (But that's not the case with SDL, and when DX is implemented - depending on approach it may or may not report it)

Added hldx/hlsdl support
Removed ongoing events (only drop action handled)
Removed string/file distinction (now only files are supported)
Removed synchronous API
Added dropX/dropY fields to drop event
Change drop event from interface to abstract
@Yanrishatum
Copy link
Contributor Author

  • Added hldx/hlsdl support from SDL: Add drag&drop support HaxeFoundation/hashlink#512 and [hldx] Implement Drag&Drop api HaxeFoundation/hashlink#630
  • Simplified API
    • Event kind is removed as ongoing event is only possible on JS (because I'm not implementing the "not just file drop event" winapi version for dx). Now it's just a list of files and drop position.
    • Removed synchronous methods, as that is easily achieved by using sys.io.File.getBytes(ev.file.file).
    • For JS DroppedFile exposes .native field that reference the data transfer File instance in case user needs it.
    • Removed distinction between text/file. Only some unix systems have support for text D&D on native based on SDL source code and it adds extra complexity to JS side. Now only file drops are processed.
    • Made fields read-only.
    • DroppedFile became an abstract class instead of interface for simplicity.

Tested all 3 backends, and everything works properly (JS was a bit finicky with preventDefault() being mandatory on both dragover and drop events in order to prevent browser from opening the file in the tab instead of dropping). In general implementation got simpler without too many bells and whistles, as they are mostly accessible only on JS, and user can just bind drop event listeners manually if they really want to do ongoing drop event capture.

This PR is ready for review/merge. @ncannasse

@ncannasse
Copy link
Member

Please change DroppedFile.new to be crossplatform, so only the native field is js specific.

@Yanrishatum
Copy link
Contributor Author

Done.

@Yanrishatum
Copy link
Contributor Author

A friendly reminder that this PR is still open and ready. :)

@ncannasse ncannasse merged commit 6ccc6ad into HeapsIO:master Nov 26, 2023
@ncannasse
Copy link
Member

Merged, ty !

@Yanrishatum Yanrishatum deleted the feature_drag_drop branch November 27, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for dropping files into window
2 participants