[Folder view]Make shadow scale aware
ClosedPublic

Authored by gvgeo on Feb 11 2020, 11:14 AM.

Details

Summary

Added devicePixelRatio to shadow's radius.

Test Plan

Check the shadow of items on the desktop.
Compare scale 100% vs big display scale (e.g. 300%).
Before: Shadow is invisible.
After: Visible shadow at every display scale.

Before vs After
Scale 300% and 100%

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gvgeo created this revision.Feb 11 2020, 11:14 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 11 2020, 11:14 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gvgeo requested review of this revision.Feb 11 2020, 11:14 AM
gvgeo edited the test plan for this revision. (Show Details)Feb 11 2020, 11:17 AM
gvgeo updated this revision to Diff 75453.Feb 11 2020, 11:30 AM
gvgeo edited the test plan for this revision. (Show Details)

Enclosed in Math.round()
Although works fine without it.

Oh gosh, are shadows not scale-aware anywhere? We'd have to replicate this code in a lot of places... might it make more sense to fix this in Qt itself?

gvgeo updated this revision to Diff 75476.Feb 11 2020, 3:17 PM
gvgeo added a reviewer: VDG.

Rebase.

gvgeo added a comment.Feb 11 2020, 4:44 PM

Shadows look fine in wayland. Like many other elements, the problem is with X11.

gvgeo added a comment.Feb 18 2020, 4:53 PM

Can I close this patch?

I don't know enough to say whether it makes sense to fix this here, or in Qt. I'd like a Plasma or maybe KWin person to weigh in.

apol added a subscriber: apol.Feb 18 2020, 5:51 PM

+1 patch makes sense to me

containments/desktop/package/contents/ui/FolderItemDelegate.qml
433

Why's the + 1?

gvgeo added inline comments.Feb 18 2020, 6:00 PM
containments/desktop/package/contents/ui/FolderItemDelegate.qml
433

https://doc.qt.io/qt-5/qml-qtgraphicaleffects-dropshadow.html#samples-prop
Ideally, this value should be twice as large as the highest required radius value plus

gvgeo added inline comments.Feb 18 2020, 6:03 PM
containments/desktop/package/contents/ui/FolderItemDelegate.qml
433

And I ate the "one"
Ideally, this value should be twice as large as the highest required radius value plus one

gvgeo marked 3 inline comments as done.Feb 18 2020, 6:07 PM
apol accepted this revision.Feb 18 2020, 6:39 PM
This revision is now accepted and ready to land.Feb 18 2020, 6:39 PM
ngraham accepted this revision.Feb 18 2020, 6:41 PM

So I guess we do need to do this in all places where we use drop shadows, urgh.

Any chance you'd be interested in tackling that, @gvgeo?

As for this patch, seems like a candidate for the stable branch.

davidedmundson requested changes to this revision.Feb 18 2020, 7:06 PM
This revision now requires changes to proceed.Feb 18 2020, 7:06 PM

That commit has been there since 5.12.5

So I imagine you have it on your system.
It's possible it doesn't work correctly, but it needs some investigation.

gvgeo added a comment.Feb 18 2020, 7:31 PM

I use arch's Qt 5.14.1
Any chance I only have this problem? My build is not exactly stable.

Btw found some places that use radius spallSpacing or gridUnit, like knewstuff. I wonder how that look, with that commit.

I don't know

You can find

./qml/QtGraphicalEffects/private/DropShadowBase.qml

somewhere in /usr on your local system,

add in some
console.log lines and see what's happening.

gvgeo added a comment.Feb 18 2020, 8:09 PM

It was more of a general question...

Anyway I tried, nothing happens. I Believe it uses the qmlc files.

I don't know

You can find

./qml/QtGraphicalEffects/private/DropShadowBase.qml

somewhere in /usr on your local system,

add in some
console.log lines and see what's happening.

Screen.devicePixelRatio is always 1, while units.devicePixelRatio is correct.

Checked in ./qml/QtGraphicalEffects/DropShadow.qml and other qml in ./qml/ including PC3 files.

I have the same problem with the distro's build. So I guess this is a problem.

plasmashell doesn't use Qt scaling, so using units.devicePixelRatio is correct. It will be 2 when not used, and then divide by Qt scaling so Qt scales back up resulting in the same effect.

davidedmundson accepted this revision.Feb 19 2020, 12:42 PM

Oh, of course

This revision is now accepted and ready to land.Feb 19 2020, 12:42 PM

So will we need to remove this multiplying by units.devicePixelRatio bit if and when we finally make plasmashell respect Qt scaling and fix https://bugs.kde.org/show_bug.cgi?id=356446?

we would not need to remove it

This revision was automatically updated to reflect the committed changes.

You've got so many landed patches under your belt now that I think it's time to apply for a developer account. :)

https://community.kde.org/Infrastructure/Get_a_Developer_Account