D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:aa29f344928e: Switch to using Kirigamis 
ShadowedRectangle (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28220?vs=78430=79109

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Kai Uwe Broulik]

broulik accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  switch-to-kirigami-shadowedrectangle (branched from master)

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Arjen Hiemstra]

ahiemstra added a comment.


  It's fine with me but I'm going to leave the final call to @broulik since he 
did a more in-depth review.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added a comment.


  Ping? Not super critical, i guess, but it just feels sad to have things 
pending for weeks...

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir marked 8 inline comments as done.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in EntryScreenshots.qml:134
> The x and y offsets should be 0 by default, so there's little reason to 
> specify that I'd say.

Ha, yes, quite, setting defaults does seem a little silly ;)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 78430.
leinir added a comment.


  Address comment by @ahiemstra
  
  - Remove some unneeded values

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28220?vs=78342=78430

BRANCH
  switch-to-kirigami-shadowedrectangle (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> EntryScreenshots.qml:134
> +Kirigami.Theme.colorSet: Kirigami.Theme.View
> +shadow.xOffset: 0
> +shadow.yOffset: 0

The x and y offsets should be 0 by default, so there's little reason to specify 
that I'd say.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added a subscriber: davidedmundson.
leinir added a comment.


  In D28220#633557 , @ngraham wrote:
  
  > Just an observation, we should maybe consider making a shared component for 
this screenshots list so that Discover can use it too.
  
  
  i would like that :) Perhaps something for kirigami-addons? (was that 
@davidedmundson or someone else?)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Nathaniel Graham]

ngraham added a comment.


  Just an observation, we should maybe consider making a shared component for 
this screenshots list so that Discover can use it too.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 78342.
leinir added a comment.


  - Less magic numbers
  - Remove some unneeded bits

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28220?vs=78341=78342

BRANCH
  switch-to-kirigami-shadowedrectangle (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir edited the test plan for this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added a reviewer: VDG.
leinir marked 3 inline comments as done.
leinir added a comment.


  (pending some screenshots of the visible change)

INLINE COMMENTS

> broulik wrote in EntryScreenshots.qml:138
> You used `largeSpacing` as `verticalOffset` before

Hm, on second thought, it looks /terrible/ doing it that way... i'm going to 
call consistency on this one and tag in the VDG, thanks for making me notice :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir marked 5 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> broulik wrote in EntryScreenshots.qml:133
> Not needed given you `anchors.fill`

Ah yes, quite :)

> broulik wrote in EntryScreenshots.qml:135
> What's this for?

That's a good question, really - i found that in the KCM GridDelegate and just 
sort of left it in... but yup, probably not needed :)

> broulik wrote in TileDelegate.qml:123
> `shadow.size` corresponds to the old `radius`, so you'd want to set 
> `Kirigami.Units.largeSpacing` here

Less magic numbers are a nice thing ;)

> broulik wrote in TileDelegate.qml:124
> `#80` used in the old code is 128, i.e. `0.5` alpha, unless you want to make 
> it look consistently with the shadows used elsewhere?

Yeah, that was for consistency, but well spotted :)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> EntryScreenshots.qml:133
>  anchors.fill: thumbnail
> -verticalOffset: Kirigami.Units.largeSpacing
> -horizontalOffset: 0
> -radius: 12.0
> -samples: 25
> -color: Kirigami.Theme.disabledTextColor
> -cached: true
> +width: thumbnail.width;
> +height: thumbnail.height;

Not needed given you `anchors.fill`

> EntryScreenshots.qml:135
> +height: thumbnail.height;
> +Kirigami.Theme.inherit: false
> +Kirigami.Theme.colorSet: Kirigami.Theme.View

What's this for?

> EntryScreenshots.qml:138
> +shadow.xOffset: 0
> +shadow.yOffset: 0
> +shadow.size: Kirigami.Units.largeSpacing

You used `largeSpacing` as `verticalOffset` before

> EntryScreenshots.qml:139
> +shadow.yOffset: 0
> +shadow.size: Kirigami.Units.largeSpacing
> +shadow.color: Qt.rgba(0, 0, 0, 0.3)

You used `12` as `radius` before

> TileDelegate.qml:123
> +shadow.yOffset: 0
> +shadow.size: 10
> +shadow.color: Qt.rgba(0, 0, 0, 0.3)

`shadow.size` corresponds to the old `radius`, so you'd want to set 
`Kirigami.Units.largeSpacing` here

> TileDelegate.qml:124
> +shadow.size: 10
> +shadow.color: Qt.rgba(0, 0, 0, 0.3)
> +}

`#80` used in the old code is 128, i.e. `0.5` alpha, unless you want to make it 
look consistently with the shadows used elsewhere?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 78341.
leinir added a comment.


  - Increase Kirigami runtime dep version to 2.12

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28220?vs=78290=78341

BRANCH
  switch-to-kirigami-shadowedrectangle (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added reviewers: ahiemstra, broulik, mart.
leinir added a comment.


  tagging in a couple of people who definitely know things about the new thing 
:)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added reviewers: KNewStuff, Frameworks, Plasma.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
leinir requested review of this revision.

REVISION SUMMARY
  This switches from using the QML DropShadow to using the
  new, high efficiency ShadowedRectangle in Kirigami.
  
  - Modify (and equalise logic) in GridTileDelegate
  - Clean up EntryScreenshots, and use ShadowedRectangle
  - Also use ShadowedRectangle in the bigpreview and tile delegates

TEST PLAN
  No visible changes (as expected) from the switch
  Minor visible change from the EntryScreenshots modifications (now correctly 
shows the shadows all the way around)

REPOSITORY
  R304 KNewStuff

BRANCH
  switch-to-kirigami-shadowedrectangle (branched from master)

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

AFFECTED FILES
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml

To: leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns