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: Qt 6: QApplication: Make exec_() a simple alias for exec() #473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rear1019
Copy link
Contributor

See commit message for details. The change is required for Spyder to start with PyQt6/PySide6. Instead of changing qtpy, we also could opt for removing the monkey patch of QApplication in Spyder: Spyder seems to work without it (However, mind that I haven’t checked beyond “Spyder starts”).

[1] See https://github.com/spyder-ide/spyder/blob/e624b704959c965ce5a83991f9e374a7759dc6cc/spyder/app/utils.py#L302-L314

Make exec_() a simple alias for exec() instead of using the
possibly_static_exec() helper. Using the helper is not required.
Moreover, the helper doesn’t behave correctly if QApplication is
monkey patched [1].

[1] as in

    from qtpy import QtWidgets
    class PatchedQApplication(QtWidgets.QApplication):
        ...
    QtWidgets.QApplication = PatchedQApplication
@coveralls
Copy link

Coverage Status

coverage: 96.739% (-0.04%) from 96.781%
when pulling de78b84 on procitec:qt6_simplify_qapp_exec_
into 3238de7 on spyder-ide:master.

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.

Hi @rear1019 sorry for such a late response and thank you for your help here! I think the exec_ handling via the helper was added at #422 but if I understand correctly then seems like its use is not needed at least for QApplication and it can cause issues for subclasses, right?

Could it be possible to add a test to check for the QApplication subclassing and exec/exec_ calling case please?

Also, just in case what do you think @ccordoba12 @StSav012 ?

Thanks again for your work here!

@ccordoba12
Copy link
Member

This looks good to me, although it's no longer necessary for Spyder.

@StSav012
Copy link
Contributor

StSav012 commented Apr 11, 2024

@rear1019, thank you for bringing the issue up. I haven't dag deeper than the following paragraph.

Well, the exec method is actually static [QCoreApplication.exec, QGuiApplication.exec, QApplication.exec]. I'm genuinely surprised that no one calls it this way. I don't.

However, in PySide6, QCoreApplication.exec_ (with the underscore) is not static. Report it if you wish to. That's yet another incompatibility between the flavors. I don't have time to add the fix to QtPy until May, probably.

Therefore, I thought that as QCoreApplication.exec are static, the possibly_static_exec is semantically appropriate there.

You're totally right that as the calls are always to the static method there, the possibly_static_exec is excessive there. The PR looks good, although I'd add comments into the code to describe the inconsistency between the exec_ implementations for QApplication, QDialog, and QMenu.

Finally, please, change QGuiApplication.exec and QCoreApplication.exec similarly.

@ccordoba12 ccordoba12 modified the milestones: v2.4.2, v2.4.3 Oct 12, 2024
@ccordoba12
Copy link
Member

@dalthviz, do you want to address this one for 2.4.2 (according to @rear1019's comments) or leave it for later?

@dalthviz
Copy link
Member

Leaving it for 2.4.3 should be fine

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.

5 participants