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: Change _TimeViewer slider style #7509

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 25, 2020

It occurs to me that I never asked for feedbacks for the slider style, I just went with what I found practical but apparently the current design has some limitations. For example, since the slider tube is very thin it's easy to miss it and rotate the brain instead (reported in #7247 (comment) and offline by @hoechenberger). This PR tries to address that by changing the style of the slider (and by doing so, modify it's hitbox).

I suggest 2 styles of sliders. In both styles I removed the end cap. In style 1, I increased the tube width but the overall style stays similar. The style 2 is a bit extreme but fixes the original issue quite easily.

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.1
mne.viz.set_3d_backend('pyvista')
brain = stc.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                 clim=dict(kind='value', pos_lims=[3, 6, 9]),
                 hemi='lh',
                 )
master Style 1 Style 2
image image image

Except the size, it's also possible to change the colors (c.f. pyvista/pyvista#511):

image

Sadly, changing the shape or replacing the handle by a texture would require way more work on the VTK part.

It's an item of #7162

@codecov
Copy link

codecov bot commented Mar 25, 2020

Codecov Report

Merging #7509 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7509      +/-   ##
==========================================
- Coverage   90.18%   90.12%   -0.07%     
==========================================
  Files         453      453              
  Lines       81875    82703     +828     
  Branches    13056    13057       +1     
==========================================
+ Hits        73843    74537     +694     
- Misses       5203     5346     +143     
+ Partials     2829     2820       -9     

@GuillaumeFavelier
Copy link
Contributor Author

What do you think @agramfort, @hoechenberger, @larsoner, @drammock ?

@hoechenberger
Copy link
Member

+1 for style 2 with slightly less thick tube & slider. This then also mimics the sensor-level plots (e.g. Raw.plot()) and it's also the kind of slider used in all (?) modern operating systems / GUIs: tube has the same width as the slider.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Mar 25, 2020

Style 3 is just a rework of 2) but with the same size used in master.

master Style 1 Style 2 Style 3
image image image image

@GuillaumeFavelier
Copy link
Contributor Author

Also feel free to share if you have any suggestions for the colors.

@hoechenberger
Copy link
Member

Style 3 is just a rework of 2) but with the same size used in master.

That could work! What do the others think? Sure, I think it's more aesthetically pleasant to look at those sliders where the tube is narrower than the slider element, but I think this here is an important tradeoff to make between eye candy and usability.

Regarding the colors, can you adjust the tube color to be slightly less salient? Very-light-gray-ish or something?

@GuillaumeFavelier
Copy link
Contributor Author

Regarding the colors, can you adjust the tube color to be slightly less salient? Very-light-gray-ish or something?

Good idea!

@hoechenberger
Copy link
Member

hoechenberger commented Mar 25, 2020

It's not by any chance possibly to specify a color for the outline (border) of the tube, is it? Bc then you could make the tube black (as the background) and still indicate that this is a slider widget by having a fine outline drawn in a different color, inside which the slider can move.

@GuillaumeFavelier
Copy link
Contributor Author

AFAIK not just with sliders but maybe with vtkBorderWidgets...

Reference: https://vtk.org/Wiki/VTK/Examples/Cxx/Widgets/BorderWidget

@hoechenberger
Copy link
Member

AFAIK not just with sliders but maybe with vtkBorderWidgets...

Seems too complicated… let's see if a less prominent tube color does the trick for now :)

@GuillaumeFavelier
Copy link
Contributor Author

Trying with my firefox slider on KDE:

image

I obtain:

image

@hoechenberger
Copy link
Member

@GuillaumeFavelier looks good to me!!

@hoechenberger
Copy link
Member

hoechenberger commented Mar 25, 2020

@GuillaumeFavelier I just pulled and tried this out and I noticed another (minor) oddity:

When I click on a slider somewhere left or right of its horizontal center point and then start dragging, as soon as dragging begins, the slider jumps such that it's centered horizontally below the cursor.

@GuillaumeFavelier
Copy link
Contributor Author

If you click the tube it's normal that the slider jumps but when I drag, the slider moves only at the end of the interaction for me. I don't think I can reproduce this

@hoechenberger
Copy link
Member

@GuillaumeFavelier I've recorded a video:
https://drive.google.com/open?id=1EkPfPqRT47euD87_E8Idrr4P98PQX2hp

@agramfort
Copy link
Member

agramfort commented Mar 25, 2020 via email

@GuillaumeFavelier
Copy link
Contributor Author

I've recorded a video

Thank you, now I understand what you mean.

@larsoner
Copy link
Member

Looks great to me, I do like the slider filling up the entire height of the tube

@GuillaumeFavelier GuillaumeFavelier changed the title Change _TimeViewer slider style MRG: Change _TimeViewer slider style Mar 25, 2020
@GuillaumeFavelier
Copy link
Contributor Author

This is ready for merge then

@larsoner larsoner merged commit b3af2f6 into mne-tools:master Mar 25, 2020
@larsoner
Copy link
Member

Thanks @GuillaumeFavelier

@larsoner larsoner added this to the 0.20 milestone Mar 25, 2020
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.

4 participants