Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve remaining Qt wrapper selection UX issues #7741

Merged
merged 30 commits into from
Jun 14, 2023

Conversation

The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jun 13, 2023

This tries to resolve various UX issues (and code-style issues) with the initial Qt wrapper prototype, turning it into something I'm comfortable with unleashing on users 馃檭

In particular, it:

  • Changes the wrapper selection to happen by explicitly calling machinery.init(), rather than doing it implicitly at import-time of machinery.py. This ensures that we can do additional logic (more complex than just reading from an environment variable) when initializing qutebrowser. Basically, anything that can be done during early initialization (before any Qt imports, i.e. mostly qutebrowser.py and earlyinit.py) is now fair game.
    • This means that the globals in machinery.py are unset and cause a NameError before initialization is done. This is deliberate: That way we will detect any unintended access to them before initialization, but without having to create a magic NotYetKnown() object which raises in __eq__ and/or __bool__.
  • Adds a machinery.init_implicit() which performs an implicit initialization if any of the qutebrowser.qt.* modules is imported, but init() has not been called so far. This is needed so that we can still import qutebrowser modules in tests, for some linters, for interactive usage during development, etc.
  • Exposes more information about how the wrapper selection has happened (and any ImportError exceptions encountered with details) via a SelectionInfo dataclass and SelectionReason enum.
  • Shows that information in all places where it seems relevant:
    • :version
    • logging on startup (later in earlyinit.py, as machinery.py is too early to use qutebrowser.utils.log, due to that having Qt dependencies)
    • Error messages in earlyinit.py, e.g. QtCore/QtWidgets not being found
    • Error messages in backendproblem.py, e.g. QtWebKit/QtWebEngine not being found
  • Improves the upgrade/downgrade messages a bit by using an information icon for upgrades, emphasizing some more text, showing the relevant data directory, and at least giving a pointer on how to override the wrapper selection.
  • When initialized explicitly, defers any error messages to earlyinit.py which can show a graphical dialog again (using Tk if needed) instead of showing an error on stdout only.
  • Makes sure that no unintended import errors are hidden during autoselection. Only a ModuleNotFoundError should lead to the tried wrapper to be skipped. Any other ImportError is an unintended breakage of the setup, and leads to the wrapper selection process being aborted, and the error being surfaced by earlyinit.py.
  • Fixes linting issues and adds docstrings (with Qt documentation links) to all Qt wrapper modules
  • Does not enable autoselection by default yet (this should be done as soon as Qt 6.5.2 is in Archlinux, probably!). However, it already exposes it via QUTE_QT_WRAPPER=auto for any brave early adopters (and tests).
  • Adds a --qt-wrapper which has precedence over QUTE_QT_WRAPPER, very useful during development.
  • Adds lots of tests for all the new logic in machinery.py.
  • Simplifies the logic in qutebrowser/qt/sip.py a bit.
  • Updates the constants for mypy in tox.ini to drop USE_PYSIDE2 (we decided to not support that due to many missing APIs), but add IS_PYQT and IS_PYSIDE from machinery.py.

I will add some screenshots and answers to #7656 in a follow-up comment over there.

Closes #7656
See #7202

Contains the exact same string we have in .WRAPPER already anyways.
This means we will now get errors via the usual mechanisms
(e.g. a Tk error dialog) when all Qt wrappers failed to import.

We also add information about the picked Qt wrapper (and any errors)
to the error message.
This also moves the checking for sys.modules into _select_wrapper.
Copy link
Member

@toofar toofar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a huge improvement! You've managed to thread quite a few threads neatly through the new SelectionInfo class and a few supporting functions which made this pretty easy to review.

I haven't ran this up locally or thought too critically about messaging and such since I don't have much time available. But I've read through the code changes a few times looking for broad themes I disagree with or blockers I could spot and I haven't seen anything so I fully support this being merged as is.

I've left some line comments about a few things, nothing blocking.

And some rambling non-committal comments while I sign off: I don't fully understand the implicit vs explicit init thing and the NameError if init() isn't called first thing. Like there is error messaging in the machinery module that tells you you did something wrong in the init module, which seems like a bit of coupling. There's probably a good reason for it but it's not in my head at the moment. Same for the implicit init thing. Not quite getting why that is needed and what alternatives there are. It seems like there is copypasta being added to every qt module just for tests, which makes me feel a bit suspicious that maybe it isn't the perfect solution.

tests/unit/test_qt_machinery.py Outdated Show resolved Hide resolved
qutebrowser/qt/machinery.py Outdated Show resolved Hide resolved
qutebrowser/qt/machinery.py Outdated Show resolved Hide resolved
qutebrowser/qt/machinery.py Outdated Show resolved Hide resolved
@The-Compiler
Copy link
Member Author

The-Compiler commented Jun 14, 2023

And some rambling non-committal comments while I sign off: I don't fully understand the implicit vs explicit init thing and the NameError if init() isn't called first thing. Like there is error messaging in the machinery module that tells you you did something wrong in the init module, which seems like a bit of coupling. There's probably a good reason for it but it's not in my head at the moment. Same for the implicit init thing. Not quite getting why that is needed and what alternatives there are. It seems like there is copypasta being added to every qt module just for tests, which makes me feel a bit suspicious that maybe it isn't the perfect solution.

The idea there was that there is basically two ways that an import qutebrowser.[...] (which in turn most likely will import qutebrowser.qt.[...]) can happen:

  • When using qutebrowser normally as an application, where we can have a controlled initialization sequence, and take things like --qt-wrapper into account
  • Anywhere else. That includes tests, but also things like using the REPL and doing:
>>> from qutebrowser.utils import utils
>>> utils.format_seconds(123)
'2:03'

to try something out.

For the latter case, I'd still like that to work, without having to carefully do from qutebrowser.qt import machinery and machinery.init() manually every time something imports a qutebrowser module.

Thus, importing anything without an explicit init first will implicitly ensure that things are initialized - but if that happens in qutebrowser as an application, that would be a bug, because it's not supposed to import anything from qutebrowser.qt.* before we can call machinery.init() and take things like --qt-wrapper into account.

Does that make things clearer? Thanks for the review! Going to look at the things you commented on and then merge this, but not sure if I'll get around to it today.

@The-Compiler The-Compiler merged commit e8ed81d into master Jun 14, 2023
29 of 57 checks passed
@The-Compiler The-Compiler deleted the better-qt-wrappers branch June 14, 2023 18:04
@The-Compiler The-Compiler mentioned this pull request Jun 14, 2023
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

UX requirements around switching to PyQt6 by default.
2 participants