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 multi-resolution icons to AppImage #181

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Conversation

ravener
Copy link
Contributor

@ravener ravener commented Oct 24, 2024

fixes #158

Opening this as a draft to get some feedback.

  • I created the usr directory to include the additional files, like the icons at usr/share/icons/hicolor/xx/apps/osu!.png
  • Copied over the existing icons from the flathub repo which already included the different sizes.
  • Moved the root osu!.png in the appdir into the 1024x1024 directory
  • This usr directory could also be used to include any additional files in the future, such as mime type definitions.

A root icon is still needed though, but 1024x1024 is kinda too big, so perhaps copy over the 256x256 version in the root of the AppDir which is the typical recommended size, and the desktop can pickup the higher versions if needed.

TODO

  • I did not include the root icon right now, hoping that perhaps that could be done as a copy action from code, to save some repo size, or do you want me to just copy it in for simplicity?
  • The next concern to address is in the LinuxBuilder.cs where files in the appdir are copied into the staging directory, there's a loop to copy all files but that probably does not handle directories if I'm right, so we'd need a recursive copy option now.

@peppy
Copy link
Member

peppy commented Nov 13, 2024

I did not include the root icon right now, hoping that perhaps that could be done as a copy action from code, to save some repo size, or do you want me to just copy it in for simplicity?

Include it please. Let's keep things simple.

The next concern to address is in the LinuxBuilder.cs where files in the appdir are copied into the staging directory, there's a loop to copy all files but that probably does not handle directories if I'm right, so we'd need a recursive copy option now.

Are you saying this PR doesn't work yet? Please make a working implementation before PRing if you can.

@ravener ravener marked this pull request as ready for review November 13, 2024 09:38
@ravener
Copy link
Contributor Author

ravener commented Nov 13, 2024

@peppy Done. should be ready I hope. I was just afraid of touching the C# but I just followed https://learn.microsoft.com/en-us/dotnet/standard/io/how-to-copy-directories so now it should copy over everything

@peppy
Copy link
Member

peppy commented Nov 14, 2024

Going to need someone with a linux setup to test this at the end of the day.

@ravener
Copy link
Contributor Author

ravener commented Nov 14, 2024

It doesn't change much, honestly. It just adds extra files inside the AppImage, which can be utilized by anyone who needs them (such as tools that integrate app images), but otherwise, it makes zero difference for the average user.

AppImages are just portable executables, so technically, they don't need this, but it's convenient to have every piece of information needed inside them for those who really want to 'install' them. That's also how I've seen the major app images in the Linux ecosystem do it.

@peppy
Copy link
Member

peppy commented Nov 15, 2024

You implied that this change will fix AppImageLauncher at least, so we'd want to test that that is the case.

@bdach
Copy link
Collaborator

bdach commented Nov 15, 2024

Yeah I don't think this works, entirely.

Testing with this appimagelauncher thing shows some change. Output changes from

$ ail-cli integrate bin/osu.AppImage  
Processing /home/dachb/bin/osu.AppImage
Moving AppImage to integration directory
WARNING: No icons found at "usr/share/icons"
WARNING: Using .DirIcon as default app icon
ERROR: Entry doesn't exists: .DirIcon
ERROR: No icon was generated for: /home/dachb/Applications/osu_7407d56675f47f882228974a84b820a4.AppImage
ERROR: appimage_register_in_system : Entry doesn't exists: .DirIcon
Error: Failed to register AppImage in system via libappimage

to

$ ail-cli integrate osulazer-linux-x64.AppImage
Processing /home/dachb/Documents/opensource/osu-deploy/releases/osulazer-linux-x64.AppImage
Moving AppImage to integration directory
Extracting usr/share/icons/hicolor/1024x1024/apps/osu!.png to "/home/dachb/.local/share/icons/hicolor/1024x1024/apps/appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png"
Extracting usr/share/icons/hicolor/128x128/apps/osu!.png to "/home/dachb/.local/share/icons/hicolor/128x128/apps/appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png"
Extracting usr/share/icons/hicolor/256x256/apps/osu!.png to "/home/dachb/.local/share/icons/hicolor/256x256/apps/appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png"
Extracting usr/share/icons/hicolor/512x512/apps/osu!.png to "/home/dachb/.local/share/icons/hicolor/512x512/apps/appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png"
Extracting usr/share/icons/hicolor/64x64/apps/osu!.png to "/home/dachb/.local/share/icons/hicolor/64x64/apps/appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png"
WARNING: Unable to resize the application icon into a 128x128 image: "Unable to load image.". It will be written unchanged.
WARNING: Unable to resize the application icon into a 256x256 image: "Unable to load image.". It will be written unchanged.

The first one says "failed" but it doesn't seem to have failed since the app did make it to the launcher both times. However, both times...

image

An icon was promised, and an icon does not appear to have been delivered.

And, for reference:

image

@bdach
Copy link
Collaborator

bdach commented Nov 15, 2024

Yeah no I can't get this to work.

There appear to maybe be at least 2 issues here:

  • on my machine $XDG_DATA_DIRS did not contain /home/dachb/.local/share, which may be a distro issue and which I fixed I guess

  • the .desktop file installed by appimagewhateverwangs contained this spec:

    Icon=appimagekit_60f08d8cc86389ff399dbee071c8a611_osu_
    

    which seems not correct given the files were named appimagekit_60f08d8cc86389ff399dbee071c8a611_osu!.png.

I've fixed both and it still doesn't work, so I give up. I don't want to spend a second more on this stupid garbage. In typical linux fashion nothing works. Why do I bother with this OS.

@ravener
Copy link
Contributor Author

ravener commented Nov 16, 2024

the .desktop file installed by appimagewhateverwangs contained this spec:
Icon=appimagekit_60f08d8cc86389ff399dbee071c8a611_osu_

It appears to be an issue with the handling of ! exclamation mark as an identifier, interesting, it did at least find and extract the images though so that's a good start, now probably changing the names to just osu.png may solve this.

if you did try renaming them, icon changes aren't always instant so you may need to run update-desktop-database / update-icon-caches to get it to work.

I agree Linux is a mess, it's just not a full operating system in itself and so there's constantly differing community solutions and standards instead of a single central standard which makes it a bit annoying to some devs but oh well.

@ravener
Copy link
Contributor Author

ravener commented Nov 16, 2024

I've updated the icons to simply osu.png it better work this time

@peppy
Copy link
Member

peppy commented Nov 18, 2024

Let's just get this in since it's doing no harm.

@peppy peppy merged commit 937ed19 into ppy:master Nov 18, 2024
2 checks passed
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.

Improve icons support on Linux
3 participants