<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/125307/">https://git.reviewboard.kde.org/r/125307/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 22nd, 2015, 11:55 a.m. UTC, <b>Marco Martin</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;">not sure about it, as it would only change the behavior of IconItem but not QIconItem, that is the most used for various reasons, so the two would end up using different themes</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:03 p.m. UTC, <b>David Rosca</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 found only 8 instances: plasma-desktop/applets (3 - kickoff, kicker), plasma-workspace/applets (2 - only in config ui), kdeplasma-addons/applets (3 - only in QtQuick1 code). </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So it is only used in kickoff/kicker and it should be safe to change it to PlasmaCore.IconItem there (if there is not some special reason to use QIconItem).</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:07 p.m. UTC, <b>Marco Martin</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;">no, it should stay qiconitem.
since in those cases shouldn't use the svg icons from the paasma theme and being iconitem more complicated, the difference in time in loading submenus in kicker is distinguishable</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:12 p.m. UTC, <b>Marco Martin</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;">Also I can see in that case bugreports of people complaining that plasma refuses to use their $favoriteicontheme they did set on systemsettings</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:28 p.m. UTC, <b>andreas kainz</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;">can you link to the mailing list or the bugreports please</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:39 p.m. UTC, <b>David Rosca</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;"><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;">since in those cases shouldn't use the svg icons from the paasma theme </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It wouldn't use svg icons, because those QIconItems are given QIcons and not QStrings, so the performance penalty there would be only if the icon name wasn't found in "desktop icon theme" (eg. the icon was in Breeze but not in Breeze Dark). But yeah, using IconItem brings that fade animation, which doesn't look good when listing through app menu.</p>
<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 I can see in that case bugreports of people complaining that plasma refuses to use their $favoriteicontheme they did set on systemsettings</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right now, the plasma is using part of icons from plasma theme and part of the icons from $favoriteicontheme, which I think is worse than using consistently just one icon theme.</p>
<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;">can you link to the mailing list or the bugreports please</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ML: https://mail.kde.org/pipermail/plasma-devel/2015-September/044438.html
I didn't find any open bugs with similar issue, but I'm sure I can't be the only one using dark plasma theme with light applications theme.</p></pre>
</blockquote>
<p>On September 22nd, 2015, 12:52 p.m. UTC, <b>Marco Martin</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 may make sense to add to iconitem a property to disable plasma icons and maybe even the animation.
i don't think it would improve loading times much tough as is just a "bigger" class that just takes more to be instantiated due to just its size</p></pre>
</blockquote>
<p>On September 22nd, 2015, 1:04 p.m. UTC, <b>David Rosca</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;">Well, kicker can stay as it is. There are bunch of icon themes mixed (some from icon theme and some are own application icons) anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The issue is more evident in other parts of desktop (eg. icons in some applets, desktop toolbox, etc.).</p></pre>
</blockquote>
<p>On September 22nd, 2015, 2:33 p.m. UTC, <b>Marco Martin</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;">in general i like the idea, and most of the patch on this part is valid i think.
But i still think that having a fixed icon theme in the desktop is quite a no go.
what's missing here i think is for themes to say "i'm a dark version of the theme X" then the palsma theme would tell to use a dark version where available, not forcing a particular theme tough</p></pre>
</blockquote>
<p>On September 22nd, 2015, 2:45 p.m. UTC, <b>David Rosca</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;"><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;">But i still think that having a fixed icon theme in the desktop is quite a no go.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's more or less the current situation though. Most of the icons in Breeze plasma theme are SVG ones from desktoptheme/icons, only few of the icons not available there are used from "global icon theme" (QIcon::fromTheme).</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;">not really, those are used mostly in the systray and kickoff tab icons</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the pincipal parts, that are task manager, launchers, folderview and $whateverstartmenu contents are and should be from the theme</p></pre>
<br />
<p>- Marco</p>
<br />
<p>On September 19th, 2015, 8:48 a.m. UTC, David Rosca 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.</div>
<div>By David Rosca.</div>
<p style="color: grey;"><i>Updated Sept. 19, 2015, 8:48 a.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;">It is now possible to set preferred icon theme in desktoptheme metadata.desktop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As discussed on ML, this fixes using light breeze icon theme with breeze dark desktoptheme.</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;">Icon theme is picked correctly with PlasmaCore.IconItem. Changing the desktoptheme updates the icons.</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/plasma/theme.cpp <span style="color: grey">(c49ad4c)</span></li>
<li>src/plasma/private/theme_p.cpp <span style="color: grey">(31a6512)</span></li>
<li>src/plasma/theme.h <span style="color: grey">(3f49719)</span></li>
<li>src/desktoptheme/breeze/metadata.desktop <span style="color: grey">(07bbfc3)</span></li>
<li>src/plasma/private/theme_p.h <span style="color: grey">(5b8f71c)</span></li>
<li>src/declarativeimports/core/iconitem.h <span style="color: grey">(3ef0306)</span></li>
<li>src/declarativeimports/core/iconitem.cpp <span style="color: grey">(692cd8d)</span></li>
<li>src/desktoptheme/breeze-dark/metadata.desktop <span style="color: grey">(77647a4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125307/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>