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

Add Some WindowState #1349

Merged
merged 19 commits into from
Aug 29, 2022
Merged

Add Some WindowState #1349

merged 19 commits into from
Aug 29, 2022

Conversation

zandercodes
Copy link
Contributor

@zandercodes zandercodes commented Apr 24, 2022

Add Some WindowStates, States not tested.
Add WindowStates for

  • Is Fullscreen
  • Is Maximised
  • Is Minimised
  • Is Normal

Issue: #1348

  • Implement window state as a native event
  • Testing on Windows
  • Testing on macOS
  • Fixing Merge conflicts
  • Testing on Linux
  • Documetation

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this! There's a few questions I had and a couple of suggestions. Am scheduling this in for v2.1

v2/internal/frontend/desktop/darwin/WailsContext.h Outdated Show resolved Hide resolved
v2/internal/frontend/desktop/linux/window.go Outdated Show resolved Hide resolved
v2/internal/frontend/desktop/windows/win32/window.go Outdated Show resolved Hide resolved
v2/pkg/runtime/window.go Show resolved Hide resolved
@zandercodes zandercodes changed the title Add Some WindowState [WIP] Add Some WindowState Apr 25, 2022
@zandercodes zandercodes marked this pull request as draft April 26, 2022 13:41
@leaanthony
Copy link
Member

Great stuff! Thanks for all this hard work. I'm wondering if, at the API level, we should have WindowGetState() that returns a WindowState type (int). It would cut the number of functions needed down quite significantly. We could use the IsWindow* methods behind the scenes to determine this. Thoughts?

@zandercodes
Copy link
Contributor Author

That would be optimal for Windows. But I don't know how it is on macOS or Linux. I think it might be a possibility to offer this as a separate event as a callback when the state of the window changes.
I'll see what I can do :D

@leaanthony
Copy link
Member

I think it should work on all platforms. Great idea on the events, just use the standard EventEmit call with "wails:window:maximised" etc. That's the convention 👍👍👍

@leaanthony
Copy link
Member

leaanthony commented Apr 28, 2022

Not sure why there's an emoji there. 🤣:wails:window:eventname 😂

@zandercodes
Copy link
Contributor Author

zandercodes commented May 2, 2022

@leaanthony I thought again and looked at Electron to understand how they map this case.
Electron itself also uses the native window state functions.
In the javascript, the resize event is accessed and the state is retrieved from the native functions.
So, the way I have this built in, I have done everything right and could be released that way. The only thing left to do, test it on macOS and Linux. Windows worked in my testing.

@@ -766,6 +772,15 @@ func (w *Window) IsMaximised() bool {
return result > 0
}

func (w *Window) IsMinimised() bool {
result := C.IsMinimised(w.asGTKWidget())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if this needs to be run on the main thread...

Copy link
Contributor Author

@zandercodes zandercodes Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested this yet. I am currently testing this in an Ubuntu VM. Windows and macOS works fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @zandercodes 👋 Just following up on the Linux testing. I can help with this if you need? It would be good to get this in!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone through and resolved the conflict. This should be good but let me know if there are issues 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @leaanthony, at the moment I don't have time to test this on Linux. But I will do it this week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I might be able to. Will update here

@zandercodes
Copy link
Contributor Author

Small status update. Merge conflicts are complicated to resolve.
I will delete a the merge commits and edit my last commit so that the conflicts can be fixed.
Sorry, since I'm doing this free, I can't always take care of it.

…llscreen

Solve conflicts

# Conflicts:
#	v2/internal/frontend/desktop/darwin/WailsContext.m
#	v2/internal/frontend/desktop/darwin/frontend.go
#	v2/internal/frontend/desktop/linux/window.go
#	v2/internal/frontend/desktop/windows/window.go
…llscreen

Solve conflict
# Conflicts:
#	v2/pkg/runtime/window.go
# Conflicts:
#	v2/internal/frontend/desktop/darwin/WailsContext.h
#	v2/internal/frontend/desktop/windows/win32/consts.go
#	v2/internal/frontend/desktop/windows/win32/window.go
# Conflicts:
#	v2/internal/frontend/devserver/devserver.go
# Conflicts:
#	v2/internal/frontend/dispatcher/systemcalls.go
#	v2/internal/frontend/runtime/desktop/window.js
#	v2/internal/frontend/runtime/package-lock.json
#	v2/internal/frontend/runtime/runtime_prod_desktop.js
#	v2/internal/frontend/runtime/wrapper/runtime.d.ts
#	v2/internal/frontend/runtime/wrapper/runtime.js
# Conflicts:
#	v2/cmd/wails/internal/commands/initialise/templates/generate/assets/common/frontend/wailsjs/runtime/runtime.js
#	v2/internal/frontend/runtime/package-lock.json
#	v2/internal/frontend/runtime/runtime_dev_desktop.js
Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any show stoppers so far 👍 Just the merge conflict characters that are checked in 👍

@@ -154,10 +193,35 @@ func getWindowLong(hwnd uintptr, index int) int32 {
return int32(ret)
}

<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this snuck in :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail 😂

@leaanthony
Copy link
Member

I can test on Linux 👍

@zandercodes
Copy link
Contributor Author

I can test on Linux 👍

Yes, you can try, but i don´t know if it will work.

@leaanthony
Copy link
Member

Tested on Linux. Worked fine apart from when the window is fullscreen, IsFullscreen() and IsMaximised() are true. Pushed a quick fix for that.

I used this for testing - thought it might be useful:

// Greet returns a greeting for the given name
func (a *App) Greet(name string) string {
	println("WindowIsFullscreen:", runtime.WindowIsFullscreen(a.ctx))
	println("WindowIsMaximised:", runtime.WindowIsMaximised(a.ctx))
	println("WindowIsMinimised:", runtime.WindowIsMinimised(a.ctx))
	println("WindowIsNormal:", runtime.WindowIsNormal(a.ctx))
	println("\n\n")
	return fmt.Sprintf("Hello %s, It's show time!", name)
}

and

window.WindowState = function() {
    WindowIsNormal().then((result) => {console.log("IsNormal: " + result);});
    WindowIsMinimised().then((result) => {console.log("IsMinimised: " + result);});
    WindowIsMaximised().then((result) => {console.log("IsMaximised: " + result);});
    WindowIsFullscreen().then((result) => {console.log("IsFullscreen: " + result);});
}

window.testGo = function() {
    WindowMinimise();
    setTimeout(() => {
        document.getElementsByTagName("button")[0].click();
        WindowUnminimise();
    }, 3000);
}

window.testJS = function() {
    WindowMinimise();
    setTimeout(() => {
        WindowState();
        WindowUnminimise();
    }, 3000);
}

@zandercodes
Copy link
Contributor Author

zandercodes commented Aug 3, 2022

Tested on Linux. Worked fine apart from when the window is fullscreen, IsFullscreen() and IsMaximised() are true. Pushed a quick fix for that.

I used this for testing - thought it might be useful:

// Greet returns a greeting for the given name
func (a *App) Greet(name string) string {
	println("WindowIsFullscreen:", runtime.WindowIsFullscreen(a.ctx))
	println("WindowIsMaximised:", runtime.WindowIsMaximised(a.ctx))
	println("WindowIsMinimised:", runtime.WindowIsMinimised(a.ctx))
	println("WindowIsNormal:", runtime.WindowIsNormal(a.ctx))
	println("\n\n")
	return fmt.Sprintf("Hello %s, It's show time!", name)
}

and

window.WindowState = function() {
    WindowIsNormal().then((result) => {console.log("IsNormal: " + result);});
    WindowIsMinimised().then((result) => {console.log("IsMinimised: " + result);});
    WindowIsMaximised().then((result) => {console.log("IsMaximised: " + result);});
    WindowIsFullscreen().then((result) => {console.log("IsFullscreen: " + result);});
}

window.testGo = function() {
    WindowMinimise();
    setTimeout(() => {
        document.getElementsByTagName("button")[0].click();
        WindowUnminimise();
    }, 3000);
}

window.testJS = function() {
    WindowMinimise();
    setTimeout(() => {
        WindowState();
        WindowUnminimise();
    }, 3000);
}

Thanks for testing and fixing :D

I'll finish the documentation for the website and then we can merge it.

@leaanthony
Copy link
Member

leaanthony commented Aug 3, 2022

Do you want to just double check with those test methods to be sure?

@zandercodes
Copy link
Contributor Author

Do you want to just double check with those test methods to be sure?

Good idea. I will test it again on windows and macos. So that we are sure that they really work.

@zandercodes
Copy link
Contributor Author

Do you want to just double check with those test methods to be sure?

Good idea. I will test it again on windows and macos. So that we are sure that they really work.

@leaanthony Works well on Windows and Mac

@leaanthony
Copy link
Member

Nice. Just let us know when the docs are ready and mark it ready for review 👍

# Conflicts:
#	v2/internal/frontend/runtime/package-lock.json
#	v2/internal/frontend/runtime/runtime_prod_desktop.js
#	v2/internal/messagedispatcher/dispatchclient.go
@zandercodes zandercodes marked this pull request as ready for review August 22, 2022 07:28
@zandercodes zandercodes changed the title [WIP] Add Some WindowState Add Some WindowState Aug 22, 2022
@leaanthony
Copy link
Member

If @stffabi has no objections with this, I think we can merge in pre v2.

@leaanthony leaanthony enabled auto-merge (squash) August 23, 2022 10:55
auto-merge was automatically disabled August 29, 2022 13:45

Head branch was pushed to by a user without write access

@leaanthony
Copy link
Member

Thanks for all your work on this! 🙏I did a quick update to the docs. Let's merge this and see how we go :-)

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

Successfully merging this pull request may close these issues.

2 participants