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

Add infrastructure for embedding Win32 icons #308

Closed
wants to merge 1 commit into from

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Feb 1, 2024

The icons are embedded into osu.Game.Resources.dll and are readable by the shell, this can be tested with IconsExtract.

The manual build procedure (see Icons/README.md) uses the windows resource compiler, unsure if that is available for use on non-windows platforms. And even if it isn't available, the generated Icons.res file is committed to the repository, so there shouldn't be any issues building the entire project on macOS or Linux.

The only problem with using <Win32Resource> is that the assembly version information is no longer added to the resulting DLL. This seems to only be a cosmetic thing, since osu! can use the resources normally.

Here's what the version information of the last release contains:

FILEVERSION    2024,129,0,0
PRODUCTVERSION 2024,129,0,0
FILEFLAGSMASK  0x3F
FILEFLAGS      0x0
FILEOS         VOS_UNKNOWN | VOS__WINDOWS32
FILETYPE       VFT_DLL
FILESUBTYPE    0x0
{
  BLOCK "VarFileInfo"
  {
    VALUE "Translation", 0x0, 1200
  }
  BLOCK "StringFileInfo"
  {
    BLOCK "000004b0"
    {
      VALUE "CompanyName",       "ppy Pty Ltd"
      VALUE "FileDescription",   "osu.Game.Resources"
      VALUE "FileVersion",       "2024.129.0.0"
      VALUE "InternalName",      "osu.Game.Resources.dll"
      VALUE "LegalCopyright",    "Copyright (c) 2019 ppy Pty Ltd"
      VALUE "OriginalFilename",  "osu.Game.Resources.dll"
      VALUE "ProductName",       "osu!resources"
      VALUE "ProductVersion",    "2024.129.0+ed0da559d35b4e69af8d760b29dfc9190920146e"
      VALUE "Assembly Version",  "2024.129.0.0"
    }
  }
}

To be expanded with additional icons and used for ppy/osu#26658
/// String that can be used with the <c>DefaultIcon</c> key in the registry to point to this icon.
/// </summary>
// The icon id needs to be negative, see https://devblogs.microsoft.com/oldnewthing/20100505-00/?p=14153
public string RegistryString => $"{Assembly.Location},{-Id}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

So my primary question here would be.......... can we skip all of this legacy nonsense and just use plain ICO files?

I'd prefer that we didn't have to use some win32 resource incantations and some executables that are only available on windows and also only if you have visual studio enabled.

I would much rather put the .ico assets directly in osu.Game.Desktop and point the registry entries at a plain ICO file rather than deal with whatever this thing is. The fact that this is having to cite a Raymond Chen blogpost is a red flag in my eyes in that this is underdocumented legacy cruft, because if that is the best available citation then oh boy.

Copy link
Collaborator

@bdach bdach Feb 5, 2024

Choose a reason for hiding this comment

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

In fact as far as I can tell even microsoft doesn't wanna deal with this stuff anymore:

image

The above is a VS Code .md file association. It points at a plain .ico.

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 would much rather put the .ico assets directly in osu.Game.Desktop and point the registry entries at a plain ICO file

I agree, would make things simpler.

I tried including the icons as Content and it seems to work well. I just tested this with velopack and a sample app, and the icons are available in the install directory.

<ItemGroup>
    <Content Include="Icons\*.ico"
             CopyToPublishDirectory="PreserveNewest"
             CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>
C:\Users\...\AppData\Local\VPTest> tree /F
│   Update.exe
│
├───current
│   │   Microsoft.Extensions.DependencyInjection.Abstractions.dll
│   │   Microsoft.Extensions.Logging.Abstractions.dll
│   │   NuGet.Versioning.dll
│   │   sq.version
│   │   Velopack.dll
│   │   VelopackTest.deps.json
│   │   VelopackTest.dll
│   │   VelopackTest.exe
│   │   VelopackTest.runtimeconfig.json
│   │
│   └───Icons
│           lazer.ico
│
└───packages
        VPTest-1.0.0-full.nupkg

@Susko3
Copy link
Member Author

Susko3 commented Feb 5, 2024

Closing since a better approach was found.

@Susko3 Susko3 closed this Feb 5, 2024
@Susko3 Susko3 deleted the add-win32-icons branch February 5, 2024 11:23
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.

2 participants