Clean up #435

pchampio · 2020-05-10T13:08:17Z

No description provided.

provokateurin · 2020-05-10T17:56:29Z

@pchampio flutter/engine#18146 got merged flutter/engine@d96f962

embedder/embedder.go

GeertJohan · 2020-05-11T12:18:12Z

embedder/embedder.go

+}
+
+// GoError convert a FlutterEngineResult to a golang readable error
+func (res Result) GoError(caller string) error {


Instead of taking the caller name as argument to GoError(), we could use runtime information to obtain the caller name.
Example usage:

pc, _, _, ok := runtime.Caller(1) callerDetails := runtime.FuncForPC(pc) if ok && callerDetails != nil { fmt.Printf("called from %s\n", callerDetails.Name()) }

I'm not sure what scenario's we could encounter where ok is false. It will be false when too many frames are skipped, but for value 1 that can never be the case in Go.

Nevermind, I think this won't work when debug symbols are stripped, which hover tells Go to do when building a release build. (-ldflags="-s -w")

I really like this new (Result).GoError() by the way, a lot better to pass errors arround instead of Result. 👍 😎

pchampio · 2020-05-11T17:50:09Z

Tested on linux using: https://github.com/flutter-rs/engine-builds/releases/tag/f-d96f962ca21a104b08836f34814eb6b267937511 using profile mode.

…lutter into clean_up_and_aot

pchampio · 2020-05-13T08:38:42Z

TODO: add go-flutter/plugin tag and clean the plugin repo / update plugin docs.

GeertJohan · 2020-05-13T09:32:02Z

Can we keep versions for the plugin submodule and the root module the same? Afaik if you version tag without submodule prefix (such as plugin/v1.2.3), it automatically uses the root tag (v1.2.3).

pchampio · 2020-05-13T09:39:40Z

Yes, I intent to tag with: plugin/v1.2.3!
Similarly to what we are doing in the plugin repo: https://github.com/go-flutter-desktop/plugins/blob/master/image_picker/go.mod#L1 https://github.com/go-flutter-desktop/plugins/releases

And it should not use the 'root' (do you mean the go-flutter?) tag, when developing I had error like:

go: github.com/go-flutter-desktop/[email protected] requires
	github.com/go-flutter-desktop/go-flutter/[email protected]: invalid version: unknown revision 000000000000

It couldn't find any revision.

GeertJohan · 2020-05-18T08:01:06Z

I think that may be because there is no tag on the repository yet where the plugin folder contains a go.mod?

If it works to publish by using only the 'root' tag, I think that would be preferable. Easier to maintain and less prone to errors. Always the same version for both go-flutter and plugins.

pchampio · 2020-05-18T17:25:48Z

Oh!!
Ok, didn't understand your last comment. Sorry.

I'm not sure about connecting both modules into the same tag.
IMHO:

ATM the plugin repo can be tagged as >v1 while I like to keep go-flutter <1.
it's been more than 6month since the last update on the plugin module, If we connect both modules tags, it would mean that we would have updates on plugin that are empty (and a lot of them..).

pchampio · 2020-05-22T20:48:24Z

Ready for review!
I've commented the code that required compiling go-flutter against the master channel of flutter (AOT feature).
Once this PR is merged, I'll open a new PR un-commenting the AOT related sutff.

@GeertJohan with my above comment, especially the 2nd point?

GeertJohan · 2020-05-24T09:36:02Z

Thanks for the elaboration @pchampio, I now agree with you that the plugins package could have it's own version tags. Indeed it can be tagged 1.0

Will this always play well and not cause dependency cycles and version issues?

go-flutter requires go-flutter/plugin v1.x
path_provider requires go-flutter/plugin v1.x
path_provider requires go-flutter to perform a compile time type check var _ flutter.Plugin = &PathProviderPlugin{}. This is optional, but nice for plugin developers to do as it increases compile-time safety.

The third item does introduce the go-flutter package as dependency, we could avoid that by moving the Plugin interface into the plugin package, and replacing the Plugin interface in the flutter package with a retype of the plugin.Plugin interface (package flutter; import "github.com/go-flutter-desktop/go-flutter/plugin"; type Plugin plugin.Plugin).

One other thing we should consider at this point:
Should the plugin package be moved to its own repo? I don't have a clear opinion on this yet, but I was just thinking that IF we ever want to move the plugin package to its own repo, then right now would be the perfect time (pre 1.0, with go-flutter being marked unstable (0.x) the breaking change would still be ok).
It does break existing imports of the package, but maybe creating the module will break that anyway?
What are the downsides of having a module in a module?

GeertJohan · 2020-05-24T09:38:17Z

plugin/standard-message-codec.go

@@ -66,8 +66,6 @@ const (
 //
 type StandardMessageCodec struct{}

-var _ MessageCodec = StandardMessageCodec{} // compile-time type check


Isn't this a useful assertion?

textinput.go

…lutter into clean_up_and_aot

pchampio · 2020-05-25T10:33:41Z

platform.go

+	channel.HandleFuncSync("Clipboard.setData", p.handleClipboardSetData)
+	channel.HandleFuncSync("Clipboard.getData", p.handleClipboardGetData)
+	channel.HandleFuncSync("SystemNavigator.pop", p.handleSystemNavigatorPop)


uses of HandleFuncSync instead of a glfwTasker

pchampio · 2020-05-25T10:44:24Z

Will this always play well and not cause dependency cycles and version issues?

True, a change on go-flutter/plugin might require the use of an newer go-flutter/.
Ok, I now get your point.

After more though, I now think we should NOT add a module in go-flutter/plugins.
It's best if both are connected for the dependency (go-flutter/plugin -> go-flutter) requirement, and the best way to perform this is to having the go-flutter/plugin in the same tag as go-flutter, so, the same go.mod.

pchampio · 2020-05-25T11:21:31Z

messenger.go

@@ -125,8 +126,7 @@ func (r responseSender) Send(binaryReply []byte) {
 	// TODO: detect multiple responses on the same message and spam the log
 	// about it.

-	// It would be preferable to replace this with channels so sending
-	// doesn't have to wait on the main loop to come around.
+	glfw.PostEmptyEvent()


Before:

flutter: 00:00 +0: Test Battery Plugin result Send took 25.112162ms flutter: -- Battery level at 55 % . flutter: 00:00 +1: Complex plugin Test StandardMethodCodec array of map Send took 25.079956ms flutter: 00:00 +2: Complex plugin Test golang InvokeMethodWithReply Send took 25.104811ms flutter: 00:03 +3: Complex plugin Custom errors go-flutter: handler for method 'getError' on channel 'instance.id/go/data' returned an error: Some error Send took 25.076353ms flutter: 00:03 +4: Complex plugin Test golang CatchAllHandleFunc Send took 25.06359ms flutter: 00:03 +5: (tearDownAll) flutter: 00:03 +6: All tests passed! Send took 25.213678ms go-flutter: closing application flutter: 00:03 +6: All tests passed!

After:

flutter: 00:00 +0: Test Battery Plugin result Send took 53.526µs flutter: -- Battery level at 55 % . flutter: 00:00 +1: Complex plugin Test StandardMethodCodec array of map Send took 77.784µs flutter: 00:00 +2: Complex plugin Test golang InvokeMethodWithReply Send took 72.655µs flutter: 00:03 +3: Complex plugin Custom errors go-flutter: handler for method 'getError' on channel 'instance.id/go/data' returned an error: Some error Send took 53.314µs flutter: 00:03 +4: Complex plugin Test golang CatchAllHandleFunc Send took 209.681µs flutter: 00:03 +5: (tearDownAll) Send took 82.624µs go-flutter: closing application flutter: 00:03 +6: All tests passed!

Wow that's a serious speed improvement! Nice 🚀

GeertJohan · 2020-05-25T11:38:08Z

platform.go

-	p.glfwTasker.Do(func() {
-		p.window.SetClipboardString(newClipboard.Text)
-	})
+	p.window.SetClipboardString(newClipboard.Text)


So, just make sure I get this right; this code is guaranteed to run in the glfw main thread, which is why we don't need to add this as task on the glfwTasker?

Yes, HandleFuncSync runs on the main thread.

pchampio added 2 commits May 8, 2020 13:06

WIP refactor & aot support

8817416

fix test and 'error' everything

1f929e3

james-lawrence reviewed May 11, 2020

View reviewed changes

embedder/embedder.go Outdated Show resolved Hide resolved

GeertJohan reviewed May 11, 2020

View reviewed changes

use helper to set CGO union type

c52da7d

pchampio marked this pull request as ready for review May 11, 2020 17:47

Merge branch 'master' into clean_up_and_aot

a7e7eb2

pchampio added 2 commits May 13, 2020 10:36

feat: add plugin gomod isolate plugin

882dc4d

Merge branch 'clean_up_and_aot' of github.com:go-flutter-desktop/go-f…

b59629f

…lutter into clean_up_and_aot

pchampio mentioned this pull request May 19, 2020

Implement flutter/mousecursor #445

Merged

chore: rm code that require Flutter master channel

2b0cccb

pchampio requested a review from GeertJohan May 22, 2020 20:46

Merge branch 'master' into clean_up_and_aot

c0b243e

pchampio changed the title ~~Clean up and aot~~ Clean up May 22, 2020

GeertJohan reviewed May 24, 2020

View reviewed changes

textinput.go Show resolved Hide resolved

pchampio added 3 commits May 25, 2020 12:23

cleanup

9cfd36c

fix HandleFuncSync

97913ba

Merge branch 'clean_up_and_aot' of github.com:go-flutter-desktop/go-f…

ec0b000

…lutter into clean_up_and_aot

pchampio commented May 25, 2020

View reviewed changes

pchampio added 3 commits May 25, 2020 12:44

chore: adress review comments

4478475

fix go.mod

e64bb60

PostEmptyEvent faster platform message sent

4c54ef7

pchampio commented May 25, 2020

View reviewed changes

pchampio requested a review from GeertJohan May 25, 2020 11:32

GeertJohan reviewed May 25, 2020

View reviewed changes

GeertJohan approved these changes May 25, 2020

View reviewed changes

pchampio merged commit 85b3586 into master May 25, 2020

pchampio deleted the clean_up_and_aot branch May 25, 2020 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up #435

Clean up #435

pchampio commented May 10, 2020

provokateurin commented May 10, 2020

GeertJohan May 11, 2020

GeertJohan May 11, 2020

GeertJohan May 11, 2020

pchampio commented May 11, 2020

pchampio commented May 13, 2020 •

edited

Loading

GeertJohan commented May 13, 2020

pchampio commented May 13, 2020 •

edited

Loading

GeertJohan commented May 18, 2020

pchampio commented May 18, 2020 •

edited

Loading

pchampio commented May 22, 2020 •

edited

Loading

GeertJohan commented May 24, 2020

GeertJohan May 24, 2020

pchampio May 25, 2020

pchampio commented May 25, 2020

pchampio May 25, 2020

GeertJohan May 25, 2020

GeertJohan May 25, 2020

pchampio May 25, 2020

Clean up #435

Clean up #435

Conversation

pchampio commented May 10, 2020

provokateurin commented May 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchampio commented May 11, 2020

pchampio commented May 13, 2020 • edited Loading

GeertJohan commented May 13, 2020

pchampio commented May 13, 2020 • edited Loading

GeertJohan commented May 18, 2020

pchampio commented May 18, 2020 • edited Loading

pchampio commented May 22, 2020 • edited Loading

GeertJohan commented May 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchampio commented May 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchampio commented May 13, 2020 •

edited

Loading

pchampio commented May 13, 2020 •

edited

Loading

pchampio commented May 18, 2020 •

edited

Loading

pchampio commented May 22, 2020 •

edited

Loading