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

[Milian Wolff]

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

Ship it!


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.


autotests/kmessagewidgetautotest.cpp
<https://git.reviewboard.kde.org/r/120160/#comment46606>

    btw, are there signals for these animations? then you could use the much faster approach to wait for the signal via a QSignalSpy.



autotests/kmessagewidgetautotest.cpp
<https://git.reviewboard.kde.org/r/120160/#comment46605>

    instead, I'd add a QVERIFY(shownHeight) above when you get the value of shownHeight.



autotests/kmessagewidgetautotest.cpp
<https://git.reviewboard.kde.org/r/120160/#comment46607>

    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.
    
        qApp->processEvents();



autotests/kmessagewidgetautotest.cpp
<https://git.reviewboard.kde.org/r/120160/#comment46608>

    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.



autotests/kmessagewidgetautotest.cpp
<https://git.reviewboard.kde.org/r/120160/#comment46609>

    similar to above


- Milian Wolff


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/ae6e19db/attachment.html>