Fix issue #564, that in wayland fractional scaling situtation, screenshot preview size is wrong.

[SFBB]

Fixing screenshot preview size is not correct because wayland dpr is not correct when using fractional scaling. This fix supports the idea that we will use dpr in the xcb platform because it is correct in that case.

Tested on Manjaro wayland session with fractional scaling of 125%, both QT_QPA_PLATFORM=xcb and QT_QPA_PLATFORM=wayland work as expected.

Multi-monitor environment is not tested because I don't have one.

[b0ch3nski]

EDIT: It actually works, see my comment below.

Unfortunately, this doesn't seem to work on a multimonitor setup (3 horizontal, 1 vertical - 2 of them have a fraction scaling of 1.45). All screens on the screenshot preview seems to be very much upscaled which makes it rather unusable.

This is the screen config I've been using on Sway:

output "XXX" enable mode [email protected] pos 0 0
output "XXX" enable mode [email protected] scale 1.45 pos 1920 0
output "XXX" enable mode [email protected] scale 1.45 pos 1920 1241
output "XXX" enable mode [email protected] pos 3906 0 transform 270

[SFBB]

Unfortunately, this doesn't seem to work on a multimonitor setup (3 horizontal, 1 vertical - 2 of them have a fraction scaling of 1.45). All screens on the screenshot preview seems to be very much upscaled which makes it rather unusable.

Thank you for your testing!

Yes, that's true. In theory, it only works on multiple monitors if all monitors share the same fractional ratio.
If you need an urgent fallback, you can use add QT_QPA_PLATFORM=xcb to run flameshot, this will force flameshot run as a X11 app and it might work.

I think it is because Qt doesn't support high dpi on Linux Wayland well. It still doesn't use fractional_scale_v1 for its QWaylandScreen IIRC. And this will cause if you display a physical size image on your screen, it will not render in the logical size, at least the correct one. As far as I know, Qt should render image based on per monitor fractional scaling, but right now it is still based on a interger instead of float.
Anyway, if they don't support this, I don't think it is a good idea to handle such a complex situation in a downstream application, or even this is impossible.

My PR only fixes simple cases. And such fix should be reverted if Qt does its job well.

[b0ch3nski]

@SFBB It seems that I wrote my comment too early, dang! I've tracked the issue back to grim. I've disabled it, build with your patch and now it works as described even with the complex multimonitor setup with different orientations and fractional scaling! Thank you for this, it was bugging me for months!

[DmitrySkibitsky]

This solved the problem with fractional scaling
Thanks to the author of this fix
#3852 (comment)

[borgmanJeremy]

@mmahmoudian I can confirm this fixes fractional scaling issues (at least on KDE). It does not fix other wayland issues, but this is a good step forward.

It does not regress anything on MacOS. I have no windows machine to test on.

@SFBB have you tested that all 3 of the code paths in the if/else statement are hit under various circumstances?

[SFBB]

@borgmanJeremy I tested it with QT_QPA_PLATFORM=xcb(running as X11 app) and QT_QPA_PLATFORM=wayland(running as wayland app), and the first path gets hit in xcb, and the third path gets hit in wayland. The second path is not tested, but it should be hit when the fractional scaling is set as integer. All hit paths work normally in my single monitor with 1.25 fractional scaling setup.

I reported an issue in qt and they said:

Wayland originally only had integer scaling on a per-monitor basis.
When fractional scaling was added to the Wayland protocol this was done a per-window basis.

We do not have a fractional value of the monitor provided to us that we can provide to applications. We could do some educated guessing, but that tends to just create more problems down the line.

There is little we can do other than improving the documentation to stress that QWindow::devicePixelRatio should always be preferred.

But I tested using Qt5, QWindow.devicePixelRatio is still not right, I don't know if Qt6 works...

[mmahmoudian]

@borgmanJeremy do you believe this PR is ready to be merged?

[borgmanJeremy]

Generally yes, but it would be good for someone to test the 2nd code path. I personally think the variable names are too long but that's more of a preference than an issue. I think it's at least worth approving the CI so the CI can run. I think the code formatting will be rejected.

This scaling issue still exists on Qt6 btw, I've tested it with a minimal example.

[mmahmoudian]

@borgmanJeremy

I think it's at least worth approving the CI so the CI can run. I think the code formatting will be rejected.

This is something I've been struggling with lately. In some PRs the CIs are not available. I approved the code here but still ... . I should look into this.

[SFBB]

@mmahmoudian @borgmanJeremy

Thanks for the feedback!

I'm considering renaming the variables as follows:
approximatedPhysicalDesktopGeometryBasedOnDevicePi -> approxPhysGeo
accurateLogicalDesktopGeometry -> logicalGeo

Though it requires a bit of effort to understand, it is good for me. But I'd like your thoughts. Are you okay with me updating the pull request with these changes, or do you prefer I avoid modifying it dynamically?

If you have any other suggestions or changes you'd like me to make, please let me know! 😄

[borgmanJeremy]

@mmahmoudian I have some time lately so I can probably fix the CI if you re-add me as a contributor (or whatever permissions needed to run CI).

@SFBB Yeah I think thats much better thanks! You can just update this PR. Curious if that triggers a successful CI kick off.

[mmahmoudian]

@SFBB

I'm considering renaming the variables as follows: approximatedPhysicalDesktopGeometryBasedOnDevicePi -> approxPhysGeo accurateLogicalDesktopGeometry -> logicalGeo

Though it requires a bit of effort to understand, it is good for me. But I'd like your thoughts. Are you okay with me updating the pull request with these changes, or do you prefer I avoid modifying it dynamically?

I think the proposed variable changes are good, but considering that your original variable names are descriptive, I suggest write them as descriptive comments to be more clear about the intention behind each variable.

@borgmanJeremy Thanks, I sent you an invitation.

[SFBB]

@borgmanJeremy @mmahmoudian
Thanks! I have changed the variable names and added descriptive comments about them.
Let's see if it works for CI.

[SFBB]

@borgmanJeremy @mmahmoudian
Based on the clang-format output, I changed the code style to meet the requirement. I hope we can pass the CI this time.