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

Improve MacOS icon #2858

Merged
merged 1 commit into from Dec 12, 2021
Merged

Improve MacOS icon #2858

merged 1 commit into from Dec 12, 2021

Conversation

Starbuck5
Copy link
Contributor

Fixes #2791

In 1.9.6, the icon looks like this:
regression

In 2.0+, the icon looks like this:
Screen Shot 2021-11-21 at 11 29 39 PM

After this patch, the icon looks like this:
Screen Shot 2021-11-21 at 11 22 11 PM

I'm not 100% sure on this, but my hypothesis is that setting the window icon in SDL1 didn't set the icon on the dock, which is where the window icon should go on MacOS. So the pygame developers of the day put custom code into SDLmain_osx.m that would automatically put a logo on if it wasn't running as an app bundle. It used pygame_icon.tiff. Now, I believe that happens but it is instantly overwritten by the default icon that display.c sets.

The regression icon is the icon set by display.c. It is very pixelated, but that actually looks fine on Windows, since the logo field is much smaller than a space on the MacOS dock. So this PR just changes the default icon filename in display.c for MacOS. (And it adds an attribute for the colorkeying because it can't colorkey away black unconditionally now).

I deleted the outdated / unused logo svg and tiff files.

I also deleted the obsolete macosx.py / SDLmain_osx.m code path, although it is some cool code.

Thanks to @TomSchimansky for bringing up to Mac standard and putting the snake on a rounded corners white background, providing the image files for this PR.

On a side note: why do we have .icns and .ico files in src_py anyway? I updated this with the new icns, but it doesn't seem like it would be used anywhere.

@ankith26 ankith26 added this to the 2.1.1 milestone Nov 22, 2021
Copy link
Contributor

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Even better if it also fixes a crash.

Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Yeah, this PR looks good! 👍 🎉
I'd still like if we could get more reviews from people who can test on a wider range of macs because this change seems like it could break something unexpected?

@MyreMylar
Copy link
Contributor

I actually think that perhaps the best way to get this tested on a wide range of macs is to merge it in and release it.

On the one hand the icon quality is not a show stopping issue, on the other almost everyone who uses pygame on mac will see it immediately so by releasing it we will likely get lots of feedback if it looks worse on some subset of macs. I'd certainly rather pygame 2 had a modern and approachable icon on relatively modern macs, and perhaps a slightly worse icon on really old macs (if that is the case). One of pygame's issues on the wider internet (sadly) is people seeing it as old fashioned and a pixelated, artefact-y icon likely reinforces that perception.

@ankith26 ankith26 added the Awaiting Merge For PRs that have at least one approving review and can be merged on subsequent reviews label Dec 12, 2021
@ankith26 ankith26 linked an issue Dec 12, 2021 that may be closed by this pull request
@Starbuck5
Copy link
Contributor Author

I don't think this is very high risk, so I'm going to merge it in

But if @TomSchimansky or @rethanon wanted to test this, the mac wheels for this branch are in https://github.com/pygame/pygame/actions/runs/1489122638

@Starbuck5 Starbuck5 merged commit e5d4480 into main Dec 12, 2021
@Starbuck5 Starbuck5 deleted the macos-logo branch December 12, 2021 10:11
@Starbuck5 Starbuck5 added Platform: MacOS and removed Awaiting Merge For PRs that have at least one approving review and can be merged on subsequent reviews labels Dec 12, 2021
@rethanon
Copy link
Contributor

I don't think this is very high risk, so I'm going to merge it in

But if @TomSchimansky or @rethanon wanted to test this, the mac wheels for this branch are in https://github.com/pygame/pygame/actions/runs/1489122638

Tested on my Intel MacBook Pro running python 3.10 and MacOS 12.0.1, working great, thanks.

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.

Fatal Python error: (pygame parachute) Bus Error Improve MacOS default image
5 participants