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

[davidedmundson (David Edmundson)]

davidedmundson added inline comments.

INLINE COMMENTS

> iconitem.cpp:325
> +    const QSize &paintedSize = 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 &newSize = 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170101/454df702/attachment-0001.html>