-
Notifications
You must be signed in to change notification settings - Fork 133
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
macOS: external drag and drop support #111
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this. Some comments inline, the most important being the use of the io/transfer
package which is intended to cover drag-and-drop.
Also, please remove unrelated and debug commits from the PR.
@eliasnaur Updated implementation to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, thanks. Next issue is that the data events should arrive at the transfer target.
Also, please squash your commits into logically independent units (or just one commit with everything).
b80e27f
to
cce63c5
Compare
@eliasnaur Many thanks for your feedback. I have adjusted the implementation which now allows to register a TargetOp with the desired mime type and the event is now routed to the appropriate tag. Furthermore, the sample implementation above has been updated and all commits have been squashed into one. |
@StarHack thanks for working on this! I really need it in my own app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. More comments below. I'm sorry for the bait-and-switch; you're hit by the fact that you're the first to implement platform support for drag-n-drop. Before your PR we only had internal dnd.
@eliasnaur @fjl Many thanks for your feedback! I appreciate it. I changed the implementation to return the result of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have verified the browser logic for handling external file DnD, and have to say, I was wrong about text/url-list
.
Here is a JSFiddle where you can try out DnD: https://jsfiddle.net/6yezxnqp/1/
In browsers, file DnD has special handling. The drag event has a property ev.dataTransfer.types
, which is an array of object types. These can be MIME types, but don't have to be. When files are dragged, the HTML spec requires a type "Files"
to be included in this array.
For each dropped file, the browser does perform MIME type detection, so each transfer item will have a MIME type, and it is based on the file extension. I confirmed this by dropping a PDF file with the extension removed. The ev.dataTransfer.items[0].type
property was ""
in that case.
So, overall there is lots of inspiration to be found in the way browsers handle it. For obvious security-related reasons, the browser does not provide access to the full file path (anymore). However, the basename of the file is there.
I think that gio should provide the full path, or at the very least the file basename, alongside the open function. One must also consider the possibility of dropping multiple files, directories, symlinks etc. Maybe a good way to handle all this would be changing DataEvent
like this:
package transfer
import "fs"
import "io"
type DataEvent struct{
Type string
Open io.ReadCloser
Files []File
}
type File interface{
fs.FileInfo
Open() (io.ReadCloser, error)
}
i.e. we add special handling for files. This might be required anyway in order to handle dragging files out from a Gio application into an external location.
mime := mime.TypeByExtension(fileExtension) | ||
|
||
w.w.Event(transfer.DataEvent{ | ||
Type: mime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a new field in the event that includes the filename/URL please?
In my app, I would like to use the filename and do not require access to the file content when the DnD event happens. This kind of logic is impossible when the event only contains the Open
function, and it's especially weird since the filename is there, the framework just isn't providing it....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation triggers an event for every file dropped so providing multiple Files
at once probably would be inconsistent as the Open
function still only provides access to one file. But I agree that including the (file) URL would make sense, or at least the filename, as Gio users might need them depending on their implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted another API idea specifically adding files in the main review comment. Let's see what @eliasnaur has to say about it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think nothing prevents you from getting the filepath:
evt := transfer.DataEvent{} // Received on main-loop
file, _ := evt.Open()
var filepath string
if f, ok := file.(*os.File); ok {
filepath = f.Name()
}
That works because Open()
returns os.File
in the current implementation. However, that is not guaranteed on other OSes, such as WASM.
Just a thought:
Gio does run in the browser when compiled to WASM, so whatever we do needs to be implementable within the browser. I suppose we could degrade gracefully from full OS paths to whatever the browser offers though, so long as we document that you may get a full OS file path. |
@fjl adding all that information seems a bit much to promise. Questions:
|
@eliasnaur Do you have further feedback on the existing implementation? I'd like to finalize this pull request at one point or another and add support for Linux and Windows as soon as I've time to do so. |
I proposed Delivering multiple files as individual events is not great, but this could just be my personal opinion. I have no experience to back that up. The act of dropping a set of files is 'one event', logically speaking. In some contexts, dropping multiple files will make sense, and requires handling from the app. This can be dealt with by accumulating events. In other cases, e.g. 'avatar image box' in a messenger, only a single file is acceptable. If multiple files are dropped, the app will process every file (in case of image: open, parse, crop etc.) and then use the last one. It can't do anything else because it has no knowledge about multiple drop events being in the queue. |
I don't want to be the guy holding up this extremely useful PR. If you feel it is the simplest, let's just go with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had this review comment, but it wasn't submitted because of my reply to @fjl . Sorry.
@@ -182,6 +182,8 @@ func (q *Router) Queue(events ...event.Event) bool { | |||
} | |||
case clipboard.Event: | |||
q.cqueue.Push(e, &q.handlers) | |||
case transfer.DataEvent: | |||
q.pointer.queue.notifyPotentialTargets(&pointerHandler{sourceMimes: []string{e.Type}}, &q.handlers, e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is quite right: as written, notifyPotentialTargets still gives every handler the event. It's made for transfer.InitiateEvent
that tells every potential handler a change to change appearance in anticipation of a drop.
2470368
to
6afd8b9
Compare
0d543a0
to
1686874
Compare
67c77c9
to
46cc311
Compare
f8029f2
to
026d3f9
Compare
3d36537
to
74ccc9c
Compare
Wish this would be resolved . Very useful feature for gioui… |
Adds some very basic DnD support for external files on macOS.
Usage
go.mod
main.go
Demo
rec.mov