Fix handling of notification disappearing under cursor #28816
Conversation
| setWindowOpacity(_a_opacity.value(_hiding ? 0. : 1.) * _manager->demoMasterOpacity()); | ||
| const auto opacity = _a_opacity.value(_hiding ? 0. : 1.) * _manager->demoMasterOpacity(); | ||
| if (opacity == 0. && underMouse()) { | ||
| // The notification is leaving from under the cursor, but in such case leave hook is not |
There was a problem hiding this comment.
This doesn't sound correct. The Qt documentation:
A leave event is sent to the widget when the mouse cursor leaves the widget.
There was a problem hiding this comment.
I suppose, in Qt's terminology the widget becoming invisible is not "mouse cursor leaves the widget"?..
There was a problem hiding this comment.
Not sure. I guess it might be platform-dependent if it's not mentioned in documentation.
There was a problem hiding this comment.
In that case, our code should not rely on that, should it?
There was a problem hiding this comment.
If I got your idea right, just adding manager()->startAllHiding(); before manager()->removeWidget(this); in opacityAnimationCallback should be enough
There was a problem hiding this comment.
I feel like it's even more logical to start hiding the rest of the notifications when this one is already destroyed (from the manager's perspective)
There was a problem hiding this comment.
yeah, personally I'm fine with either ordering
There was a problem hiding this comment.
although, thinking more about it, the ordering you proposed would lead to use after free, as you would access this->_manager after this is destroyed
There was a problem hiding this comment.
Omg, does removeWidget free the notification from memory? It is then very surprising that it is called from inside a notification. Is it even defined behavior if an object is freed/deleted from within it's member method, even if it's the last statement?
There was a problem hiding this comment.
Yeah, look at Manager::removeWidget, all it does is finds the notification in the vector, erases it and calls showNextFromQueue()
| // The notification is leaving from under the cursor, but in such case leave hook is not | ||
| // triggered automatically. But we still want the manager to start hiding notifications | ||
| // (see #28813). | ||
| manager()->startAllHiding(); |
There was a problem hiding this comment.
As far as I can see from the code, this function gets called from hide animation. As the animation is already playing, it means the hiding has already started but you call it again. This doesn't look correct.
There was a problem hiding this comment.
Agreed. I will try to look into this again in a few days.
|
I believe the right fix here is finding why leave event is not being emitted for you and fixing that. Otherwise there likely to be way bigger issues as the code relies on this event in lots of places. |
kolayne
force-pushed
the
notification-fade-out-under-cursor
branch
from
c771a4b to
83e0403
Compare
Previously, notifications disappearing under cursor (e.g., because closed manually or open from another device) did not notify the manager properly, as the leaveEventHook was not triggered. This could lead to notifications staying around when not supposed to (see telegramdesktop#28813). This commit fixes that by explicitly notifying manager when the notification widget disappears under the cursor. Fixes telegramdesktop#28813.
kolayne
force-pushed
the
notification-fade-out-under-cursor
branch
from
83e0403 to
89a8c6f
Compare
|
@ilya-fedin, I fixed the previously discussed issues. Additionally, I added a comment at the call to Note, that I only call |
Previously, notifications disappearing under cursor (e.g., because closed manually or open from another device) did not notify the manager properly, as the leaveEventHook was not triggered. This could lead to notifications staying around when not supposed to (see #28813).
This commit fixes that by explicitly notifying manager when the notification widget disappears under the cursor.
Fixes #28813.