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

Fix macOS fullscreen incorrect mouse position #51718

Closed
wants to merge 2 commits into from
Closed

Fix macOS fullscreen incorrect mouse position #51718

wants to merge 2 commits into from

Conversation

danielkariv
Copy link
Contributor

In relation to #40061.

This PR is fixing the incorrect mouse position in fullscreen mode which result UI click registered below expect point.
From my testing, it seems like the issue is that when the window getting into or out fullscreen mode, it wouldn't call the resize event to get correct sizing and scale.
I also notice that when i fullscreen and changing resolution of the display, it will call resize event, and the UI will work as it should. but when getting out from fullscreen mode, it will get back to the same issue, now in window mode.

So my PR is adding a call to resize event when changing to/from fullscreen mode.

In relation to #40061. It seems that the issue is when the window get fullscreen, it doesn't change the window size like when it is maximized or scale up and down.
So the change i made was forcing the resize event function, when getting in to or out from fullscreen mode.
@danielkariv danielkariv requested a review from a team as a code owner August 16, 2021 07:54
@akien-mga akien-mga added this to the 4.0 milestone Aug 16, 2021
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 16, 2021
@@ -190,6 +190,8 @@ - (void)windowDidEnterFullScreen:(NSNotification *)notification {

[wd.window_object setContentMinSize:NSMakeSize(0, 0)];
[wd.window_object setContentMaxSize:NSMakeSize(FLT_MAX, FLT_MAX)];
//Force window resize event
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but we usually style our comments like this:

Suggested change
//Force window resize event
// Force window resize event.

Changes the comments to fit with godot's styling.
@danielkariv
Copy link
Contributor Author

danielkariv commented Aug 16, 2021

Changed the comments to fit with the style you requested.

@mhilbrunner
Copy link
Member

Thanks for the fix! Can you squash your comment fixup commit into the first one?

Then this should be good to go.

@danielkariv
Copy link
Contributor Author

I am sorry but I am new to using git and I think can't do that anymore. I thought the PR got approved so i had remove the fork/branch from account and now I can't figure out how to restore/get the pr branch back. If it isn't possible to do from your end, I could try to redo it with a new PR.

@mhilbrunner
Copy link
Member

I am sorry but I am new to using git and I think can't do that anymore. I thought the PR got approved so i had remove the fork/branch from account and now I can't figure out how to restore/get the pr branch back. If it isn't possible to do from your end, I could try to redo it with a new PR.

So, before doing anything, make sure you make a backup of your changes. :)

  1. Make sure you're in the right branch (the one for this PR)
  2. Git rebase, and select "squash" for the second commit (the one with the comments)
  3. Finish rebasing, commit, force push

If you need a more detailed tutorial, searching for the above should help.
But yes, if its easier for you you can also open a new PR and reference this one and then close this one - but really, thats not necessary if you follow the above. And if you need any help, let us know on chat.godotengine.org

@danielkariv danielkariv closed this Sep 3, 2021
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants