<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/124648/">https://git.reviewboard.kde.org/r/124648/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 7th, 2016, 9:45 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/124648/diff/1/?file=390797#file390797line495" style="color: black; font-weight: bold; text-decoration: underline;">src/kwindowsystem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">495</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">width</span> <span class="o">*=</span> <span class="n">qApp</span><span class="o">-></span><span class="n">devicePixelRatio</span><span class="p">();</span></pre></td>
</tr>
<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">496</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">height</span> <span class="o">*=</span> <span class="n">qApp</span><span class="o">-></span><span class="n">devicePixelRatio</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This part looks wrong to me and I think is the reason why reading icons fails in kwin_wayland now.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We are dealing here with real pixels. E.g. kwin asking for the icon in size 16x16, because the icon is in 16x16 on the window property. By multiplying here it picks a different icon, e.g. 32x32, which might not even exist on the window.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think the scaling needs to be done by the user of the API.</p></pre>
</blockquote>
<p>On April 7th, 2016, 3:50 p.m. CEST, <b>David Edmundson</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;">kwin doesn't have a DPR set, and shouldn't. I doubt that's the reason.
Also how are you checking what icons exist in the window? Arguably they should be scaled down.</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also how are you checking what icons exist in the window?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KWin code just tries to get all available icon sizes which are normally installed on a window. That is currently:</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%">readIcon(16);
readIcon(32);
readIcon(48, false);
readIcon(64, false);
readIcon(128, false);
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All those pixmaps are combined into one QIcon. So it just shouldn't need any scaled values. Also note that the method call takes an argument "bool scale" - I find it strange that I have an API where I say "don't scale the icon" and it scales it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The problem with adjusting the value here is that this is a really low level method with different behavior. Depending on the flags it tries to get the icon from different sources. E.g. it first tries to get the icon installed as a property, then tries to resolve from the legacy installed bitmap, then from class hint. Due to the added scaling it can happen that a smaller icon gets returned. E.g. try Firefox, it installs a max icon of 48, but also has the legacy bitmap set. When you now request the icon in size 48, you cannot get it (there is never an icon with size 96) and falls back to the 16x16 legacy bitmap. Not what one wants.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 7th, 2016, 9:45 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/124648/diff/1/?file=390797#file390797line588" style="color: black; font-weight: bold; text-decoration: underline;">src/kwindowsystem.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">581</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">d</span><span class="o">-></span><span class="n">workArea</span><span class="p">(</span><span class="n">desktop</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">588</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">d</span><span class="o">-></span><span class="n">workArea</span><span class="p">(</span><span class="n">desktop</span><span class="p">)</span><span class="hl"> </span><span class="o"><span class="hl">/</span></span><span class="hl"> </span><span class="n"><span class="hl">qApp</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">devicePixelRatio</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;">This looks wrong to me! This method is supposed to return the actual work area as set on X11, not something multiplied by magic values.</p></pre>
</blockquote>
<p>On April 7th, 2016, 3:50 p.m. CEST, <b>David Edmundson</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;">This one is right.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The whole point of the dpr is that the client code sees <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">everything</em> scaled by the "magic value.
QScreen's size of the monitor is scaled, the QWindow x/y are scaled. We need our library calls to be scaled too, otherwise they're just broken.</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;">to me that looks wrong and confusing. Could you please add that to the documentation of the API that the devicePixelRatio is considered and that if one doesn't want that one should use the more low level NETWM API?</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On August 7th, 2015, 11:47 p.m. CEST, David Edmundson 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 KDE Frameworks and Plasma.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated Aug. 7, 2015, 11:47 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwindowsystem
</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;">Qt scales down the size of QScreens by the device pixel ratio; we should
make our windowing APIs match.
X (and KWin) deal with device dependent pixels, so everything needs to
be converted when communitcating geometry.
Abstraction happens in the main kwindowsystem so X and Wayland are both
supported.
BUG: 350865
BUG: 350614
BUG: 347951</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;">Ran yakuake, it looks all right again.
Re-enabled system DPR support in plasmashell (which is currently disabled) and tested notifications and panel struts are all sensible.</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/kwindowsystem.cpp <span style="color: grey">(0f8ec0ef470b3a3dcd353a1052dc80ed2bb3f992)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124648/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>