Repository navigation
Cooperative __init__ for Qt4 canvas. - #9052
Conversation
tacaswell
added
the
Release critical
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
One paranoid comment, but overall seems reasonable.
|
|
||
| @functools.wraps(__init__) | ||
| def wrapper(self, **kwargs): | ||
| QtWidgets.QWidget.__init__ = cooperative_qwidget_init |
There was a problem hiding this comment.
This needs to be in a try..finally block.
There was a problem hiding this comment.
The initial monkey-patch should also be in the try block... ctrl-c can happen anywhere.
There was a problem hiding this comment.
Wow, I'm a bit ashamed I never thought about this point :-)
|
I lean towards this as the better solution (I started writing a comment about how I was even between them and in the process convinced my self to prefer this one). This has the advantage of being cleaner and more localized but hard-codes the non-cooperative modules in. However, odds of another binding showing up are low and doing the sub-classing and replacing #9048 uses 'simpler' python, but it makes the class zoo one class bigger and has to use two differently named 'magic' cut-out function, and is spread across a couple of files. It makes the unlikely extension case easier (once you understand the code), but harder to understand to begin with. |
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
Be more paranoid in re-setting the __init__
5ee6e51 to
7b09c20
Compare
|
try... finally added. |
7b09c20 to
f007419
Compare
|
Appveyor failure is just a hang at the start of the conda-build on 3.6. |
Alternate for #9048 (basically pulling out the relevant parts of #8771, but using a local monkey-patch approach.
I personally find this approach clearer in intent ("PyQt4 does not support multiple inheritance so let's just locally patch it so that it does") than #9048.
PR Summary
PR Checklist