search

labscript-suite / runviewer

π—Ώπ˜‚π—»π˜ƒπ—Άπ—²π˜„π—²π—Ώ is a graphical interface for visualising hardware-timed experiments composed using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
BSD 2-Clause "Simplified" License
2 stars 39 forks source link

Filepath issue, possible Qt API issues #10

Closed philipstarkey closed 7 years ago

philipstarkey commented 7 years ago

Original report (archived issue) by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

With current mainline runviewer I get an exception opening a shot file:

#!python

Traceback (most recent call last):
  File "/home/cjb7/labscript_suite/runviewer/__main__.py", line 291, in on_add_shot
    selected_files = [os.path.abspath(shot_file) for shot_file in selected_files]
  File "/usr/lib/python2.7/posixpath.py", line 367, in abspath
    if not isabs(path):
  File "/usr/lib/python2.7/posixpath.py", line 61, in isabs
    return s.startswith('/')
AttributeError: 'QString' object has no attribute 'startswith'

This is clearly because the code selected_files = [os.path.abspath(shot_file) for shot_file in selected_files] has been copied from lyse or runmanager, which are configured to use version 2 of the PyQt4 API, in which selected_files is a list of ordinary Python strings, whereas runviewer uses the older version 1, in which selected_files is a list of QString objects.

The fix for me is to change it to: shot_files = [os.path.abspath(str(shot_file)) for shot_file in shot_files] to convert the QStrings into normal strings, but I'm wondering why this seemingly doesn't break for you, @shaunj. Perhaps the import stuff at the start is making runviewer use PySide? Perhaps PySide has changed to use the new API?

str(shot_file) will work for either version of the API, so is a fix regardless. I also wonder if this is the source of the confusion over whether some QColor object is a function or not.

Keep in mind that PyQt API version 2 is the future, so if current code is compatible with it, the following should be added at the top of programs to enable it:

#!python

import sip

# Have to set PyQt API via sip before importing PyQt:
API_NAMES = ["QDate", "QDateTime", "QString", "QTextStream", "QTime", "QUrl", "QVariant"]
API_VERSION = 2
for name in API_NAMES:
    sip.setapi(name, API_VERSION)

Which will make it much easier to port to PyQt5 and Python 3, both of which only support the new API.

I am unsure how PySide factors in to this, the above code doesn't do anything with it, and I'm not sure what API version it uses. PySide apparently doesn't work with Qt 5 or Python 3.5 yet, meaning if we port to those versions before they do, we will probably just have to drop support for PySide.

There is also the complication of pyqtgraph - it also chooses what Qt wrapper to use depending on what has already been imported, and so it can influence runviewer's choice because it is imported before runviewer makes the decision. However pyqtgraph appears to have always preferred PyQt4.

Just some things to consider. I'm sure @philipstarkey has opinions about PySide etc.

philipstarkey commented 7 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

The QColor issue is not related to this. Somewhere between version 4.11.1 and 4.11.4, they dropped support for automatically calling functions that were passed in. QColor(lambda: Qt.Red) works on 4.11.1 and not on 4.11.4. I probably didn't mean to be passing in a function, but it used to work anyway!

I think the issue with the strings here is because abspath is implemented differently on windows and linux, so Shaun didn't see the error (it doesn't show up on windows). He's implemented your fix with str for now and will make a new pull request shortly. We should probably turn on API v2, but that requires more debugging and effort to make sure that PySide still works properly too (assuming we don't just drop PySide support...)

philipstarkey commented 7 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

Also, FYI pyqtgraph is specifically imported after the PyQt4/PySide is imported (there is a comment to this effect in the code)

philipstarkey commented 7 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

Ah, but the version check on pyqtgraph (which import pyqtgraph) happens prior.

philipstarkey commented 7 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

Oh, well that's a bug then!

philipstarkey commented 7 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

As for the fix: Thanks! Anyone going to do any major rejigging of GUI code should decide whether it's worth making the switch to API 2 before or after, but other than that this is entirely sufficient for the moment.

We'll see if Pyside is up to scratch with the latest Python 3 and Qt 5 by the time the other programs are being ported, if it is then we don't have to choose to drop it.

philipstarkey commented 7 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

Somewhat relatedly, had a bug recently where pandas 0.20.1 would import PyQt4 and set the API to version 1. Lyse of course was not happy about this. Pandas has since fixed it to make those imports optional. Didn't seem to happen on my system though, despite using the same version of pandas, but if anyone sees that, there's your explanation.

philipstarkey commented 7 years ago

Original comment by Shaun Johnstone (Bitbucket: shjohnst, GitHub: shjohnst).

Fixed in pull request #8