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

WIP: Time viewer tool bar with SVG icons #7589

Merged
merged 14 commits into from
Apr 14, 2020

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 9, 2020

This PR, following a discussion with @hoechenberger and strongly inspired by cbrnr/mnelab#61, adds a tool bar with useful bindings to the features of _TimeViewer:

output

It's also an item of #7162 since it adds buttons to the interface.


This is not compatible with #7592

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 9, 2020

I considered removing the main menu but it's still useful when time_viewer=False for taking screenshot

Latest version:

image

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Apr 9, 2020
86 tasks
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 9, 2020

I also thought about a button like the following:

image

that would display additionnal informations. It could be used to clarify the auto-scaling feature or time index @larsoner (relevant for #7247 (comment))

@larsoner
Copy link
Member

larsoner commented Apr 9, 2020

@GuillaumeFavelier any way we can use unicode for the icons rather than bundling new binary images?

@hoechenberger
Copy link
Member

any way we can use unicode for the icons rather than bundling new binary images?

I don't think we'd find anything even closely as good-and-balanced-looking, tbh… also if you use a font, the icons could end up looking different on every system! -1 on unicode icons from my side for now :)

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

@larsoner What could work, potentially, are FontAwesome fonts, but personally I'm in favor of bundling SVG icons

@GuillaumeFavelier
Copy link
Contributor Author

any way we can use unicode for the icons rather than bundling new binary images?

It should be possible but it may have a totally different result on old version of PyQt5. It may require some particular processing like we did in #7257 (comment)

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

@cbrnr Since you've probably been through this decision process personally with MNELAB, what's your stance on this?

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

@GuillaumeFavelier Just tested on macOS and looks and works beautifully. I really like it! Also the availability of buttons now could compensate for some of the shortcomings of PyVista. For example, I don't think we'd need to hold on to the orientation slider, but we could add toolbar buttons for that instead (like in FreeView).

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 9, 2020

rather than bundling new binary images

Sorry, we can exclude the images and mne.qrc from MANIFEST.in since they're already built in resources.py

@cbrnr
Copy link
Contributor

cbrnr commented Apr 9, 2020

I'd go with SVG icons, I didn't even consider using unicode symbols. Just make sure to set app.setAttribute(Qt.AA_UseHighDpiPixmaps) (https://github.com/cbrnr/mnelab/pull/102/files).

@GuillaumeFavelier GuillaumeFavelier changed the title Time viewer tool bar Time viewer tool bar with SVG icons Apr 9, 2020
@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 9, 2020

Just to be clear, to make it work with PyQt, only the built resources.py is necessary. So if the issue is about storing the images, they can be hosted in another repo. and reused only when the interface changes to update resources.py.

@GuillaumeFavelier
Copy link
Contributor Author

Closing in favour of #7592 to move forward. At least a prototype is available here if needed.

@GuillaumeFavelier GuillaumeFavelier changed the title Time viewer tool bar with SVG icons WIP: Time viewer tool bar with SVG icons Apr 10, 2020
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.

LGTM once CI goes green. Thanks for your work, @GuillaumeFavelier

MANIFEST.in Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #7589 into master will increase coverage by 0.07%.
The diff coverage is 90.74%.

@@            Coverage Diff             @@
##           master    #7589      +/-   ##
==========================================
+ Coverage   90.05%   90.12%   +0.07%     
==========================================
  Files         452      454       +2     
  Lines       83035    83387     +352     
  Branches    13127    13208      +81     
==========================================
+ Hits        74780    75156     +376     
+ Misses       5403     5375      -28     
- Partials     2852     2856       +4     

@larsoner larsoner merged commit 550dd1b into mne-tools:master Apr 14, 2020
@larsoner
Copy link
Member

Works great, thanks @GuillaumeFavelier

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