Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Set macOS icon when using Qt backend#21784
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
On most platforms, windows have their own icons that are displayed in conjunction with windows. On macOS, icons are used differently. Individual windows should not have an icon unless they are associated with a file. On the other hand, processes ("Apps") have icons that can be set.If Matplotlib is running a FigureManagerBase, then we can assume that Matplotlib can be responsible for setting the process icon.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 can confirm this works. Is there a GUI app we can run that will show this doesn't stomp on whatever icon they are showing?
@jklymak Oh, no, it definitely stomps on the icon if you run it via something that uses a FigureManagerBase. If you want to verify it doesn't always stomp on the icon, you just need to run an example of embedding. I'll see if I can find something... |
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.
This looks good to me and works on linux and mac as expected for me, I don't have a Windows box to test on.
Just one minor comment about whether the if blocks are necessary.
| @@ -121,6 +120,10 @@ def _create_qApp(): | |||
| except AttributeError: # Only for Qt>=5.14. | |||
| pass | |||
| qApp = QtWidgets.QApplication(["matplotlib"]) | |||
| if sys.platform == "darwin": | |||
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.
Is there a harm in setting this regardless of platform? I just tested on a Linux box and it didn't seem to have an impact.
Ditto to the block below, where even if we are on mac, I don't think it hurts anything to callsetWindowIcon on the windows.
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 don't have a problem removing this one.
With the other one, theoretically window icons on macOS are supposed to only be set along with a file path when the window represents a file. However, if you don't set the file path, the icon won't show. So while it's an unnecessary call that's semantically incorrect, it's effectively a noop.
So I guess it's really up to your priorities: semantics? Performance? Less code? Up to you!
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.
Here's the documentation from Qt re:QWidget::setWindowIcon():
https://doc.qt.io/qt-5/qwidget.html#windowIcon-prop
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.
Does it make sense to keep Darwin for now, and if someone wants this on another system they can add 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.
This looks good to me and works well for me locally. I don't have a huge preference one way or the other on the if blocks, so I'll approve it as-is and someone else can comment if they feel it should be a different way.
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'll approve as-is If someone wants to add support beyond Darwin, they can do so, but this is the more conservative change...
PR Summary
On most platforms, windows have their own icons that are displayed in conjunction with windows. On macOS, icons are used differently. Individual windows should not have an icon unless they are associated with a file. On the other hand, processes ("Apps") have icons that can be set.
@greglucas suggested applying these fixes within derivations of FigureManagerBase. FigureManagerBase should only be instantiated when Matplotlib is responsible for running the GUI and its event loop. In this case, it makes sense for Matplotlib to control the App icon. (see#14930 (comment))
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).