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

Started adding support for PyQt5 #3

Merged
merged 13 commits into from
Jun 19, 2015
Merged

Conversation

astrofrog
Copy link
Member

This starts to add support and tests for PyQt5. There is a subtlety however, which is that even conda doesn't complain if I install PyQt5 and PyQt4/Pyside in a same environment, PyQt5 is unable to create a QApplication in this case because the environment contains both the Qt4 and Qt5 libraries and it gets confused.

In addition, I found that some strange behaviors were due to the use of reload in tests, which doesn't actually clean up sys.meta_path which ends up containing many ImportDenier instances (because when it reloads the module it adds a new hook without removing the old one, and worse, all old instances are instances of a different class since it redefines the ImportDenier class every time). Anyway, because this is complex, I've tidied it up so that we don't actually call reload but intead have a reload_qt that ensures everything is clean and avoids this kind of issue.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - I'll leave this open for a few days in case you want to review. But with this and with a few small changes elsewhere in glue, I can get glue to open with PyQt5.


def reload_qt():

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what this does in a docstring? I assume this is only meant to be used to reimport the same Qt binding (since trying to import several leads to trouble)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do - basically it's a cleaner way than doing reload(qt_helpers) - if you change the environment variable for example, you can call this and it will load the qt bindings again with a different library (e.g. pyside instead of pyqt4) if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that work though? I thought loading two bindings in the same process was prone to crashing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work according to test_submodule_import. What doesn't work though is importing PyQt4 then importing PyQt5. But PyQt4 <-> PySide seems to work.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - I added a docstring for reload_qt

@ChrisBeaumont
Copy link
Member

It seems to work according to test_submodule_import

Huh -- I seem to recall this behaving badly (though maybe this was because my PySide/PyQt4 versions were linked against different Qt libraries. Who knows). One more question -- can you actually pull up a QWidget after calling reload? If so, then I'm fine with the function. If that breaks (and reload is only useful as a test utility since the tests don't actually instantiate widgets or applications), then reload() should probably live as a non-public function inside the test file.

(Sorry, I'd check myself but I only have one Qt binding set up ATM)

@astrofrog
Copy link
Member Author

@ChrisBeaumont - ah, good point, will try that out!

@astrofrog
Copy link
Member Author

@ChrisBeaumont - it seems to work properly, see new test in 7358dd8 which works fine. However, I got segmentation faults if I didn't del the QApplication instance after quitting and before opening the next one, so that seems to be important for shutting down the application properly.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - does this look ok now, or do you think I should make the reload function private anyway?

@ChrisBeaumont
Copy link
Member

Looks good!

On Friday, June 19, 2015, Thomas Robitaille notifications@github.com
wrote:

@ChrisBeaumont https://github.com/ChrisBeaumont - does this look ok
now, or do you think I should make the reload function private anyway?


Reply to this email directly or view it on GitHub
#3 (comment).

@astrofrog
Copy link
Member Author

Thanks!

astrofrog added a commit that referenced this pull request Jun 19, 2015
Started adding support for PyQt5
@astrofrog astrofrog merged commit e666b2a into glue-viz:master Jun 19, 2015
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

2 participants