Skip to content

Convert Qt Enums to Qt6 Namespace#1818

Merged
j9ac9k merged 8 commits intopyqtgraph:masterfrom
j9ac9k:convert-enums
Jun 9, 2021
Merged

Convert Qt Enums to Qt6 Namespace#1818
j9ac9k merged 8 commits intopyqtgraph:masterfrom
j9ac9k:convert-enums

Conversation

@j9ac9k
Copy link
Copy Markdown
Member

@j9ac9k j9ac9k commented Jun 6, 2021

While doing benchmarking for #1796 I noticed on PyQt6 bindings, a lot of time was being spent shuffling the Enum namespace during the initialization while loading Qt.py. The benchmark I used was to update the line plot 500 times. 43% of the runtime was spent in Qt.py, of which promote_enums consumed 38% of the overall runtime. Attached is the output of of the call-graph.

It was brought to my attention by @The-Compiler that the Qt6 Enum namespace has been present in Qt5 from at least Qt 5.12; and after doing some testing, sure enough it was; so decided a migration was in order.

The fantastic PyQtEnumConverter library handled the bulk of the work. It missed a lot of cases of self.EnumValue, but luckily the test suite was more than happy to error out along the way. As I patched known issues in the test suite, I used my editors "find all" functionality to find other usages of those particular enums to try and convert cases that the test suite had no coverage of. I think I got them all at this point.

Lastly, I took the opportunity to remove Qt 6.0 support from the README and some of the specific shims we had in place for it. I have a hard time imagining what a use-case for someone needing Qt 6.0 support when 6.1 is released.

pyqt6-plotlinespeedtest

Sorry for the huge diff, I don't mean to give everyone head-aches with merge-conflicts.

tagging @pijyoi due to their work in making the enum namespace equivalent; figure you may have some input

Looking through the diff before I hit submit, I see one of my changes in MultiPlotSpeedTest.py got caught up. I don't mind restoring it if it would make people feel better.

QtGui.QPainter.CompositionMode.CompositionMode_Plus Both the alpha and color of the image and background pixels
are added together.
QtGui.QPainter.CompositionMode_Multiply The output is the image color multiplied by the background.
QtGui.QPainter.CompositionMode.CompositionMode_Multiply The output is the image color multiplied by the background.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These seem to break the docstring alignment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ooooooooof... yea I'm going to have to take a closer look at those :|

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 6, 2021

PyQtEnumConverter should not be run on the generated *Template_{binding}.py files.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 6, 2021

PyQtEnumConverter should not be run on the generated *Template_{binding}.py files.

yuuuup... just realized that 🤦🏻

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 6, 2021

You might as well remove the following chunk from Qt.py

if QT_LIB == PYQT6:
    # shim the old names for QPointF mouse coords
    QtGui.QSinglePointEvent.localPos = lambda o : o.position()
    QtGui.QSinglePointEvent.windowPos = lambda o : o.scenePosition()
    QtGui.QSinglePointEvent.screenPos = lambda o : o.globalPosition()

    QtWidgets.QApplication.exec_ = QtWidgets.QApplication.exec

Since PySide6 6.1.0, use of localPos, windowPos, screenPos, exec_ generates a deprecation warning. The codebase has since been changed to use the following style: ev.position() if hasattr(ev, 'position') else ev.localPos()

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 6, 2021

I have a hard time imagining what a use-case for someone needing Qt 6.0 support when 6.1 is released.

Qt6 <= 6.0.3 had a soft memory leak for every queued signal emitted. This makes it unsuitable for long running applications. Since PyQt6 didn't release a 6.0.4, it means that to get the fix for this bug, you need PyQt6 >= 6.1.0. That should be good enough a reason to move to 6.1.

This memory leak is actually quickly visible in the various SpeedTest examples where a QTimer is being fired as fast as possible.

@j9ac9k j9ac9k force-pushed the convert-enums branch 5 times, most recently from fbbb748 to f3c5c9f Compare June 6, 2021 04:40
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 6, 2021

The intermittent segfault I'm seeing on pyside2/python3.7 has me concerned.

I think at this point everything else should be good.

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 6, 2021

DateAxisItem_QtDesigner.ui and designerExample.ui are loaded during runtime, so don't generate files for them.

@j9ac9k j9ac9k force-pushed the convert-enums branch 2 times, most recently from 738025d to 888ece9 Compare June 6, 2021 05:14
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 6, 2021

I'm going to call it a night; will try some more trial and error regarding the segfault tomorrow.

Thanks for looking this over @pijyoi @NilsNemitz

@j9ac9k j9ac9k force-pushed the convert-enums branch 3 times, most recently from f0037f2 to 0f38b14 Compare June 6, 2021 05:36
@j9ac9k j9ac9k force-pushed the convert-enums branch 3 times, most recently from b6568b5 to 084f0f3 Compare June 6, 2021 06:16
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 6, 2021

my git game is weak; I keep running into this silly conflict, eventually I'll figure out how to resolve it...

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 6, 2021

Doing a search for PyQt6 there may be some other places to clean up the code, like RemoteGraphicsView.py, perhaps ROI.py, and maybe ndarray_to_qimage in functions.py

Ok, calling it a night this time for sure.

@j9ac9k j9ac9k force-pushed the convert-enums branch 5 times, most recently from bb835d7 to 73c56d4 Compare June 6, 2021 22:03
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 7, 2021

Well, an identical segfault occurred in an unrelated PR and given this PR hasn't merged, I'm now confident in saying this segfault is not the result of this PR.

https://github.com/pyqtgraph/pyqtgraph/pull/1726/checks?check_run_id=2764478711

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 7, 2021

Doing a search for PyQt6 there may be some other places to clean up the code, like RemoteGraphicsView.py, perhaps ROI.py, and maybe ndarray_to_qimage in functions.py

Yes. ROI.py has special handling for KeyboardModifiers for PyQt6 6.0.x

RemoteGraphicsView.py and ndarray_to_qimage have special handling for QImage for PyQt6 6.0.0

@j9ac9k j9ac9k force-pushed the convert-enums branch 3 times, most recently from abd68f5 to 2612209 Compare June 9, 2021 03:51
j9ac9k added 7 commits June 8, 2021 21:23
This namespace appears to be valid in PySide2/PyQt5 5.12+ so we may as
well migrate to the newer namespace ourselves.
Simplify some PyQt6 code branches
Update PySide6 templates to Qt 6.1
This has been demonstrated to not work in PyQt6.
There seems to be some unintentional side effect when running examples and the
same time.  This change breaks up the execution into two separate calls to
pytest in an attempt to bypass whatever issue is being created.
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 9, 2021

Sorry for the huge diff, let's hope the merge conflicts aren't too bad!

@j9ac9k j9ac9k merged commit 6f0ffcb into pyqtgraph:master Jun 9, 2021
@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Jun 10, 2021

@j9ac9k , I think there are 3 instances of ItemIsFocusable in https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/GraphicsScene/GraphicsScene.py that may need to be changed to GraphicsItemFlag.ItemIsFocusable

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Jun 11, 2021

@j9ac9k , I think there are 3 instances of ItemIsFocusable in https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/GraphicsScene/GraphicsScene.py that may need to be changed to GraphicsItemFlag.ItemIsFocusable

you're right... have some work to do tonight will try and get to this in the next day or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants