[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: Do not fallback to Hicolor when Oxygen provides a
From: "Michael Pyne" <mpyne () kde ! org>
Date: 2010-06-21 2:05:55
Message-ID: 20100621020555.32030.45530 () localhost
[Download RAW message or body]
> On 2010-06-20 18:39:13, Michael Pyne wrote:
> > 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.
>
> Riccardo Iaconelli wrote:
> 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.
> Aurélien Gâteau wrote:
> 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.
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.
> On 2010-06-20 18:39:13, Michael Pyne wrote:
> > trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp, line 932
> > <http://reviewboard.kde.org/r/4403/diff/1/?file=29183#file29183line932>
> >
> > 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.
>
> Aurélien Gâteau wrote:
> I thought "name" was always an icon name at this point. If it can be a path, then \
> it's definitly wrong.
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.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4403/#review6198
-----------------------------------------------------------
On 2010-06-20 09:18:37, Aurélien Gâteau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4403/
> -----------------------------------------------------------
>
> (Updated 2010-06-20 09:18:37)
>
>
> Review request for kdelibs and Rafael Fernández López.
>
>
> Summary
> -------
>
> 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.
>
> Diffs
> -----
>
> trunk/KDE/kdelibs/kdeui/icons/kiconloader.cpp 1140302
> trunk/KDE/kdelibs/kdeui/tests/kiconloader_unittest.cpp 1140302
>
> Diff: http://reviewboard.kde.org/r/4403/diff
>
>
> Testing
> -------
>
> - Added a unit-test entry to check we get video-x-generic.
> - Icons are no longer blurry after the patch.
>
>
> Thanks,
>
> Aurélien
>
>
[Attachment #3 (text/html)]
<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>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic