Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 22, 2021

This PR follows #8997 (comment) and brings support for docked traces:

This is how it looks by default for me on i3:

image

And the dock widgets can be detached/resized/reattached:

image

Locally, Brain testing works but I don't know how macOS CIs will react 🙏

Details
import os
import mne
from mne.datasets import sample

data_path = sample.data_path()
sample_dir = os.path.join(data_path, 'MEG', 'sample')
subjects_dir = os.path.join(data_path, 'subjects')
fname_stc = os.path.join(sample_dir, 'sample_audvis-meg')
stc = mne.read_source_estimate(fname_stc, subject='sample')
initial_time = 0.096
brain = stc.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                 clim=dict(kind='value', lims=[3, 6, 9]),
                 surface="white",
                 size=600,
                 background="white")

@GuillaumeFavelier GuillaumeFavelier self-assigned this Mar 22, 2021
@GuillaumeFavelier
Copy link
Contributor Author

Maybe show_traces="separate" is not necessary anymore and should be removed.

@GuillaumeFavelier
Copy link
Contributor Author

I think the result is better when the margins are disabled for the traces widget :)

margin=True margin=False
image image

@GuillaumeFavelier
Copy link
Contributor Author

It's ready for reviews @larsoner, @agramfort

@larsoner
Copy link
Member

larsoner commented Mar 22, 2021

From a quick test this is my experience / thoughts:

  1. This will be a great enhancement!

  2. The matplotlib widget has a minimum width that is too big -- I should be able to resize it smaller than this before having scroll bars appear at the bottom (it should resize the plots to fit):

    Peek.2021-03-22.09-35.mp4
  3. The resize-boundary-grabber (what you can click on to actually resize the widget) once it has been removed appears to be 1 pixel. Maybe this is related to the removal of some border or padding (?) but it makes it very difficult to resize

  4. Removing the control and matplotlib widgets is easy because they have titles. The "brain plots widget" should also have a title so it can just as easily be moved

@GuillaumeFavelier
Copy link
Contributor Author

2) should be fixed after aa23887, I modified _window_ensure_minimum_sizes so that the minimum size for the mplcanvas is more flexible. I did not manage to make it work without the margins but maybe it will help with 3) in the end.

About 4), I still have to find a way to do that :)

Locally, the notebook test was failing so I fixed it in here. I don't know why it was not detected.

E   ~/source/mne-python/mne/viz/_brain/_brain.py in _configure_tool_bar(self)
E      1232     def _configure_tool_bar(self):
E      1233         self._renderer._tool_bar_load_icons()
E   -> 1234         self._renderer._tool_bar_set_theme(self.theme)
E      1235         self._renderer._tool_bar_initialize()
E      1236         self._renderer._tool_bar_add_screenshot_button(
E   
E   AttributeError: '_Renderer' object has no attribute '_tool_bar_set_theme'

@larsoner
Copy link
Member

About 4), I still have to find a way to do that :)

Could you embed the plotter in a group box, and make that group box what's movable?

@larsoner
Copy link
Member

I did not manage to make it work without the margins but maybe it will help with 3) in the end.

Still 1 pixel for me, can you replicate? If so, could you try the margin stuff suggested here:

https://stackoverflow.com/questions/60279502/floating-qdockwidget-resize-change-size-of-the-resize-handler-grip-on-the-bord

If not, I can work on it, since it might actually be the fault of my Ubuntu theme:

https://forum.qt.io/topic/60122/increase-floating-qdockwidget-resize-margin-size

@GuillaumeFavelier
Copy link
Contributor Author

Could you embed the plotter in a group box

I know how to do this

make that group box what's movable?

But not that. I will make some research 😅

Still 1 pixel for me, can you replicate?

With i3, I resize with the keyboard. I should switch to my KDE to replicate 👍

@larsoner
Copy link
Member

But not that. I will make some research 

I'd assume that however you're currently making the matplotlib window movable, you "just" need to do that same thing with a new groupbox widget, then make the plotter a child of that group box. Hopefully it's not more complicated than this, but if it is, I apologize :) If it ends up being really complicated maybe it's not worth pursuing.

@GuillaumeFavelier
Copy link
Contributor Author

however you're currently making the matplotlib window movable

Ah this came basically for free with QDockWidget, it's part of the default features:

https://doc.qt.io/qt-5/qdockwidget.html#DockWidgetFeature-enum

@larsoner
Copy link
Member

So add a new GroupBox to that widget, put the plotter in that group box, and maybe it works?

@hoechenberger
Copy link
Member

And the dock widgets can be detached/resized/reattached:

Haven't tried this out yet, but hopefully one cannot "accidentally" detach the docks? Because that's something I hate :D These multi-window designs are … idk, is anyone even still using that? Not even GIMP comes with that stuff anymore by default! 😅

@larsoner
Copy link
Member

From trying it out, I find it works pretty well. You can move/detach/reattach easily.

@GuillaumeFavelier GuillaumeFavelier changed the title ENH: Traces dock widget WIP,ENH: Traces dock widget Mar 24, 2021
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Mar 24, 2021

I tried in 8d836cd but it was some sort of frankenstein, it's way too hacky and not stable in my opinion 😅

output.mp4

Also, because there is no real central widget but a "fake" one (hidden empty QLabel), the dock widgets can only be aligned vertically or horizontally (not both at the same time).

@larsoner
Copy link
Member

Those interactions seem okay. Why not make the QDockWidget the central widget?

@larsoner
Copy link
Member

Ohhhhh I see. You need a central widget and things go around it. I can live with the brain plots being this central widget :)

@larsoner
Copy link
Member

In that case we should be good to go here, then?

@GuillaumeFavelier
Copy link
Contributor Author

Ready on my end. Do you want to take care of 3) in a different PR maybe?

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provided CIs are green

@larsoner
Copy link
Member

Fixed the grip issue

@GuillaumeFavelier GuillaumeFavelier changed the title WIP,ENH: Traces dock widget ENH: Traces dock widget Mar 24, 2021
@larsoner larsoner merged commit 2c888c5 into mne-tools:main Mar 24, 2021
@larsoner
Copy link
Member

Thanks @GuillaumeFavelier !

@GuillaumeFavelier GuillaumeFavelier deleted the enh/dock_traces branch March 24, 2021 15:47
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 this pull request may close these issues.

4 participants