Ticket #7950 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

Switch to PyQt v2 API for QString and QVariant

Reported by: Russell Taylor Owned by: Russell Taylor
Priority: blocker Milestone: Release 3.0
Component: GUI Keywords: IPython
Cc: Blocked By: #8038
Blocking: #5939 Tester: Michael Reuter

Description

This is required in order for us to be able to bring in IPython as the interpreter window. In this API, QString & QVariant are mapped to equivalent Python types instead of being wrapped objects as they are with the v1 API. There are other types that have a v2 API (see here), but these are the only ones we must move so that's as far as I will go.

Effecting the switch itself is easy, it's hunting down the places that break when we do it that's the hard part. It's no longer possible to instantiate these types on the Python side (easier to find), or to use methods of those types on returned objects (harder to find).

Change History

comment:1 Changed 7 years ago by Russell Taylor

  • Status changed from new to inprogress

Re #7950. Fix usage of QString method.

Changeset: c0121b27257d52f9a73b32e945cdf6c5e0ebc6af

comment:2 Changed 7 years ago by Russell Taylor

Hmmm... none of yesterday's commits showed up here.

comment:3 Changed 7 years ago by Russell Taylor

  • Status changed from inprogress to verify
  • Resolution set to fixed

Tester: The branch is feature/7950_pyqt_v2_api_for_qstring_qvariant. It has 10 commits, all but one of which didn't make it to here.

The main place this impacts is the PyQt reduction interfaces (ORNL SANS, DGS Reduction, REFM/L & SNS Powder Diffration). The creators of all these interfaces were asked to check them at the start of this week so have had ample time to do so. As this should have been checked (and I attempted to do so as well) I suggest just a little exploratory testing. Any problems will manifest themselves as Python errors that will abort the script.

The change could also theoretically affect the mantidplot(.)py code, though I couldn't find anywhere that it did. It's also possible, though unlikely I would think, that it could affect user scripts that deal with graphical outputs.

Last edited 7 years ago by Russell Taylor (previous) (diff)

comment:4 Changed 7 years ago by Alex Buts

  • Status changed from verify to verifying
  • Tester set to Alex Buts

comment:5 Changed 7 years ago by Alex Buts

  • Status changed from verifying to verify
  • Tester Alex Buts deleted

Can not test it properly untill #8038 is fixed

comment:6 Changed 7 years ago by Alex Buts

  • Blocked By 8038 added

comment:7 Changed 7 years ago by Russell Taylor

  • Status changed from verify to reopened
  • Resolution fixed deleted

It transpires that things are broken on RHEL6. There's apparently some difference there compared to the Mac (where the work was done). I thought python was supposed to be completely portable....

comment:8 Changed 7 years ago by Russell Taylor

  • Status changed from reopened to inprogress

Re #7950. Make sure variable is converted to the right type.

It seems that the settings are stored differently on different platforms and on linux this was reading in as a string instead of a boolean.

Changeset: 3bc3d43aabcb36ea85ec4dd9bab3f643a5bdc220

comment:9 Changed 7 years ago by Russell Taylor

Re #7950. Make sure empty list is loaded back correctly.

An empty list of recent files is saved to the QSettings as "@Invalid". Reading this back in sets the variable to 'None', which causes problems. Change it back to an empty list.

Changeset: 88889ae7c16ced215a0d570c75573bbf08f28193

comment:10 Changed 7 years ago by Russell Taylor

Re #7950. Remove barrier to starting reduction gui outside MantidPlot.

There are other places with an unguarded import of MantidPlot, but this one is trivial to fix.

Changeset: 3bb0f093bc5e974572578fd8576f6915c7da7dee

comment:11 Changed 7 years ago by Russell Taylor

Re #7950. Set PyQt API version for stand-alone running.

If running in Mantid, this setting is done within the C++ code. I'm adding it here to make sure things work if running outside of MantidPlot (which doesn't work at the moment, but could cause confusion later if that's fixed). Note that this has to be done before the first import of PyQt.

Changeset: 6bf36880a7baab19219c488bacf92c1e016d7780

comment:12 Changed 7 years ago by Russell Taylor

Re #7950. More changes for PyQt API change.

This is exactly the same change in several places. Previously the toPyObject() turned the returned QVariant into the desired type. Now the actual type is returned in the first place.

Changeset: 0d502b776166d0ad9930813c70b41817bbefe635

comment:13 Changed 7 years ago by Russell Taylor

  • Status changed from inprogress to verify
  • Resolution set to fixed

Tester: See comment:3. This is probably best tested by someone familiar with at least one of the python-based reduction interfaces, but the more important thing is that it gets tested soon!

Several more changes were required because it turned out that the the settings (QSettings) behaved differently on the Mac (where I did the initial work) to elsewhere. I'd also missed some things in the reflectometery GUI code. I've now checked again that simple reductions work for each interface on my branch and with mantidplotunstable. Of course there's no way I've executed all code paths. It seems unavoidable that some of the testing will effectively be by users after it's on master, but this needs to be merged to allow IPython to proceed.

Last edited 7 years ago by Russell Taylor (previous) (diff)

comment:14 Changed 7 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:15 Changed 7 years ago by Owen Arnold

  • Status changed from verifying to verify
  • Tester Owen Arnold deleted

The code changes look fine to me, but I'm not familiar enough with any of the interfaces that have been changed to say with any confidence that they have not been broken.

comment:16 Changed 7 years ago by Michael Reuter

  • Status changed from verify to verifying
  • Tester set to Michael Reuter

comment:17 Changed 7 years ago by Michael Reuter

  • Status changed from verifying to reopened
  • Resolution fixed deleted

Things seem to be working OK, but there's an annoying print statement left in line 274 in the reduction_application.py file. Please remove it.

comment:18 Changed 7 years ago by Russell Taylor

  • Status changed from reopened to inprogress

Re #7950. Remove debug print statement left in by mistake.

Changeset: 3da90953facab114d5edcad1a5b03e9e1b84c0bb

comment:19 Changed 7 years ago by Russell Taylor

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:20 Changed 7 years ago by Michael Reuter

  • Status changed from verify to verifying

comment:21 Changed 7 years ago by Michael Reuter

  • Status changed from verifying to closed

Looks like the automatic closure on commit forgot this one.

comment:22 Changed 7 years ago by Russell Taylor

Adding a link to the full changeset for possible future reference: https://github.com/mantidproject/mantid/commit/a7e6cdd03f92d7d7e9413228231e0781893f0576

comment:23 Changed 7 years ago by Russell Taylor

  • Keywords IPython added

comment:24 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 8795

Note: See TracTickets for help on using tickets.