KMessageWidget: fix handling of rapid succession of animatedHide+animatedShow calls
Review Request #120160 - Created Sept. 12, 2014 and discarded
Hi, I've found that if you call animatedShow() immediatly after animatedHide(), without waiting for the hide animation to finish first, the hide animation goes on and, in the end, the message widget becomes hidden.
The attached patch adds checks in animatedShow and animatedHide so that, if an opposite animation is currently running, they just reverse it.
I've added buttons to invoke animatedShow and animatedHide in tests/kmessagewidget.cpp and used them to test the behavior.
I've also added an autotest that checks the final height and isVisible() value, both after single animatedShow/animatedHide calls and after animatedShow+animatedHide and animatedHide+animatedShow.
|Couldn't this actually check that no animation is running at all now? We triggered the hide directly after the show, ...||Milian Wolff|
|same as above||Milian Wolff|
|I believe this is missing after line 411: emit showAnimationFinished();||Dominik Haumann|
|I believe this is missing after line 441: emit hideAnimationFinished();||Dominik Haumann|
|btw, are there signals for these animations? then you could use the much faster approach to wait for the signal ...||Milian Wolff|
|instead, I'd add a QVERIFY(shownHeight) above when you get the value of shownHeight.||Milian Wolff|
|imo, this should be replaced by an explicit call to the event loop instead of waiting in a loop. If ...||Milian Wolff|
|considering that the show was instantly stopped by a hide, I would say these checks should be done before handling ...||Milian Wolff|
|similar to above||Milian Wolff|
|Would it make sense to emit hideAnimationFinished() before reversing the direction to "showing"? Likewise for the other case.||Christoph Feck|
Nice, I think I've seen this behavior before and always wondered about it. Thanks for this contribution, it makes it behave much better. Also a big +1 for the unit test. Just some suggestions below on how to make that even more explicit.
Couldn't this actually check that no animation is running at all now? We triggered the hide directly after the show, without going into the eventloop inbetween. I'd expect no animation to be triggered at all.
generally, I'd urge you to split this test function into smaller ones and give each test a good name so that its clear what you check.
Then also add a case for the case you try to test here, but don't actually (as far as I can see). Namely:
w.animatedShow(); QTest::qWait(10); QVERIFY(w.isShowAnimationRunning()); w.animatedHide(); QVERIFY(w.isHideAnimationRunning()); QTest::qWait(10); QVERIFY(!w.isHideAnimationRunning()); QVERIFY(!w.isVisible());
and similar for the animatedShow/Hide when it is shown already.
same as above
All in all, the patch looks good, but it misses emitting the signals hideAnimationFinished() and showAnimationFinished().
Why? Without your patch, you can be sure that after calling show() or animatedShow() the signal showAnimationFinished() will eventually be emitted.
With the current version of your patch, this is not true anymore. For instance, KatePart relies on these signals to work.
I'd be fine with committing this to KF5, but I would suggest to not backport to 4.x.
some more nitpickery from my side, but otherwise this is good already. I'd like to see this in and not hold you up much longer.
btw, are there signals for these animations? then you could use the much faster approach to wait for the signal via a QSignalSpy.
instead, I'd add a QVERIFY(shownHeight) above when you get the value of shownHeight.
imo, this should be replaced by an explicit call to the event loop instead of waiting in a loop. If I understand you correctly, the animation just needs to work through the pending events to know its not running anymore. there should not be any time delay involved otherwise.
considering that the show was instantly stopped by a hide, I would say these checks should be done before handling any events and then afterwards as well.
similar to above
I would really like to hear Aurelien's opinion, but maybe you can also explain: If a running hideAnimation is interrupted, and reversed by showing a different message, how does the widget handle the resize interruption? Will it resize smoothly during the animation? How does it handle the text change while it is visible?