-
-
Notifications
You must be signed in to change notification settings - Fork 781
New Backend - Qt #3769
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?
New Backend - Qt #3769
Conversation
@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:
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?
Thank you! |
@freakboy3742 Could you please respond to those questions I've asked about above? Thank you! |
No. Platform identification is only used because You'll also note that the current GTK backend doesn't describe its platform as GTK - it's 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,
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
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 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? |
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.
For my own benefit. Deleting comments after they get applied.
class AppSignalsListener(QObject): | ||
appStarting = Signal() | ||
|
||
def __init__(self, impl): | ||
super().__init__() | ||
self.impl = impl | ||
self.interface = impl.interface | ||
self.appStarting.connect(self.on_app_starting) | ||
QTimer.singleShot(0, self.appStarting.emit) | ||
|
||
def on_app_starting(self): | ||
self.interface._startup() | ||
|
||
|
||
appsingle = QApplication() |
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.
The logic right here must get rewritten a bit; rather than storing appsingle in app.py, it'd be safe to move it to factory.py with a single QApplication() call and use QApplication.instance in App class below. The SignalsListener thing I had must be refactored as well -- we could directly call the startup function or directly QTimer.singleShot it; no need for an extra signal.
s for s in screens if s != primary | ||
] # Ensure first is primary | ||
|
||
return [ScreenImpl(native=monitor) for monitor in QGuiApplication.screens()] |
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.
Likely typo here, should be for monitor in screens. Will need to fix later
def __del__(self): | ||
self.native = None |
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.
TODO -- investigate if this is needed.
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.
Seems to be copied from iOS and macOS and something about constraints...
@property | ||
def top_offset(self): | ||
return 0 ## Stub (?) |
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.
Needs to address this, seems like unreached code from iOS copying
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 shim is moved into factory.py; no longer nessacary.
async def restore_standard_app(self): | ||
pytest.skip("not impld") | ||
|
||
async def open_initial_document(self, monkeypatch, document_path): | ||
pytest.skip("not impld") | ||
|
||
def open_document_by_drag(self, document_path): | ||
pytest.skip("Not impld") |
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.
These need to get audited.
async def undo(self): | ||
pytest.skip("Undo not supported by default on widgets") | ||
|
||
async def redo(self): | ||
pytest.skip("Redo not supported by default on widgets") |
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.
These will need to actually get impld
# [better not write anything more here just in case of | ||
# anything about size accidentally being an inappropriate | ||
# joke] | ||
pass |
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.
Missing a probe here
def assert_taller_than(self, initial_height):
pass
assert app.in_presentation_mode | ||
assert window1_probe.instantaneous_state == WindowState.PRESENTATION | ||
# Do this assertion after in order to give platforms that cannot support | ||
# window states a chance to exit this test by skipping in instantaneous_state | ||
assert app.in_presentation_mode |
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.
hack is outdated. windowstates are supported on Wayland now, this should be removed.
Effectively, yes.
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
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
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.
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. |
Noted.
Will look into this.
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: aka. Really similar to what Apple has been doing with their menubar icons...
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! |
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.
I've done a quick review of the testbed; the broad strokes look good, but I've flagged a couple of specific improvements.
if backend_override is not None: | ||
toga_backend = backend_override | ||
else: |
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.
Is this needed? Can't you use the TOGA_BACKEND
approach that is already there?
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.
🤦 yes.
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.
Wait hold on... no. There's only 1 backend installed, and figuring out what backends are installed here would be... baroque. So an override is easier 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 any opinions on this? Toga core exploits the fact that there is only toga_qt installed so it uses that; however that logic would be too redundant to replicate in this testfile.
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.
There's only 1 backend installed in CI, but that won't necessarily be true on a desktop setup (at least, not until beeware/briefcase#1735 is resolved). For now, I think I agree that duplicating the logic isn't worth it; if we can't use the core logic as-is, then lets assume there's only one backend installed in the testbed environment and just document the limitation.
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.
Sure, let's leave this unresolved for marking the documentation TODO (I haven't gotten around to docs yet... I
m busy).
@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? |
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.
More things for me to fix...
DISPLAY=:99 blackbox & | ||
sleep 1 | ||
briefcase-run-prefix: 'DISPLAY=:99' | ||
briefcase-run-prefix: 'DISPLAY=:99 TOGA_BACKEND=toga_gtk' |
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.
Now unnessacary, since only 1 backend is installed
mutter --nested --wayland --no-x11 --wayland-display toga & | ||
sleep 1 | ||
briefcase-run-prefix: "WAYLAND_DISPLAY=toga" | ||
briefcase-run-prefix: "WAYLAND_DISPLAY=toga TOGA_BACKEND=toga_gtk" |
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.
See above
From a short term pragmatic solution, I'm not too concerned either way. 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. |
WIP: Fixes #1142
PR Checklist: