mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.71k stars 1.32k forks source link

Automatic light/dark mode switching of open STC plot windows #9182

Open hoechenberger opened 3 years ago

hoechenberger commented 3 years ago

On macOS, apps that support light and dark themes switch between the two if the system theme is changed, without requiring a restart. It would be good if our STC plotters could do the same.

https://user-images.githubusercontent.com/2046265/112053418-9e0b8180-8b54-11eb-8a9a-d49b3e467c45.mov

In this screencast you can also see that the color of the title bar is not correct in dark mode, but that might be a Qt issue?

cc @GuillaumeFavelier

larsoner commented 3 years ago

This one to me seems like it will be a lot of work and/or potentially fragile (probably need to hook into some system notification thing) for very small added benefit (since theme changes are generally rare).

hoechenberger commented 3 years ago

This one to me seems like it will be a lot of work and/or potentially fragile (probably need to hook into some system notification thing) for very small added benefit (since theme changes are generally rare).

Yeah, no idea, I thought maybe Qt can trigger a callback upon theme change, no idea.

But the light title bar … would be super nice if we could make this one dark ;)

hoechenberger commented 3 years ago

cc @marsipu

hoechenberger commented 3 years ago

since theme changes are generally rare

plus the STC views are typically not long-lived and can simply be recreated, unlike e.g. a text editor or terminal window, which you may want to keep open for days or weeks.

marsipu commented 3 years ago

This one to me seems like it will be a lot of work and/or potentially fragile (probably need to hook into some system notification thing) for very small added benefit (since theme changes are generally rare).

Yeah, no idea, I thought maybe Qt can trigger a callback upon theme change, no idea.

But the light title bar … would be super nice if we could make this one dark ;)

Is the titlebar white when you start in darkmode?

To dynamically change with the mode, one could maybe setup a QTimer and check periodically for a change from darkdetect. But it is maybe questionable as you already said, if it was worth it (and I don't know @GuillaumeFavelier, how that would influence the performance of for example the BackgroundPlotter).

I read some forum-entries about the titlebar, it seems that the titlebar-color is not influenced by Qt but by the OS. There seem to be a workaround in cpp for MacOS, but I don't know if that could be translated to (MNE-)Python. And I found none for Windows or Linux.

But there seems to be another possibility by replacing the whole window-surroundings including the titlebar. as qtmodern does it. If we'd be willing to add another dependency, this may be the best solution, which would also unify the look across platforms as a side-effect. What do you say?

cbrnr commented 3 years ago

Take a look how I do it in MNELAB. It works on macOS and Linux, but unfortunately Qt doesn't support that on Windows (yet).

marsipu commented 3 years ago

Nice, what do you do? Is it the part with the NSBundle in main.py?

cbrnr commented 3 years ago

No, I'm catching a QEvent here: https://github.com/cbrnr/mnelab/blob/main/mnelab/mainwindow.py#L1011-L1019

This event is triggered whenever the OS theme changes. Therefore, you can use it to change themes on the fly, but as I said not on Windows. However, maybe this isn't that bad, because Windows doesn't have a dynamically changing dark/light theme (it's more like a setting you choose once).

cbrnr commented 3 years ago

However, since you're using style sheets this will probably not work. I think the white title bar is also due to style sheets.

marsipu commented 3 years ago

Nice thank you, maybe it would work if we connect the Stylesheet-Change with your PaletteChange-Event? Could it be, that Qt changes to dark automatically on MacOS and Linux? Or how do you influence the appearance apart from the icons in mnelab? Coming from Windows (unfortunately I have not access to other devices at the moment) I just thought about stylesheets because they seemed to work with all OS.

cbrnr commented 3 years ago

Nice thank you, maybe it would work if we connect the Stylesheet-Change with your PaletteChange-Event?

It's worth a try.

Could it be, that Qt changes to dark automatically on MacOS and Linux? Or how do you influence the appearance apart from the icons in mnelab?

Exactly, I only change the icons manually. On Windows this doesn't work unfortunately, mainly because Qt still does not provide a native dark theme for Windows (not even Qt6).

Coming from Windows (unfortunately I have not access to other devices at the moment) I just thought about stylesheets because they seemed to work with all OS.

They do (except for the title bar apparently), but style sheets never look like a native app (although I think it's very close).

larsoner commented 3 years ago

This one to me seems like it will be a lot of work and/or potentially fragile

I'm catching a QEvent here

Ahh if it's based on some Qt event then ignore what I said about being much work or fragile, it should be neither of those. And even detection of the style name is two lines of Foundation code which seems okay.

Personally my vote would be to use Qt's builtin dark support/styling as much as possible (OSX via icon change) and resort to the style sheet method where Qt does not support it or we don't know how to do it more natively (Windows and Linux).

cbrnr commented 3 years ago

Personally my vote would be to use Qt's builtin dark support/styling as much as possible (OSX via icon change) and resort to the style sheet method where Qt does not support it or we don't know how to do it more natively (Windows and Linux).

:+1: but native styling works on both macOS and Linux, so style sheets should only be required on Windows.

larsoner commented 3 years ago

Even better!

marsipu commented 3 years ago

I wonder, why dark mode was not automatically applied already on MacOS and Linux before we added the stylesheets. Could it be, that the dark-mode is not supported yet by the default pyqt-version (5.12.3) we get from conda-forge?

cbrnr commented 3 years ago

I wonder, why dark mode was not automatically applied already on MacOS and Linux before we added the stylesheets. Could it be, that the dark-mode is not supported yet by the default pyqt-version (5.12.3) we get from conda-forge?

I just tried this with MNELAB, and indeed it seems like 5.12 has not fully implemented theme switching. Only the title bar and icon bar adapt with the theme, but the main widget and status bar remain with the theme they had when I started the app. Therefore, it might be a good idea to use at least 5.15. There's already an open issue: https://github.com/conda-forge/pyqt-feedstock/issues/98

larsoner commented 3 years ago

Qt 5.14+ VTK does not render in HiDPI EDIT: and the controls / clicking are broken, which to me is (much) more important than properly setting the colors of the Qt controls / icons / theme:

https://gitlab.kitware.com/vtk/vtk/-/issues/17953

If theme switching works on 5.13 then we can use that, though, since VTK works properly with that.

Also on Linux I've never had my Qt be anything other than light despite using a dark theme (and latest PyQt5, 5.15+), but maybe that's because I use a GTK-based front-end (Gnome-shell and/or cinnamon) rather than KDE. @cbrnr where have you seen the Qt controls be automatically dark on Linux?

cbrnr commented 3 years ago

Re Qt 5.14+, that's really a pity. Being stuck with an old version will only make things worse over time, so I really hope that VTK fixes this bug (BTW this is another reason why I would want to avoid VTK for a raw viewer and rather stick to pure Qt).

I will test if theme switching works with Qt 5.13, but this would mean that we'd have to mix pip installs with conda, which is never a great idea.

I use GNOME on Arch Linux (so very likely the latest versions of everything), and I'm pretty sure that it works. However, I might be wrong because it's been a while since I've worked on my Linux box (due to WFH), but I can test it next week.

cbrnr commented 3 years ago

Yes, theme switching on macOS works with Qt 5.13!

larsoner commented 3 years ago

There is some suggestion that to trigger it you have to call setStyle, so maybe MNELAB always calls this, and if we do it (on 5.13+) it will work automagically on Qt and/or Linux as well:

https://stackoverflow.com/a/59625439/2175965

cbrnr commented 3 years ago

I'm not calling setStyle anywhere in MNELAB.

larsoner commented 3 years ago

Ahh I thought you did, guess that's not it. Unfortunately for me even on Adaiwata-dark and Qt 5.15.3 mnelab produces a light theme, so I can't investigate on my Linux machine:

Screenshot from 2021-03-23 10-31-19

I can perhaps investigate on my macOS laptop later. Presumably there is some important difference between what we do for stc.plot (or at least what we did before the stylesheet stuff and things were still light) and what mnelab does...

cbrnr commented 3 years ago

I'd be very interested in anything you find out! It's weird that you always get a light theme, but maybe that's how it is. I'll let you know as soon as I get to my Linux box!

GuillaumeFavelier commented 3 years ago

On PyVistaQt, the event() function of the QMainWindow is available in:

https://github.com/pyvista/pyvistaqt/blob/master/pyvistaqt/window.py#L29-L34

I think it would be possible to open a PR there to emit a signal_palette_changed or something. Then in mne-python, this signal can be connected to the theme switcher :)

cbrnr commented 3 years ago

Can you not override the event function and catch the signal?

GuillaumeFavelier commented 3 years ago

It would be hacky since we don't design the window itself in mne but just borrow the one created by BackgroundPlotter in PyVistaQt.

cbrnr commented 3 years ago

Hm, to me reimplementing event sounds less hacky than emitting a (more or less) arbitrary signal upstream. What about deriving a custom BackgroundPlotter class where you reimplement event?

cbrnr commented 3 years ago

Also, I think you can connect the QEvent.PaletteChange signal to another slot?

GuillaumeFavelier commented 3 years ago

BackgroundPlotter inherits from QtInteractor and uses composition for QMainWindow. So it could be possible to modify the parameters at __init__ so that it can accept a window object for example and this one would be custom in mne-python. I still think the signal is simpler. Also, I would rather maintain only one version of the plotter if possible xD

GuillaumeFavelier commented 3 years ago

Another solution would be to drop BackgroundPlotter completely, use QtInteractor directly and design our own QMainWindow of course :)

I think you can connect the QEvent.PaletteChange signal to another slot?

I don't understand, do you mean directly without intermediate signal?

cbrnr commented 3 years ago

Another solution would be to drop BackgroundPlotter completely, use QtInteractor directly and design our own QMainWindow of course :)

Or derive from MainWindow that's already there and just reimplement event.

I don't understand, do you mean directly without intermediate signal?

Yes. Wouldn't that work? You write a slot and connect it to this signal.

GuillaumeFavelier commented 3 years ago

You write a slot and connect it to this signal

Which signal though? By looking online, I found only some enums like QEvent::PaletteChange and QEvent::StyleChange.

https://doc.qt.io/qt-5/qevent.html#Type-enum

cbrnr commented 3 years ago

QEvent::PaletteChange is the one I'm processing in MNELAB.

GuillaumeFavelier commented 3 years ago

I think you can connect the QEvent.PaletteChange signal

To me, it's not a signal, just an enum. You use it in MNE-lab to filter events and process theme change directly. What I suggest in https://github.com/mne-tools/mne-python/issues/9182#issuecomment-805642080 is to emit a signal instead, something like:

    def event(self, ev):
        """Catch system events."""
        if ev.type() == QEvent.PaletteChange:  # detect theme switches
            self.palette_change.emit()

And this will turn the event into a signal that can be connected.

cbrnr commented 3 years ago

You're right, I mixed up events and signals. I still think translating QEvent::PaletteChange is a bit awkward, creating a custom MainWindow (which inherits from PyQtVista's MainWindow) seems to be the better solution to me.

In addition, what about implementing a custom eventFilter method? This should also be able to handle system events.

In any case, I'm not familiar with PyQtVista, so maybe you can just go ahead and modify the main window upstream.

GuillaumeFavelier commented 3 years ago

creating a custom MainWindow (which inherits from PyQtVista's MainWindow) seems to be the better solution to me.

This is a cleaner design for sure. It also gives more leeway.

what about implementing a custom eventFilter method? This should also be able to handle system events.

I did not even know about this, it sounds really useful :sweat_smile:

larsoner commented 3 years ago

I would look into the automatic theme support (instead of using CSS) on macOS. Having custom signals and event handling functions just to take care of when a user switches a theme during use of these usually transient windows is starting to sound like more work to implement and maintain and think about than it's worth at the user end.

hoechenberger commented 3 years ago

+1 @ what @larsoner just said. Unless somebody is really willing to put that much effort and time into this -- which I would appreciate, but I can live without it. We've made a great step forward already!

cbrnr commented 3 years ago

@larsoner you were right, Qt doesn't seem to support dark theming on Linux :disappointed: (only the title bar changes).

cbrnr commented 3 years ago

Although I did manage to get it to use a dark theme with the qt5ct tool (you have to manually select "Adwaita-Dark"). Now I'm pretty sure all this could be done with environment variables so that at least we could use the correct light/dark theme when starting a Qt app. Changing the theme on the fly will probably be still difficult. Update: It even works on the fly :rocket:!

larsoner commented 2 years ago

Now that https://github.com/pyvista/pyvistaqt/pull/152 is in, we can subclass pyvistaqt.window.MainWindow to have the palette change signal connected or use an eventFilter. I think the signal connection method would be my slight preference. Feel free to give it a shot @cbrnr @GuillaumeFavelier if you're motivated !

cbrnr commented 2 years ago

Nice! Just to make sure I didn't miss anything, this still works only on macOS, right?

I'd also prefer either processing the event directly (as I'm doing in MNELAB) or emitting a signal and connecting it to a method that does the processing. I don't think it makes a big difference. I'd not write a custom MainWindow class just for that.

GuillaumeFavelier commented 2 years ago

this still works only on macOS, right?

Good question, I will try a simple code snippet on my KDE desktop to find out :+1: No idea for Windows though.

I'd not write a custom MainWindow class just for that.

I think it's just a misunderstanding. On mnelab you design your own MainWindow class. In mne-python, we borrow the one created by default by pyvistaqt. What @larsoner means is exactly what you suggest above:

creating a custom MainWindow (which inherits from PyQtVista's MainWindow) seems to be the better solution to me.

To clarify, I think we would end up doing the same as mnelab.

GuillaumeFavelier commented 2 years ago

I will try a simple code snippet on my KDE desktop to find out

Nope, it did not work for me unfortunately. I'm really surprised considering everything KDE is basically Qt under the hood.

https://user-images.githubusercontent.com/18143289/157717220-fc55a060-b10b-4823-b798-0d4c52deb364.mp4

The rough code snippet I used:

```py from qtpy.QtCore import QEvent from qtpy.QtWidgets import QApplication, QMainWindow class MainWindow(QMainWindow): def event(self, ev): """Catch system events.""" print(ev) if ev.type() == QEvent.PaletteChange: # detect theme switches print("theme change") return super().event(ev) if __name__ == "__main__": app = QApplication([]) win = MainWindow() win.show() app.exec() ```
cbrnr commented 2 years ago

@GuillaumeFavelier OK, now I got it! I also confirmed that your example is working on macOS. I think that's unfortunately still the only OS, it doesn't work on Windows either.