[MAINT] Remove redundancy in Abtract Qt Classes

[alexrockhill]

E.g. here https://github.com/mne-tools/mne-python/blob/main/mne/viz/backends/_abstract.py#L491. Why do methods of a Qt abstract object have the name of the object in them? i.e.

toolbar = _QtToolBar(...)
toolbar._tool_bar_add_button(...)

seems very redundant when

toolbar = _QtToolBar(...)
toolbar._add_button(...)

seems like it will do and will be much easier to maintain.

Maybe @larsoner, you have thoughts on this/a good reason for why it is this way?

[alexrockhill]

It seems very not DRY because, for instance, toolbar._tool_bar_add_button is pretty much the same as adding a button for a status bar or a lot of other widgets and they could easily inherit unchanged most of the time or save redundancy by calling super()._add_button first.

[alexrockhill]

Oh I see because you can call self._renderer._layout_add_widget for instance to add a widget to a layout. This seems rather inelegant. I would be in favor of doing something like self._status_bar = self._configure_status_bar() so that the attribute doesn't appear from nowhere and then it's much easier to read and maintain without having to look in the _qt.py classes for what attribute self._configure_status_bar() initializes. What do people think?

[alexrockhill]

Maybe I need to dwell on this and see if I warm up to it, but this method of abstraction in #10565 seems terribly confusing to me. When you call main_layout = self._renderer._layout_create(orientaton='vertical') you get an object that you have to manage and confusingly pass to the renderer just to add widgets or layouts to it (main_layout._add_widget(...) is much more natural than self._renderer._layout_add_widget(main_layout, ...)). That seems very inelegant and confusing to me, especially when self._configure_status_bar() doesn't return anything at all and just adds attributes.

I think the best solution is more toward the first one, otherwise it's going to be impossible to keep track of all the attributes that are created behind the scenes in the abstract classes and obviously the layout had to be returned because you can have more than 1 of them. You probably don't want more than one toolbar but to me a toolbar and a status bar are pretty arbitrary distinctions and the more general, powerful abstraction is that of a bar which can be top, bottom or either side and potentially doubled up (I was considering a spectogram bar above the toolbar for a vertical layout).

In summary, a big -1 from me on the direction that this is going although I do think it's salvageable as long as everything gets returned from the abstract classes and then you can interact with abstract object methods that are documented in the abstract class like _AbstractRenderer. That seems like the perfect example of exactly how we want to do things, I'm not sure why things ended up so different and confusing. A lot of it has been merged already in https://github.com/mne-tools/mne-python/blob/main/mne/viz/backends/_notebook.py though but I don't think it's really deployed and used yet in any GUIs so I think now is the crucial time to intervene.

[larsoner]

I am wary of a huge refactoring because stuff almost always breaks. But if in the end it makes things more understandable and maintainable, it could be worth it. But the risk of potential additional breakage is high, especially since GUI stuff is very difficult to test with unit tests rather than direct manual interaction / use.

+1 for dwelling on it a bit and waiting -- it's always a challenge to understand someone else's code and why they made some choices. You've already started out thinking "this naming scheme doesn't any sense" but realized quickly why it had to be that way (due to how the subclasses are actually used). Let's hold off on a big refactor until you've used the code a bit and understand it through experience a bit. Especially because...

A lot of it has been merged already in https://github.com/mne-tools/mne-python/blob/main/mne/viz/backends/_notebook.py though but I don't think it's really deployed and used yet in any GUIs so I think now is the crucial time to intervene.

... this whole abstraction came about precisely because we wanted to use it in Brain so that we could do things equivalently in the notebook and qt backends. So if you continue to look at the code, you should see all of these abstractions in use in Brain already somewhere.

So I would say let's wait until you need something new from the classes, then add it and get it working. Once that's done, if you still think things could be clearer, a refactoring is much more likely to go smoothly because you have direct experience modifying/improving/fixing bugs with the existing structure.

[alexrockhill]

Yeah it's definitely in the coreg GUI, I just wasn't following those changes very closely. I don't get the whole concept of docks and toolbars and status bars being separate, pretty much unrelated entities, they're all things that hang around the sides of an application, why are they all separate? That's one of two main critiques. I know the Qt interface well enough to know that it can be abstracted as a bar so I assume it must be because of needs on the notebook side. The other main one is the different styles of returning objects vs setting attributes. This seems like a big code smell issue to me.

So I would say let's wait until you need something new from the classes, then add it and get it working. Once that's done, if > you still think things could be clearer, a refactoring is much more likely to go smoothly because you have direct experience > modifying/improving/fixing bugs with the existing structure.

The issue with that is that as soon as you stop running things through QMainWindow and run everything through the renderer, then you are either all in on abstraction or have this weird mix of calling abstract classes and also importing Qt objects which also sounds terrible. Since I clearly want/need to change things before using them, I'd have to see how that impacts the coreg GUI in Qt and also in the notebook implementation which is a huge overhead. I know that the PR that was written works, I just think it makes things 10x less elegant and I think harder to maintain down the road. I can move forward with incremental PRs that improve and synchronize across coreg Qt and notebook and ieeg_locate Qt but that will move slowly.

[larsoner]

Yeah it's definitely in the coreg GUI

Yes, and Brain. So the iEEG GUI is the one remaining part of MNE-Python proper that doesn't use the Qt/notebook abstraction. (FWIW, I would have pushed for mne-qt-browser to use such an abstraction if possible/reasonable, but it wouldn't be because the underlying display must go through Qt because it's the drawing backend, so there is no real way to abstract it.)

I don't get the whole concept of docks and toolbars and status bars being separate, pretty much unrelated entities, they're all things that hang around the sides of an application, why are they all separate?

I could see it going either way, and really I don't think it makes a huge difference either way, does it? Typically you only create a toolbar (or don't), and a statusbar (or don't). Thinking of these abstractly as "things around the sides" doesn't seem that helpful as an abstraction to actually implement, even if it's true of where they typically are.

I know the Qt interface well enough to know that it can be abstracted as a bar

I guess, but typically in practice they are constructed by end users to direct calls to add/create QToolBar and add/create QStatusBar, not via some abstract class. So I'm not sure that Qt having them under the hood implemented as subclasses of a Bar or something really matters/helps us, does it? If it doesn't help us in some way, then we shouldn't bother implementing that deeper level of abstraction.

then you are either all in on abstraction

Yes this is the maintainable idea I think

Since I clearly want/need to change things before using them, I'd have to see how that impacts the coreg GUI in Qt and also in the notebook implementation which is a huge overhead

Indeed, the price we pay for this abstraction is that, instead of just using Qt directly, we need to create an abstraction for whatever new feature we need. This needs to be implemented and tested both places. To me the tradeoff so far has been worth it to have Notebook + Qt support in Brain and coreg.

I just think it makes things 10x less elegant and I think harder to maintain down the road

Maybe this particular implementation isn't perfect or easily understandable. But in the end it should make things simpler, assuming we want to support both Qt and Notebook in MNE-Python, and if we ever want to support one for any GUI. You understand the abstraction and you get the backend flexibility. The cost is that enhancements to the abstraction require implementation and testing at all levels.

So to me it sounds like you are arguing two points: first is that our abstraction is unclear, or not implemented as cleanly as it could be -- and this is entirely possible. But we've covered that above (sure let's change it, but hopefully later).

The second, though, seems to be that the abstraction is not useful, or that we shouldn't use it for the iEEG GUI. This part I think makes MNE-Python less maintainable in the long run, because instead of having to understand, implement, and test Qt in one place (our abstraction), we have to do so in two places (the iEEG GUI code and our abstraction).

[alexrockhill]

The second, though, seems to be that the abstraction is not useful, or that we shouldn't use it for the iEEG GUI. This part I think makes MNE-Python less maintainable in the long run, because instead of having to understand, implement, and test Qt in one place (our abstraction), we have to do so in two places (the iEEG GUI code and our abstraction).

I think the abstraction is very useful, just think the things that were done for coreg that would apply to the ieeg_locate GUI need major rewrites. I might be wrong but there seems to be a growing consensus from people who did not write the coreg GUI that it's not very intuitive, which, to me means that it's probably good to have some rewriting there anyway. Maybe you could say the same about the ieeg_locate GUI as well but since it's just in Qt, it's much easier to change and doesn't effect the coreg GUI. Maybe the abstraction should mature with Qt and the coreg, ieeg_locate and TFR source viewer that I said I would write for GSoC before being ported to notebook so that the abstraction works for everyone rather than pigeonholing things to how they were done for coreg.

[alexrockhill]

Also is there not a tutorial for the coreg GUI? I see https://mne.tools/dev/auto_tutorials/forward/20_source_alignment.html#defining-the-headmri-trans-using-the-gui but probably it would be nice to add executed code that pops up the GUI. I can open a separate issue.

[larsoner]

so that the abstraction works for everyone rather than pigeonholing things to how they were done for coreg.

Hmm... you keep saying "just coreg", so somehow you must have missed the couple of times I've mentioned so far that it's used for Brain, too (and thus stc.plot, etc.). IIRC this is actually where it started, even though that's more a historical point:

• Brain: MRG: Pyvista background plotter #6551
• Brain: WIP: Prototype of notebook viz (screencast) #7758
• Brain: MRG: Prototype of notebook viz (ipyvtk) #8503
• And later, coreg ported from traits/traitsui/mayavi to the existing abstraction: ENH: Coreg GUI #9689

So the idea has always been IIUC to have this abstraction and use it across MNE-Python wherever possible. Hence why I'm pushing back on the idea of scrapping what we already have and starting from scratch. We already have an abstraction that works decently even if it's not perfect, or ideally implemented. But maybe others have opinions on this point.

FWIW to me the coreg usability is really a separate issue, that has to do with user interactions and the user-facing GUI design and interaction. It seems like your concerns so far are about internal implementation details that are much less user-facing.

[larsoner]

I can open a separate issue.

Please search first, I seem to recall that there are already one or two issues about coreg and its usage needing to be documented better

[alexrockhill]

I need to look more into the Brain backend to comment more on that but that seems like it would be a bit easier to test and modify since the GUI is lighter.

The importance of returning the objects for things like status and toolbars is that then they can be modified in inherited objects (i.e. adding items to the toolbar) which seems pretty important. I think that would be one of the main things.

EDIT: Reading your last comment more closely, I don't think we need to scrap the whole thing and start from scratch but all the toolbar code that led to the origination of this issue should really be examined.

[alexrockhill]

After thinking about it, the main issue comes from the Qt concept of a window vs no window for notebook as Guillaume had mentioned. I still think you need a holder, I'll just have to see how Render implements it. This abstraction should work for GUIs without a 3D component but right now it doesn't because of this issue.

[alexrockhill]

Ok after looking more closely at both the brain and coreg implementations, I think the main issue that is the root cause of this redundancy is that everything is run through the renderer. I think this is hugely limiting not just because it creates inelegant code but also because it limits this abstraction to only apply to GUIs with 3D brains in them. The ieeg_locate GUI original plans did not have a 3D brain and the NutmegTrip GUI does not have a 3D brain so I think there are plenty of use-cases for that. As @larsoner had mentioned is not at all user-facing so the redundancy/inelegance of the code won't show up outside private functions but I still think this is a huge burden on developing new GUIs, especially when reusing parts as in the slice browser abstraction as well as limiting GUIs with this framework to those with 3D brains.

Hopefully the solution is to just make an AbtractApplication/AbstractGUI/AbstractWindow class and just reimplement things more elegantly using the Qt style. I can look into it, not sure if that will work with notebooks per previous comments of Guillaume although the renderer handles windows in this way so maybe it won't be hard at all.

[larsoner]

hugely limiting not just because it creates inelegant code but also because it limits this abstraction ... this is a huge burden on developing new GUIs ... Hopefully the solution is to just make an AbtractApplication/AbstractGUI/AbstractWindow class and just reimplement things

@alexrockhill it sounds like your plan here is to start from scratch / start over, is that right? I am pretty opposed to this idea as it loses / discards all the progress we have made so far by testing and using the existing framework over the last year or two.

I think the main issue that is the root cause of this redundancy is that everything is run through the renderer.

A faster and hopefully easier solution (rather than starting an opposing/replacement implementation from scratch) would be the incremental addition of something like use_3d=True:

1. use_3d=True: a 3D scene use the old code (and window creation, etc.) to get a layout that you can put widgets into.
2. use_3d=False: a Qt window with just a layout (probably?) and a Notebook (probably?) are created directly by us instead of PyVista / ipyvtk.

So the only new code is really (2). At least conceptually this makes sense. There are probably bigger but still incremental refactorings we could do, too, but I'd probably save that for later. For one example, we could:

1. Refactor the 3D plotting stuff out of Renderer
2. Rename Renderer to a new name like Window, Display, or GUI or something that has no 3D connotation, to make clear it just means "a Qt window or Notebook ipywidgets/layout-capable thing"
3. Add a way to add a 3D renderer to such a GUI. I think it should be possible in principle with both ipyvtklink and pyvistaqt, but we'd have to check.

But for any refactoring this big or bigger, I think it's critical before deciding what to do or beginning work that we have some visual depiction (ASCII art, graphviz, powerpoint, whatever) of 1) what we have now, and 2) what the proposed change is. I think it will really help guide things here, and seems like necessary due diligence for any larger change.

[alexrockhill]

I think we're actually saying very close to the same thing, and I agree about talking about it more before writing code. Whether it's moving the 3D code out of Renderer or making a new class that handles just the window/scene or whatever you want to call it is a really important step but it's just semantics whether it's moving out of Renderer or making a new class, we're talking about the same thing.

I don't know about making new art or diagrams, I would just propose following more closely to what is done in Qt. I think I need to look into Renderer and the parts that are not 3D and implemented for the notebook backend to know what will work for notebook.

I'm not in favor of a use_3d that just seems like pushing the problem down the road. I don't really want to do a major refactor, I have better uses of my time, but it's possible that the refactor can be done in small steps and not be all that major. I agree with the second half of your last message, that seems like it should be the goal and is hopefully pretty reasonable.

[larsoner]

I don't know about making new art or diagrams

It's nothing fancy, probably 10-20 minutes of work (easy) once you understand the actual hierarchy (hard). But understanding the existing hierarchy (the hard part) I think is a prerequisite here...

Here are a couple of simple examples you could work from

1. ASCII, probably the best example because it actually shows the inheritance: https://github.com/pyvista/pyvistaqt/blob/9334f9d937792cfa1f692e936b35b89a46260851/pyvistaqt/plotting.py#L5-L31

2. ASCII:

   mne-python/mne/gui/_coreg_gui.py

   Lines 9 to 32 in 9335956

   	::
   	CoregFrame: GUI for head-MRI coregistration.
   	|-- CoregModel (model): Traits object for estimating the head mri transform.
   	| |-- MRIHeadWithFiducialsModel (mri) [1]: Represent an MRI head shape (high and low res) with fiducials.
   	| | |-- SurfaceSource (bem_high_res): High-res MRI head
   	| | |-- SurfaceSource (bem_low_res): Low-res MRI head
   	| | +-- MRISubjectSource (subject_source) [2]: Find subjects in SUBJECTS_DIR and select one.
   	| |-- FiducialsSource (fid): Expose points of a given fiducials fif file.
   	| +-- DigSource (hsp): Expose measurement information from a inst file.
   	|-- MlabSceneModel (scene) [3]: mayavi.core.ui.mayavi_scene
   	|-- DataPanel (data_panel)
   	| |-- HeadViewController (headview) [4]: Set head views for the given coordinate system.
   	| | +-- MlabSceneModel (scene) [3*]: ``HeadViewController(scene=CoregFrame.scene)``
   	| |-- SubjectSelectorPanel (subject_panel): Subject selector panel
   	| | +-- MRISubjectSource (model) [2*]: ``SubjectSelectorPanel(model=self.model.mri.subject_source)``
   	| +-- FiducialsPanel (fid_panel): Set fiducials on an MRI surface.
   	| |-- MRIHeadWithFiducialsModel (model) [1*]: ``FiducialsPanel(model=CoregFrame.model.mri, headview=CoregFrame.headview)``
   	| |-- HeadViewController (headview) [4*]: ``FiducialsPanel(model=CoregFrame.model.mri, headview=CoregFrame.headview)``
   	| +-- SurfaceObject (hsp_obj) [5*]: ``CoregFrame.fid_panel.hsp_obj = CoregFrame.mri_obj``
   	|-- CoregPanel (coreg_panel): Coregistration panel for Head<->MRI with scaling.
   	| +-- FittingOptionsPanel (fitting_options_panel): panel for fitting options.
   	|-- SurfaceObject (mri_obj) [5]: Represent a solid object in a mayavi scene.
   	+-- PointObject ({hsp, eeg, lpa, nasion, rpa, hsp_lpa, hsp_nasion, hsp_rpa} + _obj): Represent a group of individual points in a mayavi scene.

3. Or if you prefer being programmatic and letting dot do the laying out for you, you can do what our doc builds do: https://github.com/mne-tools/mne-python/blob/main/doc/sphinxext/flow_diagram.py#L25-L72

4. Open up powerpoint, make boxes and lines

I really think it would help both the implementation and review stages. (1) and (2) would even be super helpful for maintenance. Just think about how such a thing could have helped you when looking at the code the first time...

[alexrockhill]

::

   _AbstractGUI: The abstract class representing a Qt/notebook instance/window not necessarily 3D
   |-- Slice Browser: The window with panels that can be selected to change a slice and a 3D brain
   |    |--  ieeg_locate: The ieeg location GUI
   |    |   |--  Abstract bars, actions etc.
   |    |--  tfr_stc_viewer: The proposed TFR viewing GUI
   |-- coreg: The coregistration GUI
   |-- stc_viewer: The GUI that is initialized with `mne.viz.Brain.setup_time_viewer`

   _AbstractWindow: The abstract class representing a Qt/notebook instance/window not necessarily 3D
   |-- _AbstractPlayback: Abstraction of playback
   |-- _AbstractKeyPress: Keypress
   |-- _AbstractDialog: Dialog
   |-- _AbstractAction: Action
   |-- _AbstractLayout: Class for grid and ordered layouts
   |    |-- _AbstractDock: An abstract menu bar encompassing what is currently _AbstractToolBar, _AbstractMenuBar, _AbstractStatusBar (redundant)
   |-- _AbstractWidgetList: Controls list of widgets can have widgets added to
   |-- _AbstractWidget: the widgets
   |-- _AbstractMplInterface: mpl interface
   |-- _AbstractMplCanvas: canvas

Okay, I think that was pretty helpful, a lot of it is flat (not inherited) but it's nice to enumerate the functionality. I think the main thing is just making use of the _AbstractWindow that I didn't realize already exists and then having the layouts (toolbar, menu etc.) inherit from the _AbstractLayout. I think it's actually really simple, just 1) use the main window so that nothing has to get passed to the renderer and 2) manage the names of the menus within each gui specific to its use, that way the assembling of the components is done specifically for that GUI's use.

[alexrockhill]

Just a little to do note for fixing this issue, it's not going to be done today but I don't think it's going to take that long actually, mostly just bookkeeping.

Abstract classes that need to be fixed:

• _AbstractToolBar
• _AbstractDock
• _AbstractMenuBar
• _AbstractStatusBar
• _AbstractPlayback
• _AbstractKeyPress
• _AbstractDialog
• _AbstractLayout
• _AbstractWindow

Methods that already inherit so that they are not redundant and so are much easier to use 👍

• _AbstractAction
• _AbstractWidgetList
• _AbstractWidget