<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="http://git.reviewboard.kde.org/r/112009/">http://git.reviewboard.kde.org/r/112009/</a>
</td>
</tr>
</table>
<br />
<p>On August 12th, 2013, 10:57 p.m. UTC, <b>Patrick Spendrin</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;">In general of course: thanks for your review request, one of the small nice overdue features!
How does this work on Linux, e.g. do the changes to the .desktop file affect Linux here?</pre>
</blockquote>
<p>On August 13th, 2013, 6:07 a.m. UTC, <b>Pali Rohár</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;">In desktop file is bug. Thumbnail for "application/x-win-lnk" is not supported by this plugin, so it should be removed - and it fixing by this patch.
I already proposed it in my patch https://svn.reviewboard.kde.org/r/5156/ but in Qt is *bug* which caused that my patch not working correctly. My patch removed dependency on icoutils and used only Qt functions. But due to Qt bug it not worked. (See that review request).
What is preferred way to fix bugs in Qt4 which affects KDE?</pre>
</blockquote>
<p>On August 13th, 2013, 8:11 a.m. UTC, <b>Patrick Spendrin</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;">You should fix the bug in the 5.X version of Qt and backport it to Qt4. You must let the patch be reviewed on http://codereview.qt-project.org/ (see http://qt-project.org/wiki/Gerrit-Introduction and http://qt-project.org/wiki/Setting-up-Gerrit how to do that).</pre>
</blockquote>
<p>On August 13th, 2013, 8:18 a.m. UTC, <b>Pali Rohár</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 have no motivation to try it again. I pushed my patch to nokia qt bug tracker 3 years ago, but nobody look at it. Next, I created merge request on qt gitorious project and waited... and then was merge request removed due to migration from gitorious...
So if you can push it to review for me, I will prepare patch again.</pre>
</blockquote>
<p>On August 13th, 2013, 8:22 a.m. UTC, <b>Patrick Spendrin</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;">If you can prepare that patch, I can try to get it in instead.</pre>
</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;">IIRC Pali's patch to Qt is related to loading cursor files in QtIcoHandler, I think this review can go independently of that patch. Regarding the LNK files, I intend to adapt http://reviewboard.kde.org/r/4817/ too.
--
I'll fix the other issues in the next few days.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 12th, 2013, 10:53 p.m. UTC, <b>Patrick Spendrin</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="http://git.reviewboard.kde.org/r/112009/diff/2/?file=178067#file178067line160" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/thumbnail/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">160</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">set</span> <span class="p">(</span> <span class="s">windowsimagethumbnail_SRCS</span> <span class="o">${</span><span class="nv">windowsimagethumbnail_SRCS</span><span class="o">}</span> <span class="s">icoutils_win.cpp</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;">this could be moved out of the parentheses right?</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;">I didn't get this one, what can be moved out?</pre>
<br />
<p>- Andrius da Costa</p>
<br />
<p>On August 11th, 2013, 1:59 p.m. UTC, Andrius da Costa Ribas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Runtime, kdewin and Pali Rohár.</div>
<div>By Andrius da Costa Ribas.</div>
<p style="color: grey;"><i>Updated Aug. 11, 2013, 1:59 p.m.</i></p>
<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;">This patch intends to enable Windows exe/dll thumbnailing by using winapi. It derives from the unsubmitted patch from Pali Rohár from https://svn.reviewboard.kde.org/r/5156/ as a starting point. I've made a few adjustments on the original patch, split that patch into a common part and a icoutils-specific part, and then created the winapi-based part to replace the icoutils one on Windows (porting icoutils to windows wasn't going to be easy).</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;">Tested on a Windows 7 64-bit machine, with intel compiler (32-bit).
Tested using ico files and both 32-bit and 64-bit executables and dlls, including jumbo-size icons.
I've used QLibrary for all winapi functions in order to avoid issues with MinGW compiler, but I don't have a MinGW setup to check.
Not tested on *nix, but the original patch was not changed except for iterating order in the common part and namespacing.
</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>kioslave/thumbnail/CMakeLists.txt <span style="color: grey">(b81339b)</span></li>
<li>kioslave/thumbnail/icoutils.h <span style="color: grey">(6468bc1)</span></li>
<li>kioslave/thumbnail/icoutils.cpp <span style="color: grey">(31db85d)</span></li>
<li>kioslave/thumbnail/icoutils_common.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kioslave/thumbnail/icoutils_icotools.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kioslave/thumbnail/icoutils_win.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kioslave/thumbnail/windowsexecreator.h <span style="color: grey">(a407982)</span></li>
<li>kioslave/thumbnail/windowsexecreator.cpp <span style="color: grey">(9e24aee)</span></li>
<li>kioslave/thumbnail/windowsexethumbnail.desktop <span style="color: grey">(f10efef)</span></li>
<li>kioslave/thumbnail/windowsimagecreator.h <span style="color: grey">(0b68cc6)</span></li>
<li>kioslave/thumbnail/windowsimagecreator.cpp <span style="color: grey">(08b063d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112009/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/08/11/Icons.png">screenshot</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>