Skip to content

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.

Sign up for GitHub

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

Merged
larsoner merged 1 commit into from
Nov 23, 2020
Merged

FIX: qt5 macOS 11 compatibility #8554

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mne/gui/_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# License: BSD (3-clause)

import os
import sys

from ..utils import warn, _check_pyqt5_version

Expand Down Expand Up @@ -35,6 +36,9 @@ def _check_backend():
backend = _get_pyface_backend()
if backend == 'qt4':
_check_pyqt5_version()
# QT 5 macOS 11 compatibility:
if sys.platform == 'darwin' and 'QT_MAC_WANTS_LAYER' not in os.environ:
Copy link
Member

@hoechenberger hoechenberger Nov 23, 2020 •
edited
Loading

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

Copy link
Contributor

@cbrnr cbrnr Nov 23, 2020

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.

Copy link
Member Author

@christianbrodbeck christianbrodbeck Nov 23, 2020

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

Copy link
Contributor

@cbrnr cbrnr Nov 23, 2020

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.

Copy link
Member

@hoechenberger hoechenberger Nov 23, 2020

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

Copy link
Member Author

@christianbrodbeck christianbrodbeck Nov 23, 2020

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.

Copy link
Member

@hoechenberger hoechenberger Nov 23, 2020 •
edited
Loading

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.

Copy link
Member

@larsoner larsoner Nov 23, 2020

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

$

Copy link
Member

@hoechenberger hoechenberger Nov 23, 2020

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

Copy link
Contributor

@cbrnr cbrnr Nov 23, 2020

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.

os.environ['QT_MAC_WANTS_LAYER'] = '1'
else:
msg = ("Using the currently selected Pyface backend %s is not "
"recommended, and it might not work properly. We recommend "
Expand Down