Skip to content

Conversation

@marsipu
Copy link
Member

@marsipu marsipu commented Nov 28, 2021

Description

This PR adapts to the changes made in mne-tools/mne-python#10048 for the centralization of the Qt-Application intialization.
It should only be merged after the aforementioned PR is merged in the maintenance-branch of mne-python.

@marsipu marsipu linked an issue Nov 28, 2021 that may be closed by this pull request
@marsipu
Copy link
Member Author

marsipu commented Nov 30, 2021

When mne-tools/mne-python#10048 and this PR are merged, I would suggest bumping to a new version. @larsoner I can do it when you approved and merged mne-tools/mne-python#10048

@marsipu marsipu requested a review from larsoner November 30, 2021 07:38

from mne.viz import plot_sensors
from mne.viz.backends._utils import _init_qt_resources
from mne.viz.backends._utils import _init_mne_qtapp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a problem for people on 0.24. We won't backport the _init_mne_qtapp since it's an enhancement, so you'll have to try/except it here for now.

One way to take care of this is to try to import, and if it fails, just have a copy-paste copy here for a couple of months until we release 1.0 (and 0.24 is thus no longer supported).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I didn't know that backport-policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we have to make sure that a new release of mne-qt-browser works for both people on stable MNE (currently 0.24) and latest dev MNE (1.0.dev0). Hopefully the CIs already have this setup, in theory they should catch this error!

# Reshape data to reasonable size for display
max_pixel_width = QApplication.desktop().screenGeometry().width()
if QApplication.desktop() is None:
max_pixel_width = 1920 # defaul=FHD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know people who have had 4K for years, and I think it's more the standard nowadays...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I wish I had one too... ;)

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will merge once CIs come back happy. Thanks @marsipu !

@larsoner larsoner merged commit eeb5989 into mne-tools:main Nov 30, 2021
@marsipu marsipu deleted the centralize_qapp branch December 15, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MAINT: Centralize creation of QApp

3 participants