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

MRG: Update time slider label #7263

Merged
merged 4 commits into from
Jan 30, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 29, 2020

This PR updates the slider time label instead of the default time actor. It improves consistency so now the size of the time label scales with the window size.

Known issue

  • The time_actor doesn't come back when the interface is hidden

It's an item of #7162

@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #7263 into master will decrease coverage by <.01%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master    #7263      +/-   ##
==========================================
- Coverage   89.79%   89.79%   -0.01%     
==========================================
  Files         447      447              
  Lines       80506    80518      +12     
  Branches    12873    12878       +5     
==========================================
+ Hits        72289    72299      +10     
- Misses       5390     5391       +1     
- Partials     2827     2828       +1

@GuillaumeFavelier GuillaumeFavelier changed the title Update time slider label MRG: Update time slider label Jan 29, 2020
@agramfort
Copy link
Member

works great except that now if I hide the controls I've lost the time information. It should appear as before.

@GuillaumeFavelier
Copy link
Contributor Author

Good catch! I'll fix this one.

@GuillaumeFavelier GuillaumeFavelier self-assigned this Jan 29, 2020
@GuillaumeFavelier GuillaumeFavelier changed the title MRG: Update time slider label WIP: Update time slider label Jan 30, 2020
@GuillaumeFavelier
Copy link
Contributor Author

Actually @agramfort I see 2 ways to solve this issue:

  1. we bring back the standard time_actor but this one does not scale with the window size
  2. we hide all the slider parts except the title. This will scale with the window but the slider is still active meaning the user can still click on it in hidden state

@larsoner
Copy link
Member

We want a time label visible, it's useful.

Why not show the old actor when not in interactive mode, but hide it in interactive mode? It won't scale in non-interactive mode but I think that's okay. (Personally I think it would be better if none of the text actors scaled, it's how standard UI and plotters like mpl work.)

@GuillaumeFavelier
Copy link
Contributor Author

Going for option 1 then

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Update time slider label MRG: Update time slider label Jan 30, 2020
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.

works like a charm for me

@larsoner
Copy link
Member

Works for me as well, thanks @GuillaumeFavelier

@larsoner larsoner merged commit bd3d30c into mne-tools:master Jan 30, 2020
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Update time slider

* Remove unused variable

* Refactor set_time_point

* Bring back time_actor when the interface is hidden
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Update time slider

* Remove unused variable

* Refactor set_time_point

* Bring back time_actor when the interface is hidden
@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_time_actor branch June 11, 2020 09:41
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.

3 participants