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

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

Open
hoechenberger opened this issue Mar 22, 2021 · 45 comments
Open

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

hoechenberger opened this issue Mar 22, 2021 · 45 comments

Comments

@hoechenberger
Copy link
Member

hoechenberger commented Mar 22, 2021

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.

Screen.Recording.2021-03-22.at.21.18.53.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
Copy link
Member

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
Copy link
Member Author

hoechenberger commented Mar 22, 2021

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
Copy link
Member Author

cc @marsipu

@hoechenberger
Copy link
Member Author

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
Copy link
Member

marsipu commented Mar 22, 2021

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
Copy link
Contributor

cbrnr commented Mar 22, 2021

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
Copy link
Member

marsipu commented Mar 22, 2021

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

@cbrnr
Copy link
Contributor

cbrnr commented Mar 23, 2021

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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
Copy link
Member

marsipu commented Mar 23, 2021

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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
Copy link
Member

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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).

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

@larsoner
Copy link
Member

Even better!

@marsipu
Copy link
Member

marsipu commented Mar 23, 2021

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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: conda-forge/pyqt-feedstock#98

@larsoner
Copy link
Member

larsoner commented Mar 23, 2021

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

Yes, theme switching on macOS works with Qt 5.13!

@larsoner
Copy link
Member

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

I'm not calling setStyle anywhere in MNELAB.

@larsoner
Copy link
Member

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
Copy link
Contributor

cbrnr commented Mar 23, 2021

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
Copy link
Contributor

GuillaumeFavelier commented Mar 24, 2021

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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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

@GuillaumeFavelier
Copy link
Contributor

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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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

@GuillaumeFavelier
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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
Copy link
Contributor

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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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

@GuillaumeFavelier
Copy link
Contributor

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 #9182 (comment) 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
Copy link
Contributor

cbrnr commented Mar 24, 2021

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
Copy link
Contributor

GuillaumeFavelier commented Mar 24, 2021

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 😅

@larsoner
Copy link
Member

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
Copy link
Member Author

+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
Copy link
Contributor

cbrnr commented Mar 30, 2021

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

@cbrnr
Copy link
Contributor

cbrnr commented Mar 30, 2021

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 🚀!

@larsoner
Copy link
Member

Now that pyvista/pyvistaqt#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 !

@larsoner larsoner added this to the 1.1 milestone Mar 10, 2022
@larsoner larsoner added the EASY label Mar 10, 2022
@cbrnr
Copy link
Contributor

cbrnr commented Mar 10, 2022

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
Copy link
Contributor

GuillaumeFavelier commented Mar 10, 2022

this still works only on macOS, right?

Good question, I will try a simple code snippet on my KDE desktop to find out 👍
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
Copy link
Contributor

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.

output.mp4

The rough code snippet I used:

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
Copy link
Contributor

cbrnr commented Mar 10, 2022

@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.

@larsoner larsoner modified the milestones: 1.1, 1.2 Jun 4, 2022
@larsoner larsoner modified the milestones: 1.2, 1.3 Sep 13, 2022
@larsoner larsoner removed this from the 1.3 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants