Skip to content

Set application icon from within XIVLauncher.Core#364

Open
Dormanil wants to merge 2 commits into
goatcorp:mainfrom
Dormanil:feat/appicon
Open

Set application icon from within XIVLauncher.Core#364
Dormanil wants to merge 2 commits into
goatcorp:mainfrom
Dormanil:feat/appicon

Conversation

@Dormanil

Copy link
Copy Markdown
Contributor

So turns out we've been relying on desktop environments to set icons for us, when we could have done that ourselves. Thankfully, since I last touched SDL in this project, we switched to a better C# wrapper, so this was halfway painless.

I am giving SDL the ability to choose from multiple alternate sizes as appropriate for the use case. Initially it was going to contain 48x48, 32x32, 24x24, and 16x16 as alternate sizes as well, like the (in XLCore unused) dalamud_icon.ico provides, but it didn't look good on my machine. I considered using the ico file, but SDL will only actually look at the highest resolution image within it, so I would have to roll my own image loader, which is entirely beyond the scope of this.

During testing on both X11 and Wayland, the new application icon graced my title bar for the first time since switching to XLCore from XL for Windows. It also sets my taskbar icon properly, though I had to remove the currently existing .desktop file because I couldn't see any changes otherwise. We may want to consider removing the StartupWMClass in it or consider setting it to ffxiv_dx11.exe, like someone else suggested, to share the XIVLauncher icon with the game.
Before:
image
After:
image

Fixes #363.

@Dormanil
Dormanil requested a review from a team as a code owner July 12, 2026 21:34

@wolfcomp wolfcomp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This way of handling the error and retun of the function would make it more versitile if there are other places it such as SDL.ScaleSurface which can fail but isn't checked

Comment thread src/XIVLauncher.Core/Program.cs Outdated
Comment on lines +496 to +500
if (tempicon is null)
{
Log.Error($"Error: SDL_LoadPNG_IO failed: {SDL.GetErrorS()}");
return SDL.CreateSurface(0,0,SDLPixelFormat.Abgr8888);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (tempicon is null)
{
Log.Error($"Error: SDL_LoadPNG_IO failed: {SDL.GetErrorS()}");
return SDL.CreateSurface(0,0,SDLPixelFormat.Abgr8888);
}
if (tempicon is null)
{
icon = null;
return false;
}

Comment thread src/XIVLauncher.Core/Program.cs Outdated
Comment thread src/XIVLauncher.Core/Program.cs Outdated
Comment on lines +292 to +293
icon = GetApplicationIcon();
SDL.SetWindowIcon(window, icon);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
icon = GetApplicationIcon();
SDL.SetWindowIcon(window, icon);
if(!GetApplicationIcon(out icon))
{
Log.Error($"Error: Could not get application icon correctly: {SDL.GetErrorS()}");
}
else
{
SDL.SetWindowIcon(window, icon);
}

@mihawk90

mihawk90 commented Jul 13, 2026

Copy link
Copy Markdown

We may want to consider removing the StartupWMClass in it or consider setting it to ffxiv_dx11.exe, like someone else suggested, to share the XIVLauncher icon with the game.

That was me, and the commit is ready, I just had some more testing to do after making it and then got caught up in other stuff (the issue this addresses among other things). I'll see if get around to opening the PR tomorrow.


Just one question regarding this PR:
Maybe I'm missing it, but this sets the icon with a static graphic, correct? Does SDL provide a way to set it by icon name? That would allow it to follow user's icon themes too (not important, but nice to have).

@Dormanil

Copy link
Copy Markdown
Contributor Author

Just one question regarding this PR:
Maybe I'm missing it, but this sets the icon with a static graphic, correct? Does SDL provide a way to set it by icon name? That would allow it to follow user's icon themes too (not important, but nice to have).

As far as I can tell, SDL does not provide a way to set an icon by its name, in fact, SDL discourages loading images into memory with its LoadPNG methods:

This is intended as a convenience function for loading images from trusted sources. If you want to load arbitrary images you should use libpng or another image loading library designed with security in mind.

Best I can do ignoring SDL's suggestions is looking up the icon theme, figuring the file out from there, and loading it by file name. Problem with that is that there is no desktop-agnostic way of getting the user's icon theme, so now I have to support multiple code paths based on the user's preference of Qt vs GTK, in addition to the current method as the macOS and Windows fallback.

@Dormanil

Copy link
Copy Markdown
Contributor Author

@wolfcomp I was quite happy with moving to the TryParse pattern, but I would prefer actually knowing which of the SDL calls failed if we're going with it.

@mihawk90

Copy link
Copy Markdown

Best I can do ignoring SDL's suggestions is looking up the icon theme, figuring the file out from there, and loading it by file name.

Yeah no, don't bother with that then, it's just a luxury anyway 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tarball binary is missing application icon, defaults to platform icon

3 participants