enthought / pyface

pyface: traits-capable windowing framework
Other
105 stars 55 forks source link

Use "exec()" instead of "exec_()" with PySide6 #1179

Closed rahulporuri closed 1 year ago

rahulporuri commented 1 year ago

We see the following message when running our testsuite -

C:\Users\rporuri\.edm\envs\ds-msrl-dev\lib\site-packages\pyface\ui\qt4\util\event_loop_helper.py:135: DeprecationWarning: 'exec_' will be removed in the future. Use 'exec' instead.
  self.qt_app.exec_()

The two instances of exec_() that I can find in event_loop_helper are the following - https://github.com/enthought/pyface/blob/af6da55986a8b94773c4ddcc57e8284f5be617dc/pyface/ui/qt4/util/event_loop_helper.py#L135

https://github.com/enthought/pyface/blob/af6da55986a8b94773c4ddcc57e8284f5be617dc/pyface/ui/qt4/util/event_loop_helper.py#L169

Note that there are a number of other uses of exec_() sprinkled throughout the codebase - https://github.com/enthought/pyface/search?q=%22exec_%28%29%22

This is included in the porting guide for pyside2 to pyside6 - https://doc.qt.io/qtforpython/porting_from2.html?highlight=exec_#class-function-deprecations

corranwebster commented 1 year ago

This is a lingering effect fo the 2->3 transition (exec was a keyword in Python 2).

IIRC we can't migrate to using exec everywhere because there was one of the backends which didn't support it (PyQt5, perhaps). So we need to live with these warnings for now - if they are problematic for your app you can silence them with appropriate warnings filters.

There is a possibility that it was PyQt4 which was the problem, and we aren't supporting that any more, but I'd need to dig in a bit more to see exactly which backends support this.

rahulporuri commented 1 year ago

There is a possibility that it was PyQt4 which was the problem, and we aren't supporting that any more, but I'd need to dig in a bit more to see exactly which backends support this.

It looks like PyQt4 and PySide2 are both a problem - PySide6, PyQt5 and PyQt6 seem happy. Ref experimentation in https://github.com/enthought/pyface/pull/1180. The documentation made me believe PySide2 would be happy with exec() but that's not really the case. Ref https://doc.qt.io/qtforpython-5/PySide2/QtCore/QCoreApplication.html#PySide2.QtCore.PySide2.QtCore.QCoreApplication.exec_ .

I'm not sure/I don't remember if we still support pyqt because we're not testing against it in CI but it's still explicitly mentioned as an option in the dependencies.

corranwebster commented 1 year ago

We don't care about any of the Qt4 backends - we haven't ripped the code out yet, but we will eventually when there is time.

One possibility is to monkeypatch the base PySide2 Qt Application class (QCoreApplication I think) in pyface.qt.... so it gains an exec method that just calls exec_. That's essentially what pyface.qt is for - to smooth over the differences between the various backends.

mdickinson commented 1 year ago

It would be good to fix this; it's causing significant noise in downstream tests for PySide6-using applications.

One possibility is to monkeypatch the base PySide2 Qt Application class [...]

I think there are other classes too that would need this - e.g., QDialog and friends. Not sure how big the complete list of classes that needed the patching would be.

How about just a simple-minded

if hasattr(thingy, "exec"):
    thingy.exec()
else:
    thingy.exec_()

everywhere, or an equivalent try / except? Eventually (when Qt 5 is but a distant memory) we'd be able to get rid of the exec_ path.