[KStandardItemListWidget] Avoid needless image resizing
ClosedPublic

Authored by broulik on Mar 14 2018, 9:37 AM.

Details

Summary

There were a couple of mistakes when porting it to use devicePixelRatio resulting in it constantly recreating pixmaps when on a high dpi screen. There can also be rounding errors when scaling pixmaps when keeping aspect ratio, allow a 1px size difference.

Test Plan

Only Folder thumbnails are still scaled up but that needs further investigation. Regular image thumbnails are no longer scaled due to off-by-1 rounding error

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik requested review of this revision.Mar 14 2018, 9:37 AM
broulik created this revision.
davidedmundson requested changes to this revision.Mar 20 2018, 2:35 PM
davidedmundson added inline comments.
src/kitemviews/kstandarditemlistwidget.cpp
1014–1015

It's best to avoid the term "scale".

It could be scaling from logical to device pixels, or scailng from device to logical.
So it tells me literally nothing.

Sticking to either the term "logical" or "device" seems to work best.

1385

This is overly complex.

We have the pixmap at size in device pixels (logical size * DPR)

When we divide back to logical pixels, we lose resolution. So....don't.

You can do it all in device pixels, which is the part that's relevant here.

if (m_scaledPixmapSize * pixmap.devicePixelRatio != pixmap.size()) {

}

This revision now requires changes to proceed.Mar 20 2018, 2:35 PM
broulik added inline comments.Mar 21 2018, 11:03 AM
src/kitemviews/kstandarditemlistwidget.cpp
1385

Even if I do it here, I still get cases where requested size QSize(256, 192) is off by one pixel to QSize(257, 192)

I think the scale(Qt::KeepAspectRatio) causes rounding errors... somewhere.

Restricted Application added a project: Dolphin. · View Herald TranscriptJul 9 2018, 12:38 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
broulik updated this revision to Diff 37561.Jul 11 2018, 1:27 PM
broulik edited the test plan for this revision. (Show Details)
  • Fix suggested by David
davidedmundson accepted this revision.Jul 11 2018, 1:28 PM

Needs a comment in the code explaining why.

This revision is now accepted and ready to land.Jul 11 2018, 1:28 PM