<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120160/">https://git.reviewboard.kde.org/r/120160/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 13th, 2014, 3:20 p.m. UTC, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120160/diff/1/?file=311761#file311761line115" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kmessagewidgetautotest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">115</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">while</span> <span class="p">(</span><span class="n">w</span><span class="p">.</span><span class="n">isShowAnimationRunning</span><span class="p">()</span> <span class="o">||</span> <span class="n">w</span><span class="p">.</span><span class="n">isHideAnimationRunning</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then also add a case for the case you try to test here, but don't actually (as far as I can see). Namely:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">w.animatedShow();
QTest<span style="color: #666666">::</span>qWait(<span style="color: #666666">10</span>);
QVERIFY(w.isShowAnimationRunning());
w.animatedHide();
QVERIFY(w.isHideAnimationRunning());
QTest<span style="color: #666666">::</span>qWait(<span style="color: #666666">10</span>);
QVERIFY(<span style="color: #666666">!</span>w.isHideAnimationRunning());
QVERIFY(<span style="color: #666666">!</span>w.isVisible());
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and similar for the animatedShow/Hide when it is shown already.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Couldn't this actually check that no animation is running at all now?" -> Well, with the current implementation that would not be true because, as far as I can see, the QTimeLine stays in running state (see updated test in patch v2). I don't know QTimeLine internals, but I guess it needs to process some events before it can notice that there's actually nothing to do.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the new version of the patch, I've simplified the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">while (w.isShowAnimationRunning() || w.isHideAnimationRunning())</code> so that it only checks one condition instead of two. Previously I had put these two checks because unpatched KMessageWidget runs the wrong animation and I wanted to wait for <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">any</em> animation to finish before checking the outcome, but in the new version I've added a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QVERIFY(w.isHideAnimationRunning())</code> before the loop, which guarantees that the correct animation is running.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About the splitting, I'm using the widget height to test if the message is fully shown or hidden (see <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">shownHeight</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">hiddenHeight</code> variables). These values are first obtained through simple single <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedShow</code>/<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedHide</code> calls in the first part of the test, and then used in the second part of the test as reference height. So, if I split the test into smaller pieces, I would not have this data anymore and I'd have to drop the height checks. So we have two options: one big test that checks the height, several small tests that don't check the height. The height check is not strictly necessary, I've added it for extra safety, what do you think?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About your last comment, actually I am testing if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedShow</code> immediatly followed by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedHide</code> works (and vice versa). But I think I see your point, so I've added a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QVERIFY(w.isShowAnimationRunning())</code> after every <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">w.animatedShow()</code> and a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QVERIFY(w.isHideAnimationRunning())</code> after every <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">w.animatedHide()</code>, what do you think, is it better now?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
On the other hand there's no need to insert qWait between <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedShow</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">animatedHide</code>, because it would still test the same code paths, at least given the current implementation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, I'm not a huge fan of the fixed timining you're proposing, because it would depend on the first <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QTest::qWait(10)</code> to sleep no longer than the second one, which is something the OS can't actually guarantee.</p></pre>
<br />
<p>- Fabio</p>
<br />
<p>On September 18th, 2014, 10:40 a.m. UTC, Fabio D'Urso wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, Christoph Feck and Aurélien Gâteau.</div>
<div>By Fabio D'Urso.</div>
<p style="color: grey;"><i>Updated Sept. 18, 2014, 10:40 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwidgetsaddons
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The attached patch adds checks in animatedShow and animatedHide so that, if an opposite animation is currently running, they just reverse it.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've added buttons to invoke animatedShow and animatedHide in tests/kmessagewidget.cpp and used them to test the behavior.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/kmessagewidgetautotest.h <span style="color: grey">(062e2b3)</span></li>
<li>autotests/kmessagewidgetautotest.cpp <span style="color: grey">(f46bab0)</span></li>
<li>src/kmessagewidget.cpp <span style="color: grey">(bc7ba32)</span></li>
<li>tests/kmessagewidgettest.cpp <span style="color: grey">(f621b5a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120160/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>