QuickEditor: Allow to take screen region screenshot under Wayland
Needs ReviewPublic

Authored by meven on Apr 29 2020, 5:21 PM.

Details

Reviewers
davidedmundson
davidre
bport
Group Reviewers
Spectacle
Summary

Since Wayland has per-screen scaling, make QuickEditor paint screen by screen.
Make adaptation consequently.

Add a dependency to KScreen to get the real hardware scaling (QScreen does not allow to get the real scaling on Wayland).

Adds a GrabMode::AllScreensNativeSize that the image should follow the pixel on screen output rather than the scaled output.
GrabMode::AllScreens now means grab image with scaled output.

It is useful in Wayland since the QuickEditor needs pixel-on-screen output but multi-screen screenshot wants scaled output that is not distorted.
Under X the two do the same since the scaling is the same on all screens, the pixel on screen output cannot be distorted.

Improve bottom Help text positioning in multi-screen.

See also D29122

BUG: 420863
BUG: 409762
CCBUG: 415153

Test Plan

Tested under X and Wayland in multi-screen with and without some scaling.
In Wayland using different scaling per-screen.

Diff Detail

Branch
arcpatch-D29282
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27159
Build 27177: arc lint + arc unit
meven created this revision.Apr 29 2020, 5:21 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptApr 29 2020, 5:21 PM
meven requested review of this revision.Apr 29 2020, 5:21 PM
meven added a reviewer: bport.Apr 29 2020, 5:22 PM

Can you add some more explanation what the code does in the summary?

meven updated this revision to Diff 81565.Apr 30 2020, 7:07 AM

Rename dpr and dprI to devicePixelRatio and devicePixelRatio to differenciate to dpr variable that is per-screen, fix stroke color around selection

meven edited the summary of this revision. (Show Details)Apr 30 2020, 9:11 AM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 81570.Apr 30 2020, 9:13 AM

Schedule an update instead of repaint in QuickEditor::onScreenConfigurationReceived

meven edited the summary of this revision. (Show Details)May 6 2020, 2:38 PM
meven edited the summary of this revision. (Show Details)May 7 2020, 7:28 AM
meven added a comment.May 10 2020, 1:49 PM

Let me know @davidre if I can ease your review ;)

So on screen we have the non rectangular pixel perfect pixmap but we return a rectangular pixmap? Which size will that have?

src/Platforms/PlatformKWinWayland.h
49

Can we avoid the duplication? I guess parameter packs seem a bit like overkill?

meven added a comment.EditedMay 12 2020, 5:38 AM

So on screen we have the non rectangular pixel perfect pixmap but we return a rectangular pixmap? Which size will that have?

non rectangular pixel perfect pixmap The pixmaps are all rectangular (the screen assembled pixmap can have transparent areas though).
we return a rectangular pixmap you mean the selected rectangle ?
It has the size of the area selected (unscaled) pixel perfect, WYSIWYG.

src/QuickEditor/QuickEditor.cpp
501

Would it make sense to add this to KScreen ? for instance a static KScreen::pixelGeometry()` that would return {QScreen* or kscreen/output*-> Geometry} (either QVector or Hashmap)
Since this function is in KWin already and would be essentially copied here.

meven added inline comments.May 12 2020, 3:09 PM
src/Platforms/PlatformKWinWayland.cpp
89–90

Please notes this changes the default grab mode "One screen fullscreen" from GrabMode::AllScreens to Platform::GrabMode::CurrentScreen.

This is because GrabMode::AllScreens is now rendered with scaling applied by default, whereas Platform::GrabMode::CurrentScreen is not.
It will change the wording to the user other than nothing really.

meven edited the summary of this revision. (Show Details)May 16 2020, 9:07 AM
abetts added a subscriber: abetts.May 21 2020, 10:41 PM

In the current implementation of this, the cursor turns to the selector for the screen. The cursor does not draw a squared selection of screen where there is black outside the selection. The app crashes or closes after the screenshot is taken. This behavior happens on Fedora 32/Plasma 5.18

meven added a comment.May 22 2020, 2:31 PM

In the current implementation of this, the cursor turns to the selector for the screen. The cursor does not draw a squared selection of screen where there is black outside the selection. The app crashes or closes after the screenshot is taken. This behavior happens on Fedora 32/Plasma 5.18

I don't understand the situation you are describing.
This is a current bug right ?
Have you tested with this patch ?

bam added a subscriber: bam.May 29 2020, 9:51 PM
ngraham added a subscriber: ngraham.Jun 1 2020, 3:51 AM

I don't think making KScreen a mandatory dependency is going to work. If we do that, then installing Spectacle on a platform other than Plasma is going to try to pull in all of KScreen and its dependencies. It would be half of the Plasma stack.

meven added a comment.Jun 1 2020, 8:24 AM

I don't think making KScreen a mandatory dependency is going to work. If we do that, then installing Spectacle on a platform other than Plasma is going to try to pull in all of KScreen and its dependencies. It would be half of the Plasma stack.

Under Wayland Spectacle already can't run in other platform than plasma (and other features only work in Plasma I believe).
And there is no Wayland protocol to take screenshots, meaning only screenshot tools with KWin support can work.
And there is no interface to get real scale per screen, IIANM other than adding a hard dependency on KSCreen.
So I am simply constrained here and IMHO this doesn't hurt much.
I would be happy to have a better alternative.

Hmm, that's unfortunate. Is there really no cross-compositor screenshot API in Wayland? Every compositor needs to re-invent the wheel?

meven added a comment.Jun 2 2020, 5:24 AM

Hmm, that's unfortunate. Is there really no cross-compositor screenshot API in Wayland? Every compositor needs to re-invent the wheel?

Currently no, either to take screenshot.
https://bugs.freedesktop.org/show_bug.cgi?id=98894

Is there a way we can make it a runtime dependency rather than a build-time dependency? I'm just worried about people trying to install Spectacle on a non-Plasma platform and then getting half of Plasma dragged in, which would also complicate packaging for Snap, Flatpak, and AppImage.

Maybe that https://phabricator.kde.org/D23932 would come in handy?

One main idea of it was to separate the backend from spectacle, to allow for distros to offer them as separate packages with different dependencies. Although I thought more about other platforms like Windows, it of course would work the same for wayland/xcb oder kwin/noKwin. What do you think?

I plan to be more active again on my open revisions in a few weeks if I passed all my uni tests. Would that be a solution, should I continue working on it? There was some discussion if it really is necessary or just over engineered etc. ,I would like to clarify that beforehand. In my opinion it still makes Spectacle a lot cleaner and more flexible and is therefore worth the effort.

Needs a rebase.

And I guess we still need to figure out a way to make KScreen not a mandatory runtime dependency.

meven added a comment.Jun 17 2020, 8:40 AM

Needs a rebase.

Affirmative

And I guess we still need to figure out a way to make KScreen not a mandatory runtime dependency.

I feel we might want to add a DBus interface to either kscreen or KWin to make this dependency a runtime one, that would expose screens as KScreen::GetConfigOperation() and KScreen::ConfigPtr would.

Ah, that sounds like a sensible idea.

meven updated this revision to Diff 83306.Jun 23 2020, 10:46 AM

Rebased on master

Tried this out and Rectangular Region mode disappeared from the combobox.

Maybe move this over to GitLab?

meven added a comment.Jul 25 2020, 4:45 PM

Tried this out and Rectangular Region mode disappeared from the combobox.

I think this is because there is a now check in spectacle that plasma is > 5.20 for the shuttermode.
You can edit the PlatformKWinWayland::supportedShutterModes to work locally.
The patch needs adjustment.

Maybe move this over to GitLab?

For now this patch is blocked until we can get the screen scale factor from somewhere else than libkscreen.
I wrote on wayland-dev mailing suggesting to add a protocol to this end, but I need to follow through still.

Tried this out and Rectangular Region mode disappeared from the combobox.

I think this is because there is a now check in spectacle that plasma is > 5.20 for the shuttermode.

Should check for > 5.19.80 then

For now this patch is blocked until we can get the screen scale factor from somewhere else than libkscreen.

The status is:

Kwin has N screens each has with 1 picture and 1 scale

The current system is that we mash that into one picture which throws away all the metadata of what content is on which screen.

Then this patch effectively is splitting it up again! and it needs extra metadata to do it correctly, and you're trying to find 3rd sources to fetch that.


If you tackled this at a data level and fetched each output from kwin separately you would resolve this problem, and you reduce the problem spectacle has to deal with here.