[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: make the DeviceNotifier Plasmoid scale with high-res screens
From: "Marco Martin" <notmart () gmail ! com>
Date: 2012-11-26 13:08:01
Message-ID: 20121126130801.13733.58759 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Nov. 26, 2012, 9:35 a.m., Marco Martin wrote:
> > This is really an issue that needs addressing, thanks for taking time f=
or it.
> > =
> > i'm very in favor of removing all those hardcoded sizes from the code.
> > =
> > however i see two problems with that approach
> > =
> > 1) icons are cleanly painted only when they have a size that is the "ri=
ght" one (16,22,32,48,64,128,512 exported in qml with theme.smallIconSize, =
smallMediumIconSize etc) for big sizes doesn't matter if the pixmap ends u=
p being scaled, but small icons really look horrible when scaled (and in th=
e new code there is no check the size ends up being one of those).
> > =
> > 2) it's anyways quite arbitrary, so i can easily see each plasmoid usin=
g more or less its own logic, with the end result looking a bit like a patc=
hwork.
> > =
> > the"proper" solution i think is to export in Theme the configurable KIc=
onLoader sizes, (Desktop, Toolbar, SmallIcons etc) and those can be contr=
olled by a kcm, so will be properly set on higher resolution displays
> =
> Marco Martin wrote:
> just added the bindings:
> use
> theme.iconSizes.dialog for most icons
> theme.iconSizes.toolbar for unmount
> theme.iconSizes.small for all that is normally 16x16
> =
> =
> so the icon settings in systemsettings will affect those icons
> =
> Michael Zanetti wrote:
> Hey Marco,
> =
> I'm not sure if this is really a good idea. Using KIconLoader::IconSi=
ze() is a good idea for toolbars and such. However, in a plasma layout I do=
n't think its a good idea to let the user change the size of the icons. It =
can and will mess up the layout. For example I use the same size for "Toolb=
ar" and "Dialog" because I like big toolbars. I certainly don't want to the=
unmount icon to be that huge in the devicemanager plasmoid because if that=
...
> =
> regarding the scaling: I was thinking that Plasma uses SVG icons... R=
eading throught the QIconItem code I see now that it doesn't. In that case =
its obviously not good to freely scale it around. Maybe it should be replac=
ed by PlasmaCore.SvgItem?
what is important is that the sizes of the icons are something that is
a) consistent everywhere (not having each applet decide with its criteria)
b) be something that can depend from dpi, those being settable are fine, si=
nce will be the same as things around application
now, indeed can be not a good idea to have the unmount icon as toolbar, sin=
ce the size can be messed up (so keeping a size relative to the size used t=
o the main icon, that i think dialog is ok)
This i think makes the case for exposing a currently internal component tha=
t i'm using in buttons and toolbuttons called IconLoader
the behavior is:
* use an svg if it does exist in the theme: requiring to have a complete sv=
g theme (and with enough quality) is neither feasible nor desiderable
* fallback to a qicon if the svg is not found
* always paint in the nearest (smaller) kiconloader standard size (even for=
svg, for consistency in layouts and because they aren't as scalable as the=
y seem, if their shape edges are not grid aligned they look horrible)
* will probably also have to directly support the on mouse over highlight a=
nimation or people will keep using the horrible PlasmaWidgets.IconWidget
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107428/#review22548
-----------------------------------------------------------
On Nov. 23, 2012, 10:11 p.m., Michael Zanetti wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107428/
> -----------------------------------------------------------
> =
> (Updated Nov. 23, 2012, 10:11 p.m.)
> =
> =
> Review request for Plasma and Kai Uwe Broulik.
> =
> =
> Description
> -------
> =
> This adjustst the DeviceNotifier Plasmoid to scale nicely with high resol=
ution screens.
> =
> The patch changes also one label from wordWrapping to clipping behavior b=
ecause otherwise it messes up the layout.
> =
> =
> Diffs
> -----
> =
> plasma/generic/applets/devicenotifier/package/contents/ui/ActionItem.qm=
l 3087a07 =
> plasma/generic/applets/devicenotifier/package/contents/ui/DeviceItem.qm=
l 1fab0ef =
> plasma/generic/applets/devicenotifier/package/contents/ui/devicenotifie=
r.qml f8728d0 =
> =
> Diff: http://git.reviewboard.kde.org/r/107428/diff/
> =
> =
> Testing
> -------
> =
> Tested on High res screen at full DPI and at regular DPI values
> =
> =
> Thanks,
> =
> Michael Zanetti
> =
>
[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://git.reviewboard.kde.org/r/107428/">http://git.reviewboard.kde.org/r/107428/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 26th, 2012, 9:35 a.m., <b>Marco \
Martin</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;">This is really an issue that needs addressing, thanks for taking time \
for it.
i'm very in favor of removing all those hardcoded sizes from the code.
however i see two problems with that approach
1) icons are cleanly painted only when they have a size that is the "right" \
one (16,22,32,48,64,128,512 exported in qml with theme.smallIconSize, \
smallMediumIconSize etc) for big sizes doesn't matter if the pixmap ends up \
being scaled, but small icons really look horrible when scaled (and in the new code \
there is no check the size ends up being one of those).
2) it's anyways quite arbitrary, so i can easily see each plasmoid using more or \
less its own logic, with the end result looking a bit like a patchwork.
the"proper" solution i think is to export in Theme the configurable \
KIconLoader sizes, (Desktop, Toolbar, SmallIcons etc) and those can be controlled \
by a kcm, so will be properly set on higher resolution displays</pre> </blockquote>
<p>On November 26th, 2012, 10:54 a.m., <b>Marco Martin</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;">just added the bindings: \
use theme.iconSizes.dialog for most icons
theme.iconSizes.toolbar for unmount
theme.iconSizes.small for all that is normally 16x16
so the icon settings in systemsettings will affect those icons</pre>
</blockquote>
<p>On November 26th, 2012, 12:43 p.m., <b>Michael Zanetti</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;">Hey Marco,
I'm not sure if this is really a good idea. Using KIconLoader::IconSize() is a \
good idea for toolbars and such. However, in a plasma layout I don't think its a \
good idea to let the user change the size of the icons. It can and will mess up the \
layout. For example I use the same size for "Toolbar" and \
"Dialog" because I like big toolbars. I certainly don't want to the \
unmount icon to be that huge in the devicemanager plasmoid because if that...
regarding the scaling: I was thinking that Plasma uses SVG icons... Reading throught \
the QIconItem code I see now that it doesn't. In that case its obviously not good \
to freely scale it around. Maybe it should be replaced by PlasmaCore.SvgItem?</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;">what is important is \
that the sizes of the icons are something that is a) consistent everywhere (not \
having each applet decide with its criteria) b) be something that can depend from \
dpi, those being settable are fine, since will be the same as things around \
application
now, indeed can be not a good idea to have the unmount icon as toolbar, since the \
size can be messed up (so keeping a size relative to the size used to the main icon, \
that i think dialog is ok)
This i think makes the case for exposing a currently internal component that i'm \
using in buttons and toolbuttons called IconLoader the behavior is:
* use an svg if it does exist in the theme: requiring to have a complete svg theme \
(and with enough quality) is neither feasible nor desiderable
* fallback to a qicon if the svg is not found
* always paint in the nearest (smaller) kiconloader standard size (even for svg, for \
consistency in layouts and because they aren't as scalable as they seem, if their \
shape edges are not grid aligned they look horrible)
* will probably also have to directly support the on mouse over highlight animation \
or people will keep using the horrible PlasmaWidgets.IconWidget </pre>
<br />
<p>- Marco</p>
<br />
<p>On November 23rd, 2012, 10:11 p.m., Michael Zanetti wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/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 and Kai Uwe Broulik.</div>
<div>By Michael Zanetti.</div>
<p style="color: grey;"><i>Updated Nov. 23, 2012, 10:11 p.m.</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;">This adjustst the DeviceNotifier Plasmoid to scale nicely with high \
resolution screens.
The patch changes also one label from wordWrapping to clipping behavior because \
otherwise it messes up the layout.</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 on High res screen at full DPI and at regular DPI values</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>plasma/generic/applets/devicenotifier/package/contents/ui/ActionItem.qml <span \
style="color: grey">(3087a07)</span></li>
<li>plasma/generic/applets/devicenotifier/package/contents/ui/DeviceItem.qml <span \
style="color: grey">(1fab0ef)</span></li>
<li>plasma/generic/applets/devicenotifier/package/contents/ui/devicenotifier.qml \
<span style="color: grey">(f8728d0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107428/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