Add PyQt5 to Qt binding choices for Python consoles #2226
Conversation
because if the support for a newer and better version of a library is there, qhy wouldn’t we want to use it
|
I think this has to be smarter. What would happen if people has PyQt4 and 5 installed, but start Spyder with PyQt4? Since the default mpl backend is set to PyQt5, this will probably crash the console. Could we make the default to match the same bindings used by Spyder? |
| @@ -9,7 +9,7 @@ | |||
|
|
|||
| import os | |||
|
|
|||
| os.environ.setdefault('QT_API', 'pyqt') | |||
| os.environ.setdefault('QT_API', 'pyqt5') | |||
There was a problem hiding this comment.
I don't know what to think about this one. Our support for PyQt5 is still experimental, and there aren't packages on Windows for Anaconda and WinPython.
There was a problem hiding this comment.
how is it experimental? what is needed to get rid of that status?
and if pyqt5 isn’t installed, it will fall back to pyqt4, right?
There was a problem hiding this comment.
@flying-sheep Spyder, although a stand alone program by itself, is also part of a larger ecosystem of scientific computing distributions whcih includes Anaconda, WinPython .
This modification may produce problems for those distributions until they have gotten things up to date.
This change is not at the moment a need at this point because the development version is still fixing some rough edges to fully support Qt5 plus we might need to adapt the imports and some code to make the code look like Qt5 (even if bellow the curtains the user has Qt4).
I am 👎 on this one for the time being until the distributions offer complete Qt5 support, which will happen later this year
There was a problem hiding this comment.
please explain how this could possibly cause problems.
if we properly coded this to fall back to PyQt4, in case of PyQt5 non-availability, this issue (causing problems on those distributions) doesn’t exist. in this case, we merge this.
if we didn’t, the issue is that we got the fall back wrong and this issue still doesn’t exist. in that case, we fix the fallback code and merge this afterwards.
There was a problem hiding this comment.
As @ccordoba12 said, our support for pyqt5 is experimental, meaning there are many bugs that need to be fixed before we make it a default, so there is no pressing need to have this merged if the distributions that support Spyder cannot (yet) provide packages.
When the distributions provides such packages and we can reliably test how qt5 and pyqt5 works in all the different OS we can then go forward on this.
I am ok with this PR being merge later on, just not now.
There was a problem hiding this comment.
there are many bugs that need to be fixed
as said before: please point them out to me.
so there is no pressing need to have this merged
makes everything faster, smoother and better looking on systems where PyQt5 is supported, poses no problems on other systems. reason enough?
When the distributions provides such packages and we can reliably test how qt5 and pyqt5 works in all the different OS we can then go forward on this.
and until then nobody can use it, so we don’t need to worry
There was a problem hiding this comment.
The commit introducing Qt5 compatibility is only 2 month old: 6629a92
Issues will pop when more people use it.
Adding it to the menu (in the dev version) could help to have more people testing it, however.
There was a problem hiding this comment.
@flying-sheep: Here's one issue for you: #2235
There was a problem hiding this comment.
@flying-sheep, giving your opposition to revert this change, I'm going to revert it myself after merging.
It's not up to you to decide when we need to move to fully support to PyQt5, it's up to us :-) And right now, it's a consensus among the dev team that we are just not ready.
There was a problem hiding this comment.
i’m just discussing, not actively resisting to edit my PR. that would be childish ;)
what do you mean by $ export QT_API=pyqt
$ spyder3or what? in this case it would work, wouldn’t it? |
| has_pyqt4 = programs.is_module_installed('PyQt4', | ||
| interpreter=interpreter) | ||
| has_pyside = programs.is_module_installed('PySide', | ||
| interpreter=interpreter) | ||
| if has_pyside and not has_pyqt4: | ||
| if has_pyqt4 and not has_pyqt5: | ||
| self.set_option('qt/api', 'pyqt4') |
There was a problem hiding this comment.
I think this is wrong, this should be pyqt, instead of pyqt4.
|
Thanks a lot for doing the change and for understanding :-) We'll try to make PyQt5 the default before 3.0 final. |
Add PyQt5 to Qt binding choices for Python consoles
because if the support for a newer and better version of a library is there, why wouldn’t we want to use it?