Review Request 120160: KMessageWidget: fix handling of rapid succession of animatedHide+animatedShow calls

[Fabio D'Urso]

> On Sept. 14, 2014, 6:33 p.m., Dominik Haumann wrote:
> > 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.

Well, iirc I've read that KatePart internally queues messages and waits for the completion signal before showing the next message, therefore it never interrupts running animations (which is what I'm fixing), or does it?

About the 4.x backport, we're agreed: I'm targeting kf5 only (4.x doesn't even have `is{Hide,Show}AnimationRunning` and `{hide,show}AnimationFinished`)


> On Sept. 14, 2014, 6:33 p.m., Dominik Haumann wrote:
> > src/kmessagewidget.cpp, line 411
> > <https://git.reviewboard.kde.org/r/120160/diff/1/?file=311762#file311762line411>
> >
> >     I believe this is missing after line 411:
> >     emit showAnimationFinished();

Sprry, I'm not following you, do you mean the other way round? If you call `animatedShow` and `isHideAnimationRunning()` is currently true there's no way I'd expect a `showAnimationFinished()` _now_. I *might* expect a `hideAnimationFinished()`, but actually not, because the hide animation was just interrupted, not really finished.


> On Sept. 14, 2014, 6:33 p.m., Dominik Haumann wrote:
> > src/kmessagewidget.cpp, line 442
> > <https://git.reviewboard.kde.org/r/120160/diff/1/?file=311762#file311762line442>
> >
> >     I believe this is missing after line 441:
> >     emit hideAnimationFinished();

Same as above, did you mean `showAnimationFinished`?


- Fabio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120160/#review66489
-----------------------------------------------------------


On Sept. 18, 2014, 10:40 a.m., Fabio D'Urso wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120160/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2014, 10:40 a.m.)
> 
> 
> Review request for KDE Frameworks, Christoph Feck and Aurélien Gâteau.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   autotests/kmessagewidgetautotest.h 062e2b3 
>   autotests/kmessagewidgetautotest.cpp f46bab0 
>   src/kmessagewidget.cpp bc7ba32 
>   tests/kmessagewidgettest.cpp f621b5a 
> 
> Diff: https://git.reviewboard.kde.org/r/120160/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Fabio D'Urso
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140918/e5bcd994/attachment.html>