Fix crash on hiding grandparent Control on mouse exit #85313
Conversation
akien-mga
added
the
cherrypick:4.2
There was a problem hiding this comment.
I have tested and can confirm, that the PR fixes the MRP in #85277 (comment) and #85277 (comment).
| @@ -2507,6 +2513,8 @@ void Viewport::_gui_update_mouse_over() { | |||
| return; | |||
| } | |||
|
|
|||
| gui.sending_mouse_enter_exit_notifications = true; | |||
There was a problem hiding this comment.
In the implementation you keep track in every Viewport separately, if sending_mouse_enter_exit_notifications is active or not.
Have you considered the idea to keep track of the state in a global manner (for example only in the root-Viewport (even when the entry-point is not the root-Viewport))? I'm not entirely sure, if there can be edge cases, where these notifications in different Viewports could interfere with each other.
However, for now it looks like a reasonable approach.
kitbdev
force-pushed
the
fix-exit-hide
branch
from
3313b11 to
5ef12a8
Compare
kitbdev
force-pushed
the
fix-exit-hide
branch
from
5ef12a8 to
78e1702
Compare
| if (gui.sending_mouse_enter_exit_notifications) { | ||
| // If notifications are already being sent, defer call. | ||
| callable_mp(this, &Viewport::_drop_mouse_over).call_deferred(p_until_control); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not familiar with this code, but slightly worried at the possibility of a deferred infinite recursion here. Are we absolutely sure that this won't end up deferring itself over and over again without ever reaching a condition that would make gui.sending_mouse_enter_exit_notifications false?
There was a problem hiding this comment.
Yes, since every time gui.sending_mouse_enter_exit_notifications is set to true, it is immediately set to false in the same scope after it is done sending a fixed amount of notifications. So it will never be true by the time deferred calls are made.
I'm not sure if there is a way to tell if this call is being called from the deferred queue, but I could add a check just in case something changes and it is set to true?
Also this method only removes Controls from a list and cannot add any, so it cannot keep calling itself infinitely from user code, like was previously possible in _gui_update_mouse_over.
|
Thanks! |
YuriSizov
removed
the
cherrypick:4.2
|
Cherry-picked for 4.2.2. |
NOTIFICATION_MOUSE_ENTERnotification #85277 (comment)fixes crash when changing top level or mouse filter to stop during a mouse notification (#85277 (comment)).
This works by deferring sending new notifications if we are already sending notifications.
Mouse enter and exit notifications are now handled when changing visibility, top level, and mouse filter during the notification callbacks. Previously it would sending duplicate or out of order notifications.
However, with this there is a way to make an infinite loop:On mouse_entered set the child to mouse filter stop, and on mouse_exited set the child to mouse filter pass. Then mouse over the child. The parent will continuously receive mouse enter and exit notifications.
Previously, it would just send mouse notifications in the wrong order and then stop. This also won't happen with hide() and show() since show() doesn't update the mouse over immediately.