Extract in smaller PRs

So far the PR is remarkably small, so hopefully we won't need this. Once the abstraction is done and @alexrockhill is happy with the interactions, if everything works and it's still fairly small (~+300/-300 ish?) then I don't think we need to split.

... and the goal here is really to close mne-tools/mne-gui-addons#14, right?

So far so good I think!

I just noticed you say so at the top. So never mind my question, go go @GuillaumeFavelier !

... and the goal here is really to close mne-tools/mne-gui-addons#14, right?

Good call, I forgot to mention it. I'll update the first comment.

EDIT: Whoops, already edited.

Should I test it now?

Thank you for chiming in @alexrockhill !

Should I test it now?

It's not ready yet and I will probably change things around but of course your opinion is welcome at any point, I'll take it into account 👍

The PR is still WIP because I split it in multiple PRs but feel free to try it @larsoner, @alexrockhill

I will review as you split it up -- @alexrockhill can you do the hands-on testing here to see how things work?

I think this is more related to the darkmode changes than this PR but the white text on gray of the header and bottom bars don't look very good.

One thing that would be nice to add I'm realizing is a zoom text box, that's for a PR after this though. That way if you zoom too far out or in you can reset to 1.

The only thing related to this PR that seems a bit off is that when you go to manually assign the color group of the contact with the dropdown color menu, as you scroll your cursor along the different colors, each is covered by the current color so you can't see the color you're about to select. I think before, the color wasn't changed until it was selected, not just hovered over.

I think this is more related to the darkmode changes than this PR but the white text on gray of the header and bottom bars don't look very good.

I understand the situation with the screenshot. For now, theming is disabled because it is not compatible with the color menu, it changes the style too. Also automatic theme detection is not implemented yet either so a workaround would be to reopen the app after changing the theme I guess.

One thing that would be nice to add I'm realizing is a zoom text box

Good idea!

as you scroll your cursor along the different colors, each is covered by the current color so you can't see the color you're about to select.

That's a strange one, I think I have the same issue on main though. I'll see how to fix this.

Also automatic theme detection is not implemented yet either

If it uses our abstraction it seems like you should get this for free, no?

I think this is more related to the darkmode changes than this PR but the white text on gray of the header and bottom bars don't look very good.

That actually should be fixed in main, I'm surprised you see it here since this branch has those changes:

https://github.com/GuillaumeFavelier/mne-python/blob/7b9f18ebe445b69abf0546ccd106f2128b35976c/mne/viz/backends/_utils.py#L227-L237

But if we aren't using the automatic theming code, then I would expect it to be broken. It seems like this PR should add a theme argument and use it properly / the same way as Brain and mne-qt-browser do.

I have to clarify two elements:

• I disabled any type of dark theme support in this PR because it is not compatible with the color menu. I introduced the theme_support variable for that, which prevents the _window_set_theme() function to set a stylesheet.

This is how it looks on my linux with MNE_3D_OPTION_THEME=dark and theme_support=True:

This is how it looks on my linux with MNE_3D_OPTION_THEME=dark and theme_support=False:

So the automatic theme code is not broken @larsoner, this is just because of this new variable.

But the screenshot shared by @alexrockhill shows that theme_support=False is not enough to mitigate this for macOS. It's probably that the system itself just overrides the widget style anyway.

I could not find an easy fix for the color menu in dark mode but maybe we could set the theme to light instead?

Note that "automatic theme switching" is a different scenario, that's why I said:

a workaround would be to reopen the app after changing the theme I guess.

• Also automatic theme detection is not implemented yet either

Sorry for the confusion, what I mean here is "automatic theme switching", which works only on macOS AFAIK but is not implemented yet. This is related to #9182 (comment)

My previous comment is long. TL;DR: I will set the theme to light until the issue with the color menu is fixed and also add support for "automatic theme switching" shortly.

I tried forcing the theme to 'light' but the color menu has the same issue. So basically as long as the palette is changed, the color menu has this bug. I'll see how to fix it.

Meanwhile I will open a PR to add support for automatic theme switching, it still benefits the other apps.

I fixed the issue with the color menu so the theme_support variable is not necessary anymore

... which means Intracranial Electrode Locator can support dark mode again with the GUI API.

@GuillaumeFavelier, what's the status on this? I was planning on refactoring the GUI to have an abstract slice browser class that it inherits so that it can be reused more easily but I don't want to run into merge conflict issues!

@alexrockhill could you take over and finish?

Ok, I've looked this over thoroughly now and my opinion is to slow this way down. We are definitely not getting not getting the notebook abstraction for free, it's coming at a rather large readability and usability cost of passing everything through the renderer instead of properly abstracting each widget/GUI item. I really do not want to be involved in maintaining the iEEG location GUI if it's going to have notebook support at this cost. I am happy to slowly move each item over PR-by-PR but since a lot of this is already written in abstract classes that have already been merged, I fear the roadmap has been laid in a direction I am very much not in favor of. I propose starting over by removing and re-adding everything in mne/viz/backends/_abstract.py that has been added that would go into the iEEG GUI. It can just be changed not removed, but clearly that will impact the notebook backend and things that depend on it. Looking now, it looks like all of this is already in the coreg GUI and so would have to be rewritten. Since this was a big undertaking, that's not really something I want to take on so it looks like there's an impasse as far as the two Qt-Dependent MNE-Python GUIs. @hoechenberger, mentioned a top-to-bottom rewrite again of the coreg GUI since it's not at all intuitive so maybe that's a direction forward. I just foresee this being a huge time sink although I am happy to work on abstracting this more slowly in coming PRs on GUIs.

@alexrockhill should we close this in favor of transitioning the existing GUI to your new framework? Do you see this as a lot of work?

@alexrockhill should we close this in favor of transitioning the existing GUI to your new framework? Do you see this as a lot of work?

I don't think it will be too much work but it will definitely take some PRs. I did it for the stc viewer and it took a couple hours. It would be nice to move that to the new GSoC GUI though so mne.viz.Brain can be a bit cleaner.