wails icon indicating copy to clipboard operation
wails copied to clipboard

Potential solution for getting the calling window of a bound method.

Open leaanthony opened this issue 1 year ago • 5 comments

Description

** This PR has been opened up to start a discussion on what the right solution is **

There will be times when you want to know what window made a call to a bound method. This PR offers 2 solutions for 2 different scenarios:

  1. When a bound method uses a standard context.Context in the first parameter, the context will have a value "window" which will return the calling window.
  2. For scenarios where you want to call these methods from Go, you can simply use an application.Window as the first parameter. It will always be populated with the calling window if called from JS.

The binding example has been updated to show both usages.

leaanthony avatar Apr 25 '24 07:04 leaanthony

Deploying wails with  Cloudflare Pages  Cloudflare Pages

Latest commit: a788b3a
Status:⚡️  Build in progress...

View logs

I was having a look at the code again and I noticed that checks for the window parameter happen in processCallMethod from messageprocessor_call.go.

Do you have a reason for that? I'm asking because @stffabi added very similar code in bindings.go for context parameters:

  • https://github.com/wailsapp/wails/blob/v3-alpha/v3/pkg/application/bindings.go#L281
  • https://github.com/wailsapp/wails/blob/v3-alpha/v3/pkg/application/bindings.go#L328

and I was thinking this change probably belongs there too?

Also please note that my PR #3431 is going to break this in its current form, because CallOptions.Args is changing from a slice of any to a slice of json.RawMessage.

Sorry for the late review, didn't notice this before.

fbbdev avatar Apr 28 '24 07:04 fbbdev

Also, I would ignore a window parameter in second place unless the first place is a context. I would do something along these lines:

In getMethods

if (!boundMethod.needsContext && inputIndex == 0) || (boundMethod.needsContext && inputIndex == 1) {
	if  input.AssignableTo(windowType) {
		boundMethod.needsWindow = true
	}
}

In BoundMethod.Call:

if b.needsWindow {
	args = append([]any{wnd}, args...)
}
if b.needsContext {
...

fbbdev avatar Apr 28 '24 07:04 fbbdev

I was having a look at the code again and I noticed that checks for the window parameter happen in processCallMethod from messageprocessor_call.go.

Do you have a reason for that? I'm asking because @stffabi added very similar code in bindings.go for context parameters:

Yeah, the bindings doesn't know about windows.

Also, I would ignore a window parameter in second place unless the first place is a context.

Would you pass nil instead?

leaanthony avatar Apr 28 '24 10:04 leaanthony

Yeah, the bindings doesn't know about windows.

That's true, but do you plan on using the binding system for anything else? If not, you could just pass the (possibly nil) window as an additional argument to BoundMethod.Call and handle things in there.

Anyways if you prefer the current approach that's ok, I can rework #3431 once this gets merged.

Also, I would ignore a window parameter in second place unless the first place is a context.

Would you pass nil instead?

I would just ignore it and let the binding system throw an error. We can also add a warning in the binding generator.

Let me explain better what I have in mind.

With the current implementation the following method:

func (s *Service) Method(num int, wnd application.Window, str string)

would receive a window as its second argument.

I think this is a bit contrived and should be avoided.

I'd go with something similar to the following check:

windowIndex := 0
if boundMethod.needsContext {
	windowIndex = 1
}
if windowIndex < len(boundMethod.Inputs) && boundMethod.Inputs[windowIndex].ReflectType == reflect.TypeFor[Window]() {
	options.Args = append([]any{window}, options.Args...)
}

Any other situation should trigger a runtime error.

fbbdev avatar Apr 28 '24 19:04 fbbdev

git fun...

leaanthony avatar May 05 '24 08:05 leaanthony

Moved to https://github.com/wailsapp/wails/pull/3457

leaanthony avatar May 05 '24 08:05 leaanthony