I'm not sure about unit tests. Could somebody provide feedback? I don't really have a way to unit-test any of the code, aside from perhaps checking that the ctypes/Apple api's say that I did, in fact, successfully change the icon. It sort of seems like circular reasoning: "Do the API's say they worked?"

At any rate, the icon is set for every plot on macOS with Qt5, so in a sense we are piggy-backing on those unit tests too.

Thoughts?

I didn't see a reason to reset the icon, although that is also possible. Thoughts?

My knee-jerk reaction as that this is too much complexity for the problem it is solving.

From the linked bug report it suggests that the issue is that the at-conversion of svg -> QIcon does not provide a list of available sizes and then there is a bug in the OSX specific code that results in no icon being shown. Could we create a QIcon and set the missing size information?

Failing that, rendering a hi-dpi png (which we may already have due to the other backends) and using that seems like a better option.

I did play around with trying to get the QIcon to render/rasterize the SVG and failed. I can make another pass at it.

I did check for a high-res png and came up empty. None of them were large enough on my display. I wouldn't be surprised if the default on other backends were to use SVG, that's what Qt5 uses. I would recommend generating a larger logo if we go this route. I'm not sure how much larger- I'd want to do some math for render sizes when using a 5k monitor. Maybe just follow Apple's recommended icon sizes.

Neither are perfectly ideal if the system supports vector icons and we're providing a raster. From a performance standpoint, I'd say creating the logo from scratch is worst, rastering a SVG is better, loading a high-res png is even better, and loading a vector graphic is best.

I can also put this in the macos backend code instead of ctypes. If we need the code for the macos backend anyways, then I could expose the Objective-C functionality as an internal Python API for other backends to use. Of course this also violates encapsulation of the backends, which may or may not be a big deal here, and might require a Python wrapper so that other platforms don't complain about the missing macos module.

I can implement everything except for generating a new icon. I could probably figure that out, I'm just not sure of the process for doing that and adding a photo to the repo.

What would you like me to do?

I can also put this in the macos backend code instead of ctypes. If we need the code for the macos backend anyways, then I could expose the Objective-C functionality as an internal Python API for other backends to use. Of course this also violates encapsulation of the backends, which may or may not be a big deal here, and might require a Python wrapper so that other platforms don't complain about the missing macos module.

This seems like a promising path to explore.

generating (and caching) the png on the fly seems like a good option too.

The icons are generated with tools/make_icons.py (#14941 may help a bit). You'll need to edit it to generate bigger sized icons.

I can also put this in the macos backend code instead of ctypes. If we need the code for the macos backend anyways, then I could expose the Objective-C functionality as an internal Python API for other backends to use. Of course this also violates encapsulation of the backends, which may or may not be a big deal here, and might require a Python wrapper so that other platforms don't complain about the missing macos module.

This seems like a promising path to explore.

My last push went with this solution. If you like it, I'll add tests.

Wrote tests, fixed a few things. This only addresses the matplotlib icon not showing up on macosx and Qt5Agg backgrounds on macOS. I haven’t tested or worked on other backends.

@anntzer Okay, pushed fixes, could use another review.

Okay @anntzer, another push another review?

looks fine to me, but now you need to find an actual mac user (ie not me) to test this :p

Any suggestions? I assume I don’t count

Tagged the PR, I'm sure they'll show up :p

@anntzer Should we bump this somehow? Seems like nobody has noticed, but I'm not sure how long that usually takes here.

I've been away, but wasn't popping this to the top of my queue because I wasn't clear that Matplotlib is an application, and hence am not clear why we would set the dock icon.

@jklymak Honestly didn't consider it, since the Qt backends set the app icon on other platforms. Maybe the icon shouldn't be set in general?

I'm sure it needs cleanup and style changes. I'd welcome any suggestions/tweaks! There are quite a few choices that could be done many ways and I don't know what would be preferred.

I think the segfault you were experiencing was due to other code which has since been resolved. I rebased to a new point in main, could you test again?

Could we also modify the PR to not target Qt? I've moved Qt into another PR.

Yep, it seems to work well now without a segfault. Lets see if others have additional comments over the next few days about the architecture comments, but I think this is looking good and splitting it into separate PRs is also nice, thanks for doing that!

@greglucas I figured it's been long enough, so I went back to a set_icon method. I made it static because it doesn't reference any saved state.

@greglucas Should be good now?

Right now passing tests let us know that set_icon is working. If you want to do that we'd have to add a test for set_icon because it would no longer be covered.

@greglucas What do I need to do to wrap this up?

You've got my approval, but someone else will need to test it out before merging still, so just waiting on the second review which can take a bit of time since it is all volunteer basis. Thanks for your patience!

@tacaswell @anntzer Any chance you’d know somebody who we could ping to merge this?

Perhaps @dstansby or @jklymak can help on that?

Thanks @joelfrederico for sticking with us and getting this done!