<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 />










<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>



 <p>On August 14th, 2013, 10:03 p.m. UTC, <b>Andrius da Costa Ribas</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 didn't get this one, what can be moved out?</pre>
 </blockquote>





 <p>On August 15th, 2013, 7:37 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;">This line seems to be in both parts of the if(WIN32) clause, so either it is wrong? or you can move it out of the parentheses?
    set ( windowsimagethumbnail_SRCS ${windowsimagethumbnail_SRCS} icoutils_win.cpp )</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;">oops, shouldn't be _win on the "not win32" side. Done!</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=178072#file178072line29" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/thumbnail/icoutils_win.cpp</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">29</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span class="k">typedef</span> <span class="n">HMODULE</span> <span class="p">(</span> <span class="n">WINAPI</span> <span class="o">*</span><span class="n">tLoadLibraryEx</span> <span class="p">)(</span> <span class="n">LPCTSTR</span> <span class="n">lpFileName</span><span class="p">,</span> <span class="n">HANDLE</span> <span class="n">hFile</span><span class="p">,</span> <span class="n">DWORD</span> <span class="n">dwFlags</span> <span class="p">);</span> <span class="c1">// kernel32.dll</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;">I don't think we should use dynamic loading of those functions here, only for those functions that might not be available in one of the Windows versions we support (XPsp3, Vista, 7, 8). For most of these functions you can simply use the linked version (e.g. directly use LoadLibraryEx instead of loading kernel32, finding the function, using that function etc.) That might save you quite some code. In case one of the functions is not available, you might need a workaround or a proper error handling.</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;">removed all the dynamic loading.</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=178073#file178073line30" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/thumbnail/windowsexecreator.h</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </span>
   </th>
  </tr>
 </thead>



 
 

 <tbody>

  <tr>
    <th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">30</font></th>
    <td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
    <th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
    <td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">style issue: check that you keep the eol at the end of the file (your patch seems to have "no eol at the end of file" in it)</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;">It actually had 2 linefeeds at end, and I changed to one.</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=178074#file178074line49" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/thumbnail/windowsexecreator.cpp</a>
    <span style="font-weight: normal;">

     (Diff revision 2)

    </span>
   </th>
  </tr>
 </thead>



 
 

 <tbody>

  <tr>
    <th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">48</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb">   </span><span class="k">if</span> <span class="p">(</span> <span class="o">!</span> <span class="n">img</span><span class="p">.</span><span class="n">load</span><span class="p">(</span><span class="n">pngTempFile</span><span class="p">.</span><span class="n">fileName</span><span class="p">())</span> <span class="p">)</span></pre></td>
    <th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
    <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span class="k">if</span> <span class="p">(</span> <span class="o">!</span> <span class="n">IcoUtils</span><span class="o">::</span><span class="n">loadIcoImageFromExe</span><span class="p">(</span><span class="n">path</span><span class="p">,</span> <span class="n">img</span><span class="p">,</span> <span class="n">width</span><span class="p">,</span> <span class="n">height</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;">You might simply return the output of !IcoUtils::loadIcoImageFromExe here ;-)</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;">done!</pre>
<br />




<p>- Andrius da Costa</p>


<br />
<p>On August 17th, 2013, 1:43 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. 17, 2013, 1:43 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_win.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>kioslave/thumbnail/icoutils_wrestool.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>