-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
FIX: qt5 macOS 11 compatibility #8554
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
Conversation
| if backend == 'qt4': | ||
| _check_pyqt5_version() | ||
| # QT 5 macOS 11 compatibility: | ||
| if sys.platform == 'darwin' and 'QT_MAC_WANTS_LAYER' not in os.environ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check whether we're actually running on Big Sur, and if we are, we don't even need to check whether the environment variable exists -- we'll need to set it correctly anyway in order to make things work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even simpler, just set the variable if running on darwin – I just checked on macOS Mojave and my PyQt5 app still works after setting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing @cbrnr!
@hoechenberger I am reluctant to silently overwrite an environment variable that the user already set, unless you are sure that the user would never want it to be a different value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's highly unlikely that a user set this variable for something other than fixing the PyQt5 bug. In this case, the value must be 1, so if you do not want to overwrite an existing variable the problem isn't fixed. I would set the variable to 1 when running on macOS, and if that variable has already been set to a value other than 1 I'd issue a warning. After all, this is a workaround, which hopefully we won't have to use for long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But with the current implementation you're also changing the environment permanently (setting a new variable if it doesn't already exist, changing behavior)
In the Study Template we had to deal with a similar issue and solved it this way:
https://github.com/mne-tools/mne-study-template/blob/766cc0e2dcab1fecf61804e1b013ba37988f212c/run.py#L135-L148
Wonder if this is something we could re-use here too..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we would have to combine the platform conditional with modified_env, isn't that a bit overkill? I just wanted to avoid messing with a power user's setting, but did not think setting this variable would interfere with the average use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't mean to make this more complicated than it needs to be.
I'd also be fine with simply setting the env variable no matter what, and setting ourselves a reminder to revert this change as soon as new (fixed) MPL binaries have been released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoechenberger FWIW I'm not sure why you have any atexit, no env vars are saved/written when doing python:
$ echo $foo
$ python
Python 3.8.6 (default, Sep 25 2020, 09:36:53)
[GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.environ['foo'] = 'bar'
>>> exit()
$ echo $foo
$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait... what?! Wow. I thought those were changed permanently. Good to know!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoechenberger if you do e.g. signal.signal(signal.SIGINT, lambda: pass) (like it's implemented right now except instead of lambda: pass you have _restore_env (a function with no parameters), you will (1) get an error because you need to pass a function with two parameters, and (2) even if you pass a function with the correct signature (e.g. signal.signal(signal.SIGINT, lambda x, y: pass)) you need to make sure that the previous behavior of SIGINT is retained (i.e. a KeyboardInterruptException should be thrown at the end to halt the program), otherwise the program will continue to run even if the user presses Ctrl + C.
|
There might be other places that we want something like this -- for example in |
Sets `QT_MAC_WANTS_LAYER=1` for STC plots via PyVista. Was required on my system with macOS 11.2.1, Qt 5.12.9, pyqt 5.12.3 (installed from `conda-forge`) Previous fix via mne-tools#8554 didn't cover STC plots. cc @cbrnr, @GuillaumeFavelier
Sets `QT_MAC_WANTS_LAYER=1` for STC plots via PyVista. Was required on my system with macOS 11.2.1, Qt 5.12.9, pyqt 5.12.3 (installed from `conda-forge`) Previous fix via mne-tools#8554 didn't cover STC plots. cc @cbrnr, @GuillaumeFavelier
Sets `QT_MAC_WANTS_LAYER=1` for STC plots via PyVista. Was required on my system with macOS 11.2.1, Qt 5.12.9, pyqt 5.12.3 (installed from `conda-forge`) Previous fix via mne-tools#8554 didn't cover STC plots. cc @cbrnr, @GuillaumeFavelier
* macOS 11 compatiblity for PyVista STC plots Sets `QT_MAC_WANTS_LAYER=1` for STC plots via PyVista. Was required on my system with macOS 11.2.1, Qt 5.12.9, pyqt 5.12.3 (installed from `conda-forge`) Previous fix via #8554 didn't cover STC plots. cc @cbrnr, @GuillaumeFavelier * Remove backend check * Update changelog * Better changelog
Reference issue
Fixes #8525 by setting the environment variable before launching GUIs.
TODO