-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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,MNT: Use the GUI API in Intracranial Electrode Locator #10565
Conversation
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 ! |
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 !
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 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.
Good idea!
That's a strange one, I think I have the same issue on |
If it uses our abstraction it seems like you should get this for free, no?
That actually should be fixed in 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 |
I have to clarify two elements:
This is how it looks on my linux with output.mp4This is how it looks on my linux with output.mp4So the automatic theme code is not broken @larsoner, this is just because of this new variable. But the screenshot shared by @alexrockhill shows that 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:
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 |
I tried forcing the theme to |
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 |
... 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 |
@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. |
This PR follows mne-tools/mne-gui-addons#14 and refactors the
IntracranialElectrodeLocator
to use the mne GUI API. The goal is to allow consistent support on bothpyvistaqt
andnotebook
(hopefully). It also centralizes the bare "Qt" code inmne.viz.backends._qt
.This is still work in progress. For example, I plan to refactor the "button bar" concept by the
_AbstractToolBar
and "bottom bar" by_AbstractStatusBar
.ToDo:
_MNEMainWindow
_AbstractToolBar
_AbstractStatusBar
_AbstractDialog
_AbstractDock
QListView
_key_press_event
QLineEdit
Further consideration:
_CoreWidget
class)Closes mne-tools/mne-gui-addons#14