<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/127826/">https://git.reviewboard.kde.org/r/127826/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 6th, 2016, 7:50 a.m. CEST, <b>Martin Gräßlin</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/127826/diff/1/?file=464078#file464078line245" style="color: black; font-weight: bold; text-decoration: underline;">src/declarativeimports/core/windowthumbnail.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">245</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">icon</span> <span class="o">=</span> <span class="n">KWindowSystem</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">icon</span><span class="p">(</span><span class="n">m_winId</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">245</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">icon</span> <span class="o">=</span> <span class="n">KWindowSystem</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">icon</span><span class="p">(</span><span class="n">m_winId</span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n"><span class="hl">boundingRect</span></span><span class="p"><span class="hl">().</span></span><span class="n"><span class="hl">width</span></span><span class="p"><span class="hl">(),</span></span><span class="hl"> </span><span class="n"><span class="hl">boundingRect</span></span><span class="p"><span class="hl">().</span></span><span class="n"><span class="hl">height</span></span><span class="p"><span class="hl">()</span>);</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;">showing you the icon look up code from KWin, which is close to perfection (doesn't get the 256x256 icon yet):</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%"><span style="color: #008000; font-weight: bold">QIcon</span> <span style="color: #008000; font-weight: bold">icon</span><span style="color: #666666">;</span>
<span style="color: #008000; font-weight: bold">auto</span> <span style="color: #008000; font-weight: bold">readIcon</span> <span style="color: #666666">=</span> <span style="color: #BC7A00">[</span>this, <span style="color: #666666">&</span>icon<span style="color: #BC7A00">]</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">int</span> <span style="color: #008000; font-weight: bold">size</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">bool</span> <span style="color: #008000; font-weight: bold">scale</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">true</span><span style="color: #666666">)</span> {
const QPixmap pix <span style="color: #666666">=</span> KWindowSystem<span style="color: #666666">::</span><span style="color: #008000; font-weight: bold">icon</span>(window()<span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">size</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">size</span><span style="color: #666666">,</span> scale<span style="color: #666666">,</span> KWindowSystem<span style="color: #666666">::</span>NETWM <span style="color: #666666">|</span> KWindowSystem<span style="color: #666666">::</span>WMHints<span style="color: #666666">,</span> info);
if (<span style="color: #666666">!</span>pix<span style="color: #666666">.</span>isNull()) <span style="border: 1px solid #FF0000">{</span>
<span style="color: #008000; font-weight: bold">icon</span><span style="color: #666666">.</span>addPixmap(pix);
}
<span style="border: 1px solid #FF0000">}</span><span style="color: #666666">;</span>
<span style="color: #008000; font-weight: bold">readIcon</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">16</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">readIcon</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">32</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">readIcon</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">48</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">false</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">readIcon</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">64</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">false</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">readIcon</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">128</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">false</span><span style="color: #666666">);</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The problem here is that we don't specify from where to take the icon (we should always try to prefer NETWM) and don't fetch all possible sizes. This is rather important because we want neither upscaling, nor downscaling.</p></pre>
</blockquote>
<p>On May 6th, 2016, 8:51 a.m. CEST, <b>Anthony Fieroni</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;">We specified http://api.kde.org/frameworks-api/frameworks5-apidocs/kwindowsystem/html/kwindowsystem_8cpp_source.html#l00485
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">return icon(win, width, height, scale, NETWM | WMHints | ClassHint | XApp);</em></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;">having written the code in question: I think that was just bad code on my side. It's the fallback and I didn't spend much time on it. If I would have thought properly about it, I would have taken KWin's code.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On May 3rd, 2016, 8:28 p.m. CEST, Anthony Fieroni 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, David Edmundson and Martin Gräßlin.</div>
<div>By Anthony Fieroni.</div>
<p style="color: grey;"><i>Updated May 3, 2016, 8:28 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</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;">I'm toking when app is minimized and when hover taskitem in task manager it's drawed a thumnail icon window. So what is the real problem, some apps, many as KDE', not provides their icons in window props and NETWinInfo fails to get them. So in this case we must specified size explicitely to KWindowSystem::icon. But this is not enough because KWindowSystem::icon makes incorrect decision to crop icon to 48x48. Explanation the shots:
1. Current code not specified size explicitely, returned icon is 16x16
2. New code wants icon 256x256 but KWindowSystem::icon ignore it and returning icon 48x48
3. New code in 2 frameworks both, it works.
So i makes shots with Dragon but many applications are affected, even KDE' akregator, ktimer, kalarm and so on, i don't start to comment other toolkit apps, when we violates KDE'.</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;">Depends on https://git.reviewboard.kde.org/r/127809/</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>src/declarativeimports/core/windowthumbnail.cpp <span style="color: grey">(251aaa4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127826/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/f81bc88b-fe3b-4f00-b2f2-502effe4c245__Screenshot_20160503_202946.png">current plasma-framework and current kwindowsystem</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/36d7baa3-e875-420f-b2c2-39c2e5d409b9__Screenshot_20160503_203337.png">new plasma-framework, current kwindowsystem</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/857380fe-cdfd-478d-9df2-743f87d4c1ec__Screenshot_20160503_203946.png">new plasma-framework, new kwindowsystem</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>