ScreenShotEffect: Add a shouldReturnNativeSize argument to screenshotFullscreen
ClosedPublic

Authored by meven on Apr 23 2020, 9:15 AM.

Details

Summary

Add a shouldReturnNativeSize to screenshotFullscreen so that KWin can export
screenshot that are screen pixel accurate.

Useful for spectacle to be able to do rectangular selection kind of screenshot.

Test Plan

Example of a top screen using a 1.25 scale ratio being export in native resolution


(The top screen has a bigger size than its virtual geometry and next screen doesn't overlap)

Example of the same screen in virtual resolution:

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Apr 23 2020, 9:15 AM
Restricted Application added a project: KWin. · View Herald TranscriptApr 23 2020, 9:15 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Apr 23 2020, 9:15 AM
meven edited the test plan for this revision. (Show Details)Apr 23 2020, 9:21 AM
broulik added inline comments.
effects/screenshot/screenshot.h
104

I think we could consider this interface public API and adding new arguments changes the DBus signature iirc.
Perhaps we should just add a new overload.

broulik added inline comments.Apr 23 2020, 9:30 AM
effects/screenshot/screenshot.cpp
198

Don't iterate keys(). Use iterators.

207

qFuzzyCompare

214

auto

davidedmundson added a comment.EditedApr 23 2020, 9:34 AM

I think we could consider this interface public API and adding new arguments changes the DBus signature iirc.
Perhaps we should just add a new overload.

You're right that DBus would fail if you don't call with the right number of arguments to match a method signature, but you underestimate QtDBus. It's /amaaaazing/

It knows some arguments are defaults, for the existing code it currently generates:

<method name="screenshotScreen">
  <arg name="fd" type="h" direction="in"/>
  <arg name="captureCursor" type="b" direction="in"/>
</method>
<method name="screenshotScreen">
  <arg name="fd" type="h" direction="in"/>
</method>

In this case it'll just add a 3rd.

Though @meven can you just run qdbus org.kde.KWin /Screenshot org.freedesktop.DBus.Introspectable.Introspect and double check.

effects/screenshot/screenshot.cpp
408

Please clear m_cacheOutputsImages so we're not wasting ram

but you underestimate QtDBus

What if we have a third party non-Qt screenshot library using it :)

but you underestimate QtDBus

It's kwin (and therefore definitely QtDBus) doing the introspection generation

meven updated this revision to Diff 80981.Apr 23 2020, 10:42 AM
meven marked 4 inline comments as done.

Use more iterators, add some const

I think we could consider this interface public API and adding new arguments changes the DBus signature iirc.
Perhaps we should just add a new overload.

You're right that DBus would fail if you don't call with the right number of arguments to match a method signature, but you underestimate QtDBus. It's /amaaaazing/

Though @meven can you just run qdbus org.kde.KWin /Screenshot org.freedesktop.DBus.Introspectable.Introspect and double check.

QtDbus is quite nice indeed, it added the new method assuming it will use defaults:

<method name="screenshotFullscreen">
  <arg name="fd" type="h" direction="in"/>
  <arg name="captureCursor" type="b" direction="in"/>
  <arg name="shouldReturnNativeSize" type="b" direction="in"/>
</method>
<method name="screenshotFullscreen">
  <arg name="fd" type="h" direction="in"/>
  <arg name="captureCursor" type="b" direction="in"/>
</method>
<method name="screenshotFullscreen">
  <arg name="fd" type="h" direction="in"/>
</method>

Still perhaps we should have a distinct dbus to make the API clearer.
AFAIK there is only one user currently : spectacle, that I am updating too anyway.

effects/screenshot/screenshot.cpp
408

Done line 442

meven marked an inline comment as done.Apr 23 2020, 10:43 AM
davidedmundson accepted this revision.Apr 29 2020, 9:10 AM
This revision is now accepted and ready to land.Apr 29 2020, 9:10 AM
This revision was automatically updated to reflect the committed changes.