<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>
<p>On June 20th, 2010, 9:14 p.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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>
</blockquote>
</blockquote>
<pre>OK, I didn't see the wording about icons other than mime type icons. It sounds perfectly cromulent to prefer Oxygen's video-x-generic over the parent theme's video-mp4, and your reasoning about findMatchingIcons() makes sense, so my only concern is the mangling of the icon relative path.</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>
<p>On June 20th, 2010, 9:02 p.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre>I thought "name" was always an icon name at this point. If it can be a path, then it's definitly wrong.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">Looking at KIconLoader, the KIconLoader::iconPath() seems most relevant, and it appears to me that the icon name as passed to findMatchingIcon() can be a relative path still, although absolute paths have already been handled by that point. This is the only concern I have left, it may be easy to handle this case manually in KIconLoader::loadMimeTypeIcon() though.</pre>
<br />
<p>- Michael</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>