<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/130094/">https://git.reviewboard.kde.org/r/130094/</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 21st, 2017, 9:12 vorm. UTC, <b>Kai Uwe Broulik</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;">Thanks for your patch!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Plasma reviews are nowadays handled on https://phabricator.kde.org/ - could you perhaps upload it there again? I just don't want it to get lost here :/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, I'm not sure about having a separate FileIconProvider class. You also seem to reimplement quite a lot of logic. Can you perhaps try KIO::iconNameForUrl (in KIOCore I think) which has all of that mimetype resolution and special folders logic already built-in, potentially allowing you to reduce your patch to like 10 lines of code :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(Btw using Qt 5.7 features is fine, Plasma-integration is released alongside Plasma, not KDE Frameworks, and the upcoming Plasma 5.10 release will depend on Qt 5.7 anyway)</p></pre>
</blockquote>
<p>On April 21st, 2017, 10:07 vorm. UTC, <b>Eugene Shalygin</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 patch was applying against frameworkintegration for a long time, and there I wanted no dependencies. BTW, do you know what platform theme plugin can be extended to make this work in non-Plasma environment (like GNOME) too?</p></pre>
</blockquote>
<p>On April 21st, 2017, 10:36 vorm. UTC, <b>Eugene Shalygin</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;">Took a look at KIO::iconNameForUrl() and not sure whether to use it and discard iconOptions parameter to this function or not. Do you want to finish with this question here, or should I move to phabricator right away?</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;">Right, Plasma's platform theme used to be shipped in frameworks but was moved to Plasma as it should only be used there and apps running on other environments should use whatever platform's plugin there is (usually Qt built-in). Don't know about other environments, Qt should have implementations for those.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 21st, 2017, 9:12 vorm. UTC, <b>Kai Uwe Broulik</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/130094/diff/1/?file=494561#file494561line122" style="color: black; font-weight: bold; text-decoration: underline;">src/platformtheme/kdeplatformtheme.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">121</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">QIcon</span> <span class="n">KdePlatformTheme</span><span class="o">::</span><span class="n">fileIcon</span><span class="p">(</span><span class="k">const</span> <span class="n">QFileInfo</span> <span class="o">&</span><span class="n">fileInfo</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;">Why is this only built in 5.7 and below? Was this removed in 5.8? (didn't check Qt code)</p></pre>
</blockquote>
<p>On April 21st, 2017, 10:07 vorm. UTC, <b>Eugene Shalygin</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;">Signature of the function was changed in 5.8 if you are asking about the #ifdef. Did I understand you correctly?</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;">Yep, that's what I've been asking. So, there's no method returning a QIcon anymore? Pity.</p></pre>
<br />
<p>- Kai Uwe</p>
<br />
<p>On April 21st, 2017, 9:11 vorm. UTC, Eugene Shalygin 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 Eugene Shalygin.</div>
<p style="color: grey;"><i>Updated April 21, 2017, 9:11 vorm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-integration
</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;">Implement QPlatformTheme::fileIconPixmap() to make QFileIconProvider work.</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;">Manual testing.</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/platformtheme/kdeplatformtheme.h <span style="color: grey">(7835439)</span></li>
<li>src/platformtheme/kdeplatformtheme.cpp <span style="color: grey">(704f176)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130094/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>