macOS App Icon fix #4
Conversation
DigiH
requested review from
emericg and
1technophile
and removed request for
emericg and
1technophile
| @@ -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) | |||
There was a problem hiding this comment.
The Q_OS_LINUX will also catch Android (and every linux derivative), that's why this check is done that way.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done and done. Please let me know what you think about the icon with wider margins now.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
DigiH
force-pushed
the
macOS-AppIcon
branch
3 times, most recently
from
803ffb8
to
1e438b5
Compare
| @@ -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) | |||
| #if !defined(Q_OS_ANDROID) && !defined(Q_OS_IOS) && !defined(Q_OS_MACOS) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Even after I put the previously missing proper .icns file into the app resources it was still replaced by this as the dock icon.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
• Apple UI guidelines adherent Finder icon and Dock icon • Delete now unnecessary Image.xcassets directory and its content • Required changes in Theengs.pro and main.cpp • New Icon with wider margins
|
I guess we can merge that? |
|
I suppose you with doing the reviews should be fine, what does @1technophile say? |
|
Ultimately it's a look and feel issue, he should have the last say in what the icon looks like. |
|
Didn't really show the new icon with the wider margins, and not sure if @1technophile can view the .icns file, so here are a few different size screenshots
|
|
looks great! @DigiH |
• Apple UI guidelines adherent Finder icon and Dock icon
• Delete now unnecessary Image.xcassets directory and its content
• Required changes in Theengs.pro and main.cpp
Bulding fine with desired icon results
https://github.com/theengs/app/actions/runs/2524476503