(Py)Qt6 readiness: Use typed enums by ferdnyc · Pull Request #4833 · OpenShot/openshot-qt

ferdnyc · 2022-06-22T23:36:04Z

I've been plugging away, the past few weeks, at trying to come up with a PyQt6-compatible OpenShot. It's very slow going. (Libopenshot was easy, literally one line in the GUI code for the openshot-player tool had to be changed. That's it! Other than that, it's already fully Qt6-compatible. OpenShot? Not so much.)

The changes necessary to make it work are huge, and because Riverbank chose to make a clean break and drop everything that's deprecated from PyQt5 (even the stuff that Qt, itself, has kept in the Qt5 Compatibility Layer module), it may not be (realistically) possible to simultaneously support PyQt5 and PyQt6 in the same codebase. Branching and maintaining parallel versions, at least for a period, may be the only realistic option, unfortunately.

Because let's face it, eventually OpenShot will need to make the move to PyQt6. So, there are plenty of things that can be done now to get it at least "more ready", since a lot of mandatory-in-PyQt6 features are already available today in PyQt5, and have been for some time. We just haven't adopted them yet.

I'm going to make an effort to push as many of those adaptations as possible back into the PyQt5 code, so that the PyQ6-compatibility transition is at least less painful and world-breaking. This PR is the first of those changes.

I'm also going to make a plea for a bit of fast-tracking on this one, because while it's very focused, it's also very sweeping, and edit conflicts will crop up quickly if it's left open and unmerged while other changes get committed ahead of it. Obviously some conflicts are likely to come up, and I'll fix those as they come, but there is a certain degree of time-sensitivity to this PR, if no these changes. (Which are, in one sense, admittedly "totally unnecessary", but I hope I've adequately sold the reasons why they're still a good idea.)

Adoption of typed enums.

In PyQt6, in order to provide better type safety, enum values are no longer all lumped together under their owner classes, or thrown in a big pile inside PyQt6.QtCore.Qt.

Now, every enum has its own, distinct type, which means that enum
values can be type checked. It's no longer possible to do something
crazy like call QImage.scale(newSize, Qt.SolidPattern) and have
that work, because Qt.SolidPattern has the value 1 which is
interpreted by QImage::scale() as the mode Qt.KeepAspectRatio.
Mypy won't catch it either, because in PyQt5
Qt.SolidPattern == Qt.KeepAspectRatio returns True.
After all, they both have the integer value 1.

Safety through precise typing

In PyQt6 the enums have all been separated from each other.
They're partitioned into type silos, each type holding only
a set of members that represent all the possible values for
that specific enum.So even though KeepAspectRatio has the value 1 in the Qt.AspectRatioMode enum, and SolidPattern is 1 in the Qt.BrushStyle enum, in PyQt6 they won't be equal or interchangeable with each other, because they'll have their own, separate type classes.

Aspect ratio modes have the type Qt.AspectRatioMode.
Brush styles have the distinct type Qt.BrushStyle.

...Which means they have to be accessed that way. Exclusively that way, in PyQt6.

So, come PyQt6:

Qt.KeepAspectRatio becomes Qt.AspectRatioMode.KeepAspectRatio
Qt.SolidPattern becomes Qt.BrushStyle.SolidPattern
QImage.Format_ARGB32 becomes QImage.ImageFormat.Format_ARGB32

And so on, for Every. Single. Enum!

Like Qt.green? Instead cozy up to Qt.GlobalColor.green.
Friends with Qt.Key_Escape? Embrace Qt.Key.Key_Escape.
Want Qt.AA_ShareOpenGLContexts? Hope you're ready for
Qt.ApplicationAttribute.AA_ShareOpenGLContexts.

It does make for much stronger typing, which when combined with
mypy and code that's type hinted means stronger code, period.

This comparison:
Qt.AspectRatioMode.KeepAspectRatio == Qt.BrushStyle.SolidPattern
will now properly return False, yay!

The per-enum classes are already present in PyQt5, they're just
not type-siloed into distinct, non-equivalent classes. (That
previous comparison still returns True, in PyQt5.) But they're
there, so we can choose to start using them now, since we'll
HAVE to eventually.

But it's definitely a big adjustment. (At least by making the
switch before it's forced, we have a safety net in the event
that we've missed any old-style enum accesses. Instead of a
traceback, the code will still function as expected. Only in
PyQt6 do the old-style "short" enums get taken away.)

Secondary changes

The PR also carries a couple of minor additional changes that got swept up in the enum typing. (Meaning, they were changes that also appeared on lines where enums changed, so they got committed together.)

It replaces all of our remaining QRegExp usage with QRegularExpression. QRegExp has been deprecated long time now, and it's removed from PyQt6. QRegularExpression has been the official replacement since Qt 5.12.
It replaces some uses of outdated, aliased, trailing-underscore methods with their unaliased counterparts. (Mostly QDialog/QMessageBox .exec_() calls, which haven't required the trailing underscore for, again, a long time. Since Python 3.0, in fact. Python 2 was picky about builtin words as class methods, but Python 3 isn't, so it's fine to just call QDialog.exec() and those aliases have been deprecated for a while. They're also removed in PyQt6.)

I'll submit a separate PR at some point that fixes the rest of them, but a few got swept up in this change.

Note, BTW, that certain methods are still aliased, if they're Python KEYWORDS. Like, QWidget::raise() is still PyQt{5,6}.QtWidgets.QWidget.raise_(), because calling a method raise is still a terrible idea in any version of Python.

In PyQt6, in order to provide better type safety, enum values are no longer all lumped together under their owner classes, or thrown in a big pile inside `PyQt6.QtCore.Qt`. Now, every enum has its own, distinct type, which means (among other things) that enum values can be type checked. * Aspect ratio modes have the type `Qt.AspectRatioMode`. * Brush styles have the distinct type `Qt.BrushStyle`. So in PyQt6: - `Qt.KeepAspectRatio` becomes `Qt.AspectRatioMode.KeepAspectRatio` - `Qt.SolidPattern` becomes `Qt.BrushStyle.SolidPattern` - `QImage.Format_ARGB32` becomes `QImage.ImageFormat.Format_ARGB32` While still staying within PyQt5, convert our enum accesses over to the longer, class-member form of access, so that the code is more future-proof. (Typing benefits still aren't available until PyQt6. Even if accessed as class members, two members from different enums with the same underlying numeric value will test as equal.)

jonoomph · 2022-06-23T00:52:01Z

PR: Support for PyQt6 (initial steps)

jonoomph · 2022-06-23T00:58:54Z

src/windows/region.py

-        self.buttonBox.addButton(self.process_button, QDialogButtonBox.AcceptRole)
-        self.buttonBox.addButton(self.cancel_button, QDialogButtonBox.RejectRole)
+        self.cancel_button = self.buttonBox.addButton(
+            QDialogButtonBox.StandardButtons.Cancel)


Will StanardButtons.Cancel be translated? Our previous code had a manually translated _('Cancel')

jonoomph · 2022-06-23T01:01:36Z

src/windows/views/add_to_timeline_treeview.py

        self.setIndentation(0)
-        self.setSelectionBehavior(QTreeView.SelectRows)
-        self.setSelectionBehavior(QAbstractItemView.SelectRows)
+        self.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows)


This line looks like a duplicate

Good call, my regular-expression-based updates even turned them into identical duplicates, one can clearly go.

jonoomph · 2022-06-23T01:02:06Z

src/windows/views/blender_listview.py

    def Cancel(self):
        """Cancel the current render, if any"""
-        #QMetaObject.invokeMethod(self.worker, 'Cancel', Qt.DirectConnection)
+        #QMetaObject.invokeMethod(self.worker, 'Cancel', Qt.ConnectionType.DirectConnection)


Commented line can be removed

jonoomph · 2022-06-23T01:02:32Z

src/windows/views/changelog_treeview.py

-        self.setSelectionBehavior(QAbstractItemView.SelectRows)
-        self.setSelectionMode(QAbstractItemView.ExtendedSelection)
-        self.setSizePolicy(QSizePolicy.Expanding, QSizePolicy.Expanding)
+        self.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows)


Duplicate line of code

jonoomph · 2022-06-23T01:02:52Z

src/windows/views/credits_treeview.py

-        self.setSelectionBehavior(QAbstractItemView.SelectRows)
-        self.setSelectionMode(QAbstractItemView.ExtendedSelection)
-        self.setSizePolicy(QSizePolicy.Expanding, QSizePolicy.Expanding)
+        self.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows)


Duplicate line of code

jonoomph · 2022-06-23T01:06:15Z

src/windows/views/tutorial.py

-        self.win.dockProperties.visibilityChanged.connect(functools.partial(self.process, "dockProperties"))
-        self.win.dockVideo.visibilityChanged.connect(functools.partial(self.process, "dockVideo"))
-        self.win.dockEmojis.visibilityChanged.connect(functools.partial(self.process, "dockEmojis"))
+        for d in [


This is actually more lines of code than he original, with the humble brag of not repeating any code, lol

I ask you... which version is easier to glance at and understand?

Fair question, but I'll throw one back at you: Which version will crash if self.win doesn't have a dockProperties attribute, or has one that's not a QDockWidget / lacks a visibilityChanged signal, and which version won't? The intent isn't merely to avoid cut-and-paste, it's to avoid assumptions about the structure of external classes.

jonoomph · 2022-06-23T01:11:15Z

A few Codacy issues:

src/windows/process_effect.py: Undefined variable 'Qt'
src/windows/process_effect.py: Undefined variable 'QSizePolicy'
src/windows/process_effect.py: Undefined variable 'QDialogButtonBox'
src/windows/process_effect.py: Undefined variable 'QDialogButtonBox'
src/windows/region.py: Undefined variable 'QSizePolicy'
src/windows/region.py: Undefined variable 'QDialogButtonBox'
src/windows/title_editor.py: Undefined variable 'QFontInfo'

jonoomph · 2022-06-23T01:12:47Z

@ferdnyc Thanks for taking a shot at this one! It's a big one, which seems fairly high-risk (as with any PR that changes 40+ files I suppose, lol). I'll give this a try locally (and hopefully @JacksonRG can do the same local testing). Let us run through a full set of regression tests, to verify we don't see a bunch of stack-traces. 👍 We'll keep you posted!

…his is mostly work from ferdnyc and #4833. I've made a few additional changes (formatting, bug fixes)

jonoomph · 2022-06-23T23:34:54Z

@ferdnyc Moving this conversation to #4836 (closing this one)

jonoomph · 2022-06-24T20:59:37Z

Re-opening this PR, and waiting for changes from branch: https://github.com/OpenShot/openshot-qt/tree/qt6-prep-enums to be applied to this one.

github-actions · 2022-06-29T23:32:50Z

Merge conflicts have been detected on this PR, please resolve.

github-actions · 2022-06-30T19:56:30Z

Merge conflicts have been detected on this PR, please resolve.

pva · 2024-06-13T10:33:55Z

@ferdnyc so what's the status of this pull request? I found only two MR both closed and it looks like job was stalled. Still, some distributions (e.g. Gentoo) already try to avoid qt5 (https://bugs.gentoo.org/925718) for a good reason, so this job is appreciated!

ferdnyc · 2024-06-13T14:05:18Z

@pva There is no status. I am no longer involved in the project.

jonoomph reviewed Jun 23, 2022

View reviewed changes

jonoomph added a commit that referenced this pull request Jun 23, 2022

Updating code base (especially Qt enums) to be compatible with Qt6. T…

f9b1102

…his is mostly work from ferdnyc and #4833. I've made a few additional changes (formatting, bug fixes)

jonoomph mentioned this pull request Jun 23, 2022

Take 2: Qt6 readiness: Use typed enums #4836

Closed

jonoomph closed this Jun 23, 2022

jonoomph reopened this Jun 24, 2022

github-actions bot added conflicts A PR with unresolved merge conflicts and removed conflicts A PR with unresolved merge conflicts labels Jun 29, 2022

ferdnyc closed this Jul 7, 2022

ferdnyc deleted the pyqt-typed-enums branch June 13, 2024 14:05

Vekhir mentioned this pull request Jun 25, 2024

Move to typed enums from QtCore.Qt #5548

Open

Conversation

ferdnyc commented Jun 22, 2022

Adoption of typed enums.

Safety through precise typing

Secondary changes

Uh oh!

jonoomph commented Jun 23, 2022

Uh oh!

jonoomph Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonoomph commented Jun 23, 2022

Uh oh!

jonoomph commented Jun 23, 2022

Uh oh!

jonoomph commented Jun 23, 2022

Uh oh!

jonoomph commented Jun 24, 2022

Uh oh!

github-actions bot commented Jun 29, 2022

Uh oh!

github-actions bot commented Jun 30, 2022

Uh oh!

pva commented Jun 13, 2024

Uh oh!

ferdnyc commented Jun 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonoomph Jun 23, 2022 •

edited

Loading