[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: Plasma: Folder View: Icon displays 'pressed' on
From: "Aaron Seigo" <aseigo () kde ! org>
Date: 2010-11-12 22:00:06
Message-ID: 20101112220006.28755.45486 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-11-09 18:37:25, Aaron Seigo wrote:
> >
>
> Lindsay Roberts wrote:
> If there are no objections could I request that someone check this in?
>
> Markus Slopianka wrote:
> Does it change the behavior described in \
> https://bugs.kde.org/show_bug.cgi?id=256465 (always using the large 256x256 icon \
> for the scaling effect)? If no, I suggest to tweak the patch because ever since \
> Nuno changed the hi-res icons to look completely different from the smaller ones, \
> the scaling effect looks really weird.
> Lindsay Roberts wrote:
> I've looked a bit into this issue: This is at a much lower level than the \
> FolderView itself, it is traceable to KIcon/KIconLoader/KTheme and the strategy \
> used to generate icon's for a given size. This is not a part of, or unique to, \
> Plasma. To visualise this: open Dolphin and navigate to a folder full of movies \
> that have a generic icon, turn previews off, and scale through the available zoom \
> levels. You will see the icon alternating between the smaller icon (on sizes \
> exactly matching the /share/icons/style/SIZExSIZE) and the 256x256 icon (on all \
> other sizes).
> Having said that, its questionable whether FolderView should be requesting an Icon \
> in this way; what we really want in this instance is (existing_icon * 0.9) not \
> (most_suitable_icon_for(size * 0.9)). But. The behaviour of the icon sizer seems \
> quite clearly incorrect. If we mean to use the largest icon for scaling in all \
> scaled cases we may as well not ship anything but 256x icons, we are essentially \
> not allowing the content disparity the system pertains to provide. Admittedly it is \
> utilised far less than it perhaps could be, but that is no reason not to support \
> it.
> We could fix the issue by scaling the pixmap from the icon in FolderView itself. \
> There are some issues with regards to whether we cache those scaled pixmaps. \
> Another question: is it wise to cache pixmaps for actions that only happen once on \
> activate and then possibly not for a stretch of time? Perhaps it would be better to \
> leave the cache for icon content that will be regularly repainted. In that case we \
> can just scale on paint() when pressed and delete the pixmap on unpressed or \
> similar.
> If we don't want to fix it in that way, then there are issues to be dealt with at \
> the KDELibs level, and that would be firmly out of the scope of this patch/review. \
> Even the paint fix might be out of scope of this particular patch, I'll defer to \
> the list on that.
i completely agree with this: "what we really want in this instance is (existing_icon \
* 0.9) not (most_suitable_icon_for(size * 0.9))" and that's easy enough to do just by \
changing the icon requested in the code and then doing the scaling in folderview. in \
any case, i'll commit this patch as it fixes something regardless of that other \
behaviour.
- Aaron
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5805/#review8589
-----------------------------------------------------------
On 2010-11-09 11:50:55, Lindsay Roberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5805/
> -----------------------------------------------------------
>
> (Updated 2010-11-09 11:50:55)
>
>
> Review request for Plasma.
>
>
> Summary
> -------
>
> The folder view employs a 'pressed' effect to show when an icon is activated.
> This works as expected when global settings are set to single click: the icon
> is scaled to 90% on press, and rescales back up on activate.
>
> Under double click the icon scales on the first click, and weirdly stays scaled
> until the mouse leaves the hover area.
>
> Further, the same issue applies when modifier keys are used to change
> selection.
>
>
> This addresses bug 256297.
> https://bugs.kde.org/show_bug.cgi?id=256297
>
>
> Diffs
> -----
>
> /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 1193797
>
> Diff: http://svn.reviewboard.kde.org/r/5805/diff
>
>
> Testing
> -------
>
> Tested with both single and double click settings.
>
>
> Thanks,
>
> Lindsay
>
>
[Attachment #5 (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://svn.reviewboard.kde.org/r/5805/">http://svn.reviewboard.kde.org/r/5805/</a>
</td>
</tr>
</table>
<br />
<p>On November 10th, 2010, 12:54 a.m., <b>Lindsay Roberts</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;">If there are no \
objections could I request that someone check this in?</pre> </blockquote>
<p>On November 10th, 2010, 11:02 a.m., <b>Markus Slopianka</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;">Does it change the \
behavior described in https://bugs.kde.org/show_bug.cgi?id=256465 (always using the \
large 256x256 icon for the scaling effect)? If no, I suggest to tweak the patch \
because ever since Nuno changed the hi-res icons to look completely different from \
the smaller ones, the scaling effect looks really weird.</pre> </blockquote>
<p>On November 11th, 2010, 11:40 p.m., <b>Lindsay Roberts</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;">I've looked a bit \
into this issue: This is at a much lower level than the FolderView itself, it is \
traceable to KIcon/KIconLoader/KTheme and the strategy used to generate icon's \
for a given size. This is not a part of, or unique to, Plasma. To visualise this: \
open Dolphin and navigate to a folder full of movies that have a generic icon, turn \
previews off, and scale through the available zoom levels. You will see the icon \
alternating between the smaller icon (on sizes exactly matching the \
/share/icons/style/SIZExSIZE) and the 256x256 icon (on all other sizes).
Having said that, its questionable whether FolderView should be requesting an Icon in \
this way; what we really want in this instance is (existing_icon * 0.9) not \
(most_suitable_icon_for(size * 0.9)). But. The behaviour of the icon sizer seems \
quite clearly incorrect. If we mean to use the largest icon for scaling in all scaled \
cases we may as well not ship anything but 256x icons, we are essentially not \
allowing the content disparity the system pertains to provide. Admittedly it is \
utilised far less than it perhaps could be, but that is no reason not to support it.
We could fix the issue by scaling the pixmap from the icon in FolderView itself. \
There are some issues with regards to whether we cache those scaled pixmaps. Another \
question: is it wise to cache pixmaps for actions that only happen once on activate \
and then possibly not for a stretch of time? Perhaps it would be better to leave the \
cache for icon content that will be regularly repainted. In that case we can just \
scale on paint() when pressed and delete the pixmap on unpressed or similar.
If we don't want to fix it in that way, then there are issues to be dealt with at \
the KDELibs level, and that would be firmly out of the scope of this patch/review. \
Even the paint fix might be out of scope of this particular patch, I'll defer to \
the list on that.</pre> </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;">i completely agree with \
this: "what we really want in this instance is (existing_icon * 0.9) not \
(most_suitable_icon_for(size * 0.9))" and that's easy enough to do just by \
changing the icon requested in the code and then doing the scaling in folderview. in \
any case, i'll commit this patch as it fixes something regardless of that other \
behaviour.</pre> <br />
<p>- Aaron</p>
<br />
<p>On November 9th, 2010, 11:50 a.m., Lindsay Roberts wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://svn.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 Plasma.</div>
<div>By Lindsay Roberts.</div>
<p style="color: grey;"><i>Updated 2010-11-09 11:50:55</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; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">The folder view employs a 'pressed' effect to show when an icon \
is activated. This works as expected when global settings are set to single click: \
the icon is scaled to 90% on press, and rescales back up on activate.
Under double click the icon scales on the first click, and weirdly stays scaled
until the mouse leaves the hover area.
Further, the same issue applies when modifier keys are used to change
selection.</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;">Tested with both single and double click settings.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=256297">256297</a>
</div>
<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/kdebase/apps/plasma/applets/folderview/iconview.cpp <span \
style="color: grey">(1193797)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5805/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic