<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4403/">http://reviewboard.kde.org/r/4403/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 20th, 2010, 6:39 p.m., <b>Michael Pyne</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>I'll not that KIconLoader is not "incorrect" in that the icon-theme-spec does specify that hicolor should be searched if the desired icon does not exist in a theme. shared-mime-info kind of overrides that by saying that if no specific icon is located, to search for a generic fallback icon, but that should only be mime icons.
One question is what do we do if hicolor happens to have an exact match of the required size, do we accept hicolor or try to fallback to generic? I would say accept hicolor's more specific video-mp4 in preference to video-x-generic, but that could look ugly in several themes.</pre>
</blockquote>
<p>On June 20th, 2010, 7:13 p.m., <b>Riccardo Iaconelli</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>As an artist, I'd say ship it. If the name spec says something different, I think we should change it.
I've not reviewed this patch code-wise though.
For the second problem of yours, but still as an artistic advice, I'd say it's better to load the generic mime icon over accepting hicolor's specific version.</pre>
</blockquote>
</blockquote>
<pre>The "Icon Naming Specification" says this:
"The dash “-” character is used to separate levels of specificity in icon names, for all contexts other than MimeTypes. For instance, we use “input-mouse” as the generic item for all mouse devices, and we use “input-mouse-usb” for a USB mouse device. However, if the more specific item does not exist in the current theme, and does exist in a parent theme, the generic icon from the current theme is preferred, in order to keep consistent style."
If we interpret the part which says "for all contexts other than MimeTypes" as an explanation that for mimetypes, "/" is used rather than "-" (but actually "-" is also used), then it says that oxygen "video" (and I think, "video-x-generic" as well) should be preferred over hicolor "video-mp4".
Another way to fix this bug would be to copy/move "video-x-generic" to "video" in the oxygen theme. This is probably safer code-wise, but it does not really match the spec.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 20th, 2010, 6:39 p.m., <b>Michael Pyne</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="/r/4403/diff/1/?file=29183#file29183line932" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">K3Icon KIconLoaderPrivate::findMatchingIconWithGenericFallbacks(const QString& name, int size) const</pre></td>
</tr>
</tbody>
<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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">910</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
</tr>
</tbody>
</table>
<pre>Be careful with this, I'm pretty sure this code path can be run for any non-User icon, not just mime icons, so doing this replace would break icons under subdirectories that are currently found.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">I thought "name" was always an icon name at this point. If it can be a path, then it's definitly wrong.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 20th, 2010, 6:39 p.m., <b>Michael Pyne</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="/r/4403/diff/1/?file=29183#file29183line1068" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">K3Icon KIconLoaderPrivate::findMatchingIcon(const QString& name, int size) const</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">K3Icon KIconLoaderPrivate::findMatchingIcon(const QString& _name, int size) const</pre></td>
</tr>
</tbody>
<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; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1044</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="p">}</span> <span class="k">else</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre>Is there a reason to move generic fallback support into findMatchingIcon where it must be used for all icons? KIconLoader is plenty slow as it currently stands, I'm not sure this would help matters.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">That's the whole point of the patch. findMatchingIcon() works like this:
findMatchingIcon():
for theme in themes:
while not name is empty:
if theme has name:
return name
make name more generic
It is necessary to move the "-x-generic" code in here so that the "make name more generic" step can try the "-x-generic" version. Before the patch, findMatchingIconWithGenericFallbacks() worked like this:
findMatchingIconWithGenericFallbacks():
icon = findMatchingIcon(name)
if icon:
return icon
...
icon = findMatchingIcon(name + "-x-generic")
if icon:
return icon
This would not use "video-x-generic" for "video-mp4" because the first call to findMatchingIcon() finds the "video" icon in the "hicolor" theme.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On June 20th, 2010, 9:18 a.m., Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs and Rafael Fernández López.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated 2010-06-20 09:18:37</i></p>
<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;">When loading an icon for video-mp4, KIconLoader incorrectly returns "video.png" from Hicolor, instead of returning "video-x-generic.png" from Oxygen. Since Hicolor only has a 16x16 version, this results in blurry icons in Dolphin or Gwenview when icons are bigger than 16x16.
This is because the current code fallbacks to Hicolor before looking for a "-x-generic" version.</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;">- Added a unit-test entry to check we get video-x-generic.
- Icons are no longer blurry after the patch.</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>trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp <span style="color: grey">(1140302)</span></li>
<li>trunk/KDE/kdelibs/kdeui/tests/kiconloader_unittest.cpp <span style="color: grey">(1140302)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4403/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>