Disabling this property makes it possible to show icon of arbitrary size.
Details
Details
- Reviewers
sebas - Group Reviewers
Plasma - Commits
- R242:af2b27d1b89b: IconItem: Add roundToIconSize property
Test passed
Diff Detail
Diff Detail
- Repository
- R242 Plasma Framework (Library)
- Branch
- arcpatch-D4689
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Makes sense and the code is fine +1 from me.
I assume you have an intended usage in mind?
| src/declarativeimports/core/iconitem.h | ||
|---|---|---|
| 101 ↗ | (On Diff #11551) | add the line "the default is true" |
Comment Actions
Yes, I want to use it in plasma-pa applet for volume indicator icons (the small icon next to slider). Currently, they are too small but next round icon size is already too big. This change will make it possible to make them just few pixels bigger.
Comment Actions
can you post a screenshot on how they would look? they may end up looking a bit blurry
Comment Actions
they may end up looking a bit blurry
They are SVGs in Breeze, so they won't look blurry, with other icon themes it may be an issue. But then again, the same issue is already there when you request big icon (for which roundToIconSize() returns the passed size ~ >128 pixels
Comment Actions
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.
| autotests/iconitemtest.cpp | ||
|---|---|---|
| 524 | 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. | |
| src/declarativeimports/core/iconitem.cpp | ||
| 313 | 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. | |
| src/declarativeimports/core/iconitem.h | ||
| 147 | 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. | |
Comment Actions
Almost good, you can add the signalspy and then ship it from my side.
| autotests/iconitemtest.cpp | ||
|---|---|---|
| 526 ↗ | (On Diff #11551) | Might as well check for the roundToIconSizeChanged signal here as well. We should test what we reasonably can, and that's an easy one. |
| src/declarativeimports/core/iconitem.h | ||
| 147 | Right. :) | |