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

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?

In D29282#668027, @davidre wrote:

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.

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

In D29282#673363, @abetts wrote:

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 ?

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.

In D29282#674318, @ngraham wrote:

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?

In D29282#674390, @ngraham wrote:

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.

In D29282#675339, @ngraham wrote:

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.

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

Maybe move this over to GitLab?

In D29282#676008, @ngraham wrote:

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.

In D29282#676011, @meven wrote:

In D29282#676008, @ngraham wrote:

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.

I am the author of D29550 which adds supports for a post capture selection option for already taken screenshots. D29550 is currently blocked by this patch.

After giving it a good thought, I see the following general problems with the existing rectangular region capture mode:

1. It doesn't work when trying to capture open menus. At least on X11, menus close when doing the capture.
2. By supporting different screen scales, the QuickEditor code becomes very complex. I wouldn't feel comfortable applying a patch while keeping all the screen scaling in mind. I would have to invest quite some time to get a deep understanding of the scaling problem and even then I doubt I could make changes without introducing any new issues, which brings me to the next point.
3. Testing, how do we test any changes? With X11, Wayland, scaling factors, monitor positions and X QuickEditor features there seem to be too many combinations to test properly. Especially, I don't see a reliable and easy way to automate these kind of tests.
4. All the hard bits of translating real screen position to window positions which is already done by the display server needs to be redone in QuickEditor (more or less). Not really a problem but also not a nice thing to do.

For this reason I suggest the following approach:
a) Compose the screen capture(s) into a single pixmap as it is done already.
b) Drop support for rectangular region capture mode in favour of the post selection mode in D29550 which operates on this pixmap.

This has the advantage that a) and b) are completely decoupled. How the pixmap is composed can easily be changed and screen scales can be applied as desired. QuickEditor stays relatively simple and straightforward to modify.

In D29282#676586, @czeidler wrote:

I am the author of D29550 which adds supports for a post capture selection option for already taken screenshots. D29550 is currently blocked by this patch.

After giving it a good thought, I see the following general problems with the existing rectangular region capture mode:

1. It doesn't work when trying to capture open menus. At least on X11, menus close when doing the capture.
2. By supporting different screen scales, the QuickEditor code becomes very complex. I wouldn't feel comfortable applying a patch while keeping all the screen scaling in mind. I would have to invest quite some time to get a deep understanding of the scaling problem and even then I doubt I could make changes without introducing any new issues, which brings me to the next point.
3. Testing, how do we test any changes? With X11, Wayland, scaling factors, monitor positions and X QuickEditor features there seem to be too many combinations to test properly. Especially, I don't see a reliable and easy way to automate these kind of tests.
4. All the hard bits of translating real screen position to window positions which is already done by the display server needs to be redone in QuickEditor (more or less). Not really a problem but also not a nice thing to do.
   
   For this reason I suggest the following approach: a) Compose the screen capture(s) into a single pixmap as it is done already. b) Drop support for rectangular region capture mode in favour of the post selection mode in D29550 which operates on this pixmap.
   
   This has the advantage that a) and b) are completely decoupled. How the pixmap is composed can easily be changed and screen scales can be applied as desired. QuickEditor stays relatively simple and straightforward to modify.

Such a decision, should not be made on a technological basis, it is a UX decision.
The point of this and https://invent.kde.org/graphics/spectacle/-/merge_requests/29 is first to have the same features in Wayland as in X11, feature evolution is adjacent to this.

On feature proposal, I do see value in keeping the feature as is : it allows a very simple workflow and precise screenshots that are important to our developer community.

It doesn't work when trying to capture open menus. At least on X11, menus close when doing the capture.

What do you mean?

b) Drop support for rectangular region capture mode in favour of the post selection mode in D29550 which operates on this pixmap.

I'm completly against this. You made clear that you don't like the mode in your other post and you rather would use a second tool to crop the image. That's alright, but do not conclude from this that other people feel the same way.
As many other perhaps, I mostly use the rectangular region mode and it's by far the easiest for me to draw the rectangle directly (in the original image rather than on a small preview), especially when it's important to get a pixel perfect screenshot.
This mode is also what most other screenshot tools use that I know.

I still think that your pot selection feature is a great addition! I just wouldn't agree that it's a replacement.

I'm not familiar with the technical details here, but there are usually other ways to keep a class / file simple and small by using a better design. From my point of view QQuickEditor does already way too much right now. There is no seperation between frontend and "backend" / calculations ... just imagine doing this stuff in Qml. You wouln't write all of the calculations in qml in JS.

Thanks for the feedback. Totally understand that the rectangular region capture mode is a desirable feature and that it is not an option to drop it.

This means I am still stuck with the question on how to share code between both features. Any suggestion on that? Is there an easy way to move the transformation logic into its own class/function? Or maybe split the code in smaller parts that are easier to share? e.g. a function that only draws the selection rect? Alternatively, I could simply create a copy of QuickEditor that is responsible solely for my post edit selection feature? While this doesn't feels right it may be a practical solution for now?

@Leon0402 how did you manage to take that screenshot? X11 steals keyboard input when the menu is open, doesn't it? So I can't take a screenshot with a shortcut and when clicking "Take a New Screenshot" the menu closes...