Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 16, 2020

Thanks to #7589 we can now drastically simplify the way we manage the shortcuts in _TimeViewer. This PR also improves stability because we don't rely on the keyPressEvent callback anymore and flexibility because we use QAction instead of a dict for the bindings. It's also easier to maintain because the shortcuts are set in one place.

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Works like a charm for me!

@GuillaumeFavelier
Copy link
Contributor Author

I have to update _LinkViewer as well.

@GuillaumeFavelier GuillaumeFavelier changed the title Refactor _TimeViewer shortcuts WIP: Refactor _TimeViewer shortcuts Apr 16, 2020
@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #7610 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7610      +/-   ##
==========================================
+ Coverage   90.03%   90.12%   +0.09%     
==========================================
  Files         454      454              
  Lines       83400    83401       +1     
  Branches    13210    13209       -1     
==========================================
+ Hits        75093    75169      +76     
+ Misses       5427     5373      -54     
+ Partials     2880     2859      -21     

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Refactor _TimeViewer shortcuts MRG: Refactor _TimeViewer shortcuts Apr 16, 2020
@hoechenberger hoechenberger merged commit 18d2cee into mne-tools:master Apr 16, 2020
@hoechenberger
Copy link
Member

Thanks @GuillaumeFavelier

@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_shortcut branch June 11, 2020 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants