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 wayland app icon for flatpak. #6134

Closed

Conversation

mariakeating
Copy link

Fix wayland app icon for flatpak.

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Description

Currently, the app icon (i.e. titlebar icon in KDE) is incorrect as the
app_id is set to FreeTube instead of matching the desktop file with
io.freetubeapp.FreeTube.

Screenshots

Screenshot_20241111_001919

Testing

Testing done on an arch linux install.
Untested for distribution packages nor windows and macos builds.

Desktop

  • OS: Arch Linux
  • FreeTube version: v0.22.0 Beta

Currently, the app icon (i.e. titlebar icon in KDE) is incorrect as the
app_id is set to FreeTube instead of matching the desktop file with
io.freetubeapp.FreeTube.
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 10, 2024 16:26
@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 10, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

App icon looks the same on MacOS (yarn build:arm64)

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

No idea how to test this, without doing a full on flatpak release (we unpack the Linux zip files and use their contents for the flatpak, so I don't understand how an extra package.json field would change anything about the flatpak, unless the flatpak runtime reads the package.json file?) but as this doesn't seem to break anything (IDK if this would break the deb install for example), I'm happy to give it a try.

@absidue absidue self-requested a review November 13, 2024 22:14
@absidue
Copy link
Member

absidue commented Nov 13, 2024

Dismissing my review because it's probably worth checking that this doesn't break the deb builds, which afaik use the .desktop file that electron-builder auto-generates instead of the static one in the flatpak repo.

@mariakeating
Copy link
Author

I don't understand how an extra package.json field would change anything about the flatpak.

It's not changing the flatpak, it's changing the value of the app_id set for the xdg_toplevel to match the .desktop file the flatpak provides. If flatpak didn't require such a naming convention, I would have made a PR on the flatpak's repo to rename the file.

Dismissing my review because it's probably worth checking that this doesn't break the deb builds, which afaik use the .desktop file that electron-builder auto-generates instead of the static one in the flatpak repo.

Definitely worth checking. My understanding from the docs is that this also changes the name of the generated .desktop file but I'm not familiar with this tooling.

@mariakeating
Copy link
Author

Unfortunately, with this change, the file generated for .deb files is still named freetube.desktop instead of matching the desktopName key, resulting in wayland icons broken outside of flatpak.

Seems like electron builder makes this very frustrating. electron-userland/electron-builder#4071

Unless anybody knows how to work around this, I will close and instead make a PR for the flatpak's repo as there is another option that would achieve this (2nd paragraph).
https://docs.flatpak.org/en/latest/electron.html#using-correct-desktop-file-name

@absidue
Copy link
Member

absidue commented Nov 20, 2024

As you've opened an pull request on the flatpak repo, I'll close this one. As flathub does a release every time a pull request is merged, we'll merge your pull requests next time we do a FreeTube release, just so you don't think we've forgotten or ignoring it :).

@absidue absidue closed this Nov 20, 2024
auto-merge was automatically disabled November 20, 2024 16:34

Pull request was closed

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants