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

Make sure selected QtAPI actually provides QT support #344

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

cv3d
Copy link
Contributor

@cv3d cv3d commented Nov 6, 2018

Maybe related issues: enthought/mayavi#448, enthought/mayavi#460, enthought/mayavi#483

PyFace might select PyQt5 as the default API if it is installed even if Qt5 itself is missing.
This PR fix such an issue by ensuring that at least QtCore can be imported from the being-selected API, otherwise try the next one.

@codecov-io
Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #344 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #344   +/-   ##
=======================================
  Coverage   34.42%   34.42%           
=======================================
  Files         457      457           
  Lines       25432    25432           
  Branches     3797     3797           
=======================================
  Hits         8754     8754           
  Misses      16308    16308           
  Partials      370      370
Impacted Files Coverage Δ
pyface/qt/__init__.py 47.5% <0%> (ø) ⬆️

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 62ce970...56a8f53. Read the comment docs.

@corranwebster
Copy link
Contributor

LGTM

Thanks for the contribution!

@corranwebster corranwebster merged commit 0cd4833 into enthought:master Nov 29, 2018
@cv3d
Copy link
Contributor Author

cv3d commented Nov 29, 2018

@corranwebster You are welcome!
BTW: The order here is quite strange! pyqt should be considered before pyqt5, as per the current strategy to prefer Qt4 over 5.
Shall I PR on that as well?

@corranwebster
Copy link
Contributor

I think that order is OK - it doesn't need to match the order in the files like QtCore.py, as they aren't expressing any sort of order/preference. In general, we prefer PySide over PyQt, and more recent over older, except that PySide2 is currently experimental for ETS (eg. currently tests are failing on windows).

@cv3d
Copy link
Contributor Author

cv3d commented Nov 29, 2018

I see! I thought the warning Warning: The default toolkit if none is supplied is qt4. in the README implies that Qt4 is preferred..

@cv3d cv3d deleted the fix_missing_qt branch November 29, 2018 14:57
@corranwebster
Copy link
Contributor

Ah! No, that refers to the ETS_TOOLKIT not the PYFACE_API, which for historical reasons is called qt4 even if the PYFACE_API is pyqt5. We're planning to change that to qt in the future (and you can do that right now for some ETS projects), but that requires care since there is so much infrastructure we have that assumes ETS_TOOLKIT is qt4.

For the record, the priorities for qt vs wx are here: https://github.com/enthought/pyface/blob/master/pyface/base_toolkit.py#L94

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.

None yet

3 participants