-
Notifications
You must be signed in to change notification settings - Fork 252
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
Implement #152 #173
Implement #152 #173
Conversation
I've written a quite large application using Qt.py which uses I need to give this a whirl before commenting further, but when using e.g. PyQt5 as the binding, and when hitting issues, what would be the solution at hand? |
Did you mean this PR, or Qt.py in general?
What kind of issues? |
Initially, I meant Qt.py in general. But after having now replaced Qt.py with the one from this PR I can just say I now also refer to this PR 👍
Sorry, I was being vague. When enforcing PySide2, I was merely wondering which approach to take when code break as a result of the enforcement. For example, I noticed a very large chunk of my code base (written for Qt.py 0.6.x) works fine but I got this error early on in Python 2.7 with PySide 1.2.4: Traceback (most recent call last):
...
...
quit_app.triggered.connect(QtWidgets.qApp.quit)
AttributeError: 'NoneType' object has no attribute 'quit' Edit: the example error above is perhaps not the best kind of error to illustrate what my question is about... as I can just use QApplication instead of qApp and it'll play nice with all the bindings (PyQt4, PySide, PyQt5, PySide2) - and that's also what I should've done to begin with, I guess (even with Qt.py 0.6.x). |
I'm glancing through Qt.py top to bottom and when I reach the bottom half where all objects are mapped, I have a hard time understanding where the "_"-prefixed objects were created. For example: Where in the code was
On how Qt.py now works
This all means that if a member is not part of the long list of mappings which now takes up approx. half of the Qt.py file, this won't end up in our custom-created Like you say, this makes the file bigger but it also becomes much easier to see exactly what is supported by Qt.py. I'd say if we make sure to explain every major section of Qt.py using inline comments and docstrings this would all contribute to improved readability of the file and make it easier for people to contribute. Other questions
All in all, I like this PR and it seems all my code works great so far with it (although it was initially written for Qt.py 0.6.x and all I had to do was replace Side note |
Ok, now I understand. This kind of enforcement should only cause one problem - a member is missing - for two causes.
For (1), this layout would make it trivial to PR a new member, by simply adding a line to the long list of lines. For (2), this is where enforcement kicks in. If something is missing and should be missing, that's a signal that it wouldn't work across all bindings. With this PR, only members of PySide2 that also exists in all other bindings are included. It's a subset of PySide2, which does mean that Qt.py has the least amount of members out of all bindings.
As it happens, this is an interesting example. Now the problem is that, This is clearly a limitation of this approach. What we could do is make class QtWidgets_(object):
def __getitem__(self, attr):
return getattr(_QtWidgets, attr) But if we do that, we lose out on other module-like functionality, like this. from Qt.QtWidgets import QPushButton All in all, if we are to approach this issue of PySide2 enforcement with this particular technique - of explicitly specifying members - I'm not sure I see any other way than to bite the bullet on this one special case and accept that Worth considering! |
Okay, great - thanks for the clarification. So, overall, this clearly is an improvement as users are not tricked into writing cross-binding compatible code when they're not. Which is a important feature for my and my company's use of Qt.py; I write code which needs to work on multiple bindings. So, whenever something doesn't work using Qt.py because of a member being exclusive to a specific binding, I guess we'll recommend you resort to doing something like: from Qt import QtCompat
# Special case (member not supported by Qt.py)
if 'PyQt4' in QtCompat.__binding__:
import PyQt4
...
elif 'PySide' in QtCompat.__binding__:
import PySide
...
else:
# Just use Qt.py...
... ...or would you advise differently?
It would be interesting to get others thoughts on this. Personally, I think it would help maintain Qt.py (keeping it up to date automatically) by making it a class. Not being able to import like |
The three submodules For Qt 4, the It means you can't do this. import Qt
Qt._QtGui # This would be the original binding.
Precisely.
Simple, they aren't supported by all bindings, therefore would not be included. If you are looking for QtQML, odds are you aren't looking for compatibility with Qt 4, hence Qt.py is not what you are looking for. There is however a case for PyQt5 and PySide2 compatibility, and it's possible we would still like to facilitate that. Otherwise, we could make that a separate project. Such as
An excellent and important question. The list was generated by the included $ python build_membership.py The resulting
Members are built given the currently installed version of PySide2, on Travis this means whatever version is available on Tough call! In this PR, some members are doubly being excluded from within Qt.py as well, such as this one. Those should all ideally lie in a single spot for ease of maintenance.
Just personal preference, I don't think there is any technical advantage. |
I'm torn. On one hand, if you find yourself needing to use non-cross-compatible Qt code, you might want to question why you use Qt.py. On the other hand, there are always special cases. In some cases, one binding might be the recommended one due to some feature or bug in other bindings. In might be the case that a program runs best under PyQt5, and works less than ideal - perhaps with less features and glitter - under other bindings too. Kind of like how Xbox One games sometimes works on Xbox 360, with lesser quality. |
Regarding working around supporting a member not supported by Qt.py; I have no idea when and why I would need to do this but I wanted to raise the question to see how we feel about it.
Yes, this what actually exactly which was on my mind. I think, at least for me, it's too early to start such a project. I'm head over heels in developing for practically all the bindings with Qt.py now and QML is just out of the question (because PySide/PyQt4). But I can clearly see the benefits of one such project (
Ah, nice. So basically you run this and copy-paste the members mapping from the JSON into Qt.py?
If these exclusions wasn't generated, how did you manage to list them (here)?
A simple solution could be to just follow the vfxplatform spec and be observant of whether vendors do follow it or not and then adjust to what the industry standard actually ends up being. |
In the membership tests, after comparing members of PySide2 with each other binding, such as PyQt4, a listing of missing members is printed.
I then go in and manually add those to build_membership.py. I figure it'll be a lot of work initially, but very little in the long run. I'd imagine we'll discover one or two members missing every now and then as time goes by and PySide2 is updated and that we'll simply add those to the list. There might be more automatic ways of going at it, but I couldn't think of one that didn't make things too magical. Like the "hidden"
That is a great idea. Let's do exactly that.
My pleasure! A lot of these things I take for granted while typing it out. It sometimes doesn't occur to me that people can't read my mind until someone points it out, so do carry on! |
❤️ I was afraid you were doing that manually.
I must say, even if you think a lot is missing, my Python application (146.6 files/s, 17973.3 lines/s according to CLOC) just works with this 0.7.0 version of Qt.py. Overall, a big thumbs up from me initially on this PR! 🥇 |
Ok, I'm happy with this. I've reduced the character-count significantly, and implemented QT_STRICT (which is off by default). At the end of the day, this introduces a major shift in how Qt.py is implemented but should have minimal affect on users. The most significant change is that Qt.py is no longer replaced by the original binding. This means members other than QtWidgets, QtCore, QtGui and QtCompat are no longer available. Without QT_STRICT, this will still break code that previously worked on one binding but no others. For example, if someone was using With QT_STRICT, my hope is that code should fail only when it would have failed on one or more bindings. The goal is to enable the user of Qt.py spot instantly when using something not entirely cross-compatible, and I think this is a good step in that direction. |
This is all very excellent in my opinion 👍 The fact that you managed to get a To me, this also means we won't have to think much more about something like a " I'm going to enable
Haven't looked at your README changes just yet, but this should be prominent in the README and drive users towards wanting to enable Great job! |
My compliments on getting the module inclusion lists (of |
I'd encourage everyone to do the same. I'm expecting someone to try it and experience some issue that might prevent it to become the new default, but unless that happens I'd imagine it become the default once we hit 1.0.
Actually that still wouldn't work, because only QtCore, QtGui and QtWidgets are available via Qt.py. And also, what does "strict" mean if one is looking to use if for Qt 5 bindings only? I'd imagine Qt5.py to also have a strict mode, to exclude members exclusive to PySide2 or PyQt5. I dunno, it doesn't feel right.
Agreed. I haven't found the right spot for it in the README. Too advanced to introduce it with, yet too important to leave for later. I was thinking of making it the central message of the Release. |
Now I'm a bit confused. With QT_STRICT I thought we get this new functionality of the PR. With QT_STRICT disabled I thought we'd get the current 0.6.x behavior. No? Sorry, I still haven't looked at the code in detail so I just assumed this. |
|
||
When enabling `QT_STRICT`, Qt.py becomes a subset of PySide2. All members are guaranteed to exist across all bindings, meaning many will be missing. Including QtQml and QtQuick modules. | ||
|
||
Strict mode follows the [VFX Platform](http://www.vfxplatform.com/) to determine which version of PySide2 to use for reference. Currently version if 2.0.0. |
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.
Minor typo in last sentence:
Currently version if 2.0.0.
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.
Thanks
Not quite, no. See, before we simply made The way this is implemented now, it Qt.py wouldn't replace itself with a module, it would be the actual module you use. It's the only way we could have control over what's inside of it, and is probably how it should have been done to begin with. But it does mean things are different. |
Sorry for the long idle time. Holidays, children and other private matters have been taking up my time. I feel a bit thick, as I don't quite understand the exact definition of what will be the difference when having QT_STRICT enabled vs disabled. I'm looking at this code from your original post: $ python
>>> from Qt import QtGui
>>> widget = QtGui.QWidget()
>>> exit()
$ export QT_STRICT=True
$ python
>>> from Qt import QtGui
>>> widget = QtGui.QWidget()
AttributeError: 'module' object has no attribute 'QWidget' Also looking in the code:
For
|
Having thought more about it over these past few weeks, I'm also open to the idea of not having a To ease the transition, we could increment the major release, making it 1.0.0, and let those that prefer the current more lax alternative stick to Writing new GUIs today, it makes the most sense to me. It would mean I know with confidence that if it works with Qt.py, it would work on all bindings. What do you think? |
I totally agree with you; not having I mean, with I feel having the
👍 0.x.x would probably be nice for those who are looking to port old code to Qt5. For new UIs, 1.0.0 would most likely be the way forward. This could be mentioned in the README, perhaps. |
I don't know if it's worth doing, but we could maintain a branch which we call something like "legacy"... or something more clever. Which is in fact the 0.x behavior, retained. EDIT: No, wait. Then travis testing etc won't work, I guess. Never mind. |
Cool!
It would be for backwards compatibility. For example, to those who have already built GUIs that work well with, e.g. PyQt5 and PyQt4. This update could break that.
We could instruct Travis to pick a particular branch, but whether we should maintain the previous version via a branch at all I'm unsure of. What I'd like to do, is finish this up and make an alpha release. We make an announcement and ask for people's opinions. And move on from there. One of the things we lose out on with this release is the immediate benefit to those who transition to PyQt5. Those users won't necessarily care for the current limitations of a pre-alpha PySide2, but they would be limited by it nonetheless. I think @melevine is such a user, correct me if I'm wrong! On the upside, PySide2 will inevitably catch up, at which point this would no longer be a downside. Unless we care for compatibility with this first version of Maya 2017, which comes bundled with the pre-alpha version. Choices! |
Sounds good to me! 👍 |
These are no longer relevant, as we do not modify the original module(s)
b105174
to
21dfae1
Compare
Polished and Merging this tomorrow, but won't make an official release on PyPI until I've made an announcement and have had a few of you test this out for yourselves to find edgecases we'll need to take into account. To install the pre-release, I'll be suggesting this. $ pip install git+git://github.com/mottosso/Qt.py Which is a method of installing Qt.py directly from GitHub, before releasing it into the wild. And just to clarify, this will be a breaking release. Breaking things that would have broken on any of the four bindings, such as when using |
I'll be putting this to the test after lunch. Will be back with any impressions later today. |
Considering Qt.py no longer needs to replace itself with an original binding, we can add members to it without worrying about hiding those from the original. Also considering this is already a breaking change, it'd be ideal to break as much as possible now, as opposed to spreading out breakage over several versions. So I moved For example. # Before
>>> from Qt import QtCompat
>>> print(QtCompat.__version__)
u'0.6.9'
# Now
>>> import Qt
>>> print(Qt.__version__)
u'1.0.0.b1' |
Excellent. Would it be useful to keep some backwards compatibility to |
I just ran tests on internal stuff using Qt.py from this PR and it works fine without any hiccups. |
Would you like to still have the compatibility functions under |
As we still include compatibility wrapppers, like But it's a detail, and I'd be happy to leave all additions to QtCompat if you'd prefer. What do you think? |
Here's what I've got in mind for an announcement on the VFX Platform. Qt.py and safetyHi community, Today we've made a significant leap forwards for Qt.py and implemented a guarantee where if your program runs with Qt.py and any binding, such as PySide2, it will run in an identical fashion on any binding. This means that it will throw an error if you use any feature of any particular binding that isn't available on another binding. For example, this is currently possible with Qt.py 1: from Qt import QtGui, QtCore
2:
3: class Widget(QtGui.QWidget):
4: my_signal = QtCore.pyqtSignal(str) On the third line, we run into an immediate problem; On the 4th line, we refer to a signal as Worst yet, these problems would not make themselves known until you run it on each of the 4 bindings and make sure, first hand that it works as you'd expect. With this new change, Qt.py limits the available members of each submodule into a subset of members that exist across all bindings. The result is finding out about these problems faster and being able to guarantee that if it runs on one binding, it works on all. 1: from Qt import QtGui
2:
3: class Widget(QtGui.QWidget):
4: pass
5:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'QWidget' As a consequence of this breaking change, some of your already written software with Qt.py may need some tweaking. They would behave badly if they would have already behaved badly in any of the 4 bindings. We are confident in this change and direction, but are still in discussion about whether now is the right time. One of the bindings, namely PySide2, is so far behind the other bindings that it is currently the "weakest link" by far. These are the modules supported by Qt.py (and in effect, PySide2 (2.0.0)) with this change. __all__ = [
"QtGui",
"QtCore",
"QtWidgets",
"QtNetwork",
"QtXml",
"QtHelp",
"QtCompat"
] The second concern regards which version of this weakest link to match. As PySide2 matures, more feature will be added and Qt.py could start to grow. But Maya 2017 and others are currently fixed at version 2.0.0. Growing Qt.py beyond what Maya 2017 is capable of would limit its usefulness. For this I figure it wouldn't be too cumbersome to implement compatibility profiles; where the end-user specifies which version of, say, the VFX Platform to provide support for. $ # For example
$ export QT_BASELINE=CY2018
$ python -c "from Qt import QtOpenGL" For now, we'd like your feedback on this major change and for you to test it out for yourself. The release is still considered "alpha" and is available directly via GitHub for download or install. $ pip install git+git://github.com/mottosso/Qt.py
Thanks to Matthew Levine for the initial feature request and Fredrik Averpil for the unwavering support. :) Best, |
Nah. I think this is good. :) |
The only addition I'd perhaps do is a simple example of how to detect which binding is being used and then do something binding-specific, such as: from Qt import __binding__
if "PySide" in __binding__:
do_pyside_stuff() This should alleviate any headaches when reading... ;) Hehe. Joking aside, I think it can be good for clarity. Very nice stuff! 👍 |
Agreed and added. For the time being, and where you need functionality exclusive to any particular binding, such as if "PySide" in __binding__:
do_pyside_stuff() This would allow you to explicitly mark the parts of your codebase that depend on any particular binding, and to enable a bridge to your code today and your code sometime in the future when the required functionality is officially part of Qt.py. |
Let's do it! |
Hi all,
This is a first pass on #152, a strict enforcement of the PySide2 API.
It's a suggestion, so do not merge unless we've talked it through and are all happy with it.
Things to note:
On (1), this isn't necessarily a disadvantage; it does make the code trivially simple and require close-to-zero understanding of the overall code to make contributions. Such as adding missing members, or members that become available (as PySide2 is still under active development).
Discussion
I personally think this is the right way forwards, but it does mean less code will run on it. At the moment, Qt.py is rather forgiving but then of course punishes you secretly further on as you attempt to run it on other bindings.
I could imagine we either make this leap, force users to conform to a strict subset of PySide2, or install a switch such as
QT_STRICT=True
.If so, (1) is the added code worth the trouble? and (2) which value should be the default?
Let me know what you think.