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

Fix app icon on Wayland #417

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Fix app icon on Wayland #417

merged 2 commits into from
Nov 19, 2024

Conversation

OlegAckbar
Copy link
Contributor

Right now, application icon on Wayland could only be changed by setting application ID that is corresponding to the name of desktop entry. https://wayland.app/protocols/xdg-shell#xdg_toplevel:request:set_app_id

Before After
Снимок экрана_20241116_140354 Снимок экрана_20241116_141546

Right now, application icon on Wayland could only be changed by setting application ID that is corresponding to the name of desktop entry.
https://wayland.app/protocols/xdg-shell#xdg_toplevel:request:set_app_id
@mtkennerly
Copy link
Owner

Hi! Are you using Flatpak? I thought this should be handled by the couple of lines here:

https://github.com/flathub/com.github.mtkennerly.ludusavi/blob/a19e036d262a955c5ca601f6477f875441f469f2/com.github.mtkennerly.ludusavi.yaml#L47-L48

If you're using the standalone executable, can you call the desktop file ludusavi.desktop? Or perhaps are you using some distro-specific package?

I wouldn't be opposed to adding support for a LUDUSAVI_LINUX_APP_ID environment variable if needed, but I'd like to understand your situation a bit better :)

@OlegAckbar
Copy link
Contributor Author

I'm using a standalone executable from AUR

If you're using the standalone executable, can you call the desktop file ludusavi.desktop?

This contradicts freedesktop desktop entry spec

The name of the desktop entry should follow the "reverse DNS" convention: it should start with a reversed DNS domain name controlled by the author of the application, in lower case. The domain name should be followed by the name of the application, which is conventionally written with words run together and initial capital letters (CamelCase). For example, if the owner of example.org writes "Foo Viewer", they might choose the name org.example.FooViewer, resulting in a file named org.example.FooViewer.desktop.

@OlegAckbar
Copy link
Contributor Author

OlegAckbar commented Nov 16, 2024

I wouldn't be opposed to adding support for a LUDUSAVI_LINUX_APP_ID environment

There's no need for an environment variable for that. The right and only way to set an application icon on Wayland is to set the proper application id which corresponds to its desktop entry which in turn follows the desktop entry specification. This is how all Wayland applications are made.

@mtkennerly
Copy link
Owner

mtkennerly commented Nov 16, 2024

Thanks, that makes sense. I knew that Flatpak apps followed that kind of naming convention, but I didn't realize that it was a broader standard.

When the Flatpak app ID was created as com.github.mtkennerly.ludusavi, Flatpak didn't yet enforce its current guidance to avoid com.github since technically the project only has control over io.github. I think it's too late to fix the Flatpak ID (there is some migration support, but I'm worried it would still be disruptive/confusing for users), but I think this is a good opportunity to use a better ID elsewhere.

I've just registered mtkennerly.com (just redirects to GitHub for now), so we can use com.mtkennerly.ludusavi for the ID, if you could update the PR.

I don't maintain the distro packages myself, but I do see that both of the AUR packages (ludusavi and ludusavi-bin) set the ID to com.github.mtkennerly.ludusavi. I can try to reach out to their maintainers before the next Ludusavi release so they can update the ID on their end.

Besides setting the ID in the Rust code, I think we'll need to make some other changes. I can do these after this merges, but just wanted to bring it up in case you have any feedback/advice:

  • Replace assets/ludusavi.desktop with two files:
    • For Flatpak, assets/com.github.mtkennerly.ludusavi.desktop with Icon=com.github.mtkennerly.ludusavi
    • For other contexts, assets/com.mtkennerly.ludusavi.desktop with Icon=com.mtkennerly.ludusavi
  • Keep assets/com.github.mtkennerly.ludusavi.metainfo.xml for Flatpak, but make a copy named assets/com.mtkennerly.ludusavi.metainfo.xml that replaces the app ID in the <id> and launchable tags
  • For Flatpak to set its app ID, the Flatpak finish-args can set something like --env=LUDUSAVI_LINUX_APP_ID=com.github.mtkennerly.ludusavi

@OlegAckbar
Copy link
Contributor Author

Having two different ID doesn't make much sense. If there's already established ID for Flatpak I think we should use it, making a separate ID for regular use would just create more chaos.
I've updated desktop entry correspondingly.

@mtkennerly
Copy link
Owner

I understand where you're coming from - keeping a single ID would be simpler. It's just that, when the Flatpak was set up, I only really thought of it as choosing a Flatpak ID rather than a broader Linux ecosystem ID. Now that Ludusavi itself (at an application level) will be officially supporting the ID for Linux in general, I have reservations about committing to com.github. since I think that's better suited to GitHub's own official projects. Basically, I feel like if I don't take steps to use a better ID now, I won't get another chance 😅

I'm not sure if it would be feasible someday to migrate the Flatpak ID, especially since that also affects where the settings are stored on disk, which shouldn't be a problem for distro-specific packages that just need to rename the .desktop file. For now, I mainly just don't want to further expand the scope of where I'm officially committed to using/supporting the com.github. ID.

I'll go ahead and merge this and make some tweaks afterwards. Thanks again for your PR and for raising this concern :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants