mne-tools / mne-gui-addons

MNE-Python GUI addons
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

[ENH] Abstract Qt/notebook handling in iEEG GUI #14

Open alexrockhill opened 3 years ago

alexrockhill commented 3 years ago

The PyQt import handling should be deduplicated across MNE with one abstract function at some point. This will be especially useful when PyQt versions change.

This kind of code is what needs to be deduplicated in mne.gui._ieeg_locate_gui.py

from PyQt5.QtWidgets import (QMainWindow, QGridLayout,
                             QVBoxLayout, QHBoxLayout, QLabel,
                             QMessageBox, QWidget,
                             QListView, QSlider, QPushButton,
                             QComboBox, QPlainTextEdit)
alexrockhill commented 3 years ago

Related to https://github.com/mne-tools/mne-python/pull/9586

larsoner commented 3 years ago

Also need to deal with icon setting https://github.com/mne-tools/mne-python/blob/89dba30445e759ca98102998f6b39eaca292b0fd/mne/viz/backends/_pyvista.py#L89-L105. Ideally perhaps this would be part of the qt/notebook abstraction, too.

GuillaumeFavelier commented 3 years ago

That would be awesome if _ieeg_locate_gui used the abstract GUI API because it means that the same features would be available on notebooks so +1 for this.

I quickly went through the list of widgets in https://github.com/mne-tools/mne-gui-addons/issues/14 and VBoxLayout, HBoxLayout, Label, Slider, PushButton and ComboBox should not be too hard to migrate considering they are already available in the API.

That leaves out MainWindow, GridLayout, MessageBox, ListView and PlainTextEdit. I'm not familiar with all but I believe most of them can be adapted to ipywidgets somehow. Plus, I think it's fine even if we can't reach 100% compatibility. Anyway, it's good to improve the API.

larsoner commented 2 years ago

@GuillaumeFavelier what's required for us to have code that works on PyQt5 and PyQt6? Let's just think about Brain / pyvista / backends/_qt.py for now.

I think we can assume we can get it for free for the iEEG GUI once we make that use the same notebook / Qt abstraction.

GuillaumeFavelier commented 2 years ago

The biggest changes I see would be anything based on enums. Now they're hidden behind their namespace instead of being all exposed in QtCore.Qt. For example, Qt.WaitCursor becomes Qt.CursorShape.WaitCursor.

So I would suggest using an intermediate variable like the following:

CursorShape = Qt.CursorShape if PYQT6 else Qt
...
renderer._window_set_cursor(CursorShape.WaitCursor)

The rest should be straightforward, like changing the actual imports.

Of course, it's possible that I miss some details but this is what I remember at the moment.

larsoner commented 2 years ago

I'd be surprised if qtpy didn't offer some abstraction for this sort of thing since Spyder must have dealt with it, so maybe we should just use / depend on qtpy for our qt stuff. I haven't read all of this, but it's probably in here or some linked issue/PR:

https://github.com/spyder-ide/qtpy/issues/233

We should probably look to see how Spyder did it

GuillaumeFavelier commented 2 years ago

Hm... It could be this one:

https://github.com/spyder-ide/qtpy/pull/271

AFAIK, it provides access to unscoped enums. Maybe I didn't try the latest version 😅

Anyway I think using qtpy is a good idea.

larsoner commented 2 years ago

And if we could get PyQt5/PyQt6/PySide2/PySide6 compat all at once via qtpy, that would be great...

GuillaumeFavelier commented 2 years ago

When I experimented with this I discovered that the resource system has not been migrated to PyQt6:

https://stackoverflow.com/questions/66099225/how-can-resources-be-provided-in-pyqt6-which-has-no-pyrcc

This is problematic for our icons but multiple solutions are suggested in the thread:

I'll share what I have as a draft PR.

larsoner commented 2 years ago

We've completed the abstraction of Qt itself, now the IEEG GUI "just" needs to be migrated to the notebook/Qt abstraction framework. I'll re-title this issue to reflect this.