[Differential] [Changed Subscribers] D3892: [Icon Item] Support non-square icons

[mart (Marco Martin)]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in iconitem.cpp:325
> This makes no sense.
> You can't round it to icon sizes *after* scaling, it means the shorter size 
> is always just wrong.
> 
> If we do merge this patch, you want:
> 
> m_iconPixmap.size().scaled(QSize(Units.roundtoIconSize(actualContainerSize.width),
>  Units.roundToIconSize(actualContainerSize.height)

the final size should be:
the *biggest* between width and height should be rounded to icon size
the other dimension should be scaled accordingly keeping the same proportions, 
so for example, if the original is
200*100 pixels, and you paint of a 32x32 icon canvas, the final image should be 
32x16 (possibly painted centered)

REPOSITORY
  R242 Plasma Frameworks

REVISION DETAIL
  https://phabricator.kde.org/D3892

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, hein
Cc: mart, davidedmundson, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D3892: [Icon Item] Support non-square icons

[davidedmundson (David Edmundson)]

davidedmundson added inline comments.

INLINE COMMENTS

> iconitem.cpp:325
> +const QSize  = 
> m_iconPixmap.size().scaled(actualContainerSize, Qt::KeepAspectRatio);
> +return QSize(Units::roundToIconSize(paintedSize.width()), 
> Units::roundToIconSize(paintedSize.height()));
>  }

This makes no sense.
You can't round it to icon sizes *after* scaling, it means the shorter size is 
always just wrong.

If we do merge this patch, you want:

m_iconPixmap.size().scaled(QSize(Units.roundtoIconSize(actualContainerSize.width),
 Units.roundToIconSize(actualContainerSize.height)

> iconitem.cpp:377
>  if (m_sizeChanged) {
> -const int iconSize = 
> Units::roundToIconSize(qMin(boundingRect().size().width(), 
> boundingRect().size().height()));
> -const QRect destRect(QPointF(boundingRect().center() - 
> QPointF(iconSize/2, iconSize/2)).toPoint(),
> - QSize(iconSize, iconSize));
> -
> +const QSize  = paintedSize();
> +const QRect destRect(QPointF(boundingRect().center() - 
> QPointF(newSize.width(), newSize.height()) / 2).toPoint(), newSize);

You're using references a lot in a few patches that don't make any sense.

paintedSize returns a new QSize object; you're keeping a reference to a value 
that would just be immediately discarded. This doesn't result in an 
optimisation but instead doing something that would crash if it wasn't for a 
magic C++ feature where it lengthens the lifespan of the object.

REPOSITORY
  R242 Plasma Frameworks

REVISION DETAIL
  https://phabricator.kde.org/D3892

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, hein
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas