<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/127817/">https://git.reviewboard.kde.org/r/127817/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Mai 17th, 2016, 2:29 nachm. CEST, <b>Wolfgang Bauer</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;">Sorry for coming late, I'm not subscribed to the list (and I'm not a maintainer either), just noticed the commit.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have nothing against the patch per se, and it apparently won't cause the wrong fallback to breeze either as reported in bug#360664.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But: Won't we hit the same old problem again that applications won't find their oxygen icons (where they might have installed them in the first place) any more?
Which was the reason why the previous patch to change the default to breeze got reverted.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See https://bugs.kde.org/show_bug.cgi?id=336739#c9, and the discussion in https://mail.kde.org/pipermail/kde-frameworks-devel/2015-October/027900.html.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">At least kdenlive still seems to be affected according to http://bugzilla.opensuse.org/show_bug.cgi?id=975387...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sorry if I'm misunderstanding something here, just wanted to raise a concern for the sake of good...</p></pre>
</blockquote>
<p>On Mai 17th, 2016, 3:03 nachm. CEST, <b>Aleix Pol Gonzalez</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;">Then maybe Breeze should inherit Oxygen? Or just be extended to have such icons. KIconThemes it's the wrong place to enforce these.</p></pre>
</blockquote>
<p>On Mai 17th, 2016, 3:26 nachm. CEST, <b>Wolfgang Bauer</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;">It's not about Breeze. AFAIK Breeze does inherit from Oxygen.
The problem is that users can select other themes that don't inherit from Oxygen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do agree that KIconThemes is the wrong place for this, but the problem won't be solved by ignoring it either....</p></pre>
</blockquote>
<p>On Mai 17th, 2016, 11:07 nachm. CEST, <b>Albert Astals Cid</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;">If apps install the icon into oxygen, the apps need to be fixed to install their icon to hicolor</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;">Yes, of course.
But ideally the applications should be fixed first IMHO, before the icon loader behavior (that has been established for years) is changed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And in the case of kdenlive, it does install its own icons to hicolor.
Still, many icons are missing (like new/open/save) if the oxygen icon theme is not installed. (The strange thing is that it actually uses breeze icons, but they are missing if <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">oxygen</em> is not installed.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Although, I actually tested this patch now, and kdenlive still is fine. And actually this patch <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">fixes</em> the problem with missing icons in kdenlive if the oxygen icon theme is not installed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, sorry for the noise.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other applications I tried are missing icons when run outside of Plasma, but that's nothing new and unrelated to this patch.
I will do some more testing <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">inside</em> a Plasma session though, with an ("incomplete") icon theme other than breeze or oxygen configured...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's just one thing I still don't understand really:
the patch changes the default icon theme to hicolor AFAICS. Shouldn't this be breeze instead then?</p></pre>
<br />
<p>- Wolfgang</p>
<br />
<p>On Mai 17th, 2016, 12:23 nachm. CEST, Aleix Pol Gonzalez 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.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated Mai 17, 2016, 12:23 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kiconthemes
</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;">We were using oxygen as the default theme, this meant that oxygen was processed by every application. This was fixed in the past by changing this setting to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">breeze</code>, but this only brought new kinds of problems (e.g. review 127232).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See bug 336739, bug 360664.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">By just using hicolor, we don't make the application fetch <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">oxygen</code> for little reason.</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;">On dolphin:
This has an improvement on callgrind 1556M/1185M instructions (30% improvement).
KIconLoader allocations correspond to 3.8% instead of 4.8%, going down by: 535K/509K</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/kiconloader.cpp <span style="color: grey">(6ce7ecdc0b9c38647994c750e77980fbf9b6fdd4)</span></li>
<li>src/kicontheme.cpp <span style="color: grey">(f8c0a37754848a74e42bba5866aa4ce0998bce7c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127817/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>