<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/114726/">https://git.reviewboard.kde.org/r/114726/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 30th, 2013, 12:31 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't really understand why this fails....
IMHO it *did* make sense for the KToolBar unittest to check which icon size the toolbars will get by default.
The value of 2 on build.kde.org, for instance, should never never happen. 2 pixels for a toolbar icon is just unusable...</pre>
</blockquote>
<p>On December 30th, 2013, 11:17 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Debugged and fixed in http://commits.kde.org/kxmlgui/94e84ebd02c6ab97e6ac4288f22f17337fc7948a + http://commits.kde.org/frameworkintegration/163282ed294f5c50a1a12be9eb50d8b96d87ab84
Please discard this review request.</pre>
</blockquote>
<p>On January 2nd, 2014, 8:36 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I still think these checks shouldn't be there; why is a kxmlgui unit test checking the default icon sizes of KIconTheme? It just makes it fragile if those defaults are changed (especially as they are in different repositories).</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You see as two different frameworks, I see it as one feature from the user's point of view.
Toolbar icons start in a given size, then can be configured to different sizes (and the whole thing is quite complex, with XMLGUI and KConfig and layering of settings etc.). So the full test includes "what do I start from", which is the default icon size coming from KIconTheme. I don't want to make that test less useful just for the hypothetical situation where one day we might change the default icon size (which sounds rather unlikely).
Yes our autotests aren't all "unit" tests, some of them are more general feature tests. Anything that prevents regressions is good :)</pre>
<br />
<p>- David</p>
<br />
<p>On December 29th, 2013, 6:04 p.m. UTC, Alex Merry wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Alex Merry.</div>
<p style="color: grey;"><i>Updated Dec. 29, 2013, 6:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kxmlgui
</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;">Make sure ktoolbar_unittest does not depend on global icon settings
ktoolbar_unittest should not be testing the default settings of
KIconTheme anyway.
NB: I am away until 2nd Jan, so if it gets a "ship it", feel free to commit it in my absence in order to get Jenkins green again.</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;">Tests pass on my local machine (but they did before as well).</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/ktoolbar_unittest.cpp <span style="color: grey">(2ee490d35b517a8121aa0aeda0d6ebb4256caad0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114726/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>