-
-
Notifications
You must be signed in to change notification settings - Fork 791
Qt window fixes #4069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Qt window fixes #4069
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems to make sense; one question about a change to the image size probe.
qt/tests_backend/probe.py
Outdated
| assert [s * screen._impl.native.devicePixelRatio() for s in size] == approx( | ||
| image_size, abs=1 | ||
| ) | ||
| window = toga.App.app.current_window or toga.App.app.main_window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a significant change - it's guessing about the window, rather than using a screen that has been explicitly provided as an argument. What's the significance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freakboy3742 Looking back I can see how this gets unreliable... however, the documentation of QScreen. devicePixelRatio says that that function should not be used when there is a window to target; because QScreen. devicePixelRatio can give wrong values under Wayland with fractional scaling, so we have to have a window on that screen to assert.
So should we try to go through our windows one by one to see if one of them belongs to the screen and if none of them does, just do nothing? That'd be more reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to avoid the temptation to guess.
If the QT backend should operate on the window rather than the screen... provide that detail to the assertion. Add a window argument, and explicitly provide the window as context. Other backends may not need that detail; but if they have it, they can use it.
AFAICT the main reason the existing method accepts screen rather than window is that you can take a screenshot of a screen and it doesn't necessarily have a window on it. So - it seems to me that if you don't provide a window, the screen's DPI is the one that should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you set display settings resolution to 175%, https://doc.qt.io/qt-6/qscreen.html#devicePixelRatio-prop actually returns 2, so the function gives wrong values. Would an approach like "window provided > that window used, screen provided > look for windows on that screen, if all else then assert within a generic range, say 50% to 300%" be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... how does that relate to anything I said?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freakboy3742 I have "window provided > that window used" which was what you said.
To be completely clear about this, accounting for the fact that I have previously failed to provide enough context:
- I was agreeing with you mostly about explicitly having a window context.
- But for this part:
AFAICT the main reason the existing method accepts screen rather than window is that you can take a screenshot of a screen and it doesn't necessarily have a window on it. So - it seems to me that if you don't provide a window, the screen's DPI is the one that should be used.
... the screen's DPI may be wrong under Wayland with fractional scaling, so we must have a window to get the ratio. The key here is that we must have a window to target, at least from my testing.
So this yields the logic I mentioned:
- If window provided, use the ratio of that window
- Else, if screen provided, find windows that are on that screen, and if available, use the ratio for that window
- If all else fails, assert that using a reasonable scale, it is possible to produce the resolution of the actual image.
Refactor assert_image_size to target specific window based on screen.
List of fixes:
PR Checklist: