search

gpilab / framework

The GPI framework provides the canvas for graphically assembling algorithms.
Other
20 stars 8 forks source link

Support multiple Qt versions (PyQt4. PyQt5, PySide2, PySide) support via qtpy #11

Closed grlee77 closed 5 years ago

grlee77 commented 6 years ago

This PR uses qtpy to add support for Qt 5 via either PyQt5 or PySide2 and Qt 4 support via PySide. The previously existing PyQt4 support has been maintained. qtpy is a small python package that is easily installable via pip or conda and is used by the Spyder IDE and other projects to support multiple Qt variants.

Adding PySide support also provides the benefit of supporting an LGPL-licensed Qt backend (PyQt instead uses a GPLv3 license).

I have tested this code on the provided examples in pyqt 4.11.4 (Qt 4.8.7), pyqt 5.6.0 (Qt 5.6.2), pyqt 5.9.2 (Qt 5.9.4) and PySide2 (Qt 5.6.2).

The biggest share of changes are the move of widgets out of the QtGui module into their own QtWidgets module. Here the Qt5 API is used and qtpy takes care of making pyqt4 and pyside work with the Qt 5 API conventions. There are a couple other places where object attributes changed between Qt 4 and 5. For these a try/except AttributeError was used where needed to support both versions. These cases would not be necessary if it is desired to just drop Qt 4 support going forward.

There are only a couple areas that were a bit tricky to convert. 1.) QGraphicsItemAnimation as used by the "Macro Node" is no longer available in recent Qt. It has been replaced with equivalent code using QPropertyAnimation instead.

2.) The library node search results pop-up window was modified in 6d6f298 so that it also works in Qt 5. I wasn't entirely clear on the purpose of the FauxWindow class, but for some reason it would not display properly in Qt 5 (it was ending up hidden under the main canvas and the buttons were not functional). The new approach is simpler and works on Qt 4 and 5, but perhaps there is some subtle difference?

3.) QtWebKit is deprecated, but is still present in PyQt5 and PySide2. It was just necessary to import it from QtWebKitWidgets instead.

Finally, commit e7e444a here changes a font name from "times" to "Times New Roman". In PyQt5, I got very poorly rendered text with "times", which is fixed by changing the name. The buttons look fine for either name in PyQt4.

before: times

after: timesnewroman

If this is going to be merged, there is a smaller companion PR to core-nodes so that those also support Qt 5 and are compatible with the changes made here.

TODO:

aganders3 commented 6 years ago

Thanks @grlee77! I've been hoping to find time to update the code to support Qt5 with PyQT, but I was unaware of qtpy. I also thought the PySide project had gone stagnant, but it's good to see that included as well. I think qtpy is a reasonable additional dependency for this additional flexibility.

This LGTM but I'll test the code before merging and then we can create a new conda package.

@nckz, any thoughts? Maybe you can also comment on the FauxWindow workaround since that was implemented before my time.

grlee77 commented 6 years ago

I also thought the PySide project had gone stagnant,

Apparently, it is going to be officially supported going forward, but is now being called "Qt for Python".

I suspect given the official support, it may become more dominant going forward, so we should test against it too. There is a release available at the link below, but I haven't had a chance to try it yet. http://blog.qt.io/blog/2018/06/13/qt-python-5-11-released/

It looks like it is still imported under the name PySide2 so perhaps it will already just work as is with qtpy.

nckz commented 6 years ago

Thanks @grlee77 this looks good. I'm eager to move forward to Qt5 as well.

The FauxMenu class was made to deal with rendering the menu as the user typed characters into the node-search. It was made for some idiosyncrasies in the first version of Qt4. -if it works now without, thats even better :).

I'd like to play around with this branch a little more before pulling.

aganders3 commented 6 years ago

I have this working and will try using it as my "daily driver" for a bit to shake out any bugs.

The only bug I see so far is upon quitting GPI I get the "Do you want to quit without saving?" dialog twice.

This is kind of a separate issue that should be solved by keeping track of if/when a tab has been saved.

grlee77 commented 6 years ago

Great. Thanks for the quick review.

By all means, please test more thoroughly to make sure there aren't other issues I may have missed. I think as maintainers you two will have the necessary permissions to push additional commits directly to my branch if you want to make revisions.

aganders3 commented 6 years ago

I think I'm experiencing the problem the FauxMenu was meant to solve.

When searching in the node menu, focus is stolen from the text field as soon as the results pop up. This makes it so you have to click the text field to resume typing after each letter is entered. I agree not using the FauxMenu is preferable if we can work around this in another way - there is an issue with resolution/anti-aliasing in the FauxMenu that using a real menu resolves.

grlee77 commented 6 years ago

focus is stolen from the text field as soon as the results pop up

Yes, I see this now too. I'm not sure why I didn't notice it previously, but it is pretty annoying. It looks like we may have to work out how to update the FauxWindow code for Qt5 compatibility.

I have not seen the other issue you reported about "Do you want to quit without saving?" appearing twice.

nckz commented 6 years ago

This is on my list.

aganders3 commented 6 years ago

I've also been testing this branch with still very few issues so far. Hopefully next week I will have some time to capture some of the minor issues I've been having. I also modified FauxMenu to work without reverting to the raster version. I think I can push that up to this branch somehow...

grlee77 commented 6 years ago

Glad to hear that this has been working on your end for the most part.

I think I can push that up to this branch somehow...

Yes, any members with write permissions to gpilab/framework should be able to push commits directly to my branch (I left the "allow edits from mainters" checkbox selected on this PR in GitHub). If that doesn't work for some reason you can make a PR against my fork, but it shouldn't be necessary.

aganders3 commented 6 years ago

OK, thanks. That was a little confusing but it looks like I got it.

Please test if my new FauxMenu is working for you. It seems to work well with PyQt5, but feels slow in PyQt4. I'm not sure it's related to this code, however. I have not tested with PySide.

aganders3 commented 6 years ago

Another problem I found was in the code for dragging/dropping widgets to/from Layout windows (see /lib/gpi/widgets.py:1017).

In PyQt5 widget gets a method self.grab() to replace QtGui.QPixmap().getWidget(self), but unfortunately this is not backwards compatible. It works to test the QT_API_NAME and change the behavior based on that. I'm not too familiar but this feels like a limitation in qtpy for writing version-agnostic code. Any thoughts?

In the same function, there also seems to be a bug in handling passing of the object id as a str to QtCore.QByteArray(). I think this is a Python 3 bytes/string bug, though, so I'm not sure it's related to this branch at all.

borupdaniel commented 5 years ago

I'm giving this a test run on a fairly large node network and it seems to be working pretty well. @aganders3, I'm not having any issues with the FauxMenu — it seems to respond with no delay in PyQt4. I tried using both PyQt4 and PyQt5 (did not try PySide yet).

A couple of things popped up (mostly node-related):

PyQt4

PyQt5 (5.6.0)

Other things:

aganders3 commented 5 years ago

Thanks @borupdaniel - this is good stuff. Have you noticed the double-confirmation on quitting or is that a quirk of my system somehow?

I'll try to make the requirements changes for Matplotlib ASAP and push new builds to Anaconda.org. Hopefully I can get around to this tomorrow at least for macOS.

aganders3 commented 5 years ago

I would not prioritize testing with PySide at this point. I think it's a nice-to-have but that's a pretty big change for us and can wait. I think relaxing the PyQt requirement even to just >4, <5.6 is great progress.

borupdaniel commented 5 years ago

I do get the same issue with double-confirmation on quitting, but only with QtPy5. With 4.11.4 it behaves normally; with 5.6.0 I get the normal confirmation with the canvas open, then an extra one by itself after the canvas has disappeared.

aganders3 commented 5 years ago

I do get the same issue with double-confirmation on quitting

This doesn't seem like a showstopper but I'd rather not let it through if we can fix it. Super weird to me that it only happens with 5. Ultimately GPI needs to be more document-based so we can including auto-saving and get rid of this dialog if the network on disk is up-to-date. This may work around the issue in the long run but I'd love to see this PR merged sooner than later.

borupdaniel commented 5 years ago

I agree it would be nice to fix. I'm happy to look into it more in the next day or two to help me get some more experience with the framework.

borupdaniel commented 5 years ago

EDIT: okay, I would have sworn this worked yesterday afternoon, but now it makes no difference, so I'm not sure what's up...

ORIGINAL POST: @aganders3 I've traced this to the "close with dialog" function getting called twice, similar to this issue on SO.

A quick two-liner fix to canvasGraphy.py is the following. A new import statement:

from PyQt5.QtCore import pyqtSlot

and later, adding this @ statement .just before closeGraphWithDialog is specified.

@pyqtSlot()
def closeGraphWithDialog(self):

The import statement is obviously not flexible with different PyQt versions, but I think the info at this link should be enough to point me in the right direction to finish this fix.

aganders3 commented 5 years ago

The fix you propose seems OK to me. Maybe we need to try/except the import and assign a dummy decorator if it fails? I'm not sure if that or an if PYQT5 block or whatever is better.

But I think this is actually two problems. I am getting closeEvent called twice (the dialogs are very similar) for the MainCanvas object when I hit ⌘Q or chose "Quit GPI" from the menu bar (all my testing so far has been on a mac). Seems like this may be a Qt bug so we need a workaround or to ignore it. https://bugreports.qt.io/browse/QTBUG-43344

borupdaniel commented 5 years ago

Ah, this explains things — the issue doesn't happen if you close from the button at top left of the window. So, my fix above doesn't make any difference, I just happened to be testing it by clicking the button instead of command-Q.

aganders3 commented 5 years ago

This seems to work in Qt 5.6 but it's a bit ugly.

diff --git a/lib/gpi/mainWindow.py b/lib/gpi/mainWindow.py
index cb88656..7baf8e0 100644
--- a/lib/gpi/mainWindow.py
+++ b/lib/gpi/mainWindow.py
@@ -157,6 +157,9 @@ class MainCanvas(QtWidgets.QMainWindow):

             self.updateCanvasStatus()

+        # fix for Qt5 bug https://bugreports.qt.io/browse/QTBUG-43344
+        self.close_event_accepted = False
+
     def setStatusTip(self, msg):
         self.statusBar().showMessage(msg)

@@ -236,14 +239,16 @@ class MainCanvas(QtWidgets.QMainWindow):
     def closeEvent(self, event):
         '''close all graphs before shutting down.
         '''
-        if not self.quitConfirmed():
+        if self.close_event_accepted:
             event.ignore()
-            return
-
-        while self.tabs.count():
-            self.tabs.widget(0).close()
-            self.tabs.removeTab(0)
-        event.accept()
+        elif not self.quitConfirmed():
+            event.ignore()
+        else:
+            while self.tabs.count():
+                self.tabs.widget(0).close()
+                self.tabs.removeTab(0)
+            self.close_event_accepted = True
+            event.accept()

     def closeCanvasTab(self, index):
         '''Leave at least one tab open.
borupdaniel commented 5 years ago

I have the following, similarly ugly but functional. 

diff --git a/lib/gpi/mainWindow.py b/lib/gpi/mainWindow.py
index cb88656..de9b9d3 100644
--- a/lib/gpi/mainWindow.py
+++ b/lib/gpi/mainWindow.py
@@ -215,6 +215,7 @@ class MainCanvas(QtWidgets.QMainWindow):
                         QtWidgets.QMessageBox.No, QtWidgets.QMessageBox.No)

         if reply == QtWidgets.QMessageBox.Yes:
+            self.already_closed = True
             return True
         else:
             return False
@@ -234,11 +235,14 @@ class MainCanvas(QtWidgets.QMainWindow):
         self.updateCanvasStatus()

     def closeEvent(self, event):
-        '''close all graphs before shutting down.
-        '''
-        if not self.quitConfirmed():
-            event.ignore()
-            return
+        try:
+            self.already_closed
+        except:
+            '''close all graphs before shutting down.
+            '''
+            if not self.quitConfirmed():
+                event.ignore()
+                return
borupdaniel commented 5 years ago

@aganders3, is there a way to specify a specific build for a package? The issue with qtpy 5.9.2 doesn't show up with an earlier build, so it would be nice to say something like "use pyqt 4 or 5, but for 5.9.2, don't use build py36h655552a_2", or something. This would improve compatibility downstream, although it's a bit more complicated on our end.

Here's the change in conda that addresses this issue: 

The following packages will be DOWNGRADED:

  pyqt                                 5.9.2-py36h655552a_2 --> 5.9.2-py36h655552a_0
borupdaniel commented 5 years ago

I think we're probably ready to merge this unless you want to do any additional testing, @aganders3. I think we can specify PyQt <= 5.6 for the build and keep an eye on the 5.9.2 bug separately. What do you think — should we go ahead with this?

aganders3 commented 5 years ago

I think yes but will allow others to chime in if there are any protestations. We can merge without doing a release as well for now.

Let's discuss this week about building/uploading a release, but actually doing this should probably wait until after the ISMRM demo.

borupdaniel commented 5 years ago

I've created a new "qt5" branch and am going to go ahead with this pull request. We're thinking to make this GPI v1.1.0 and wanted to hold off on putting it in the main development branch while we get a v1.0.6 built as we work to getting GPI on conda forge.

Thanks @grlee77 for your work on this — great addition to the project!

aganders3 commented 5 years ago

@aganders3, is there a way to specify a specific build for a package? The issue with qtpy 5.9.2 doesn't show up with an earlier build, so it would be nice to say something like "use pyqt 4 or 5, but for 5.9.2, don't use build py36h655552a_2", or something. This would improve compatibility downstream, although it's a bit more complicated on our end.

Here's the change in conda that addresses this issue:

The following packages will be DOWNGRADED:

  pyqt                                 5.9.2-py36h655552a_2 --> 5.9.2-py36h655552a_0

Following up on this, you can use conda install <package_name>=<version>=<build_string> to specify a specific build when installing. I don't think there's a way to exclude a specific build from consideration but maybe you can submit it as an issue to the packager?

borupdaniel commented 5 years ago

@aganders3 is there anything you still want to do with v1.0.6? If not I'll go ahead and merge this into the develop branch preparing for 1.1.0.

aganders3 commented 5 years ago

I don't think so...I merged and tagged 1.0.6 in master so I think we're OK to merge this into develop even though we don't have 1.0.6 packages yet.

We should do this in the core nodes repo as well, and then we can create conda packages from the develop branches for final testing.

borupdaniel commented 5 years ago

I went ahead and merged this into develop, as well as in the core nodes repo. I'm going to switch over to a new issue for preparing the v1.1.0 release on conda-forge.

aganders3 commented 5 years ago

Thanks @borupdaniel - I'm working on release candidate builds for 1.0.6 as I update the build recipes to...actually work again.

borupdaniel commented 5 years ago

Sounds good.

Future reference: see #22 for the new discussion.