Differential D29408

Wayland: allow to take immediate and delayed screenshots
ClosedPublic
Actions

Authored by meven on May 4 2020, 1:53 PM.

Details

Reviewers
davidre
davidedmundson
apol
bport
ngraham
Summary

The feature adds a runtime dependency on D29407 Plasma 5.20.

BUG: 414532
BUG: 407489
FIXED-IN: 5.20

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D29408_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27129
Build 27147: arc lint + arc unit
meven created this revision.May 4 2020, 1:53 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptMay 4 2020, 1:53 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
meven requested review of this revision.May 4 2020, 1:53 PM
meven planned changes to this revision.May 5 2020, 4:21 PM

Spectacle must have its executable in its desktop entry Exec field.
To manage this, spectacle must use KDBusService::Unique instead of Multiple.
This requires some refactoring.

meven edited the summary of this revision. (Show Details)May 7 2020, 7:28 AM
davidre added a comment.May 11 2020, 8:16 AM

Why do we introduce the distinction between delayed and immediate? Is there a difference between delayed with 0 seconds and immediate? Also every screenshot triggered by the gui arrives at spectacle core with delay <= 0 because ksmainwindow waits the delay for the taskmanager animation and animating the button.

meven added a comment.May 11 2020, 9:07 AM
In D29408#668033, @davidre wrote:

Why do we introduce the distinction between delayed and immediate? Is there a difference between delayed with 0 seconds and immediate? Also every screenshot triggered by the gui arrives at spectacle core with delay <= 0 because ksmainwindow waits the delay for the taskmanager animation and animating the button.

I need to fix this.

The X-KDE-Wayland-Interfaces is no more.

This will be rebased after dependency :
D29487

meven edited the summary of this revision. (Show Details)May 12 2020, 3:15 PM
meven updated this revision to Diff 83223.Jun 5 2020, 10:43 AM

Clean up delayed shutterMode

meven added a comment.EditedJun 5 2020, 10:50 AM

This is ready to land except I need either to :

  • wait after 20.08 is released since this fix is dependent on KWin D29407 that won't be available before Plasma 5.20.
  • or add a runtime plasma version check

Adding the version check has my preference but I don't know how to add a runtime plasma version check.

Merging separately the line X-KDE-DBUS-Restricted-Interfaces=org_kde_kwin_effect-screenshot is enough to not break current functionality once D29407 has landed, it will already allow to take screenshots without click confirmation.
Separate merge request : https://invent.kde.org/graphics/spectacle/-/merge_requests/3

(and the .arcconfig file will be removed before merging ;) )

.arcconfig
2 ↗(On Diff #83223)

Add to re-add this to post a diff, will remove before merging.

pejakm added a subscriber: pejakm.Jun 6 2020, 12:42 PM
apol accepted this revision.Jun 6 2020, 9:08 PM
This revision is now accepted and ready to land.Jun 6 2020, 9:08 PM
meven updated this revision to Diff 83247.Jun 8 2020, 7:36 AM

Removed .arcconfig, rebased

meven updated this revision to Diff 83250.Jun 8 2020, 3:09 PM

Use KDBusAddon' PlasmaVersion

meven edited the summary of this revision. (Show Details)Jun 9 2020, 3:44 PM
meven updated this revision to Diff 83254.Jun 9 2020, 3:45 PM

Avoid adding the dependency to spectacle

meven updated this revision to Diff 83255.Jun 9 2020, 4:55 PM

Check only once the plasma version when needed

meven added a comment.Jun 9 2020, 4:56 PM

I am abandonning https://invent.kde.org/frameworks/kdbusaddons/-/merge_requests/1/diffs.

Adding the plasma version check here is not a big deal.

The code is pretty much ready and will land along with D29407 shortly unless I have some feedback.

davidre added a comment.Jun 10 2020, 7:44 AM

Very short nice!

davidre accepted this revision.Jun 10 2020, 7:47 AM

I think we shouldn't use QDBUsInterface but given that we already do this is probably ok

meven added a comment.Jun 10 2020, 7:56 AM
In D29408#674962, @davidre wrote:

I think we shouldn't use QDBUsInterface but given that we already do this is probably ok

The alternative is just less documented AFAICT.
IRC the alternative is about adding xml files that generate code.

davidre added a comment.Jun 10 2020, 7:57 AM
In D29408#674966, @meven wrote:
In D29408#674962, @davidre wrote:

I think we shouldn't use QDBUsInterface but given that we already do this is probably ok

The alternative is just less documented AFAICT.
IRC the alternative is about adding xml files that generate code.

I think you can also do a blind call

meven updated this revision to Diff 83258.Jun 10 2020, 1:07 PM

Make raw DBus call

apol added inline comments.Jun 10 2020, 11:55 PM
src/Platforms/PlatformKWinWayland.cpp
112

probably doesn't make a difference because it's not called often but you can call splitRef and skip allocating a few of strings you don't need.

136

Should probably make sure that it's Plasma 5, or when we release Plasma 6 this will stop working.

meven updated this revision to Diff 83260.Jun 11 2020, 7:22 AM
meven marked 2 inline comments as done.

Check Plasma major version, use splitRef

meven added inline comments.Jun 11 2020, 9:07 AM
src/Platforms/PlatformKWinWayland.cpp
112

Didn't know about splitRef, what a nice function, that makes split so much more efficient

meven edited the summary of this revision. (Show Details)Jun 16 2020, 5:04 PM
meven closed this revision.Jun 16 2020, 5:06 PM

Revision Contents
Changeset List

Diff 83260

View Options

src/Platforms/PlatformKWinWayland.cpp

Loading...