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
