Windowed capture mode

[karliss]

Properly handling multiple screens with varying scaling factors is challenging. I saw few attempts fixing it, but I am not sure if a sufficiently robust solution which works in all configurations on all the desktop operating systems will be reached anytime soon. That doesn't mean people shouldn't try.

I am suggesting an alternative simpler approach which could be user selected to make flameshot somewhat somewhat usable in setups where it couldn't be used otherwise. Instead of trying to show captured image on all the screens in fullscreen mode display it in a window with ability to move around and maybe zoom.

Having this mode might even help resolving some of the issues related to mixed display setups incrementally without solving all of them at the same time.

The code itself isn't ready yet, I wanted to discuss the approach first before spending more work on the code.

What's currently there:

• Added option for choosing one of 3 modes: all screen fullscreen, current screen fullscreen, maximized window. The first one is not available on macOS.
• CaptureWidget opens in selected mode
• In window mode view can be dragged around using middle click
• tools and preview are drawn taking current view offset into account

What's missing, needs to be done, known issues:

• Testing on all the platforms and various display configs.
• Cleanup the code to comply with this projects coding style and other contribution requirements
• in current screen fullscreen mode capture image from corresponding screen only (partially works)
• Cursor sometimes disapears in color picker
• Ctrl->C shortcut doesn't work. Copying to clipboard using tool button works.

Some other problems to consider related to multiple screens and their scaling, possibly for separate PRs:

Single screen fullscreen mode Similar to "all screen fullscreen" and "windowed" modes have third mode which captures image from single screen and displays it fullscreen on the same window. It preserves the inplace screen annotation functionality while avoiding challenging cases. From what I understand something similar Is already done for macOS. Having it as separate mode which can be selected on tested on any OS instead macOS specific fullscreen handling, would help some of Linux users. I assume that in many cases when you are selecting part of single screen anyway so doesn't need to be across mutliple screens.

Scaling factor of resulting image. It seems that currently flameshot currently creates single image consisting of all the screens scaled so that there is no information loss for highest DPI screen. This is fine when you are creating screenshot across multple screens, but if the selected rectangle is inside lower DPI screen it makes it unnecessarily upscaled. That could be handled by advanced user option choosing betweeen following modes or choosing automatically based on selection rectangle.

• scaling aware mode -> current behavior, matches how user sees things in real world (or at least desktop config), capturing parts of lower DPI screens result in unwanted upscaling
• no scaling -> glue together images at the resolution of each display, no scaling artifacts but captures across multiple screens might not fit together. If QT doesn't provide information about such layout it might be nontrivial to decide how to layout all the screens.
• automatic mode based on selection rectangle -> If selection rectangle is across multiple screens behave like in "scaling aware mode", if rectangle is on single screen use take image from that screen thus avoiding upscaling issue. This approach will require storing separate QPixmap for screencapture of each individual display in addition to current combined one since downscaling image which has been upscaled by fraction coefficient would cause some degradation.

[borgmanJeremy]

I'm open to adding this because I honestly don't have a good line of sight to fixing mixed DPI any other way. I am having issues using this on a tiling window manager though. I think you did a good job of capturing "missing items" that would need to be added before merging.

Thanks!

[karliss]

Found a little bit more time to work on this. Wanted to write down some of the current design choices. One more reminder that as indicated by draft state, this is still work in progress and a lot of stuff is in broken state, possibly more than what's listed in the initial PR description.

Coordinate systems:

• screen geometry, global mouse position, ... - mostly relevant during capture process and setting up multiscreen fullscreen window
• capture coordinates - pixel coordinates in the captured image
• view coordinates - positions relative to capture widget, mouse events from Qt report these, relevant for UI interactions

Tools and CaptureTool interface methods operate within capture coordinates and don't know anything about what parts the view is currently showing. Might need some exceptions for special tools, but should be mostly fine.

UI structure

• CaptureWidget: QScrollArea
  • viewport - part of QScrollArea structure
    • QScrollArea::widget() - parent of all the widgets inside scroll area
    • selection widget and tool buttons
  • toolSettings button
  • toolSettings panel

Things like tool settings button and panel are added directly to CaptureWidget so that they are positioned relative to window and don't move when scrolling. Selection widget and tool buttons are placed inside the scrollview so that they stick to the image.

[borgmanJeremy]

Awesome work and good progress!

You might want to make sure you are based on the master branch because it might have moved a bunch since this started.

[karliss]

There are few things remaining to fix, but afterwards comes the annoying part of testing in all the configs.

Test matrix:

Platform and mode

• Windows all screen fullscreen mode
• macOS current screen fullscreen
• Linux all screen fullscreen
• Linux window mode
• Other combinations nice to have, but less important

Screen configs (starting with most important ones)

• Single screen 1x scaling
• Multiple screens 1x scaling
• Single high DPI screen
• Multiple screens with identical >1 scaling
• Multiple screens mixed DPI

[bbbco]

So I've been waiting patiently for Flameshot to implement this feature, and when I came across this pull request, I was excited!

I went ahead and cloned the flameshot repo, built it, and tested my build to ensure it worked properly. Then I fetched your branch @karliss and rebased against origin/master to pull in the latest updates and created a fresh build.

Running on Linux Mint 20.1 with a single monitor, when I attempt to get a new screenshot, the initial display is a blank grey screen. I can use the Flameshot tools here and eventually create a screenshot (that is not obvious to me because of the lack of display). See below:

This is the terminal output that is displayed:

QPainter::begin: Paint device returned engine == 0, type: 2
QPainter::setRenderHint: Painter must be active to set rendering hints
QPainter::setCompositionMode: Painter not active
QPainter::translate: Painter not active
QPainter::setPen: Painter not active
QPainter::setBrush: Painter not active
QPainter::setBrush: Painter not active

Please let me know what other info you need from me to help you work through this issue.

Thanks for your initial work to attempt to implement this much needed feature!

[karliss]

@bbbco The PR is still work in progress, please be patient. I had seen the the gray screen problem before on Windows and resolving it was one of the next things on my list, but it's interesting to see that it also happens on Linux. I have an approximate idea what should be changed.

[bbbco]

Good to know! Let me know if I can help in any way with the testing. @bbbco

On July 24, 2021, Pedro Alonso ***@***.***> wrote: @bbbco <https://github.com/bbbco> The PR is still work in progress, please be patient. I had seen the the gray screen problem before on Windows and resolving it was one of the next things on my list, but it's interesting to see that it also happens on Linux. I have an approximate idea what should be changed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1565 (comment)- 886158628>, or unsubscribe <https://github.com/notifications/unsubscribe- auth/AAAHPWC5AAKGRGWJ7KCZQH3TZO2WLANCNFSM43ECS3PQ>.

[karliss]

Repeated the gray screen problem on Linux by forcing specific theme engine with QT_QPA_PLATFORMTHEME=kvantum .

[borgmanJeremy]

Is this planned to be finished or should I close the PR?

[karliss]

I haven't given up on this. Will try to finish soon.

[karliss]

Ok, I have done most of the the remaining things and caught up with changes in master. Time to try doing the all config testing again.

[karliss]

Last release column is for comparing against last release and ensure that this PR doesn't introduce any regressions.
To avoid stretching this PR even longer primary goal is that desktop setups that worked before should still work, it is acceptable that more complicated setups which didn't work before don't work yet.

2560x1440 X1 means 2560x1440 resolution with scaling factor set to 1. 4K X2 means 4K resolution with scaling factor 2, not two 4K displays.

Config	Fullscreen all	Current screen	Windowed mode	Last release or master
Gnome 40+wayland 2560x1440 X1	✔️ 5)	✔️ 5)	✔️ 1) 5)	✔️ 6)
Gnome 40+wayland 2560x1440 X1+1920x1080 X1	✔️❌2) 4) 5) 7)	✔️❌ 5) 8)	✔️ 1) 5)	✔️ 2) 6)
Gnome 40+wayland 4K X2	❌3) 5)	❌3) 5)	✔️❌3) 5)	❌3) 6)
X11 TODO
xwayland TODO
Windows10 2560x1440 X1	✔️	✔️	✔️	✔️
Windows10 1080x1920 X1 + 2560x1440 X1	9) 10)11)	✔️	12)	✔️❌ 11)
Windows 4K X2	❌11)	✔️)	✔️	✔️❌11)
Windows 256x1440 X1 + 4K X2	❌11) 9) 10)	✔️14)	✔️	✔️❌15)
macOS TODO	N/A

1. FIXED content size not correctly updated after resizing window.
2. U Tool button placement when selection is across two screens isn't perfect. Regression in current master, not present in last release.
3. Content drawn at 2x scale resulting in it not fitting, mouse interactions work correctly
4. FIXED help message placement see Windowed capture mode #1565 (comment) .
5. FIXED zoom screen graber is drawn at top left, most likely wayland not liking parentless widgets. Should be easy to fix.
6. U flameshot forces xcb backend even when system forces wayland
7. Can't force the correct position but the size is correct and it is on top of taskbar. Depending on screen setup can be manually moved to correct screen using Super+Arrows.
8. Due to the way wayland works can't know mouse position without any window so it always reports [0, 0] and. Maybe it's possible to come up with different heuristics for getting current screen index.
9. FIXED wrong side panel position
10. help message placement broken again
11. clicking anywhere with color graber active closes the window
12. It is possible to get color grabber under the window
13. FIXED mostly functional but UI scaling is messed up even config window. Should be possible to do better with setting correct application flags.
14. FIXED content drawn in wrong place, wrong size
15. U sidepanel incorrectly positioned

[veracioux]

I have a few suggestions. Disclaimer: I'm just a contributor so don't take these on any kind of authority. And they should be discussed first (maybe open a discussion?)

Config option

I don't see that you have added an option to change the default window size. How about renaming the windowMode config option to windowSize and have its value specified as one of:

• WIDTHxHEIGHT
• FullScreenAll
• FullScreenCurrent
• MaximizeWindow
• DebugNonFullScreen

Whichever option name we choose, the flameshot.example.ini file should document them. And please add a ValueHandler for the option in src/utils/valuehandler.{h,cpp} for config checking to work. As a consequence, the CONFIG_GETTER_SETTER macro should probably be used in ConfigHandler instead of the manual implementation that is currently in place.

MaximizeWindow does not maximize the window

Instead the window is regular size. Maybe has something to do with the fact that I'm using a tiling WM (i3wm).

NotifierBox

The notifier box that is shown when tool thickness is changed is not displayed correctly in windowed mode if the window is not maximized.

Help message

The help message is not shown properly in full screen mode when multiple displays are used. It is shown at the center of all screens which means that part of the message will be on one screen and the other half on another.

If any of the suggestions are too burdensome, I can implement them in another PR. But I would like you to implement the ValueHandler and give me feedback about the developer-friendliness of the API (which I implemented just recently).

[karliss]

Thanks for the feedback.

The value handler system is one of the things I wanted to clarify. Will try to implement it in the cleanup phase. After doing the testing and resolving major bugs.

Instead the window is regular size. Maybe has something to do with the fact that I'm using a tiling WM

Sounds like things are functioning as intended. Isn't that the point of tiling WM that they override most window resizing functionality except maybe for fixed size windows. I would prefer keeping the window mode resizable instead of forcing non resizable window. Maybe it should be renamed to just "windowed mode".

NotifierBox

Thanks for catching this, will fix together with other things I noticed while testing non maximized window.

help message is not shown properly in full screen mode

Which fullscreen mode, Fullscreen all screens or fullscreen current screen? With what screen sizes and layout did you test? I didn't notice in the tests I did so far, but maybe in those cases and my monitor layout it happened to be in somewhat reasonable position.

WIDTHxHEIGHT

I would suggest leaving addition of extra modes to follow up PRs. Rebasing this PR is already painful enough. Also what's the usecase for opening window with specific size?

[karliss]

One more thing I would like clarifying with regular devs is the debug mode. For now I added it as additional mode. That was done before introduction of build system flag. Any thoughts on how it should work?

[veracioux]

@karliss

Instead the window is regular size. Maybe has something to do with the fact that I'm using a tiling WM

Sounds like things are functioning as intended. Isn't that the point of tiling WM that they override most window resizing functionality except maybe for fixed size windows. I would prefer keeping the window mode resizable instead of forcing non resizable window. Maybe it should be renamed to just "windowed mode".

My bad. I should have been more clear. I meant for WIDTHxHEIGHT to be the default size, not to fix the window size and make it unresizable.

The flameshot window in my tiling WM appears in float mode (not restricted to the tiling layout). In this mode the window has free size. And float mode makes more sense since flameshot is not a program you would usually want to stay around for long. But the default size of the window is too small for me, so I'd like to be able to change it. Maybe the best course would be to have MaximizeWindow which opens in windowed mode but makes the window fullscreen by default and add a Windowed mode which is specified in the config as WIDTHxHEIGHT.

In a regular DE, does MaximizeWindow actually take up the whole screen?

Which fullscreen mode, Fullscreen all screens or fullscreen current screen?

FullScreenAll. I have two monitors: left one is 1680x1050, right one is 1920x1080. They are placed right beside each other, with no gap. The message appears in the center of where both screens meet. Instead I believe it should appear in the center of the primary screen. In window mode, the message should appear in the center of the window.

WIDTHxHEIGHT

I agree that we can leave this for a future PR.

One more thing I would like clarifying with regular devs is the debug mode.

I think the build system flag will become obsolete after this PR, if we add a --config option to specify the exact config file which I may have already suggested in an issue. This way developers could test flameshot using a different config file. So I vote to leave the build flag as it is until a future PR.

[karliss]

In a regular DE, does MaximizeWindow actually take up the whole screen?

Yes, at least on Gnome it takes up whole screen except the taskbar area whichis what I expect it to do. The code is calling ShowMaximized().

[karliss]

I was going to suggest as one more thing for follow up issue to add a commandline flag for running a capture in specific window mode as that would be useful not only for testing but also integration into desktop. But I just noticed that there are flags with similar intent

Arguments:
  gui            Start a manual capture in GUI mode.
  screen         Capture a single screen.
  full           Capture the entire desktop.
  launcher       Open the capture launcher.
  config         Configure flameshot.

so maybe it should be done within this PR unless they where already broken before my changes.

[veracioux]

@karliss Hey, you've done amazing work! This PR opens up some exciting new horizons (e.g. look at the issues I've mentioned this PR from). I'm sorry I haven't been actively monitoring this PR, I have been busy with other stuff.

Because I've been actively involved in managing the master branch, how about I merge master into this branch and create a pull request to karliss:mixed-scale? So that checks can run and we don't accumulate inconsistencies.

And also: let's not do anything with screen, full and the other subcommands in this PR.

[borgmanJeremy]

@veracioux that plan sounds good to me.

[borgmanJeremy]

@veracioux Are you still planning to do this merge?