[Differential] [Requested Changes] D4689: IconItem: Add roundToIconSize property

[Sebastian Kügler]

sebas requested changes to this revision.
sebas added a comment.
This revision now requires changes to proceed.


  I think a problem with using roundToIconSize as isolated property is that it really isn't. It has intended effects on the sizing of the item, but the current version of the patch doesn't reflect that. I think it needs to trigger a series of size change signals. See my comments inline.

INLINE COMMENTS

> iconitemtest.cpp:524
> +
> +    item->setProperty("roundToIconSize", false);
> +

Just checking the values here is not enough, the property change results in changes in paintedWidth, but we're currently not signalling that these props have changed. See also my comment below.

> iconitem.cpp:313
> +}
> +void IconItem::setRoundToIconSize(bool roundToIconSize)
> +{

Changing roundToIconSize may change the size of the icon, so should we trigger size changes (paintedWidth / paintedHeight / boundingRect likely? Perhaps others.) and re-rendering here as well? Right now, judging from the code, changing this property mid-flight is broken. We may even have to go as far as loading a new pixmap (in loadPixmap() from a quick glance).

Please also add unit tests for the effects on the iconitem's size properties.

> iconitem.h:147
>  
> +    bool isRoundingToIconSize() const;
> +    void setRoundToIconSize(bool roundToIconSize);

bool roundToIconSize() const;

we generally don't use "is" in new code where we can avoid it, and the ing makes this even more inconsistent. I'd much prefer the above suggestion.

Please also add api docs, at least for new code.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: drosca, #plasma, sebas
Cc: sebas, mart, davidedmundson, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170226/f887d4c0/attachment.html>