Switch to using Kirigami's ShadowedRectangle
Needs ReviewPublic

Authored by leinir on Mon, Mar 23, 2:52 PM.

Details

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)

Screenshots component without changes:

Screenshots component with changes:

Diff Detail

Repository
R304 KNewStuff
Branch
switch-to-kirigami-shadowedrectangle (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24197
Build 24215: arc lint + arc unit
leinir created this revision.Mon, Mar 23, 2:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Mar 23, 2:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
leinir requested review of this revision.Mon, Mar 23, 2:52 PM

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

leinir updated this revision to Diff 78341.Tue, Mar 24, 9:44 AM
  • Increase Kirigami runtime dep version to 2.12
broulik added inline comments.Tue, Mar 24, 9:51 AM
src/qtquick/qml/private/EntryScreenshots.qml
133–135

Not needed given you anchors.fill

135

What's this for?

138

You used largeSpacing as verticalOffset before

139

You used 12 as radius before

src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
123

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

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?

leinir marked 5 inline comments as done.Tue, Mar 24, 9:59 AM
leinir added inline comments.
src/qtquick/qml/private/EntryScreenshots.qml
133–135

Ah yes, quite :)

135

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 :)

src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
123

Less magic numbers are a nice thing ;)

124

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

leinir added a reviewer: VDG.Tue, Mar 24, 10:03 AM
leinir marked 3 inline comments as done.

(pending some screenshots of the visible change)

src/qtquick/qml/private/EntryScreenshots.qml
138

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 :)

leinir edited the test plan for this revision. (Show Details)Tue, Mar 24, 10:06 AM
leinir updated this revision to Diff 78342.Tue, Mar 24, 10:06 AM
  • Less magic numbers
  • Remove some unneeded bits

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

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?)

ahiemstra added inline comments.Tue, Mar 24, 4:07 PM
src/qtquick/qml/private/EntryScreenshots.qml
134

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

leinir updated this revision to Diff 78430.Wed, Mar 25, 8:29 AM

Address comment by @ahiemstra

  • Remove some unneeded values
leinir added inline comments.Wed, Mar 25, 9:04 AM
src/qtquick/qml/private/EntryScreenshots.qml
134

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

leinir marked 8 inline comments as done.Wed, Mar 25, 9:05 AM