Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
alexrockhill opened this issue Sep 1, 2021 · 10 comments
Open

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

alexrockhill opened this issue Sep 1, 2021 · 10 comments

Comments

@alexrockhill
Copy link
Collaborator

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
Copy link
Collaborator Author

Related to mne-tools/mne-python#9586

@larsoner
Copy link
Member

larsoner commented Sep 1, 2021

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
Copy link

GuillaumeFavelier commented Sep 2, 2021

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 #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
Copy link
Member

larsoner commented Mar 9, 2022

@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
Copy link

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
Copy link
Member

larsoner commented Mar 9, 2022

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:

spyder-ide/qtpy#233

We should probably look to see how Spyder did it

@GuillaumeFavelier
Copy link

Hm... It could be this one:

spyder-ide/qtpy#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
Copy link
Member

larsoner commented Mar 9, 2022

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

@GuillaumeFavelier
Copy link

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:

  • QDir to add the path to where the icons are located
  • importlib.resources which is a python alternative for resource management

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

@larsoner
Copy link
Member

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.

@larsoner larsoner changed the title [ENH] Abstract PyQt handling for version control and consistency [ENH] Abstract Qt/notebook handling in iEEG GUI Apr 19, 2022
@larsoner larsoner transferred this issue from mne-tools/mne-python May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants