Skip to content

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

Merged
dalthviz merged 17 commits intospyder-ide:masterfrom
StSav012:make_static_QMenu_exec_call
Apr 18, 2023
Merged

PR: Use static calls of exec_ elsewhere where needed, and test them#422
dalthviz merged 17 commits intospyder-ide:masterfrom
StSav012:make_static_QMenu_exec_call

Conversation

@StSav012
Copy link
Copy Markdown
Contributor

@StSav012 StSav012 commented Apr 5, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

CAM-Gerlach commented Apr 8, 2023

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
Copy Markdown
Member

CAM-Gerlach commented Apr 13, 2023

(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.)

CAM-Gerlach
CAM-Gerlach reviewed Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@StSav012
Copy link
Copy Markdown
Contributor Author

StSav012 commented Apr 13, 2023

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
Copy Markdown
Contributor Author

StSav012 commented Apr 13, 2023

(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
Copy Markdown
Contributor Author

StSav012 commented Apr 13, 2023

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

@StSav012 StSav012 requested a review from CAM-Gerlach April 13, 2023 11:14
CAM-Gerlach
CAM-Gerlach approved these changes Apr 14, 2023
View reviewed changes
Copy link
Copy Markdown
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
ccordoba12
ccordoba12 reviewed Apr 14, 2023
View reviewed changes
dalthviz
dalthviz approved these changes Apr 14, 2023
View reviewed changes
Copy link
Copy Markdown
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 ?

@dalthviz dalthviz requested a review from ccordoba12 April 14, 2023 22:11
ccordoba12
ccordoba12 approved these changes Apr 17, 2023
View reviewed changes
Copy link
Copy Markdown
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
@StSav012 StSav012 mentioned this pull request May 17, 2023
Merged
@dalthviz dalthviz mentioned this pull request Apr 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@ccordoba12 ccordoba12 ccordoba12 approved these changes

@dalthviz dalthviz dalthviz approved these changes

Assignees

@StSav012 StSav012

Labels

type:Task

Projects

None yet

Milestone

v2.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@StSav012 @CAM-Gerlach @ccordoba12 @dalthviz