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

Prevent disappearance of mouse when SpinBox is hidden while dragging #77804

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Jun 3, 2023

Fixes #77803. Fixes #76729.

@ajreckof ajreckof requested a review from a team as a code owner June 3, 2023 12:50
@Sauermann
Copy link
Contributor

Have you thought about using _release_mouse(); instead of drag.allowed = false;?
NOTIFICATION_EXIT_TREE uses that one to reset the mouse cursor.

@ajreckof
Copy link
Member Author

ajreckof commented Jun 3, 2023

So to give a bit more in depth. I do use _release_mouse as I fallthrough to the NOTIFICATION_EXIT_TREE case but this is only releasing when drag is enabled. Unfortunatly because of how SpinBox is implemented this is not enough. This is because of drag.allowed which is different from drag.enabled the second indicates mouse has been captured and _release_mouse will handle it but the first indicates that on InputEventMouseMotion the SpinBox can capture the mouse and do the drag.
calling _release_mouse on hiding fix part of the issue in case drag as already started but in case it hasn't started and drag.enabled is still false but drag.allowed is already true it might still appear this is in fact exactly what happens in #76729.
Another way instead of setting drag.allowed there is to call it inside _release_mouse but I wasn't sure it wouldn't create other bug.

@YeldhamDev YeldhamDev added this to the 4.1 milestone Jun 3, 2023
@korypostma
Copy link
Contributor

I was looking into this issue as well on another issue and I added a few more notification types for when a user alt-tabs or if the app loses focus while dragging and I forcibly call _release_mouse as well. I think those could be added here for more robustness. We can discuss on rocketchat and you can make it part of this PR, since it is the same region of code for the same issue.

@korypostma
Copy link
Contributor

NOTIFICATION_FOCUS_EXIT, NOTIFICATION_WM_WINDOW_FOCUS_OUT, and NOTIFICATION_APPLICATION_FOCUS_OUT

@korypostma
Copy link
Contributor

I just tested this PR and it is breaking SpinBox functionality now. The mouse cursor is always shown and only the up/down arrows work, this PR essentially breaks the dragging feature altogether.

@fire fire changed the title Prevent disapearance of mouse when SpinBox is hidden while doing a drag modification. Prevent disappearance of mouse when SpinBox is hidden while doing a drag modification. Jun 4, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 14, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jul 31, 2023

I just tested this PR and it is breaking SpinBox functionality now. The mouse cursor is always shown and only the up/down arrows work, this PR essentially breaks the dragging feature altogether.

I can't reproduce this. The PR works correctly.

@ajreckof
Copy link
Member Author

So looked a bit and effectively I can't reproduce the problem with the mrp but I think what they pointed out is how even though this is a half fix for #76729 it is not a full fix as their is a strange behaviour since it is being hidden and shown again every time the value is changed. A proper fix of this other bug would be to make sure it does hide and show it again every time it is updated. This is a problem of a long loop of update that had unforeseen consequences

@YuriSizov YuriSizov self-requested a review August 1, 2023 14:16
@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 29, 2023

Another way instead of setting drag.allowed there is to call it inside _release_mouse but I wasn't sure it wouldn't create other bug.

I think you should do that, yes. The only other place that calls _release_mouse() (aside from NOTIFICATION_EXIT_TREE, but that one is irrelevant) also sets the flag to off, so you could clean all of that up and make it a mandatory step to drag.allowed = false;.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

So this does solve the immediate problem, though you can still make your mouse disappear if you do something like hide, await 1-2 seconds, show as the drag happens.

I think this is bordering on shooting yourself in a foot at this point, though. After a rebase and a quick code clean up this should be good to go.

And this does fix #76729 btw, the spinbox is lagging like hell due to the described behavior issue, but the mouse pointer remains visible.

@YuriSizov YuriSizov changed the title Prevent disappearance of mouse when SpinBox is hidden while doing a drag modification. Prevent disappearance of mouse when SpinBox is hidden while dragging Sep 29, 2023
@akien-mga akien-mga merged commit 7469b43 into godotengine:master Sep 29, 2023
@akien-mga
Copy link
Member

Thanks!

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.

Mouse disappear when SpinBox is being hidden while there is a Drag AnimatedSprite2d: SpriteFrames Editor
7 participants