WIP: Add help window to the _TimeViewer #7305

GuillaumeFavelier · 2020-02-10T15:34:08Z

This PR adds a matplotlib figure to display help. The shortcut is h:

Refactor existing code in mne/viz/utils.py (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))
Find satisfying geometry/size (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))
Use tuple instead of dict to respect order (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))
Use ? as shortcut to show the help window (suggested in MRG: Enable point picking in _TimeViewer #7247 (comment))
Use a Help menu instead of a label (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))
Update the shorcuts (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))
Increase line spacing (suggested in WIP: Add help window to the _TimeViewer #7305 (comment))

Closes #7268
It's an item of #7162

agramfort · 2020-02-10T15:47:21Z

can you avoid so much blank space ?

larsoner · 2020-02-10T15:51:14Z

mne/viz/_brain/_timeviewer.py

+                cell._loc = 'right'
+
+        fig_help.canvas.draw()
+        plt_show(fig=fig_help, warn=False)


Please refactor this code with the other help windows in mne/viz. They should all use a single call, maybe something like:

def _show_help(options): ...

where options is a list (rows) of 2-element lists (cols)? This would be better than a dict because you make it so that some of the "rows" are actually blank, or hline, etc. This function would take that input and pop up a window showing the table, this window should be close-able with hitting esc, etc.

hoechenberger · 2020-02-10T16:19:26Z

Thanks @GuillaumeFavelier, this will be really useful 🥳

codecov · 2020-02-11T12:55:01Z

Codecov Report

Merging #7305 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7305      +/-   ##
==========================================
- Coverage   89.83%   89.81%   -0.03%     
==========================================
  Files         449      449              
  Lines       80791    80801      +10     
  Branches    12880    12881       +1     
==========================================
- Hits        72581    72571      -10     
- Misses       5384     5399      +15     
- Partials     2826     2831       +5

larsoner · 2020-02-11T14:21:27Z

mne/viz/_brain/_timeviewer.py

+            'u': 'Restore original clim',
+            'Space': 'Start/Pause playback'
+        }
+        text = [str(s) + " : \n" for s in shortcuts.keys()]


Dict order is only guaranteed in Python 3.6+, we still support 3.5. You can probably just turn this into a tuple, then do

pairs = [ ('h', 'Display help window'), ... ] text1, text2 = zip(*pairs) text1 = '\n'.join(text1) text2 = '\n'.join(text2)

GuillaumeFavelier · 2020-02-11T14:55:15Z

We might need to use Qt directly to bind the help window to ?, I could not find the ascii code for ?.

References:
https://github.com/Kitware/VTK/blob/master/GUISupport/Qt/QVTKInteractorAdapter.cxx#L577-L671
https://gitlab.kitware.com/vtk/vtk/blob/741fffbf6490c34228dfe437f330d854b2494adc/Wrapping/Python/vtkmodules/qt/QVTKRenderWindowInteractor.py#L432-446

GuillaumeFavelier · 2020-02-11T15:05:24Z

This change will affect all the current bindings.

hoechenberger · 2020-02-11T15:29:27Z

could not find the ascii code for ?.

Isn't it 63?

GuillaumeFavelier · 2020-02-11T15:31:32Z

My bad. I mean the ascii code returned by qt_key_to_key_sym()

I don't see it in the following list: https://github.com/Kitware/VTK/blob/master/GUISupport/Qt/QVTKInteractorAdapter.cxx#L577-L671

GuillaumeFavelier · 2020-02-12T13:42:33Z

It might be a good idea to test this new key binding system on different configurations actually (especially ?). It works on my linux/azerty keyboards. Is it working for you @hoechenberger, @drammock, @SophieHerbst ?

hoechenberger · 2020-02-12T13:59:37Z

It might be a good idea to test this new key binding system on different configurations actually (especially ?). It works on my linux/azerty keyboards. Is it working for you @hoechenberger, @drammock, @SophieHerbst ?

Not working for me on macOS (on an American keyboard, I tried pressing / and Shift + / (which produces a ?)), nothing happens. Other keyboard actions like y are working fine, however.

Had to install pyvista master to try this.

This reverts commit f7339c5.

GuillaumeFavelier · 2020-02-12T14:04:38Z

Using QtCore.Qt.Key_Question is my last resort now. Hopefully this one works for everyone.

hoechenberger · 2020-02-12T14:10:05Z

Using QtCore.Qt.Key_Question is my last resort now. Hopefully this one works.

This doesn't fix it for me, sorry :(

GuillaumeFavelier · 2020-02-12T14:20:34Z

Thanks a lot for your help @hoechenberger ! Apparently there is also Key_questiondown too. If this one does not work, I'll look into matplotlib code.

larsoner · 2020-02-12T14:38:48Z

mne/viz/_brain/_timeviewer.py

+
+    def keyPressEvent(self, event):
+        from PyQt5 import QtCore
+        if event.key() == QtCore.Qt.Key_questiondown:


This works for me on both macOS and Linux

(BTW if I do print(event.key()) and print(type(event.key()) on macOS I get 63 and int, so it's possible that using the ASCII char code would have also be enough :) )

But what's confusing to me is that QtCore.Qt.Key_questiondown == 191

Oh actually it's not being routed through this part of the conditional at all, it's going through the callback=self.key_mindings.get(event.text())

@hoechenberger tried both event.text() and Key_Question without success.

GuillaumeFavelier · 2020-02-12T14:51:24Z

I tested it on my windows/azerty config and it works too

GuillaumeFavelier · 2020-02-13T13:09:26Z

Trying to catch up here.

Okay let's get rid of the unnecessary conditional branch then

I just removed it now, does it still work for you @larsoner ?

So thanks to:

jupyter qtconsole

I believe I can reproduce the behaviour you have, thank you @hoechenberger

So when I add %matplotlib qt in the console, it works for me. What about you @SophieHerbst, @hoechenberger ?

I know this is a workaround but jupyter is not the first target here.

hoechenberger · 2020-02-13T13:16:10Z

So when I add %matplotlib qt in the console, it works for me. What about you @SophieHerbst, @hoechenberger ?

Working in qtconsole and VS Code interactive Python window! Only annoying thing is the window opens in the background, not sure if that can be fixed? Btw menus are working properly this way too

GuillaumeFavelier · 2020-02-13T13:18:32Z

Thank you for the fast response,

the window opens in the background, not sure it that can be fixed

This is a bug, I will report it on #7162

larsoner · 2020-02-13T14:34:24Z

mne/viz/_brain/_timeviewer.py

+        # display help
+        self.help_actor = self.plotter.add_text(
+            text="Press ? to show help window"
+        )


I would rather not sacrifice this much (or really any) screen real estate for this:

If it's always ? in all of our plotters we can just document it in a tutorial and in docstrings.

... actually the intuitive way to do it without eating up screen real estate would be to add a Help->Show MNE key bindings... menu option at the top. If it's not too hard to do it it's a nice option

Or more completely / standardly, Help-> Show MNE key bindings... ? (with the ? in the right column of the menu item to indicate it's the shortcut)

Thanks for the suggestion @larsoner. I'm on it

Just need to find this code snippet I did not so long ago...

Found it: #7200 (comment)

GuillaumeFavelier · 2020-02-13T15:03:12Z

This is how it looks for me:

larsoner

LGTM +1 for merge

hoechenberger · 2020-02-13T15:33:11Z

Still wondering if a hint in very small font size, "[?] Show key bindings", wouldn't be a good idea.

SophieHerbst · 2020-02-13T15:50:25Z

Indeed, after doing %matplotlib qt the ? works for me (spyder on Mac).

So when I add %matplotlib qt in the console, it works for me. What about you @SophieHerbst, @hoechenberger ?

Working in qtconsole and VS Code interactive Python window! Only annoying thing is the window opens in the background, not sure if that can be fixed? Btw menus are working properly this way too

'press ? for help' (I don't think the term 'key bindings' is very well understood by non-coding experts)

Still wondering if a hint in very small font size, "[?] Show key bindings", wouldn't be a good idea.

larsoner · 2020-02-13T16:01:17Z

I don't think it's worth wasting the screen real estate. Putting a "press this for help" in the "working area" is highly abnormal. Putting help in a help menu at the top is about as standard as you can get.

GuillaumeFavelier · 2020-02-13T16:04:08Z

Maybe a compromise would be to display the Press ? for help only for a few seconds when you start?
It's not necessary to display this message forever, it's just informative/a reminder.

GuillaumeFavelier · 2020-02-13T16:09:31Z

Just throwing ideas. This is something we can always improve in another PR anyway, I believe it works as is.

agramfort · 2020-02-13T16:25:01Z

works great. Can I just suggest some better shortcuts:

Toggle interface -> i
Scale -> s
Restore clim -> r

?

GuillaumeFavelier · 2020-02-13T16:45:34Z

Can I just suggest some better shortcuts:

Totally. The diff is really small.
See if you like it now @agramfort

hoechenberger · 2020-02-13T17:00:07Z

@GuillaumeFavelier Minor glitch but the default help window height is a little too small on my computer, causing visually unpleasant line spacing

If I make the window higher, it looks much better. The white space on the left is also a little irritating

agramfort · 2020-02-13T17:09:08Z

I would increase line spacing if possible but otherwise LGTM !

…

hoechenberger · 2020-02-13T17:11:29Z

(btw is there any documentation on auto-scaling somewhere? what does it do, exactly?)

drammock · 2020-02-13T19:15:34Z

Maybe a compromise would be to display the Press ? for help only for a few seconds when you start?

I'd say for now let's go with the menu. If / when PyVista adds new UI elements and some of the sliders get converted to buttons or dropdown boxes, we can revisit at that point whether there's enough screen space for "press ? for help".

GuillaumeFavelier · 2020-02-13T19:43:49Z

If / when PyVista adds new UI elements and some of the sliders get converted to buttons or dropdown boxes, we can revisit at that point whether there's enough screen space for "press ? for help".

Fair enough.

btw is there any documentation on auto-scaling somewhere? what does it do, exactly?

No documentation so far. A good start is #7153 (comment) and #7218

larsoner · 2020-02-13T19:45:11Z

Thanks @GuillaumeFavelier !

GuillaumeFavelier · 2020-02-13T19:45:32Z

Whoops. I changed the title to WIP my bad. Very bad timing I suppose.

larsoner · 2020-02-13T19:46:37Z

Sorry, missed that change. Just tack whatever you were going to do onto #7247, which we should also merge ASAP

hoechenberger · 2020-02-13T20:06:57Z

@GuillaumeFavelier awesome work!!! 🥳

* Add help window * Update window size and refactor _show_help() * Use tuple instead of dict * Refactor key bindings * Refactor keyPressEvent * Revert "Refactor keyPressEvent" This reverts commit f7339c5. * TST: try Key_questiondown * Refactor keyPressEvent * Use help menu instead of help label * Update shortcuts

Add help window

82e7bd6

GuillaumeFavelier added the VIZ label Feb 10, 2020

GuillaumeFavelier self-assigned this Feb 10, 2020

GuillaumeFavelier mentioned this pull request Feb 10, 2020

ENH: New interface for Brain #7162

Closed

86 tasks

larsoner reviewed Feb 10, 2020

View reviewed changes

GuillaumeFavelier changed the title ~~Add help window to the _TimeViewer~~ WIP: Add help window to the _TimeViewer Feb 10, 2020

Merge branch 'master' into time_viewer_help_window

76f14e0

Update window size and refactor _show_help()

711d75c

GuillaumeFavelier mentioned this pull request Feb 11, 2020

MRG: Enable point picking in _TimeViewer #7247

Merged

larsoner reviewed Feb 11, 2020

View reviewed changes

Use tuple instead of dict

c81964b

GuillaumeFavelier added 3 commits February 12, 2020 13:52

Refactor key bindings

b1e4db1

Refactor keyPressEvent

f7339c5

Merge branch 'master' into time_viewer_help_window

ad1299c

Revert "Refactor keyPressEvent"

f8e472f

This reverts commit f7339c5.

TST: try Key_questiondown

440d92a

larsoner reviewed Feb 12, 2020

View reviewed changes

larsoner reviewed Feb 13, 2020

View reviewed changes

Use help menu instead of help label

1cbcb44

larsoner approved these changes Feb 13, 2020

View reviewed changes

GuillaumeFavelier changed the title ~~WIP: Add help window to the _TimeViewer~~ MRG: Add help window to the _TimeViewer Feb 13, 2020

Update shortcuts

717ec4c

GuillaumeFavelier changed the title ~~MRG: Add help window to the _TimeViewer~~ WIP: Add help window to the _TimeViewer Feb 13, 2020

larsoner merged commit 18259f5 into mne-tools:master Feb 13, 2020

GuillaumeFavelier deleted the time_viewer_help_window branch June 11, 2020 09:49

WIP: Add help window to the _TimeViewer #7305

WIP: Add help window to the _TimeViewer #7305

Conversation

GuillaumeFavelier commented Feb 10, 2020 • edited Loading

agramfort commented Feb 10, 2020

Choose a reason for hiding this comment

hoechenberger commented Feb 10, 2020

codecov bot commented Feb 11, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

GuillaumeFavelier commented Feb 11, 2020

GuillaumeFavelier commented Feb 11, 2020

hoechenberger commented Feb 11, 2020

GuillaumeFavelier commented Feb 11, 2020 • edited Loading

GuillaumeFavelier commented Feb 12, 2020

hoechenberger commented Feb 12, 2020 • edited Loading

GuillaumeFavelier commented Feb 12, 2020 • edited Loading

hoechenberger commented Feb 12, 2020

GuillaumeFavelier commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeFavelier commented Feb 12, 2020

GuillaumeFavelier commented Feb 13, 2020

hoechenberger commented Feb 13, 2020 • edited Loading

GuillaumeFavelier commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeFavelier commented Feb 13, 2020

larsoner left a comment

Choose a reason for hiding this comment

hoechenberger commented Feb 13, 2020

SophieHerbst commented Feb 13, 2020

larsoner commented Feb 13, 2020

GuillaumeFavelier commented Feb 13, 2020 • edited Loading

GuillaumeFavelier commented Feb 13, 2020 • edited Loading

agramfort commented Feb 13, 2020

GuillaumeFavelier commented Feb 13, 2020

hoechenberger commented Feb 13, 2020 • edited Loading

agramfort commented Feb 13, 2020 via email

hoechenberger commented Feb 13, 2020

drammock commented Feb 13, 2020

GuillaumeFavelier commented Feb 13, 2020

larsoner commented Feb 13, 2020

GuillaumeFavelier commented Feb 13, 2020 • edited Loading

larsoner commented Feb 13, 2020

hoechenberger commented Feb 13, 2020

GuillaumeFavelier commented Feb 10, 2020 •

edited

Loading

codecov bot commented Feb 11, 2020 •

edited

Loading

GuillaumeFavelier commented Feb 11, 2020 •

edited

Loading

hoechenberger commented Feb 12, 2020 •

edited

Loading

GuillaumeFavelier commented Feb 12, 2020 •

edited

Loading

hoechenberger commented Feb 13, 2020 •

edited

Loading

GuillaumeFavelier commented Feb 13, 2020 •

edited

Loading

GuillaumeFavelier commented Feb 13, 2020 •

edited

Loading

hoechenberger commented Feb 13, 2020 •

edited

Loading

GuillaumeFavelier commented Feb 13, 2020 •

edited

Loading