Fix notifications when passed through with a cursor too quickly #28815

kolayne · 2025-01-01T09:38:11Z

In this PR, for Window::Notification::Default::internal::Widget I:

Rename .isShowing() to .isFadingIn() to clarify its behavior;
Fix and comment on the use of Ui::Animations::Simple::{start,change} in notifications hiding/unhiding animations.

Is blocked by desktop-app/lib_ui#254. Fixes #28811.

Kindly, see the commit messages for details.

kolayne · 2025-01-01T09:39:25Z

Telegram/lib_ui

I am not sure how I'm supposed to handle the lib_ui submodule in my commits (both of which require corresponding changes in lib_ui). Please, tell me if/how I should fix it.

kolayne · 2025-01-01T10:14:20Z

By the way, I believe that with these changes, in the Widget constructor there is no need to call setWindowOpacity(0.); anymore, as it will anyway start the animation at the end. Is that right? Should I remove it?

tdesktop/Telegram/SourceFiles/window/notifications_manager_default.cpp

Lines 472 to 497 in 4505a2b

    
           Widget::Widget( 
        
           	not_null<Manager*> manager, 
        
           	QPoint startPosition, 
        
           	int shift, 
        
           	Direction shiftDirection) 
        
           : _manager(manager) 
        
           , _startPosition(startPosition) 
        
           , _direction(shiftDirection) 
        
           , _shift(shift) 
        
           , _shiftAnimation([=](crl::time now) { 
        
           	return shiftAnimationCallback(now); 
        
           }) { 
        
           	setWindowOpacity(0.); 
        
           	setWindowFlags(Qt::WindowFlags(Qt::FramelessWindowHint) 
        
           		| Qt::WindowStaysOnTopHint 
        
           		| Qt::BypassWindowManagerHint 
        
           		| Qt::NoDropShadowWindowHint 
        
           		| Qt::Tool); 
        
           	setAttribute(Qt::WA_MacAlwaysShowToolWindow); 
        
           	setAttribute(Qt::WA_OpaquePaintEvent); 
        
           	Ui::Platform::InitOnTopPanel(this); 
        
           	_a_opacity.start([this] { opacityAnimationCallback(); }, 0., 1., st::notifyFastAnim); 
        
           }

ilya-fedin · 2025-01-01T19:27:00Z

You say in one of commit messages:

Previously, `Window::Notifications::Default::internal::Widget` was using
`Ui::Animations::Basic`'s `.animating()` method to tell if a
notification was in the showing state. That is incorrect: a notification
may be in the animating state whether it is fading out (i.e., not
showing) or fading in (i.e., to be shown).

But that's not true, here's the function in question:

	bool isShowing() const {
		return _a_opacity.animating() && !_hiding;
	}

It does check exactly !_hiding, it can't return true on hiding.

ilya-fedin · 2025-01-01T19:30:47Z

I also suspect the issue you mark here as being fixed couldn't be fixed by changes in this PR, as they likely stem from the same problem of broken QWidget::leaveEvent you try to work around in #28816?

ilya-fedin · 2025-01-01T19:38:24Z

Telegram/SourceFiles/window/notifications_manager_default.cpp

@@ -543,12 +543,17 @@ void Widget::hideStop() {
 	if (_hiding) {
 		_hiding = false;
 		_hidingDelayed = {};
-		_a_opacity.start([this] { opacityAnimationCallback(); }, 0., 1., st::notifyFastAnim);
+		// Using `.change` instead of `.start` because we want to start
+		// animation from the current opacity level, not blink it.


I see Ui::Animations::Simple::start already has some handling of unfinished animation?

Unfortunately, the lib_ui was not documented, so I can't be sure about that, but I assumed that:

Ui::Animations::Simple::start, when called on an ongoing animation, should reset it completely, set opacity to from, and go from there to to

Ui::Animations::Simple::change, when called on an ongoing animation, should not reset the from: it starts from where the previous animation got, and continues from there.

If my assumptions are correct, I think, you should agree that here we need .change() (i.e., restore the notification from whatever state it was in) rather than .start() (blink the notification by first setting its opacity to zero and then restoring it).

If my assumptions are wrong, I kindly ask you to explain the intended behavior of the .start() and .change() methods of an animation in lib_ui.

kolayne · 2025-01-01T19:55:32Z

It does check exactly !_hiding, it can't return true on hiding.

Yes, it can:

_hiding is set to true inside Widget::hideSlow, which is called by Notification::startHiding, which is called by the manager (Manager::startAllHiding) only if notification->isShowing() is false. So, the order is the other way around: first, the manager will check if the notification is showing (which involves checking the value of _hiding), and then conditionally call startHiding to change the value of _hiding. I feel like the member _hiding is intended to just avoid double hiding notifications.

Relevant code:
First, the manager checks the notification status:

void Manager::startAllHiding() {
    if (!hasReplyingNotification()) {
        int notHidingCount = 0;
        for (const auto &notification : _notifications) {
            if (notification->isShowing()) {
                ++notHidingCount;
            } else {
                notification->startHiding();
            }
        }
        // ...

and that involves the _hiding member:

	bool isShowing() const {
		return _a_opacity.animating() && !_hiding;
	}

then the manager conditionally calls notification->startHiding():

void Notification::startHiding() {
    if (!_history) return;
    hideSlow();
}

which calls Widget::hideSlow():

void Widget::hideSlow() {
    if (anim::Disabled()) {
        _hiding = true;
        base::call_delayed(
            st::notifySlowHide,
            this,
            [=, guard = _hidingDelayed.make_guard()] {
                if (guard && _hiding) { 
                    hideFast();
                }
            });
    } else {
        hideAnimated(st::notifySlowHide, anim::easeInCirc);
    }
}

and that's where _hiding is set, either directly, or through hideAnimated:

void Widget::hideAnimated(float64 duration, const anim::transition &func) {
    _hiding = true;
    _a_opacity.start([this] { opacityAnimationCallback(); }, 1., 0., duration, func);
}

kolayne · 2025-01-01T20:02:54Z

I also suspect the issue you mark here as being fixed couldn't be fixed by changes in this PR, as they likely stem from the same problem of broken QWidget::leaveEvent you try to work around in #28816?

As I understand, you are saying that (1) this is the same issue as in #28816 and (2) this PR does not fix it?

This is definitely a different issue. By either adding debug output in the code or running a debug build of Telegram in gdb, one can make sure that event hooks are generated properly here. The problem here is the incorrect behavior of isShowing(), see the explanation above. Using the same methods, one can verify that in the scenario in Fix handling of notification disappearing under cursor #28816 the hook is not triggered, so these are two very different issues.
If that would help, let me know and I will design a demo to show clearly that the hook is triggered and the decision made by Manager::hideAllAnimating() is incorrect.
I can reproduce the issue every time with the official build. I couldn't ever reproduce it with the new build. This, and the explanation in the previous message, are the reasons I believe this PR fixes the problem.

ilya-fedin · 2025-01-01T20:26:35Z

_hiding is set to true inside Widget::hideSlow, which is called by Notification::startHiding, which is called by the manager (Manager::startAllHiding) only if notification->isShowing() is false. So, the order is the other way around: first, the manager will check if the notification is showing (which involves checking the value of _hiding), and then conditionally call startHiding to change the value of _hiding.

I'm not sure what's wrong here? The hiding animation gets started only after _hiding is set to true. I don't see a moment when it could hide without _hiding being set for isShowing() to be incorrect.

ilya-fedin · 2025-01-01T20:42:03Z

It's also weird that it's _a_opacity.fadingOut() && !_hiding after the change, i.e. "hiding and not hiding". Perhaps it's meant to be just _a_opacity.fadingIn()?

ilya-fedin · 2025-01-01T21:10:42Z

I believe what you try to fix here needs a method that would do (perhaps name it startHidingTimer?)

	if (!_waitingForInput) {
		_hideTimer.start(st::notifyWaitLongHide);
	}

and then call it when isShowing() returns true. With no other changes.

kolayne · 2025-01-02T09:09:49Z

It's also weird that it's _a_opacity.fadingOut() && !_hiding after the change, i.e. "hiding and not hiding".

Oh, you're right. I just realized I misunderstood the purpose of Notification::isShowing. I agree, _a_opacity.animating() && _hiding is the condition for fading in. My solution is incorrect.

But then, @ilya-fedin, could you help me understand the following piece of code:

void Manager::startAllHiding() {
    if (!hasReplyingNotification()) {
        int notHidingCount = 0;
        for (const auto &notification : _notifications) {
            if (notification->isShowing()) {
                ++notHidingCount;
            } else {
                notification->startHiding();
            }
        }
// ...

What is the purpose of the inner condition? Why does the manager refuse to hide a notification that is fading in? Seems like it is exactly what is causing the issue: if the cursor leaves the notification too quickly, leaveEventHook calls Manager::startAllHiding but because the fade in animation is not finished, the manager won't start hiding that notification, so it will never be gone, unless interacted with again.

I believe what you try to fix here needs a method that would do (perhaps name it startHidingTimer?)
	if (!_waitingForInput) {
		_hideTimer.start(st::notifyWaitLongHide);
	}
and then call it when isShowing() returns true. With no other changes.

But why should the behavior be different depending on whether the notification is fadingIn or not?

kolayne · 2025-01-02T09:11:10Z

Btw, I would argue that isShowing() is a confusing name for this. The way I understood it initially was "is displayed and is not fading out". If you agree (and we still need this method), I would rename Notification::isShowing() to Notification::isFadingIn()

ilya-fedin · 2025-01-02T09:35:43Z

What is the purpose of the inner condition?

I guess its purpose is exactly preventing hiding while it's still showing. Apparently to give the user more time to decide whether he wants to click on the notification. The current behavior of not hiding at all may just be intended then...

The way I understood it initially was "is displayed and is not fading out".

I understand it as "in the process of showing". For your explanation, I would expect the naming to be isShown. And, well, actually there's shown() in the RpWidget class it derives from.

ilya-fedin · 2025-01-02T09:46:19Z

I asked @john-preston whether it's a bug and whether it should be fixed with just removing the condition or starting _hideTimer, he confirmed it's a bug and either way is ok if I understood him right (actually he said "fix as you wish as long as it's logical")

…ently Previously, `Window::Notifications::Default::Manager` would not start hiding notifications that are fading in when other notifications should hide. This would lead to some notifications never hiding, e.g., when the cursor passes through the notification too quickly and there was not enough time for the notification to fade in completely. Also renamed `Widget::isShowing` -> `Widget::isFadingIn` for clarity. Fixes telegramdesktop#28811.

When a notification is to stop hiding (i.e., fade in), it is support to start fading in from whatever opacity it has at the moment, not from 0. Fixed that part to call the `.change()` method instead of `.start()` and added a clarification comment. When a notification is to start hiding (i.e., fade out), it is supposed to start fading out from the maximum opacity, even if it was not fully restored (which only happens if the cursor passed through the notification too quickly). Left the call to `.start()` and added a clarification comment.

kolayne · 2025-01-02T15:56:51Z

@ilya-fedin, I have fixed the discussed issues (and updated both desktop-app/lib_ui#254 and this PR). Please, see the new changes

kolayne · 2025-01-02T15:57:35Z

Telegram/SourceFiles/window/notifications_manager_default.h

-	bool isShowing() const {
+	bool isFadingIn() const {
 		return _a_opacity.animating() && !_hiding;
 	}


As far as I can see, this method is not used anywhere anymore. Should we remove it?

ilya-fedin · 2025-01-02T18:11:24Z

Do you really want to have animation APIs change in the scope of this PR? I haven't noticed any problems without animation changes, it continues to disappear just fine from the opacity it had.

Your changes might affect other animation API usages and encounter regressions in places where the old behavior assumed...

kolayne · 2025-01-03T08:05:48Z

I haven't noticed any problems without animation changes, it continues to disappear just fine from the opacity it had.

There is one: if while a notification is disappearing (i.e., halfway through the fade out), it is again crossed with the cursor, it will be fading in while the cursor is on it, but if the cursor leaves too quickly (before 150ms), the notification is not fully restored yet at that point.

Without the lib_ui change, in that case, the notification will start fading out from the partially restored state; with the change, the moment the cursor leaves, the notification is reset to opacity 1, regardless of how long the cursor spent in the notification area.

Your changes might affect other animation API usages and encounter regressions in places where the old behavior assumed...

I guess, the only ways to ensure that is by either checking other uses, or testing. For the last few days I am running a patched version of Telegram Desktop (with my fixes applied), and it seems fine.

I want to additionally note that the behavior is only changed in the case when there was an old animation, which has not finished yet, and another one is started with .start().

ilya-fedin · 2025-01-03T17:33:57Z

There is one: if while a notification is disappearing (i.e., halfway through the fade out), it is again crossed with the cursor, it will be fading in while the cursor is on it, but if the cursor leaves too quickly (before 150ms), the notification is not fully restored yet at that point.

And?.. I built with such change:

diff --git a/Telegram/SourceFiles/window/notifications_manager_default.cpp b/Telegram/SourceFiles/window/notifications_manager_default.cpp
index 198418b585..4b078d5d11 100644
--- a/Telegram/SourceFiles/window/notifications_manager_default.cpp
+++ b/Telegram/SourceFiles/window/notifications_manager_default.cpp
@@ -190,7 +190,7 @@ void Manager::startAllHiding() {
 	if (!hasReplyingNotification()) {
 		int notHidingCount = 0;
 		for (const auto &notification : _notifications) {
-			if (notification->isShowing()) {
+			if (false && notification->isShowing()) {
 				++notHidingCount;
 			} else {
 				notification->startHiding();
@@ -500,6 +500,7 @@ void Widget::opacityAnimationCallback() {
 	updateOpacity();
 	update();
 	if (!_a_opacity.animating() && _hiding) {
+		manager()->startAllHiding();
 		manager()->removeWidget(this);
 	}
 }

Here's what I see:

_20250103_212752.mp4

I don't see any issue with the animation...

kolayne · 2025-01-03T17:59:54Z

Here's what I see:

_20250103_212752.mp4
I don't see any issue with the animation...

Same for me. I don't think this behavior is correct. In my understanding, if my cursor has crossed a notification, the notification should reset completely and start fading out from the initial state (i.e., opacity 1). That's what former experience with Telegram Desktop felt like: if a notification is fading out already and I want to keep it on the screen but my cursor happens to be too far, I can just rush and cross the notification (without making the it precise enough to stay in the notification area) and that will keep it.

ilya-fedin · 2025-01-03T18:20:16Z

if my cursor has crossed a notification, the notification should reset completely and start fading out from the initial state (i.e., opacity 1).

Hm. How your change makes it happen? There's just no time for it to fade in completely. And, well, that's why I proposed to make a new method that would start the _hideTimer: it starts hiding later, after it's completely shown.

kolayne · 2025-01-04T08:25:08Z

Hm. How your change makes it happen?

Do you mean how I make notifications fade in completely? I fix the behavior of the Ui::Animations::Simple::start() method to make sure that the from opacity is set before the animation starts. Previously, that was not the case (i.e., .start() would behave similar to .change() if there is an active animation) but this bug was not visible because Telegram Desktop would not even attempt to hide notifications that were crossed with a cursor too quickly.

There's just no time for it to fade in completely. And, well, that's why I proposed to make a new method that would start the _hideTimer: it starts hiding later, after it's completely shown.

I agree that this would also lead to the desired behavior. But I wonder if this is the right solution. I guess, the key question here is what kind of behavior of lib_ui is right.

In my understanding, if there is an ongoing animation, and the .start() method is called on it instead of .change() (so, the opacity values from and to are provided as arguments), the caller likely expects that the animation to go from from to to, rather than from the current opacity value to to. If that's the case, then there's a bug in lib_ui, and the timer solution just works around that bug. If that's not the case and the previous behavior is desired (i.e., the from parameter of .start() shall be ignored when there is a previous animation), then my PR to lib_ui is irrelevant, and the timer solution is the right one.

ilya-fedin · 2025-01-04T10:58:24Z

I fix the behavior of the Ui::Animations::Simple::start() method to make sure that the from opacity is set before the animation starts.

I.e. you make it jump to opaque rather animate?

kolayne · 2025-01-04T14:06:48Z

It switches instantly to the value of from and then animates to to. But there is no visual disturbance when I try this with notifications. I can record a demo when I'm back home.

ilya-fedin · 2025-01-04T14:08:49Z

No need to, I already tried your branch

ilya-fedin · 2025-01-04T14:10:26Z

You have to wait @john-preston's review now

kolayne commented Jan 1, 2025

View reviewed changes

ilya-fedin reviewed Jan 1, 2025

View reviewed changes

kolayne force-pushed the notifications-fading-out branch 2 times, most recently from 372a75d to a00fac7 Compare January 2, 2025 15:53

kolayne added 2 commits January 2, 2025 18:55

kolayne commented Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix notifications when passed through with a cursor too quickly #28815

Fix notifications when passed through with a cursor too quickly #28815

kolayne commented Jan 1, 2025 •

edited

Loading

kolayne Jan 1, 2025

kolayne commented Jan 1, 2025

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025

ilya-fedin Jan 1, 2025 •

edited

Loading

kolayne Jan 1, 2025 •

edited

Loading

kolayne commented Jan 1, 2025

kolayne commented Jan 1, 2025

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

kolayne commented Jan 2, 2025

kolayne commented Jan 2, 2025

ilya-fedin commented Jan 2, 2025

ilya-fedin commented Jan 2, 2025 •

edited

Loading

kolayne commented Jan 2, 2025

kolayne Jan 2, 2025

ilya-fedin commented Jan 2, 2025

kolayne commented Jan 3, 2025 •

edited

Loading

ilya-fedin commented Jan 3, 2025

kolayne commented Jan 3, 2025

ilya-fedin commented Jan 3, 2025

kolayne commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

kolayne commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

Fix notifications when passed through with a cursor too quickly #28815

Are you sure you want to change the base?

Fix notifications when passed through with a cursor too quickly #28815

Conversation

kolayne commented Jan 1, 2025 • edited Loading

kolayne Jan 1, 2025

Choose a reason for hiding this comment

kolayne commented Jan 1, 2025

ilya-fedin commented Jan 1, 2025 • edited Loading

ilya-fedin commented Jan 1, 2025

ilya-fedin Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

kolayne Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

kolayne commented Jan 1, 2025

kolayne commented Jan 1, 2025

ilya-fedin commented Jan 1, 2025 • edited Loading

ilya-fedin commented Jan 1, 2025 • edited Loading

ilya-fedin commented Jan 1, 2025 • edited Loading

kolayne commented Jan 2, 2025

kolayne commented Jan 2, 2025

ilya-fedin commented Jan 2, 2025

ilya-fedin commented Jan 2, 2025 • edited Loading

kolayne commented Jan 2, 2025

kolayne Jan 2, 2025

Choose a reason for hiding this comment

ilya-fedin commented Jan 2, 2025

kolayne commented Jan 3, 2025 • edited Loading

ilya-fedin commented Jan 3, 2025

kolayne commented Jan 3, 2025

ilya-fedin commented Jan 3, 2025

kolayne commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

kolayne commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

ilya-fedin commented Jan 4, 2025

kolayne commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin Jan 1, 2025 •

edited

Loading

kolayne Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 1, 2025 •

edited

Loading

ilya-fedin commented Jan 2, 2025 •

edited

Loading

kolayne commented Jan 3, 2025 •

edited

Loading