D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:4056e24b353d: [Folder view]Make shadow scale aware 
(authored by gvgeo, committed by ngraham).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27316?vs=75476=76007

REVISION DETAIL
  https://phabricator.kde.org/D27316

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderItemDelegate.qml

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

ngraham added a comment.


  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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[David Edmundson]

davidedmundson added a comment.


  we would not need to remove it

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  Oh, of course

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Kai Uwe Broulik]

broulik added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: broulik, davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added a comment.


  In D27316#613771 , @davidedmundson 
wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added a comment.


  It was more of a general question...
  
  Anyway I tried, nothing happens. I Believe it uses the qmlc files.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham, davidedmundson
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[David Edmundson]

davidedmundson added a comment.


  See 
https://code.qt.io/cgit/qt/qtgraphicaleffects.git/commit/src/effects/private/DropShadowBase.qml?id=f9664053132affe48a069235eb6561a32d38b2a7
  
  You're about double it twice

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham
Cc: davidedmundson, apol, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg, apol, ngraham
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added inline comments.

INLINE COMMENTS

> FolderItemDelegate.qml:435
> +radius: Math.round(5 * units.devicePixelRatio)
> +samples: radius * 2 + 1
>  spread: 0.05

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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added inline comments.

INLINE COMMENTS

> FolderItemDelegate.qml:435
> +radius: Math.round(5 * units.devicePixelRatio)
> +samples: radius * 2 + 1
>  spread: 0.05

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`

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Aleix Pol Gonzalez]

apol added a comment.


  +1 patch makes sense to me

INLINE COMMENTS

> FolderItemDelegate.qml:435
> +radius: Math.round(5 * units.devicePixelRatio)
> +samples: radius * 2 + 1
>  spread: 0.05

Why's the + 1?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added a comment.


  Can I close this patch?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo updated this revision to Diff 75476.
gvgeo added a reviewer: VDG.
gvgeo added a comment.


  Rebase.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27316?vs=75453=75476

BRANCH
  shadow2 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27316

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderItemDelegate.qml

To: gvgeo, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27316

To: gvgeo, #plasma
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo updated this revision to Diff 75453.
gvgeo edited the test plan for this revision.
gvgeo added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27316?vs=75450=75453

BRANCH
  shadow

REVISION DETAIL
  https://phabricator.kde.org/D27316

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderItemDelegate.qml

To: gvgeo, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27316: [Folder view]Make shadow scale aware

[George Vogiatzis]

gvgeo created this revision.
gvgeo added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
gvgeo requested review of this revision.

REVISION 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: Proper shadow at every display scale.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  shadow

REVISION DETAIL
  https://phabricator.kde.org/D27316

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderItemDelegate.qml

To: gvgeo, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart