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,ENH: Load Qt stylesheet #9126

Closed

Conversation

GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Mar 17, 2021

This PR follows the discussion in #9000 (comment) and comes after the discussion with @marsipu. The goal is to load a stylesheet dynamically.

This is still work in progress. For now, it just loads a data.txt test file:

QLabel {
	color: green
}

For short, it gives full control over the style of all the PyQt5 instances of _PyVistaRenderer.

image

ToDo:

  • The icon color should change with the theme

@GuillaumeFavelier
Copy link
Contributor Author

You may interested @larsoner and @hoechenberger

@larsoner
Copy link
Member

we should vendor https://github.com/ColinDuquesnoy/QDarkStyleSheet. This coupled with vendoring / adapting https://github.com/albertosottile/darkdetect is where I would start

@GuillaumeFavelier
Copy link
Contributor Author

This is what I get with qdarkstyle on plot_visualize_stc:

image

I think it's a good start and it is really easy to add but I can't say I'm satisfied with the default config. What do you think?

Because the QApplication is modified, there side effect on matplotlib plots:

image

@larsoner
Copy link
Member

Instead of using qdarkstyle directly, you could copy/vendor their .css and apply it selectively rather than at the QApplication level

@GuillaumeFavelier
Copy link
Contributor Author

apply it selectively rather than at the QApplication level

Using it on the QMainWindow directly avoids the effects on matplotlib plots, thank you 👍

you could copy/vendor their .css

Hm... It seems to be built at runtime:

https://github.com/ColinDuquesnoy/QDarkStyleSheet/blob/master/qdarkstyle/__init__.py#L241-L254

@larsoner
Copy link
Member

Ahh. Maybe it's worth having an optional dependency in this case. We could do the same thing with darkdetect I think -- use it if it's available, and if not, don't.

@hoechenberger
Copy link
Member

@GuillaumeFavelier This is unrelated to this particular PR, but in your screenshot I can see that in the double spinboxes, your decimal separators are commas, whereas underneath the time slider, it's periods. Would be good to unify this. I would say that ideally, the double spinboxes should ignore the system locale and use the American representation (periods), as we're using periods everywhere else in MNE already, ignoring locales – at least I think so

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Mar 18, 2021

@larsoner we could use qdarkstyle for the default dark theme maybe? For simplicity and maintenance reasons, it makes sense to me but I would still like to have the option to load a custom stylesheet if possible.

For example, @marsipu created a MNE Color Picker where you can edit a stylesheet and visualize the result directly on the Brain interface:

image


As @hoechenberger pointed out in an offline discussion, the icons need to be updated to follow the theme. For now, I would suggest something like:

In _pyvista.py:

...
def set_theme(theme='auto', icons='auto'):
"""
Apply a theme.

Parameters
----------
theme : str
  * 'auto' use `darkdetect` to choose between 'light' or 'dark' if `MNE_QT_STYLE_CSS` is not set
  * 'light' default Qt theme
  * 'dark' use `qdarkstyle` to set the theme
icons : str
  * 'auto' opposite of `theme` for better contrast otherwise 'dark'
  * 'light' or 'dark' icons
"""

It's even possible to use gradients! 😅

image

@hoechenberger
Copy link
Member

It's even possible to use gradients! 😅

Thanks, I hate it 😁🤣😅

@larsoner
Copy link
Member

How about a new keyword argument for brain which is

theme : str | path-like
    Can be "auto" (default), "light", or "dark" to use darkdetect (required for auto mode)
    or qdarkstyle (required for dark mode) to style the widgets. Can also be a path-like
    to a custom style sheet.

Something like this?

And then for SourceEstimate.plot I think we should just add a brain_kwargs argument, along the lines of add_data_kwargs, that allows setting things like this. We can just say "see Brain for options". Otherwise the signature of stc.plot gets too long (it's already too long, but we'll make the situation worse as we add more Brain options unless we do some brain_kwargs-type solution).

@marsipu
Copy link
Member

marsipu commented Mar 18, 2021

How about a new keyword argument for brain which is

theme : str | path-like
    Can be "auto" (default), "light", or "dark" to use darkdetect (required for auto mode)
    or qdarkstyle (required for dark mode) to style the widgets. Can also be a path-like
    to a custom style sheet.

Maybe we could even consider choosing the style on a more higher level to affect all Qt-Elements (Matplotlib with Qt-Backend, MNE-Coreg, PyVista-Plots)?
For example by adding a mne.config-Variable (MNE_QT_THEME) and a function (mne.set_qt_theme()) with similar parameters as @larsoner suggested above for manipulation.

@larsoner
Copy link
Member

I am not a big fan of global state changes made by one package that affect behaviors in other packages. It leads to all sorts of issues. I think if you want to change how matplotlib behaves everywhere, you should do it in matplotlib. So at least for now I'd rather we constrain our efforts here to just change how our plotters look, and even more focused, just Brain for now -- not all plots even made by MNE. Eventually we might be able to expand this but it's a much bigger effort and API discussion I think. See for example when package-wide matplotlib theme-setting was discussed and ultimately decided not to be done (I think):

#7955 (comment)

Hence the motivation to allow a way to configure just Brain, it's our own object made from Qt so we need to provide a way to style its Qt controls (and maybe someday Jupyter, not sure)...

@larsoner
Copy link
Member

cc @drammock since you've thought about dark mode a bit

@agramfort
Copy link
Member

I expect that 99% of mne users will not care much about this. So my only concern is that any change here does not reduce the chance of mne-python "just working out of the box". It should just work. Now if it works nicer out of the box it's even better

@drammock
Copy link
Member

my 2c: I think Alex underestimates how many users will want dark mode as an option. Having 2 presets and accepting a custom stylesheet seems reasonable as an API. I don't want to support anything more complex than that (like an interactive color picker). I strongly agree with Eric's point that we shouldn't modify MPL colors globally. Let's just start with brain (one of the harder cases) and then see how feasible it is to generalize.

@GuillaumeFavelier
Copy link
Contributor Author

Closing in favor of #9149, the discussion can continue there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants