Support for PyQt6 (and PySide6) #575
Conversation
|
Wow, you are amazing. I hope there is some kind of tool that helped you with the transition and that you did you have to to everything by hand 🙈 My general thoughts on the transition to PyQt6 (I have not yet had a close look at your code and the changes); Is there a strong necessity for supporting both version at the same time? I.e. are there some distros out there that do not support PyQt6 yet? Or are there some unforeseen issues we may get with PyQt6? Or do we want to maintain PyQt5 support only for people who do not yet want to update for some personal reason? While having a smooth transition with a transition period from PyQt5 to PyQt6 would be favorable, I am thinking about the economy of this approach. If it induces a multiple-hours overhead, compared to directly updating, I would argue that, having the limited manpower we have, it is not really feasible. Concerning the performance issues; What kind of profiling tool did you use? I will certainly look into it, but not right now... First I would like to get #467 done 😊 |
|
This has been done before, e.g. by qutebrowser, so I didn't have to do too much thinking 😂 I do think it makes sense to have the wrapper module in any case, no matter what we end up supporting, as we can e.g. also use it for PyQt6 vs. PySide6 and it really helps with experimenting. And, given most of it just works, I would leave it in until PyQt5 is basically dead. It probably makes sense to drop support for PyQt < 5.15 rather soon though, as those have some extra issues that require manual workarounds. The super-fancy Thanks, and no rush, the other open PRs are definitely more important atm 😊 |
In addition to PyQt5, the qt wrapper now also allows importing from PyQt6. The user can pick which PyQt version to use using an environment variable. Currently PyQt6 is still completely broken.
Tests run, application seems to run fine, issues with Svg known.
We can use type(QObject) to get its metaclass instead.
We still had some hard-coded lazy imports to PyQt5, which, obviously, don't mix well with PyQt6.
end2end tests unfortunately still broken, but we do get a startup. Due to the broken tests, unsure what else is still not working.
Fuzzy completion does not work with QRegularExpression for PyQt < 5.15.
Still a lot of complaints due to no-member which need to be sorted out.
* Remove hard-coded PyQt5 imports * Adapt flags
Catches and fixes a few more issues :)
There are still multiple unsolved issues. We leave it in explicitly though, so further testing and integration remains simple.
|
Since mypy, lint and tests now do what they should, the PyQt5 / PyQt6 part is now essentially merge-able and definitely ready for review 🎉. I disabled auto-detecting PySide6 for now as there is a lot more work to do. As this is always a hassle to rebase (will probably just merge After the release, I would like to prioritize getting this into In case bugs come up, we can always release a follow up |
|
Another note: replacing the Details: # slow
@api.objreg.register
def __init__(self):
...
# fast
def __init__(self):
api.objreg.register(self)As this is a pretty major issue, I think changing the behaviour, while keeping backward compatibility and logging a deprecation warning, is fine. |
|
|
||
| def _select_wrapper(): | ||
| """Select a python Qt wrapper version based on environment variable and fallback.""" | ||
| env_var = "VIMIV_QT_WRAPPER" |
There was a problem hiding this comment.
Why do you introduce a custom env variable instead of relying on the standardized QT_SELECT=5/6?
There was a problem hiding this comment.
How would we differentiate between PyQt6 and PySide6?
There was a problem hiding this comment.
Just take whichever is available. I doubt that users have both installed. And if so, why should they care which one is used?
There was a problem hiding this comment.
I care for testing both without changing the source code all the time 😄 I do think it is nice to expose this option in any case, but we could split it into QT_SELECT and USE_PYSIDE, if you think this has some benefit? Personally, I find one easier than two though.
There was a problem hiding this comment.
True, for development/testing/debugging this would certainly make sense to explicitly choose between QT and Side. I just think that if there is a standard, it would make sense to follow it. What about having only QT_SELECT but allow not only the values 5/6 but also the explicit options PyQt5/PyQt6/PySide6?
There was a problem hiding this comment.
I like this compromise 👍 So our QT_SELECT would then have:
- 5 -> PyQt5
- 6 -> PyQt6 or PySide6 with a preference that we set
- all three explicit options as currently done by
VIMIV_QT_WRAPPER
|
Thanks a lot for your hard work on this. I had a glimpse at the new QT module and that makes sense so far. To me it seems like there are no major changes between QT5 and QT6, except a few methods renames. Or did I miss anything? The rough roadmap makes totally sense to me. Everything for The issue with |
|
Thanks for checking! 😊 Yes, except for a few renames / deprecations, and the annoying new enum access, nothing major implementation-wise. Sounds good 👍 I will try to release the new version over the week-end then. I see two (maybe three) options:
Personally I am leaning towards 1. as it actually also makes the tests a bit cleaner (no need to hack-mock decorators). But from a "plugin-writer" perspective 2. is definitely easier, sure. EDIT: Forgot the third option, somehow figuring out how to get an instance without using the |
Awesome, let me know if I can help with anything!
I need to have a close look at it and investigate a bit. Do you know how other projects deal with that? |
|
Release is out, haven't heard any complaints yet 😄 I remember qutebrowser having an The way it is usually done is probably something along:
|
|
Thanks and congrats for a new release! That one took quite some time 😁 I have rebased the branch for the current master in my fork. I have also changed the env variable to |
|
Thanks, I will take a look once I find time for more changes. However, I have also merged some stuff locally without pushing into another branch and played around with the EDIT: I pushed this to New "issue" is that the PyQt6 install actually seems to "support" pdfs. I tested this with a quick function, and we do get a preview of the first page as discussed ages ago in #525. I would add pdfs to the ignore part for now, and re-consider this in a new PR if this is fine with you, as I am not sure what we want here. |
I have also created a testcase for it (same commit). I am not sure if you have seen that. Nothing amazing, but better then nothing, I guess.
Right, but they do not seem to be completely happy abut it (not related to our issue though...). I have investigated into the objreg weirdness. I have condensed the issue down to the following MVE: import time
from PyQt5.QtCore import QObject as QObject5
from PyQt6.QtCore import QObject as QObject6
def timed(func):
def inner(*args, **kwargs):
start = time.time()
return_value = func(*args, **kwargs)
elapsed_in_ms = (time.time() - start) * 1000
print(f"{func.__qualname__}: took {elapsed_in_ms:.3f} ms")
return return_value
return inner
def register_decorator(func):
def inner(component) -> None:
cls = component.__class__
cls.instance = component
func(component)
return inner
def register_function(component):
cls = component.__class__
cls.instance = component
class DecoQt5(QObject5):
@timed
@register_decorator
def __init__(self) -> None:
super().__init__()
print("QT5 decorator")
class FuncQt5(QObject5):
@timed
def __init__(self) -> None:
super().__init__()
print("QT5 function")
register_function(self)
class DecoQt6(QObject6):
@timed
@register_decorator
def __init__(self) -> None:
super().__init__()
print("QT6 decorator")
class FuncQt6(QObject6):
@timed
def __init__(self) -> None:
super().__init__()
print("QT6 function")
register_function(self)
a = DecoQt5()
b = FuncQt5()
c = DecoQt6()
d = FuncQt6()
assert hasattr(DecoQt5, "instance")
assert hasattr(FuncQt5, "instance")
assert hasattr(DecoQt6, "instance")
assert hasattr(FuncQt6, "instance")The big difference between the So, our issue is not the decorator, but the execution order. I think we can just move the diff --git a/vimiv/api/objreg.py b/vimiv/api/objreg.py
index cb974c3a..8dc350b4 100644
--- a/vimiv/api/objreg.py
+++ b/vimiv/api/objreg.py
@@ -57,10 +57,10 @@ def register(component_init: Callable) -> Callable:
Args:
component: Corresponds to self.
"""
cls = component.__class__
_logger.debug("Registering '%s.%s'", cls.__module__, cls.__qualname__)
+ component_init(component, *args, **kwargs)
cls.instance = component
- component_init(component, *args, **kwargs)
return inside
Which part of QT is supporting PDF? I only found this, but that is specific to PySide6. As long as the they are not detected by |
|
I did see it, but the comment "The test case is failing, as it should skip tests, based on installed QT binding." made me think this is a problem for future us 😄 Yep, I actually remember seeing this issue ages ago and switching from names to types, to at some point actually just having That is an excellent catch, thanks for digging!! 😊 I reverted the function commit and simply changed the order in the What I mean with "supports" is that Fixed by adding the pdf into the |
That is weird, I checked their docs, and PDFs should not be supported. Have you installed any PDF QT module? Does it also happen in the CI or only locally? Do you know what is going wrong here? I have regenerated the .venv. |
|
Both in CI and locally, I mean, Yep, |
Ah sure, silly me 🙈
True, same for me. Is there anything else I can help with concerning this PR? |
|
Perfect, not really, in case you have no further comments, I would go ahead and finalise documentation and merge into master 😊 |
|
No, from my side everything is good 👍 |
Initial attempt at supporting PyQt6 and potentially PySide6 by writing a Qt wrapper module
vimiv.qtwhich must be used instead of importing from PyQt* / PySide6 directly.This went surprisingly smooth, but still has a few rough edges, and some are breaking before being able to merge:
PyQt6:
no-membererrorsPySide6 additionally:
I also noticed performance issues upon startup with PyQt6 on my local laptop. These do not seem to affect the CI speed, and are also not relevant after startup, which puzzles me. Namely, the instantiation of any QWidget seems to be really slow,
__init__of the main window takes several hundred ms instead of less than 30 with another wrapper.If anyone, @jcjgraf 😉, would like to test this, that would be super helpful.
Going forward, I believe it makes sense to merge this after releasing
v0.9.0, but explicitly calling support for PyQt6 and especially PySide6 experimental. If we don't merge soon-ish, there will continue to be a bunch of merge conflicts for obvious reasons. This would probably mean we need av0.10.0with this in, before going tov1.0. However, we could still release both of these versions with PySide6 if issues are too large, but PySide6 is the "official" Qt for python now, and the performance issues are not there for me.fixes #318