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

Updated windows window/frontend to fix issue with html select positioning #1082

Merged
merged 6 commits into from
Jan 26, 2022

Conversation

MikeSchaap
Copy link
Contributor

@MikeSchaap MikeSchaap commented Jan 16, 2022

Fixes #1081

Need to wait for the following PR (leaanthony/go-webview2#5) to merge so go-webview2 exposes the NotifyParentWindowPositionChanged() method.

Was not sure how to correctly expose the chromium object to the window object. Do you agree with the proposed fix? Doesn't feel really solid imo

Note: First time creating a public Pull Request, so if I can improve certain parts, please let me know :)

@leaanthony
Copy link
Member

I would suggest having the notify function in the Window struct rather than options, as that's user options. In frontend Run, you can set the notify callback directly on the window struct after setting up chromium

@MikeSchaap
Copy link
Contributor Author

I moved the notifyParentWindowPositionChanged method to the Window struct and set the value in the Run method of Frontend. I was getting a nil pointer on the WndProc because it was called before the notifyParentWindowPositionChanged method was set. I moved the winc.RegMsgHandler(w) method call from the NewWindow method to the Window.Run method to make sure notifyParentWindowPositionChanged method was set before it was being used. Do you have any idea if this might interfere with anything else? From my (short) testing, everything seemed to still work.

@MikeSchaap
Copy link
Contributor Author

Also, I guess we still have to wait for a new tag on the go-webview2 repo before we can update the version in go.mod right?

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 think this is almost ready to go 👍

v2/internal/frontend/desktop/windows/window.go Outdated Show resolved Hide resolved
v2/internal/frontend/desktop/windows/window.go Outdated Show resolved Hide resolved
@leaanthony
Copy link
Member

Also need to resolve conflicts

Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
@MikeSchaap
Copy link
Contributor Author

Oh no, i'm pretty sure i messed up... I shouldn't work on sunday i guess.. Trying to figure out how to 'restore' this PR

@MikeSchaap
Copy link
Contributor Author

Looks like I managed to revert the messup. Also moved the winc.RegMsgHandler(result) back to it's original position and resolved the conflicts. I believe we should be good to go now

@leaanthony
Copy link
Member

leaanthony commented Jan 24, 2022

Also, I guess we still have to wait for a new tag on the go-webview2 repo before we can update the version in go.mod right?

Nah, just update the dependency to use latest commit. I usually specify this like go get .....@ab2c3b2
Do you need to update go.mod + go.sum in this PR?

@stffabi
Copy link
Collaborator

stffabi commented Jan 26, 2022

Also, I guess we still have to wait for a new tag on the go-webview2 repo before we can update the version in go.mod right?

Nah, just update the dependency to use latest commit. I usually specify this like go get .....@ab2c3b2 Do you need to update go.mod + go.sum in this PR?

PR #1105 already bumped the go-webview2 dependency and that PR has been merged. Therefore we could merge this PR without needing to bump it.
Should we squash the commits before merging?

@leaanthony
Copy link
Member

LGTM!

@leaanthony leaanthony merged commit 965187a into wailsapp:master Jan 26, 2022
@MikeSchaap
Copy link
Contributor Author

Cheers 🍾

@leaanthony
Copy link
Member

Congrats on your first PR!

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

Successfully merging this pull request may close these issues.

[v2] Select Dropdown doesn't update position when moving window
3 participants