Skip to content
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

Set macOS icon when using Qt backend #21784

Merged
merged 1 commit into from Nov 30, 2021

Conversation

joelfrederico
Copy link
Contributor

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

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

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.
Copy link
Member

@jklymak jklymak left a 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?

@joelfrederico
Copy link
Contributor Author

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

Copy link
Contributor

@greglucas greglucas left a 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":
Copy link
Contributor

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 call setWindowIcon on the windows.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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

image

Copy link
Member

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?

Copy link
Contributor

@greglucas greglucas left a 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.

Copy link
Member

@jklymak jklymak left a 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...

@jklymak jklymak merged commit d536acf into matplotlib:main Nov 30, 2021
@joelfrederico joelfrederico deleted the qt_mac_icon branch December 3, 2021 07:46
@QuLogic QuLogic mentioned this pull request Sep 9, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants