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

Warn when the Qt application quits 'early' #267

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Conversation

altendky
Copy link
Owner

Follows from #262.

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #267 (c1a3a97) into main (2cc53c0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #267   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines         2023      2036   +13     
  Branches       138       139    +1     
=========================================
+ Hits          2023      2036   +13     
Impacted Files Coverage Δ
qtrio/__init__.py 100.00% <ø> (ø)
qtrio/_core.py 100.00% <100.00%> (ø)
qtrio/_exceptions.py 100.00% <100.00%> (ø)
qtrio/_tests/test_core.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cc53c0...c1a3a97. Read the comment docs.

@altendky altendky marked this pull request as ready for review July 27, 2021 15:12
@altendky
Copy link
Owner Author

@vxgmichel, would you like to review or test this before I merge? Does it fulfill your interest in a notification?

Copy link

@vxgmichel vxgmichel left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

I ran a quick test with the problem I originally faced with #262. I was surprised to not see the warning appear before I realized two Qt applications were created:

  • one in qtrio.run
  • and another one in the main async function

Weirdly enough, this caused the aboutToQuit signal of the first application to not be emitted. Once I made sure the app was created before running qtrio.run, the PR worked as expected.

@altendky
Copy link
Owner Author

That's... super weird. The Qt application is a singleton. You shouldn't be able to have two.

>>> from PySide2 import QtCore
>>> a1 = QtCore.QCoreApplication([])
>>> a2 = QtCore.QCoreApplication([])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: Please destroy the QCoreApplication singleton before creating a new QCoreApplication instance.

But with PyQt5 it does let you do that... uh...

>>> from PyQt5 import QtCore
>>> a1 = QtCore.QCoreApplication([])
>>> a2 = QtCore.QCoreApplication([])
>>> a1
<PyQt5.QtCore.QCoreApplication object at 0x7f3c88b0e550>
>>> a2
<PyQt5.QtCore.QCoreApplication object at 0x7f3c8584d4c0>

And PyQt6 (6.1.1) still does.

>>> from PyQt6 import QtCore
>>> a1 = QtCore.QCoreApplication([])
>>> a2 = QtCore.QCoreApplication([])
>>> a1
<PyQt6.QtCore.QCoreApplication object at 0x7f73ce8b0550>
>>> a2
<PyQt6.QtCore.QCoreApplication object at 0x7f73ce7e80d0>

@altendky
Copy link
Owner Author

https://doc.qt.io/qt-6/qcoreapplication.html

For non-GUI application that uses Qt, there should be exactly one QCoreApplication object.

So it apparently isn't a singleton at the C++ level, you are just supposed to not create multiple. Someone did some experimentation for us as to what the implications of having multiple applications are. I recreated the signal separation issue.

from PyQt6 import QtCore

a1 = QtCore.QCoreApplication([])
a2 = QtCore.QCoreApplication([])

def f1():
    print("f1")

def f2():
    print("f2")

a1.applicationNameChanged.connect(f1)
a2.applicationNameChanged.connect(f2)

print("---")
a1.applicationNameChanged.emit()
print("---")
a2.applicationNameChanged.emit()
print("---")
---
f1
---
f2
---

Anyways, I think I'll leave dealing with multiple application instances as a separate activity, if it gets handled at all. The shutdown aspects are a thing I think are appropriate for QTrio to make efforts on since QTrio is necessarily tied up in the lifetime of the application. Creating multiple applications is just a thing you shouldn't do regardless.

@altendky altendky requested review from gordon-epc and epcAnker July 28, 2021 13:43
@altendky altendky merged commit 43c7ff2 into main Jul 28, 2021
@altendky altendky deleted the warning_for_early_quit branch July 28, 2021 17:55
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