<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/120876/">https://git.reviewboard.kde.org/r/120876/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 29th, 2014, 12:41 p.m. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The version check is actually on the bugzilla version (KDE_MAKE_VERSION is just a bitshifting macro) - it's the important part in the original patch ;-)</p></pre>
</blockquote>
<p>On October 29th, 2014, 12:43 p.m. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To prevent future confusion, one might want to add</p>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">define KDE_MAKE_BUGZILLA_VERSION KDE_MAKE_VERSION</h1>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">or sth. like this.</p></pre>
</blockquote>
<p>On October 29th, 2014, 7:56 p.m. UTC, <b>Hrvoje Senjan</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">note to self: never blindly c/p pathes ;-)
still, i wonder, how will those versions get evaluated if building with kdelibs > 4.5.x ?
i guess i should read the patch and discussion more closely.</p></pre>
</blockquote>
<p>On October 29th, 2014, 8:05 p.m. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the numbers refer to bugzilla 4.5.x - "KDE_MAKE_VERSION(a,b,c)" is just "a<<16 | b<<8 | c" (no, i don't know why it wastes 8 bits ;-)
You'd typically test against KDE_VERSION or directly use KDE_IS_VERSION to actually compare the runtime KDE library.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch however tests against numbers it queries from the bugzilla server.</p></pre>
</blockquote>
<p>On October 29th, 2014, 8:24 p.m. UTC, <b>Hrvoje Senjan</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">in order not to put back in dependancy to kdelibs4support, going to try with QT_VERSION_CHECK. hopefully to have same effect</p></pre>
</blockquote>
<p>On October 29th, 2014, 8:33 p.m. UTC, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, has.
However, now it reads even more misleading.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe put</p>
<h1 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">define MAKE_BUGZILLA_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))</h1>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">below the includes and use that to prevent future confusion?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please note: there is yet another "version" involved in Dr K, besides the KDE, QT and Bugzilla versions currently causing confusion. That is the version of the software against which the bug is being reported, i.e. the version of the program that has crashed. I do not think it comes up in the present patch, but keep an eye out for it as you work further... It certainly caused me a moment or two of confusion as I was reading Dr K's code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For improved readability and less possibility of future confusion, I suggest:
1. Change "currentVersion" to currentBugzillaVersion".
2. The macro is just a data conversion: it does not "make" a Bugzilla version. So change its name from MAKE_BUGZILLA_VERSION to INT_BUGZILLA_VERSION or, if you like traditional Unix naming, BUGZILLA_VERSION_TOINT.
3. Bring the macro definition inside setFeaturesForVersion(): it is (deliberately) not used anywhere else.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The version tests would then read like "if (currentBugzillaVersion >= INT_BUGZILLA_VERSION(4, 4, 3))".</p></pre>
<br />
<p>- Ian</p>
<br />
<p>On October 29th, 2014, 8:41 p.m. UTC, Hrvoje Senjan 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 Plasma, Ben Cooksley, Ian Wadham, and Thomas Lübking.</div>
<div>By Hrvoje Senjan.</div>
<p style="color: grey;"><i>Updated Oct. 29, 2014, 8:41 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=337742">337742</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</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;">discussion was in https://git.reviewboard.kde.org/r/120431/
removed the version checks, as we know we have kdelibs >= 4.5 ;-)</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;">builds, succesfully reported bug via patched DrKonqi, wasn't able to do so before.</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>drkonqi/bugzillaintegration/bugzillalib.h <span style="color: grey">(570169b)</span></li>
<li>drkonqi/bugzillaintegration/bugzillalib.cpp <span style="color: grey">(8fd8399)</span></li>
<li>drkonqi/bugzillaintegration/reportassistantpages_bugzilla.h <span style="color: grey">(50cf05f)</span></li>
<li>drkonqi/bugzillaintegration/reportassistantpages_bugzilla.cpp <span style="color: grey">(5a6096f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120876/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>