-
Notifications
You must be signed in to change notification settings - Fork 55
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
Fix importing QtWebEngine after toolkit selection #853
Conversation
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
==========================================
+ Coverage 41.19% 41.29% +0.10%
==========================================
Files 522 522
Lines 28128 28132 +4
Branches 4253 4253
==========================================
+ Hits 11588 11618 +30
+ Misses 16034 16017 -17
+ Partials 506 497 -9
Continue to review full report at Codecov.
|
Note that CI passing does not verify the PR has fixed the issue, because CI is installing PyQt5 from EDM with a version that includes |
pyface/__init__.py
Outdated
@@ -22,7 +22,7 @@ | |||
__extras_require__ = { | |||
"wx": ["wxpython>=4", "numpy"], | |||
"pyqt": ["pyqt>=4.10", "pygments"], | |||
"pyqt5": ["pyqt5", "pygments"], | |||
"pyqt5": ["pyqt5", "PyQtWebEngine", "pygments"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only needed for pyqt5
>= 5.12, so probably should have that encoded in the PyQt version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean "pyqt5": ["pyqt5>=5.12", "PyQtWebEngine", "pygments"]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I could leave this "PyQtWebEngine"
out but instead document this additional requirement somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the first of these, unless there is a potential problem with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did make me become aware that PyQtWebEngine is only distributed on PyPI for PyQt5>=5.12. But did you mean that it is only available for pyqt5>=5.12, rather than needed?
Specifying "pyqt5>=5.12" would to me means that there are reasons within Pyface that causes pyqt5<5.12 not to be supported, is that true?
So far the ">=5.12" bit seems to me a requirement of PyQtWebEngine, not a requirement of Pyface. If there is a distribution of PyQtWebEngine that would support PyQt5 5.11, I am not sure Pyface should disallow it (I don't think Pyface is setting these minimum requirements based on what are tested on CI - because if it does it would be very restrictive.)
I think only these two options currently make sense to me:
(1) ["pyqt5", "pygments"]
(2) ["pyqt5", "PyQtWebEngine", "pygments"]
In any case, these are extras requirements, so they are already optional - they may even be ignored by some installers (e.g. edm).
Perhaps I should revert the change to "extras_require" so we can focus on the changes in the import order which fixes the import error in #581, the error occurs even if one already has the required Qt web dependencies installed.
I have reverted the change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix #581
First of all, we need a separate distribution for QtWebEngine with PyQt5. This PR expands the
extras_require
so thatpip install pyface[pyqt5]
brings inPyQtWebEngine
.After installing
PyQtWebEngine
, importingQtWebEngineWidgets
after instantiating a QCoreApplication results in an ImportError. This PR fixes this by trying to import QtWebEngineWidgets before instantiating the QApplication.All of this has to do with import side effect so it is hard to test. If the test mentioned in #581 is run on its own, it passes on master:
But if it is run after some other tests that have instantiated the QApplication, it fails (on master), as in #581:
With this PR, the second test command also passes.