Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

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 #7589

@agramfort
Copy link
Member

agramfort commented Apr 9, 2020 via email

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Apr 9, 2020
86 tasks
@agramfort
Copy link
Member

works like a charm for me!

@hoechenberger
Copy link
Member

(I honestly think this looks so much worse than the SVG icons you had in the other PR… Looks like a proof of concept in comparison, but not like a refined UI. just my 2 ct)

@hoechenberger
Copy link
Member

@agramfort @larsoner Why is it such a big deal to ship with a few tiny SVGs? Honest question :)

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

So, regarding which toolbar looks better, this one or the SVG one – maybe it is just a matter of taste.

But if the reasoning behind using an (imho) inferior UI design is technical limitations (e.g. keeping the sdist slightly smaller or sth), I really believe this decision should be reconsidered. TimeViewer is quickly gaining in features and is already one of the BIG selling points of our 0.20 release. It will only continue to get even more useful and at one point will be the "face" of MNE when it comes to source analysis. I think it's important that this face is looking as great as possible: a beautiful app creates a better UX and higher user affinity! UI eye candy is really important to many users (and me personally – hey, I'm using a Mac for a reason ;)) Just my… USD 2.00, this time, I suppose ;)

@agramfort
Copy link
Member

agramfort commented Apr 9, 2020 via email

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #7592 into master will increase coverage by 0.08%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##           master    #7592      +/-   ##
==========================================
+ Coverage   90.05%   90.14%   +0.08%     
==========================================
  Files         452      452              
  Lines       83035    83065      +30     
  Branches    13127    13129       +2     
==========================================
+ Hits        74780    74880     +100     
+ Misses       5403     5353      -50     
+ Partials     2852     2832      -20     

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

It really looks like a KDE or Gnome app from 20 years ago on my computer. It's hideous. I don't like it. 😭

Screenshot 2020-04-09 at 18 57 17

@drammock
Copy link
Member

drammock commented Apr 9, 2020

Maybe I missed it, but why is FontAwesome not an option? If distribution size is the only concern, it is possible to create a derivative font with only the icons we need (e.g., online tool fontello does this).

@hoechenberger
Copy link
Member

hoechenberger commented Apr 9, 2020

but why is FontAwesome not an option?

I proposed FA somewhere but I think it hasn't been considered / discussed yet :) Thanks for bumping this, this could really be a middle ground? :)

@larsoner
Copy link
Member

larsoner commented Apr 9, 2020

How big was the size diff, looks like it has been force-pushed over?

+1 for the trim-font idea from @drammock. It will keep pip install small. As we add icons it will hurt our repo size diff, but seeing as the entire fontawesome in our doc/_static/fonts is 165 kB and we will use a tiny fraction, I think this will be negligible.

@hoechenberger
Copy link
Member

Just for a "newcomer", why is the repo & sdist size so crucial? Users download the sdist… how many times a year? And I clone the full repo only when I move to a new computer. Why is size so important in 2020, where we have TB hard drives and fast internet connections?

@cbrnr
Copy link
Contributor

cbrnr commented Apr 9, 2020

I also think that file sizes in the kB range shouldn't matter in 2020. Moreover, SVG icons are much smaller than a font; MNELAB's icons are between 186 bytes and 2 kB. In addition, SVG icons are text files and thus better suited for version control than binary (font) files (although I doubt that we will ever change SVG icons).

@drammock
Copy link
Member

drammock commented Apr 9, 2020

Screenshot_2020-04-09_10-46-47

Are people happy with these? I included 2 options for "help", 2 options for "trash", and 3 options for "scale" (personally I like the sliders best for that one)

@GuillaumeFavelier
Copy link
Contributor Author

Your idea looks very promising @drammock and I really want to see how it looks but I do not know how to integrate it. Do you create a new font that should be loaded by PyQt?

@drammock
Copy link
Member

drammock commented Apr 9, 2020

@GuillaumeFavelier you can take this JSON config file and load it into the Fontello website, to pick up where I left off. It will provide fonts in TTF, WOFF, WOFF2, EOT, and SVG format. I'm assuming PyQt can handle at least one of those.

@agramfort
Copy link
Member

agramfort commented Apr 9, 2020 via email

@hoechenberger
Copy link
Member

i now create more work. I will just say "be pragmatic" and keep in mind that timing is not negligible

I see what you mean, and sorry if I've been utilizing everybody's time here a little too much too :\ But I think that UI design is extemely crucial to make users happy. No-one enjoys using ugly or badly designed applications. I mean, there's a reason why big companies like Apple conduct extensive studies on their new products to finetune the UI and ensure good UX. Therefore I, personally, think that any time to discuss UI eye candy is time very well spent, if the goal is to increase the MNE adoption rate :)

@cbrnr
Copy link
Contributor

cbrnr commented Apr 9, 2020

I 100% agree with @hoechenberger. In my limited Qt experience, (SVG) icon files are the standard way to create icons. I've never seen someone use characters from special fonts. I'd just use the SVGs from the previous issue, they look good and weigh only a few kB.

@cbrnr
Copy link
Contributor

cbrnr commented Apr 9, 2020

Also, I'd try to make sure that the app looks as native as possible. This means that the menu bar should not be attached to the window, but should go in the global menu bar at the top of the screen.

@agramfort
Copy link
Member

agramfort commented Apr 9, 2020 via email

@GuillaumeFavelier GuillaumeFavelier changed the title Add basic tool bar with unicode symbols WIP: Add basic tool bar with unicode symbols Apr 10, 2020
@agramfort
Copy link
Member

I would propose to merge this for now and improve later. Any objection?

@hoechenberger
Copy link
Member

hoechenberger commented Apr 13, 2020

Current status:

  • Alex has "retracted" his vote
  • Clemens is in favor of SVG icons, based on looks and his experience with Qt (and the use by other apps)
  • Daniel suggested to look into FontAwesome symbols, however this would require further work from Guillaume's side
  • I'm in strong favor of the SVG approach in WIP: Time viewer tool bar with SVG icons #7589 too
  • So far I'm not aware of any strong technical implications "forcing" us to save on a few KB required for bundling SVGs, but correct me if I'm mistaken here

I would, therefore, like to suggest to close this one in favor of a re-opened (and merged :)) #7589 :) The work has already been done there, and we could move forward with a solution to which, to my understanding, we all could agree on (or at least feel "neutral" towards)

Edit
Oh btw I conducted a tiny user "study" by sending screenshots of the TimeViewer with both the Unicode and the SVG toolbar to two of my friends, providing no further info and just asking, just from looking at it, which one they would rather use and why. They both preferred the SVG toolbar, although one stated "I'm not a fan of flat, monochromatic icons personally." Yet they both mentioned that the Unicode toolbar looks kind of "dated" and nothing they would expect in a modern application. Now this is obviously an extremely small and biased sample, but I wanted to share this with you anyway ;)

@cbrnr
Copy link
Contributor

cbrnr commented Apr 13, 2020

I agree with @hoechenberger. I also want to add that if we have to bundle a new font this will take much more space than all SVG icons we're ever going to need. Still in the kB range though so not even important.

@agramfort
Copy link
Member

agramfort commented Apr 13, 2020 via email

@hoechenberger
Copy link
Member

+100 on moving ahead! @GuillaumeFavelier the stage is yours. 👌

@GuillaumeFavelier
Copy link
Contributor Author

Let's continue the effort on #7589 then

@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_tool_bar_unicode branch June 11, 2020 10:00
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.

6 participants