[Lock Screen] Fix album art binding loop

Authored by filipf on Fri, Nov 8, 6:40 PM.


Group Reviewers

Album art was patched in 5.16.5 to use PreserveAspectFit, but the code for sourceSize was still applying the width and the height of the container, which do not neccesarily match image dimensions anymore.

Using paintedWidth and paintedHeight should now reflect real image dimensions.

BUG: 413087
FIXED-IN: 5.17.3

Test Plan

I can't reproduce the bug so a user on the bug tracker confirmed this fixes it for them.

Diff Detail

R120 Plasma Workspace
fix-albumart-bindingloop (branched from master)
No Linters Available
No Unit Test Coverage
Build Status
Buildable 18632
Build 18650: arc lint + arc unit
filipf created this revision.Fri, Nov 8, 6:40 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Nov 8, 6:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Fri, Nov 8, 6:40 PM
filipf edited the summary of this revision. (Show Details)Fri, Nov 8, 6:41 PM
filipf edited the test plan for this revision. (Show Details)

Painted size depends on the aspect ratio of the loaded image. I think it should just be hard-coded to some icon size or grid unit rather than depend on the highly dynamic font rendering (which governs its size effectively as it's the layout's size)

davidedmundson requested changes to this revision.Sat, Nov 9, 1:34 PM

Can you talk me through how this creates a loop?

This revision now requires changes to proceed.Sat, Nov 9, 1:34 PM
davidedmundson added inline comments.Sat, Nov 9, 2:25 PM

Aha, because of this!
A change in aspect ratio affects the width, which reloads the image, which changes the height, which changes the width

paintedWidth still seems wrong though, it'll only be evaluated after it loads the image at which point the sourceSize is too late.

filipf added a comment.Sat, Nov 9, 5:41 PM

Yeah you guys are right. I tested this further and still got some binding loop errors with paintedWidth/Height.

If you have a solution feel free to commandeer the revision to speed things up, this seems like a very important bugfix.

davidedmundson added inline comments.Sun, Nov 10, 5:56 PM

We will display at a fixed height assigned from the layout and we adjust our item width to match the aspect ratio ourselves.

Therefore, it only really makes sense to set a height on sourceSize
i.e sourceSize.height: height

If only one dimension of the size is set to greater than 0, the other dimension is set in proportion to preserve the source image's aspect ratio.

which is what we want

Image loads, we resize our width, but we won't re-evaluate this, so no loop.

filipf abandoned this revision.Mon, Nov 11, 2:53 PM

continued in D25252