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

macOS App Icon fix #4

Merged
merged 1 commit into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Theengs.pro
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ macx {
QMAKE_BUNDLE = app

# OS
QMAKE_ASSET_CATALOGS_APP_ICON = "AppIcon"
QMAKE_ASSET_CATALOGS = $${PWD}/assets/macos/Images.xcassets
ICON = $${PWD}/assets/macos/theengs.icns

# OS infos
QMAKE_INFO_PLIST = $${PWD}/assets/macos/Info.plist
Expand Down
Binary file not shown.
Binary file not shown.
Binary file removed assets/macos/Image.xcassets/AppIcon.appiconset/16.png
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.

This file was deleted.

Binary file modified assets/macos/theengs.icns
Binary file not shown.
2 changes: 1 addition & 1 deletion src/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ int main(int argc, char *argv[])
app.setOrganizationName("Theengs");
app.setOrganizationDomain("Theengs");

#if !defined(Q_OS_ANDROID) && !defined(Q_OS_IOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Q_OS_LINUX will also catch Android (and every linux derivative), that's why this check is done that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood, I first had

#if !defined(Q_OS_ANDROID) && !defined(Q_OS_IOS) && !defined(Q_OS_MACOS)

but then thought why three NOTs when 2 defines can do it, not realising the predicament. So back to the initial version.

Looks good but the icon probably should have more margins to fit better with the other app icons.

My first thought as well when I saw the smaller Dock icon, but wanted to get the code and preconditions sorted first to get the actual Finder and Dock icon working. I will have a look at the other icons and make the Theengs logo smaller within wider margins to reproduce the .icns file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and done. Please let me know what you think about the icon with wider margins now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great!
By the way I don't think the problem was Image.xcassets (it's what xcode is doing by default now) but just the corresponding string in the plist that was still set to theengs.icns...

Copy link
Member Author

Choose a reason for hiding this comment

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

But that should be the expected plist string theengs.icns for CFBundleIconFile, and working fine, now that theengs.icns is actually copied into the Resources folder with

ICON = $${PWD}/assets/macos/theengs.icns

#if !defined(Q_OS_ANDROID) && !defined(Q_OS_IOS) && !defined(Q_OS_MACOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't set the dock or bundle icon anyway, this set the application icon, the one handled by the windows manager. On macOS I don't think it is shown at all nowadays. Might be on older versions, might be again in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This produces the macOS dock icon shortly after the app has started, on macOS Catalina anyway ;) the one in the first screenshot in the issue

#3

Even after I put the previously missing proper .icns file into the app resources it was still replaced by this as the dock icon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed the logo.png file on mine and it isn't shown anywhere.
It's supposed to be the icon left or right of the application title. But as I said macOS 12 doesn't show it anymore, neither does gnome, and I'm not sure about other OS.

Copy link
Member Author

Choose a reason for hiding this comment

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

And did you have a Finder icon and dock icon and which icon was it, when there was no .icns file in the Resources folder?

I wrote this issue with the artifact download

https://github.com/theengs/app/actions/runs/2511311797

which does not have any icon file included, only showing the generic app icon and which made me write up the issue in the first place. Does that download not show the same issues on your macOS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I rebuilt the app to make sure anything wasn't cached and it looks like you are right. The bundled icon is used until the setWindowIcon() is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also left the builds run with my included changes (minus the reverted NOT definition checks) here for testing the binaries

https://github.com/theengs/app/actions/runs/2524476503

At least icon wise this is working all as expected on 10.15 for the Finder and Dock.

// Application icon
QIcon appIcon(":/assets/logos/logo.png");
app.setWindowIcon(appIcon);
Expand Down