<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/122086/">https://git.reviewboard.kde.org/r/122086/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 21st, 2015, 10:07 a.m. CET, <b>Martin Gräßlin</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;">ping - do we want this approach to go in?</p></pre>
</blockquote>
<p>On January 21st, 2015, 3:56 p.m. CET, <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;">I've two concerns about this.
1. the name "icons" implies "several" (it's unfortunate that ::icon isn't ::iconPixmap) - maybe "windowIcon()" or similar
2. the behavior how an icon is build up from multiple sources (isn't)
right now, you could end up w/ one 16x16 NETWM icon pixmap.
Considerations: NETWM and WM_HINTS could be more specific than an icon from the appname, the appname could though be more size complete and finally WM_HINTS would likely be "ugly" compared to the other two (bitmasked). Overmore it might be undesirable to end up w/ an icon that has completely different looks for different sizes.
As we don't have a specific hint on the required target size, it gets harder to ever take an informed decision on what to return here, but one might want to at least check the sizes provided by NEWTM before returning early (if other sources are allowed by flags) - yes, I know that this is not a straight suggestion for improvement :-(</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;">I completely agree with your two concerns and also thought about the same. Concerning point 1 I wanted to make them another ::icon overload but that's not possible - it would be ambiguous :-(. windowIcon might indeed be a better name.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Concerning point 2: I thought about combining all items. E.g. if NETWM and WM_HINTS are set, it will include both. But if NETWM provides a size which WM_HINTS doesn't it wouldn't be set. But that wouldn't work with the app or the X icon.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have to think a little bit more about it.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On January 21st, 2015, 10:06 a.m. CET, Martin Gräßlin 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 kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Jan. 21, 2015, 10:06 a.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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">::icons tries to retrieve all available icon sizes and combine them
into a single QIcon. On the X11 platform this can significantly
reduce the time needed to fetch all available icons compared to the
already existing ::icon methods which return a QPixmap of a specific
size.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The change includes a new test application "icontest" which shows all
available icons in all available sizes for a window id passed as
command line argument.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)</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.h <span style="color: grey">(322322f12dda7279567be8420533ed22cffffd52)</span></li>
<li>src/kwindowsystem.cpp <span style="color: grey">(65d215b6dfbf4df22e871fd7187fface75abb61b)</span></li>
<li>src/kwindowsystem_p.h <span style="color: grey">(1f01145b5c7efe925fcb8242f92af17b704e01c9)</span></li>
<li>src/kwindowsystem_p_x11.h <span style="color: grey">(0d4b6ba551776d2fa08030f78226ecdb3c80c889)</span></li>
<li>src/kwindowsystem_x11.cpp <span style="color: grey">(bf958ae63b48424fc412405259f082b740928f9a)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(ce68cc505a69ea9a3cf645e9ae587bd89abe1648)</span></li>
<li>tests/icontest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122086/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>