beeware
New Backend - Qt
WIP: Fixes #1142
PR Checklist:
- [x] All new features have been tested
- [x] All new features have been documented
- [x] I have read the CONTRIBUTING.md file
- [x] I will abide by the code of conduct
@freakboy3742 By now I have not added CI testing yet because the test skips have not yet convereged at 100%
This is rough, I need to go back and clean up comments (especially some random jokes I've made -- if you have spare time I give you permission to try to spot them) but a few things I need confirmation on:
- Is using linux_qt as the platform identification name okay with you? It detects this platform when
TOGA_QTis set to 1 or if the application is running under KDE now. - I've mentioned the PySide6 import hack before -- specifically doing this:
import site
import sys
def import_pyside6():
"""Temporarily break isolation to import system PySide6."""
system_site = site.getsitepackages()
print(system_site)
old_path = sys.path.copy()
sys.path.extend(system_site)
import PySide6 # noqa
sys.path = old_path
import_pyside6()
So if https://github.com/beeware/briefcase/issues/2480 (which I've filed for a feature to replace installed PySide6 with system symlink when packaging an app) doesn't get resolved in the duration of this PR, should I still remove this hack such that the new backend will not be able to look native and access system themes?
- I've added a NativeIcon class to icons as seen at https://github.com/beeware/toga/blob/3028865577040e9d3c95f5af036cab857aee8bd2/core/src/toga/icons.py#L35-L42 since native icons are needed for undo and redo actions in Qt -- see https://github.com/beeware/toga/blob/3028865577040e9d3c95f5af036cab857aee8bd2/qt/src/toga_qt/app.py#L114 (it demands an interfaced icon there). Is this acceptable, and if not, how should I deal with this situation?
Thank you!
@freakboy3742 Could you please respond to those questions I've asked about above? Thank you!
@freakboy3742 By now I have not added CI testing yet because the test skips have not yet convereged at 100%
This is rough, I need to go back and clean up comments (especially some random jokes I've made -- if you have spare time I give you permission to try to spot them) but a few things I need confirmation on:
- Is using linux_qt as the platform identification name okay with you? It detects this platform when
TOGA_QTis set to 1 or if the application is running under KDE now.
No. Platform identification is only used because sys.platform isn't a reliable identifier on Android (and, a long time ago, wasn't a reliable identifier on web). Once Python 3.13 is the oldest supported release, toga.platform.current_platform should be deprecated.
You'll also note that the current GTK backend doesn't describe its platform as GTK - it's linux.
The approach used by Textual is the model to follow here. If you're on Linux, toga_qt is a candidate backend; if that's the only backend, it's used; if there's more than one candidate installed in the current environment, TOGA_BACKEND=toga_qt disambiguates.
- I've mentioned the PySide6 import hack before -- specifically doing this: ... So if beeware/briefcase#2480 (which I've filed for a feature to replace installed PySide6 with system symlink when packaging an app) doesn't get resolved in the duration of this PR, should I still remove this hack such that the new backend will not be able to look native and access system themes?
For now, I'd say yes. We can only cook with the ingredients we're given. If Qt doesn't work in virtual environments, that's a problem for Qt to resolve. It's not up to us to work around it - if only because there may be use cases for not adopting the global environment.
If there's a hack that could be installed, I'd argue the system_pyside6 approach I've described previously is the way to install that hack.
- I've added a NativeIcon class to icons as seen at https://github.com/beeware/toga/blob/3028865577040e9d3c95f5af036cab857aee8bd2/core/src/toga/icons.py#L35-L42 since native icons are needed for undo and redo actions in Qt -- see https://github.com/beeware/toga/blob/3028865577040e9d3c95f5af036cab857aee8bd2/qt/src/toga_qt/app.py#L114 (it demands an interfaced icon there). Is this acceptable, and if not, how should I deal with this situation?
In core - No - or, at least, there's a much bigger concept that needs to be wrapped here. If the underlying question is about "system icons" - GTK has an analogous concept, which we don't currently handle. See #2441 for some initial thoughts about this (although it doesn't mention the GTK analog that exists).
@freakboy3742 Thanks for the responses, there's much going on and I'm in the middle of a major comments cleanup (it's done through GitHub's review interface so expect a huge email next week with all the ntoes I made for myself, sorry about that)
Re to response 1: Then how the heck am I gonna get all the tests skipped by platform? Do I need a bunch of stub probes to just say "skip on Qt", sort of like GTK4?
Anyways if there's 1 platform only also for Briefcase how do we handle the testbed then? Right now I used tests_backend_qt for qt probes and hacks it at conftest.py: https://github.com/beeware/toga/pull/3769/files#diff-b845936988736dd9f884ab2aeb4175fa8ee7914217ed87d3c8aad18f803e74f0R24-R33 -- I can check the backend there [b]ut (EDITED TYPO) it just feels messy to have a hack like that at all.
Re to response 2: IMO system-pyside6 for symlinking system packages is not feasible. With all this wheel madness going on you simply don't know where the final system-pyside6 package even gets installed and you can't put symlinks in wheels either (it extracts as plain text when extracted by pip, which uses its own extraction logic). There's some options for "build destination" in setup.py but that's to like an intermediate location. Should I extract this sys.modules hackery into a system-pyside6 package that installs a shim PySide6.py to replace itself, then?
Re to response 3: Yeah, but it'd look really weird on Qt without like a native icon attached to the actions. KDE inserts extra space before an action with no icon to fill up for the icon space.
@freakboy3742 Also do you think that https://github.com/beeware/briefcase/issues/2480 is a candicate issue to be handled on Briefcase? Or are we like if PySide6 doesn't work it doesn't work and we're not going to have our packaging tool symlink a system copy?
Re to response 1: Then how the heck am I gonna get all the tests skipped by platform? Do I need a bunch of stub probes to just say "skip on Qt", sort of like GTK4?
Effectively, yes.
Anyways if there's 1 platform only also for Briefcase how do we handle the testbed then? Right now I used tests_backend_qt for qt probes and hacks it at conftest.py: https://github.com/beeware/toga/pull/3769/files#diff-b845936988736dd9f884ab2aeb4175fa8ee7914217ed87d3c8aad18f803e74f0R24-R33 -- I can check the backend there [b]ut (EDITED TYPO) it just feels messy to have a hack like that at all.
My immediate guess - a configuration for a second app. Briefcase allows you to have multiple app definitions in a single pyproject.toml; so you'd be running briefcase run -a qt-testbed (or similar), which includes all the qt libraries. We're going to need a similar app configuration for Textual.
Re to response 2: IMO system-pyside6 for symlinking system packages is not feasible. With all this wheel madness going on you simply don't know where the final system-pyside6 package even gets installed and you can't put symlinks in wheels either (it extracts as plain text when extracted by pip, which uses its own extraction logic). There's some options for "build destination" in setup.py but that's to like an intermediate location. Should I extract this sys.modules hackery into a system-pyside6 package that installs a shim PySide6.py to replace itself, then?
It's difficult to recommend a course of action when I don't fully understand the nature of the problem. However, not knowing the final install location should not be a constraint. No, you can't include a symlink in a system package - but there are lots of other things you can do to Python's import system that will activate a runtime. A .pth hack seem be the most obvious approach, but even that is only one of many options.
Re to response 3: Yeah, but it'd look really weird on Qt without like a native icon attached to the actions. KDE inserts extra space before an action with no icon to fill up for the icon space.
I'm not saying native icons can't be a part of the solution. I'm saying that if we're adding something to core, it needs to be part of a cross-platform "native icon" implementation.
@freakboy3742 Also do you think that beeware/briefcase#2480 is a candicate issue to be handled on Briefcase? Or are we like if PySide6 doesn't work it doesn't work and we're not going to have our packaging tool symlink a system copy?
I'm not sure I understand the question ... is an issue reported against Briefcase a candidate for something to fix in Briefcase? Well... yes. I'd imagine the way Briefcase solves the problem will likely be similar to the solution we recommend to Toga users.
Re to response 1: Then how the heck am I gonna get all the tests skipped by platform? Do I need a bunch of stub probes to just say "skip on Qt", sort of like GTK4?
Effectively, yes.
Anyways if there's 1 platform only also for Briefcase how do we handle the testbed then? Right now I used tests_backend_qt for qt probes and hacks it at conftest.py: https://github.com/beeware/toga/pull/3769/files#diff-b845936988736dd9f884ab2aeb4175fa8ee7914217ed87d3c8aad18f803e74f0R24-R33 -- I can check the backend there [b]ut (EDITED TYPO) it just feels messy to have a hack like that at all.
My immediate guess - a configuration for a second app. Briefcase allows you to have multiple app definitions in a single pyproject.toml; so you'd be running
briefcase run -a qt-testbed(or similar), which includes all the qt libraries. We're going to need a similar app configuration for Textual.
Noted.
Re to response 2: IMO system-pyside6 for symlinking system packages is not feasible. With all this wheel madness going on you simply don't know where the final system-pyside6 package even gets installed and you can't put symlinks in wheels either (it extracts as plain text when extracted by pip, which uses its own extraction logic). There's some options for "build destination" in setup.py but that's to like an intermediate location. Should I extract this sys.modules hackery into a system-pyside6 package that installs a shim PySide6.py to replace itself, then?
It's difficult to recommend a course of action when I don't fully understand the nature of the problem. However, not knowing the final install location should not be a constraint. No, you can't include a symlink in a system package - but there are lots of other things you can do to Python's import system that will activate a runtime. A
.pthhack seem be the most obvious approach, but even that is only one of many options.
Will look into this.
Re to response 3: Yeah, but it'd look really weird on Qt without like a native icon attached to the actions. KDE inserts extra space before an action with no icon to fill up for the icon space.
I'm not saying native icons can't be a part of the solution. I'm saying that if we're adding something to core, it needs to be part of a cross-platform "native icon" implementation.
My point was that it's difficult for me to do all the research for native icons in the middle of doing a Qt impl. Anyways I'm currently hacking this in by having the Edit menu actions be a custom class, wrap it in a NativeHandler so it doesn't get rewrapped, and then check if the action is of this custom class (or of name Quit) to apply the correct custom icon:
https://github.com/beeware/toga/pull/3769/files#diff-b9c269d868d0adbd7972fe4fffb510f2f571c3633f639147600aa10f951cc561R15-R43
https://github.com/beeware/toga/pull/3769/files#diff-9191b9f479cd6a8aaee7b25af139511ca1f1ca13e068209b02ee5d264dcfdd0dR106-R110
aka. Really similar to what Apple has been doing with their menubar icons...
@freakboy3742 Also do you think that beeware/briefcase#2480 is a candicate issue to be handled on Briefcase? Or are we like if PySide6 doesn't work it doesn't work and we're not going to have our packaging tool symlink a system copy?
I'm not sure I understand the question ... is an issue reported against Briefcase a candidate for something to fix in Briefcase? Well... yes. I'd imagine the way Briefcase solves the problem will likely be similar to the solution we recommend to Toga users.
Sure, from a dig in the commit history we used to recommend symlinking pygobject into the virtual env. so I'd try to research Briefcase to do this... but anyways the packaing on Fedora 42 is sort of broken such that once you restart your Fedora Linux computer it'd brick... so on Fedora at least we can't make it a default.
@freakboy3742 I'd like a review of all parts under testbed/ -- I've used your approach described but I parameterized a bunch of other stuff (and even symlinked testbed_qt to get around the Briefcase app package name requirement). Thanks!
@freakboy3742 Also I want to ask about the test skipping for un-implemented widgets -- it seems that the actual widget module is imported first bee-fore the probe module is imported to having a bunch of stub probes doesn't seem to skip the tests -- I'd assume this is the reason why a platform list skip is maintained. In the gtk4 situation at least the widget module is importable... I think making both stub widgets AND stub probes is just too much work... so I've added a "is testing" check to the getattr of factory to skip. Is this okay with you or should we put something like skip_on_qt into conftest.py?
@freakboy3742 Also I want to ask about the test skipping for un-implemented widgets -- it seems that the actual widget module is imported first bee-fore the probe module is imported to having a bunch of stub probes doesn't seem to skip the tests -- I'd assume this is the reason why a platform list skip is maintained. In the gtk4 situation at least the widget module is importable... I think making both stub widgets AND stub probes is just too much work... so I've added a "is testing" check to the getattr of factory to skip. Is this okay with you or should we put something like skip_on_qt into conftest.py?
From a short term pragmatic solution, I'm not too concerned either way. linux-qt was a non-starter because it violated what the core API is actually trying to do; but if we need to commit some minor code crimes to get things unstuck for now, that's fine by me. get_testing() isn't pretty, but it works for now.
Longer term, it would be good to find a way to make widget-based skipping a feature of the probes rather than needing specific handling in the testbed suite - but that's also a problem we don't have to solve right now.
@freakboy3742 Okay then, all of the things that need structural things from you are resolved now. Thanks! I'm sorry for the delay into pushing more code, I'm extremely busy at the moment. Procedurally, once the cleanups I pointed out are made and CI is added and passing I will request a review.
@freakboy3742 Coverage is now at 95.3% on Wayland. I'd like a review of Window, App, Container, [EDIT and Command] and Screens, and their corresponding probes, in addition to a rereview of the testbed to make sure you don't have additional concerns. Thank you so much for your assistance on this backend!
@freakboy3742 I have recreated my system-pyside6 repo, this time it should work correctly -- I basically put the same hack code in but using a pth file. I can't really do much better than that.
https://github.com/johnzhou721/system-pyside6/blob/master/setup.py#L9 -- this shows I referenced a differnet answer on StackOverflow however I read the code and learnt which function to call and what to override -- and then I pulled my own implementation. I'm not sure if this would be copyright infringement...
Anyways I had to include a stub package in there to use that answer's approach... not ideal but we can't copy anything else into the build dir otherwise.
Comments? Is the implementation of https://github.com/johnzhou721/system-pyside6/blob/master/PySide6.pth fine? If so let's get https://github.com/beeware/briefcase/issues/2480 closed as 'solved by standard Python tooling'.
I've got a couple of work deadlines that I need to hit; I'll try to give this a review later this week.
Also what's with GitHub having snow days? Over the last 3 days I got a glut of emails saying network-related testbed is failing...
@freakboy3742 CI is set up, most of your comments are resolved, I've made remarks in some where I have not clicked the resolve button. I am happy with the code of this PR, however it does not have any documentation, and therefore I have not marked it ready for review
However I'd like to rerequest a review from you for all the code in this PR -- if you don't have time for that in one go, then I'm also okay with you rereviewing the parts you've already reviewed, me fixing the issues in there, and then move on to other spots. Thank you!
I'd also appreciate guidance on how to document this backend as well.
John, you need to remember that every time you comment on a PR or an issue, everyone involved with it receives a notification. There are multiple instances in this PR of you posting something, and then within minutes, replying again to negate what you initially said. Others have already noted that your posting style almost reads as an inner monologue. Reiterating the point they have made - this habit causes a significant amount of noise. You regularly apologise for that noise, and try to moderate it by offering "your 2 cents" - but your actual behavior hasn't changed.
We need you to curb this behavior. Wait to send your responses until you have actually finished sorting out whatever situation you're commenting on. Type up your response, and wait a few hours to send it. This will also allow you to consolidate your responses, which will reduce the number of notifications you are producing. There is nothing so urgent in what you are doing that we can't wait a few hours - or even a few days - for an update.
Further, please stop using the GitHub review suggestion feature to fix issues on your own PRs. This results in two notifications: one for the suggested change, and one for the commit. Again - there is nothing so urgent in the work you're doing that a PR update can't wait until you're at a computer and can compose a full commit.
Russell will be providing a review by the end of the week.
@freakboy3742 I've resolved most of your comments and am waiting response on some of them, after which I would consider this PR ready for a final review.
However it seems that Briefcase sdisting is not parsing [optional extra groups] properly... any ideas on how to fix this? Should I just repeat system-pyside6 additionally in the testbed?
@freakboy3742 Most comments are replied to inline; thanks for working through this with me.
On
- The timer/callback based approach to window resizing. Having a mechanism in place that has potential to recursively cascade is definitely inadvisable
I'm not so sure on what else to do either... nasty things like automatically window's state being put back to normal will happen if we do window state transitions too fast... One thing I can do is terminate the chain early if the window state changed to the correct one already. However given the unstableness of all the stuff in Qt... I'd rather not really do that and wait a moment before we finalize the window state change. That'd make MINIMIZE putting into work because by the time the next extra WindowState==NORMAL change event emmitted whenever we put stuff into MINIMIZED. Would you like me to try that?
EDIT -- I am adapting that approach. Seems to work.
EDIT 2 -- Actually... it didn't. @freakboy3742 The problem is like this -- if you stuff in too many window state transitions too fast, weird things happens. So we need to delay the clearing of pending window state; that way we can achieve the effect of "no one do anything in the following 100ms". So the approach I suggested ultimately did not work out as intended.
I'll be attaching some diagrams here showing some stuff about this tomorrow.
@freakboy3742 This is what the window state transitions look like when you reviewed. A few labels are in the diagram below -- the first timeline is the current state of this PR which is a fix I tried to prevent recursing MINIMIZED (look at the third diagram that the first approach catches the first MINIMIZED event), however it's flawed.
What I'm trying to get at here is that in the third diagram, we see that the cause of the recursion is that Qt under Wayland can't round-trip the correct window state a while later. MINIMIZED is sort of special here that it has this bug. But all the other window states would round-trip correctly under both X11 and Wayland. But then I'd argue that if reading the window state isn't even reliable, we'd just dissupport window states entirely. But reading the window state is reliable in non-minimized case. To summarize -- recursion only happens when you request a window state but it consistently does not roundtrip correctly, and if this happens in any other cases, they should be pretty rare and in my opinion it would be obvious what the issue is.
Comments on this?
@freakboy3742 The CI is green and I have responded to almost all comments we have made and made modifiations for the rest accordingly. I need response on the things I'm asking you inline for, and another review from you. Thanks for working me through this!
P.S. Sorry for the repeated small commit pushes, my laptop is literally burning as I'm writing a draft for a book while I'm waiting for CI, I have a VM running that I just realized I forgot to close, so I did not run the testbed locally before pushes.
Comments on this?
... I don't think this has clarified things as much as you think it has.
What I'm trying to get at here is that in the third diagram, we see that the cause of the recursion is that Qt under Wayland can't round-trip the correct window state a while later. MINIMIZED is sort of special here that it has this bug. But all the other window states would round-trip correctly under both X11 and Wayland. But then I'd argue that if reading the window state isn't even reliable, we'd just dissupport window states entirely. But reading the window state is reliable in non-minimized case.
It seems like you've missed a middle ground here: allow state transitions, and document (for now, at least) that retrieving the MIMIMIZED window state on Wayland may not work, skipping any tests that exercise that code path.
Again - we don't need to solve all problems at once. We need an initial proof of life that this backend "works" for some non-trivial definition of "work", with an architecture that passes the sniff tests, with a test suite that runs reliably without generating test failures. The feature set you've got here definitely meets the first criterion. We can iterate on new features once the core Qt support lands.
Right now, I'm trying to nail down the last of the "sniff test" items. An architecture for state changes that is prone to recursive behavior definitely doesn't pass that test.
@freakboy3742 I've reworked the architechture to be much simpler to make it block the next 100ms whenever a window state change occurs (or is requested, in case there is another request between the request and the signal back from Qt). If in the 100ms block someone requests another state change from the programatic side, we buffer it and at the end of the 100ms, check for if the program requested a state change (NOT if the requested state change matches the current state), then we reapply. Multiple change events from the user in rapid succession will keep blocking until 100ms from the last one.
I'll wait for test pass, and rerequest a review. All items should be resolved.
EDIT -- To clarify: this will no longer handle rapid changes correctly if the user mashes keys or presses MINIMIZE in the middle of the changes, but you said in the Qt issue that this is not a major concern (as opposed to an infinite-loop architechutre).
@johnzhou721 Ack - I'll take a look as soon as I can; early next week seems most likely.
@freakboy3742 FYI -- While updating the support table, I also corrected some errors in it -- they should be fairly obvious.
@freakboy3742 I've addressed all your comments, and added 4 in the inline conversations. Please review again and let me know of anything that needs changing. Thank you!