[effects/sheet] Modernize code
ClosedPublic

Authored by zzag on Aug 8 2018, 9:22 AM.

Details

Summary
  • Use new connect syntax
  • Fix coding style
  • Port to TimeLine
  • Delete unused includes
  • Use interpolate helper
  • Drop WindowInfo class

Behavior of this effect hasn't been changed.

Test Plan

Opened/closed an "Open File" dialog, it still flies in/out.

Diff Detail

Repository
R108 KWin
Branch
rewrite-sheet-effect
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1624
Build 1642: arc lint + arc unit
zzag created this revision.Aug 8 2018, 9:22 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 8 2018, 9:22 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 8 2018, 9:22 AM
zzag added a comment.Aug 8 2018, 9:23 AM

5.14 would include pretty big number of effect changes so I think this patch can wait for 5.15. It's not that urgent.

anthonyfieroni added inline comments.
effects/sheet/sheet.h
73–82

I think, it's not mandatory, but since you change code style, does it better for function implementation to go in cpp files only.

zzag added inline comments.Aug 8 2018, 12:23 PM
effects/sheet/sheet.h
73–82

AFAIK, all effects implement getters for properties in header files. So, I don't want this effect to deviate from the rest.

zzag updated this revision to Diff 39317.Aug 8 2018, 2:58 PM

Rebase.

davidedmundson accepted this revision.Aug 11 2018, 10:44 PM
davidedmundson added a subscriber: davidedmundson.

Ship it!

effects/sheet/sheet.h
26

Not that I mind, but you just made a whole commit about pointless comments...

This revision is now accepted and ready to land.Aug 11 2018, 10:44 PM
zzag added inline comments.Aug 12 2018, 6:52 AM
effects/sheet/sheet.h
26

I think that's fine in this case. It would tell people where to put includes. E.g. sometimes I see the following pattern

Qt includes

KF includes

Qt includes

FWIW, in most cases, those includes are not sorted so there is a chance of adding duplicate includes

Also, most of KWin codebase does the same(adds include comments).

This revision was automatically updated to reflect the committed changes.