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

Authored by meven on Wed, Apr 29, 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

Repository
R166 Spectacle
Branch
wayland-screenshot
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26123
Build 26141: arc lint + arc unit
meven created this revision.Wed, Apr 29, 5:21 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptWed, Apr 29, 5:21 PM
meven requested review of this revision.Wed, Apr 29, 5:21 PM
meven added a reviewer: bport.Wed, Apr 29, 5:22 PM

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

meven updated this revision to Diff 81565.Thu, Apr 30, 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)Thu, Apr 30, 9:11 AM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 81570.Thu, Apr 30, 9:13 AM

Schedule an update instead of repaint in QuickEditor::onScreenConfigurationReceived

meven edited the summary of this revision. (Show Details)Wed, May 6, 2:38 PM
meven edited the summary of this revision. (Show Details)Thu, May 7, 7:28 AM
meven added a comment.Sun, May 10, 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.EditedTue, May 12, 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.Tue, May 12, 3:09 PM
src/Platforms/PlatformKWinWayland.cpp
89

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)Sat, May 16, 9:07 AM
abetts added a subscriber: abetts.Thu, May 21, 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.Fri, May 22, 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 ?