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.

Sign up for GitHub

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

Jump to bottom

BUG: Fix bug with themes on macOS #10500

Merged
merged 9 commits into from
Apr 10, 2022
Merged

BUG: Fix bug with themes on macOS #10500

merged 9 commits into from
Apr 10, 2022

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Apr 7, 2022 •
edited
Loading

@hoechenberger @mscheltienne . This code:

python -c "import mne; raw = mne.io.read_raw_fif(mne.datasets.sample.data_path() / 'MEG' / 'sample' / 'sample_audvis_raw.fif'); raw.plot(theme='auto', block=True)"

Gives

macOS mode theme='auto' theme='dark' theme='light'
dark Screen Shot 2022-04-08 at 8 43 28 AM Screen Shot 2022-04-08 at 8 44 35 AM Screen Shot 2022-04-08 at 8 45 24 AM
light Screen Shot 2022-04-08 at 8 46 07 AM Screen Shot 2022-04-08 at 8 46 33 AM Screen Shot 2022-04-08 at 8 46 59 AM

Fixes #10486
Fixes mne-tools/mne-qt-browser#100

The one real downside I think is that when the system is in light mode, dark mode uses the qdarkstyle blue-ish dark mode rather than the native macOS dark mode. I think we'd want a PR to qdarkstyle (or something) to try to fix that, as it's probably a large undertaking. In the meantime, this at least gets us closer to native styling on macOS and fixes some bugs.

Todo:

  • Add tests for bgcolors when theme is passed (or 'auto')

@larsoner larsoner added the backport-candidate on-merge: backport to maint/1.7 label Apr 7, 2022
@larsoner larsoner force-pushed the macos-theme branch 2 times, most recently from f841102 to dcf1b6f Compare April 7, 2022 18:53
@larsoner
Copy link
Member Author

larsoner commented Apr 7, 2022

Ready for review/merge from my end

agramfort
agramfort approved these changes Apr 8, 2022
View reviewed changes
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.

@hoechenberger merge if happy

@hoechenberger
Copy link
Member

Thank you, I will have a look this afternoon! In any case, thanks for putting in this effort, @larsoner!

@mscheltienne
Copy link
Member

mscheltienne commented Apr 8, 2022 •
edited
Loading

On a fresh install with bare minimum mne-python and mne-qt-browser dependencies (installed with python setup.py develop and nothing else), I am still getting a dark theme with theme='light' and I'm getting this warning as well:

To use light mode, "qdarkstyle" has to be installed! You can install it with pip install qdarkstyle

And indeed, after installing qdarkstyle I can now have the light mode with theme='light'. I'm a bit confused, isn't the light theme the default and the dark theme the extra that requires qdarkstyle? 😕

theme= main PR
'dark' dark after-dark
'light' light after-light

@larsoner
Copy link
Member Author

larsoner commented Apr 8, 2022

And indeed, after installing qdarkstyle I can now have the light mode with theme='light'. I'm a bit confused, isn't the light theme the default and the dark theme the extra that requires qdarkstyle? 😕

On macOS, when in system-dark mode you need qdarkstyle to have the light mode CSS for the browser. When in system-light mode, you need qdarkstyle to have the dark mode CSS for the browser.

@larsoner
Copy link
Member Author

larsoner commented Apr 8, 2022

(In other words, I think the behavior is expected and installing qdarkstyle was the right thing to do to fix it / the warning worked)

@mscheltienne
Copy link
Member

mscheltienne commented Apr 8, 2022 •
edited
Loading

That's unintuitive 😅
By the way, shouldn't qdarkstyle be a dependency of mne-qt-browser? Or at least in a requirement_extra.txt?
Same for psutil, I think I had a warning telling me to install psutil to retrieve available RAM at some point.

@larsoner
Copy link
Member Author

larsoner commented Apr 8, 2022

That's unintuitive 😅

Yes qdarkstyle is a bit of a misnomer

By the way, shouldn't qdarkstyle be a dependency of mne-qt-browser? Or at least in a requirement_extra.txt?
Same for psutil, I think I had a warning telling me to install psutil to retrieve available RAM at some point.

This could be useful, feel free to open an issue about this in the mne-qt-browser repo

@larsoner larsoner added backported and removed backport-candidate on-merge: backport to maint/1.7 labels Apr 8, 2022
@larsoner
Copy link
Member Author

larsoner commented Apr 8, 2022 •
edited
Loading

FYI it also looks like this problem is fixed on Qt>=5.13, so once we move to PySide6 (hopefully, as our default) the workarounds here to make the toolbar and status bar the correct color won't really be needed anymore. On this PR and mne-tools/mne-qt-browser#104 with PyQt6 (or even on mne-qt-browser main with PyQt5 5.13), I get what appears to be a nicely native dark UI when in dark mode, but our workarounds seem okay to me:

Qt >= 5.13 Qt <= 5.12
Screen Shot 2022-04-08 at 2 24 29 PM Screen Shot 2022-04-08 at 2 25 54 PM

@larsoner larsoner added backport-candidate on-merge: backport to maint/1.7 and removed backported labels Apr 8, 2022
@larsoner larsoner mentioned this pull request Apr 9, 2022
Merged
@larsoner
Copy link
Member Author

larsoner commented Apr 9, 2022

@hoechenberger any chance to get this merged? It's blocking #10510 / mne-tools/mne-qt-browser#104 etc.

This was referenced Apr 9, 2022
Closed
Merged
@larsoner larsoner mentioned this pull request Apr 10, 2022
Merged
@agramfort agramfort merged commit c276f61 into mne-tools:main Apr 10, 2022
@agramfort
Copy link
Member

thx @larsoner

@larsoner larsoner deleted the macos-theme branch April 10, 2022 22:03
larsoner added a commit that referenced this pull request Apr 10, 2022
@larsoner
BUG: Fix bug with themes on macOS (#10500)
7bd5db5 
* BUG: Fix bug with themes on macOS

* TST: Add test

* FIX: Simplify

* FIX: Fine

* FIX: Rerun

* FIX: Simpler

* FIX: Better separators

* FIX: LRU

* FIX: Only on < 5.13
@larsoner larsoner mentioned this pull request Apr 14, 2022
Closed
larsoner added a commit to wmvanvliet/mne-python that referenced this pull request Apr 19, 2022
@larsoner
Merge remote-tracking branch 'upstream/main' into multi-dipole-modeling
ae3f9f8 
* upstream/main: (40 commits)
  FIX: Flake (mne-tools#10540)
  FIX: Correct link (mne-tools#10536)
  DOC: Update installers (mne-tools#10535)
  ENH: Add dark mode to website (mne-tools#10523)
  WIP: Copy BEM surfaces by default (don't symlink) (mne-tools#10531)
  Avoid lowpass=0 in brainvision data (mne-tools#10517)
  DOC: Update installers [skip azp] [skip actions] (mne-tools#10528)
  FIX: Fix for old build (mne-tools#10527)
  Fix line noise at wrong frequencies (mne-tools#10525)
  FIX : read fids in eeglab (mne-tools#10521)
  MAINT: Prefer PySide6 in testing (mne-tools#10513)
  ENH: Add overview_mode support (mne-tools#10501)
  MRG: Updates for qtpy in mne-qt-browser (mne-tools#10509)
  BUG: Fix bug with themes on macOS (mne-tools#10500)
  MAINT: Bump installer links (mne-tools#10511)
  Add metadata to combine_channels (mne-tools#10504)
  MAINT: Standardize tests (mne-tools#10502)
  CI: Test circle (mne-tools#10506)
  ENH: Use HiDPI splash screen on HiDPI screens (mne-tools#10503)
  WIP,MNT: Add support for QtPy (mne-tools#10430)
  ...
@hoechenberger hoechenberger added backported and removed backport-candidate on-merge: backport to maint/1.7 labels May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort approved these changes

Assignees
No one assigned
Labels
backported
Projects
None yet
Milestone
No milestone
4 participants
@larsoner @hoechenberger @mscheltienne @agramfort