Need code review for file_picker plungin and test on darwin / linux before merge to pub.dev

[chunhunghan]

I have tested on Windows 10. It works fine.
But I only have Windows, can't test on linux and darwin
Could you please help me.
Thanks.

plugin
https://github.com/chunhunghan/file_picker

android
https://github.com/chunhunghan/file_picker_example

[pchampio]

From what I can tell, you are trying to port miguelpruivo/plugins_flutter_file_picker to go-flutter, it's awesome!

As @miguelpruivo said:

It wouldn't be a bad idea to have it centralized in one place

Can you please submit a PR to https://github.com/miguelpruivo/plugins_flutter_file_picker with the content of https://github.com/chunhunghan/file_picker in a directory desktop go-desktop.

The linux plugin works fine. (Just remove the unnecessary print):

filePicker fileExtension:*.png *.jpg *.jpeg
filePicker fileExtension:*.*
[....]

[pchampio]

Also the https://github.com/flutter/plugins/tree/master/packages/file_picker url in the README is wrong.

[chunhunghan]

@Drakirus Thank you for your review
unnecessary print has removed also README.md url and import has updated.

@GeertJohan could you help to check darwin? Thank you very much.

[ghost]

@chunhunghan great work!

[pchampio]

@chunhunghan GeertJohan is in vacation, @zephylac, can you give this a try?
Thank!

[zephylac]

I was doing it. Having this issue RN

hover run
Using engine from cache
go: finding github.com/go-flutter-desktop/go-flutter master
go: finding github.com/go-gl/glfw latest
# github.com/chunhunghan/file_picker/file_picker
../../../file_picker/plugin.go:17:9: channel.CatchAllHandleFunc undefined (type *plugin.MethodChannel has no field or method CatchAllHandleFunc)

[pchampio]

@zephylac you are on an older version of go-flutter and hover, update hover.
AND, Make sure you have not set a replace github.com/go-flutter-desktop/go-flutter => [...] in desktop/go.mod 😉

[zephylac]

I'm using thisexample

In your plugin package fmt is imported but no used.

The finder doesn't open except when I choose 'any' extension.
When it opens ('any' extension), I can browse normally but I cannot choose a file, they are all greyed out.

All greyed out excpet directory

ERROR when trying multiple files options

[chunhunghan]

I have folk miguelpruivo 's project version v1.3.8 to https://github.com/chunhunghan/plugins_flutter_file_picker
and put desktop folder in it,
also presume it's future version will be v1.3.9

I have checked with Windows 10. It works fine.

The reason of error is Flutter Text Widget received a nil from file_picker.go.

I don't know correct filter string of darwin, so I just copy it from image_picker file_darwin.go
image_picker

var filters string
	switch fileType {
	case "image":
		filters = `"PNG", "public.png", "JPEG", "jpg", "public.jpeg"`
	case "video":
		filters = `"MOV"`
	default:

file_picker

switch method {
	case "ANY":
		filter = `"*"`
	case "IMAGE":
		filter = `"PNG", "public.png", "JPEG", "jpg", "public.jpeg"`
	case "AUDIO":
		filter = `"MP3", "public.mp3"`
	case "VIDEO":
		filter = `"MOV"`

I am so sorry.

[miguelpruivo]

@chunhunghan by providing desktop support, as well as some other working fixes, the version will most likely jump to 1.4.0.

Anyway, take your time and feel free to issue a PR to the file_picker beta branch when you’ve it ready to work with every desktop platform.

Thank you.

[chunhunghan]

I have ask question in apple.stackexchange
https://apple.stackexchange.com/questions/365288/applescript-how-to-choose-file-for-all-files-or-list-of-extensions

Maybe for ANY, I should pass an empty string to disable "of type"

[zephylac]

I found the Apple developper page.

By following the rules I ended up with :

switch method {
	case "ANY":
		filter = `public.item`
	case "IMAGE":
		filter = `public.image`
	case "AUDIO":
		filter = `public.audio`
	case "VIDEO":
		filter = `public.movie`
	default:
		if strings.HasPrefix(method, "__CUSTOM_") {
			resolveType := strings.Split(method, "__CUSTOM_")
			filter = `"` + resolveType[1] + `"`			
		} else {
			return "", errors.New("unknown method")
		}
	}

This solution works on MacOS High Sierra 10.13 . We should test it on Mojave but I don't see any reasons why it wouldn't work.

I tested multiples files option, it works and I couldn't reproduce the error I shown in my previous comment.

[chunhunghan]

@zephylac Thank you very much.
I have updated the code.
https://github.com/chunhunghan/plugins_flutter_file_picker/tree/master/desktop

Do I need to uppercase file extension in CUSTOM? Thanks

[zephylac]

__CUSTOM_ is sent by file_picker flutter plugin
You need to keep this. This is used to know which custom extension is used.

You just need to replace

filter = `"` + resolveType[1] + `"`

by

filter = resolveType[1]

The double-quotes are messing up the filter.

Also I noticed when the dialog is open, it isn't focused.
If I try to press escape directly without clicking on the dialog I end up with:
go-flutter: failed to pop route after escape key press: failed to decode envelope: unexpected end of JSON input.

There should be a way to make it focus on display to avoid this issue.

[chunhunghan]

filter had replaced. Thank you very much.

#141 ?
I do not have enough knowledge about "failed to pop route after escape key press". Thanks

[zephylac]

The main problem here is that the dialog windows is not focused when started.
Meaning that the windows currently focused is the main application.
The example that I'm using is example.
The error message is normal. I have no route to pop.

That's why we need to find a way to focus the dialog window when it opens to avoid this error.

[pchampio]

@chunhunghan, as @stuartmorgan said in #209, we should consider adding a 'go-' before the directory desktop.
For the plugin, if you submit a PR on miguelpruivo/plugins_flutter_file_picker, do it in a go-desktop directory.

[miguelpruivo]

I’d suggest to call the directory only desktop for the file_picker.

[pchampio]

go-flutter plugins are not compatible with FDE plugins and flutter-rs plugins.

opinion: desktop is too generic, we must specialize the name.

[stuartmorgan]

not compatible with FDE plugins

To clarify slightly: the plugin API is part of what moved to the Flutter project, so it's not really "FDE plugins" anymore. They would just be Flutter plugins, with support for one or more of Flutter's desktop platforms.

[chunhunghan]

The main problem here is that the dialog windows is not focused when started.
Meaning that the windows currently focused is the main application.
The example that I'm using is example.
The error message is normal. I have no route to pop.

That's why we need to find a way to focus the dialog window when it opens to avoid this error.

I found a applescript command

set frontmost to true

might useful here https://apple.stackexchange.com/questions/34810/create-a-command-to-focus-a-specific-window-from-anywhere-in-os

osascript -e 'tell application "System Events" to tell process "iTerm2"' \
	   -e 'set frontmost to true' \
	   -e 'if windows is not {} then perform action "AXRaise" of item 1 of windows' \
	   -e 'end tell'

but I do not have Mac and it's impossible for me to test it.

[zephylac]

Either you follow dlgs logic and use osascript or you can try to re-implement the whole package in glfw. Which would reduce the dependancy graph.

For osascript here is an example :

osa, err := exec.LookPath("osascript")
		o, err := exec.Command(osa, "-e", `tell application "System Events" to tell process "iTerm2"`, "-e", `set frontmost to true`, "-e", `if windows is not {} then perform action "AXRaise" of item 1 of windows`, "-e", `end tell`).Output()
		if err != nil {
			if exitError, ok := err.(*exec.ExitError); ok {
				ws := exitError.Sys().(syscall.WaitStatus)
				// Throw error here
				fmt.Println("%v", ws.ExitStatus())
			}
		}

This solution kinda works, it set the file dialog window above all the others but doesn't focus it and when you are using gesture to switch between desktop/application. It set the file dialog above all the other even when you are another application (meaning that this solution is too intrusive).

[pchampio]

@zephylac

This solution kinda works, it set the file dialog window above all the others but doesn't focus it and when you are using gesture to switch between desktop/application. It set the file dialog above all the other even when you are another application (meaning that this solution is too intrusive).

Aren't all file picker like that?
On Linux, with my setup, the browser file picker behavior is similar to your description.

[zephylac]

It would seems like it but the go pkg which is dlgs doesn't let you focus the windows. The only issue with that is that you can't press ESC until you have clicked on the dialog. osascript is specific to Mac OS meaning we would have to find fixes for Linux & Windows. This solution is kinda makeshift repair. I don't like the way it's being handled.
I guess we have two choice, either we accept this minor issue (windows not focused) or we need to re-think this all the way

[pchampio]

@chunhunghan @zephylac, the plug-in does work, a beta version can be released.

If @miguelpruivo is still OK, @chunhunghan you can create a PR on https://github.com/miguelpruivo/plugins_flutter_file_picker under the beta branch. The content of https://github.com/chunhunghan/file_picker can be placed under any directory (I strongly suggest go-desktop) just make sure to update the Readme with the correct import url.

Conserning the notice about the issue tracker in the Readme, I suggest using the miguelpruivo/plugins_flutter_file_picker one instead of go-flutter.

[stuartmorgan]

FWIW, getting correct behavior on macOS using that approach is going to be impossible. It's launching a completely separate process to show the dialog box, which means that it can never have the correct (window-level or application-level) modality, and if it's not system-model you'll have issues with anything application-level (hiding the app, app switcher, etc.) That Go package might make sense in the context of an otherwise-headless Go script, but it's not at all the right way to implement showing a dialog box from within the context of an application.

[miguelpruivo]

Currently, what’s the advantage of Flutter GO vs the Flutter Desktop?

[pchampio]

@miguelpruivo I have written a comment in #191 about go-flutter and Flutter Desktop (previously FDE).

If you don't have a clear idea of what go-flutter brings, you might want to reconsider adding go-flutter support to your plug-in 😉.

[miguelpruivo]

@Drakirus thank you for clarifying. I got the concept from the discussion here and README file, however, I was looking for a more detailed explanation about the main reasons that took you to work on this, and #191 explained it quite well. 👍

However, I must admit that I'm going to give it a try before merging it, but definitely I don't see a reason why it shouldn't as it also adds more value for GO-Flutter users. 🙂

[chunhunghan]

Thank you all you are amazing.