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

macOS: external drag and drop support #111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

StarHack
Copy link

@StarHack StarHack commented Feb 25, 2023

Adds some very basic DnD support for external files on macOS.

  • I don't understand event routing yet, so the implementation is probably incorrect.
  • Support for Windows and Linux missing
  • It probably should be possible to add this for specific widgets and not for the whole window.

Usage

go.mod

module main

go 1.19

replace gioui.org => github.com/StarHack/gio v0.0.0-20230421195057-eb440387490a

require gioui.org v0.0.0-20230414223051-b6e0376ad2fe

require (
	gioui.org/cpu v0.0.0-20210817075930-8d6a761490d2 // indirect
	gioui.org/shader v1.0.6 // indirect
	github.com/benoitkugler/textlayout v0.3.0 // indirect
	github.com/go-text/typesetting v0.0.0-20221214153724-0399769901d5 // indirect
	golang.org/x/exp v0.0.0-20221012211006-4de253d81b95 // indirect
	golang.org/x/exp/shiny v0.0.0-20220827204233-334a2380cb91 // indirect
	golang.org/x/image v0.0.0-20220722155232-062f8c9fd539 // indirect
	golang.org/x/sys v0.0.0-20220825204002-c680a09ffe64 // indirect
	golang.org/x/text v0.7.0 // indirect
)

main.go

package main

import (
	"fmt"
	"image"
	_ "image/gif"
	"io"
	"os"

	"gioui.org/app"
	"gioui.org/io/system"
	"gioui.org/io/transfer"
	"gioui.org/layout"
	"gioui.org/op"
	"gioui.org/op/paint"
	"gioui.org/widget"
)

func loadImage(file io.ReadCloser) (widget.Image, error) {
	img := widget.Image{}
	decImg, _, err := image.Decode(file)
	if err != nil {
		return img, err
	}
	img.Src = paint.NewImageOp(decImg)
	return img, nil
}

func main() {
	go func() {
		w := app.NewWindow()
		var ops op.Ops

		f, _ := os.Open("img/placeholder.png")
		img, _ := loadImage(f)

		tag := new(int)

		for {
			e := <-w.Events()

			switch e := e.(type) {
			case system.DestroyEvent:
				return
			case system.FrameEvent:
				gtx := layout.NewContext(&ops, e)

				transfer.TargetOp{
					Tag:  tag,
					Type: "image/png",
				}.Add(&ops)

				for _, gtxEvent := range gtx.Events(tag) {
					switch e := gtxEvent.(type) {
					case transfer.DataEvent:
						file, err := e.Open()
						newImg, err := loadImage(file)
						if err == nil {
							img = newImg
						} else {
							fmt.Println(err)
						}
					}
				}

				img.Layout(gtx)
				e.Frame(gtx.Ops)
			}
		}
	}()
	app.Main()
}

Demo

rec.mov

Copy link
Contributor

@eliasnaur eliasnaur left a 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.

app/os_macos.m Outdated Show resolved Hide resolved
app/os_macos.m Outdated Show resolved Hide resolved
app/os_macos.go Outdated Show resolved Hide resolved
@StarHack
Copy link
Author

StarHack commented Apr 8, 2023

@eliasnaur Updated implementation to use io/transfer package instead and took into account the other change requests to simplify the implementation. Also updated the usage example above.

@StarHack StarHack requested a review from eliasnaur April 8, 2023 12:28
Copy link
Contributor

@eliasnaur eliasnaur left a 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).

app/os_macos.m Outdated Show resolved Hide resolved
io/router/router.go Outdated Show resolved Hide resolved
app/os_macos.go Outdated Show resolved Hide resolved
@StarHack StarHack force-pushed the main branch 6 times, most recently from b80e27f to cce63c5 Compare April 15, 2023 18:23
@StarHack
Copy link
Author

@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.

@fjl
Copy link
Contributor

fjl commented Apr 16, 2023

@StarHack thanks for working on this! I really need it in my own app.

Copy link
Contributor

@eliasnaur eliasnaur left a 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.

app/os_macos.go Outdated Show resolved Hide resolved
io/router/router.go Outdated Show resolved Hide resolved
app/os_macos.go Show resolved Hide resolved
@StarHack
Copy link
Author

@eliasnaur @fjl Many thanks for your feedback! I appreciate it. I changed the implementation to return the result of os.Open() instead and we now only send the event to the topmost element of the hitlist.

io/router/router.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fjl fjl left a 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,
Copy link
Contributor

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....

Copy link
Author

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.

Copy link
Contributor

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...

Copy link
Sponsor Contributor

@inkeliz inkeliz Apr 24, 2023

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.

@whereswaldon
Copy link
Member

Just a thought:

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.

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.

@eliasnaur
Copy link
Contributor

@fjl adding all that information seems a bit much to promise. Questions:

  • Can a list of dropped files be implemented as separate DataEvents?
  • Are you ok with just an URL field with a file: reference for local files?

@StarHack
Copy link
Author

@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.

@fjl
Copy link
Contributor

fjl commented Apr 25, 2023

I proposed fs.FileInfo mostly because it is the usual way of representing 'information about a file' in Go. If you feel it's too much, of course I'd also be fine with just a name/URL. If the URL is a complete path, the app can always resolve all necessary information itself. There are two situations where it will not work: (1) with the WASM/browser backend,
(2) when the app is running in a sandbox. But we can worry about those cases later.

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.

@fjl
Copy link
Contributor

fjl commented Apr 25, 2023

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 URL field on transfer.DataEvent and worry about the other problems later!

Copy link
Contributor

@eliasnaur eliasnaur left a 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)
Copy link
Contributor

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.

@ddkwork
Copy link

ddkwork commented Apr 14, 2024

A similar structure is also needed on Windows, we may drag and drop a folder instead of just a file, or a batch of drag-and-drop, or more likely a file and a directory to drag and drop together, and a drag-and-drop link is possible, so the original structure needs to be changed or a new event channel and data structure is opened for the drag-and-drop event on the desktop platform

@ddkwork
Copy link

ddkwork commented Apr 14, 2024

I had this review comment, but it wasn't submitted because of my reply to @fjl . Sorry.

I implemented gio's window header dark theme and drag-and-drop support by copying the wails code, but don't know how to add it to the event correctly, can you make an example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants