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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Use static calls of exec_ elsewhere where needed, and test them #422

Merged
merged 17 commits into from
Apr 18, 2023

Conversation

StSav012
Copy link
Contributor

@StSav012 StSav012 commented Apr 5, 2023

The help for QMenu.exec_ reads something like the following (the order might be different, and fully qualified Qt class names might appear):

exec_(...)
    exec_(actions: typing.Sequence[QAction], pos: QPoint, at: typing.Optional[QAction] = None, parent: typing.Optional[QWidget] = None) -> QAction
    exec_(self) -> QAction
    exec_(self, pos: QPoint, at: typing.Optional[QAction] = None) -> QAction

The first option here is a static call to QMenu.exec_. In the QtPy tests, such a call is never tested. So, here it is. As one might expect, it fails. Use the _possibly_static_exec function to fix the errors.

@StSav012
Copy link
Contributor Author

StSav012 commented Apr 5, 2023

The mistake of mine was in the following lines:

        QtCore.QTimer.singleShot(100, lambda: qtbot.keyClick(
            QtWidgets.QApplication.widgetAt(1, 1),
            QtCore.Qt.Key.Key_Escape)
            # namely, ↑↑↑ up here
        )

In PyQt5=5.9, there was no QtCore.Qt.Key.Key_Escape yet. Surprisingly, the attribute error was suppressed, and no timer shot was fired. That caused the stall. When I changed the parameter to QtCore.Qt.Key_Escape, it worked.

As I don't use Windows, and the problem appears only on Windows, I couldn't catch the problem locally. The third time's a charm.

@StSav012 StSav012 marked this pull request as ready for review April 5, 2023 12:26
@dalthviz dalthviz added this to the v2.4.0 milestone Apr 6, 2023
@dalthviz dalthviz self-requested a review April 6, 2023 19:33
@StSav012
Copy link
Contributor Author

StSav012 commented Apr 7, 2023

Oh, it's ERROR module 'importlib_metadata' has no attribute 'distributions' again. I see the error is so persistent that @dalthviz has merged df95964 into master despite the failed check.

@CAM-Gerlach
Copy link
Member

Oh, it's ERROR module 'importlib_metadata' has no attribute 'distributions' again.

Thanks, I opened #423 to document that.

@dalthviz dalthviz changed the title Test static QMenu.exec_ call PR: Test static QMenu.exec_ call Apr 10, 2023
@CAM-Gerlach
Copy link
Member

(Fixed the importlib error in #425 , BTW, while also speeding up the Conda=yes CI jobs by roughly 2x and simplifying the CI script a bit.)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Sorry, didn't realize I hadn't actually reviewed this.

If it might be used in multiple modules, maybe better to move _possibly_static_exec to utils? Thoughts?

Beyond that, I didn't spot any obvious blocking issues with this, so otherwise leaving this to @dalthviz 's expertise to review further.

qtpy/QtWidgets.py Outdated Show resolved Hide resolved
@StSav012
Copy link
Contributor Author

If it might be used in multiple modules, maybe better to move _possibly_static_exec to utils?

I've looked through

and found that QCoreApplication, QGuiApplication, QApplication, and QMenu indeed have such static calls. Thank you for the remark!

As for QTextStreamManipulator, I can't compose a test call. I just can't make an instance of QTextStreamManipulator last. According to help(QTextStreamManipulator.exec_), it isn't static, but still. That's an odd class.

@StSav012
Copy link
Contributor Author

(Fixed the importlib error in #425 , BTW, while also speeding up the Conda=yes CI jobs by roughly 2x and simplifying the CI script a bit.)

Thanks a lot! That's a marvelous solution!

@StSav012
Copy link
Contributor Author

Surprisingly, the tests for QtCore.QCoreApplication.exec_ don't result in a segfault on Ubuntu with conda=No.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks! All looks pretty reasonable to me now, but I'll leave it to @dalthviz 's greater expertise to review further, thanks.

@CAM-Gerlach CAM-Gerlach changed the title PR: Test static QMenu.exec_ call PR: Use static calls of exec_ elsewhere where needed, and test them Apr 14, 2023
qtpy/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @StSav012 ! LGTM 👍 But, just in case, what do you think @ccordoba12 ?

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @StSav012!

@dalthviz dalthviz merged commit 1874fba into spyder-ide:master Apr 18, 2023
@StSav012 StSav012 deleted the make_static_QMenu_exec_call branch May 17, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants