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

install multiple sizes of rasterized icons #4204

Merged
merged 1 commit into from
Aug 29, 2021
Merged

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Aug 15, 2021

The Flathub manifest renames the icon to work around this: https://github.com/flathub/org.mixxx.Mixxx/blob/master/org.mixxx.Mixxx.yaml#L274

@uklotzde
Copy link
Contributor

This change might break existing packaging recipes and requires a note for maintainers.

@uklotzde uklotzde added this to the 2.3.1 milestone Aug 15, 2021
@uklotzde uklotzde added the changelog This PR should be included in the changelog label Aug 15, 2021
@Be-ing Be-ing mentioned this pull request Aug 15, 2021
@Be-ing Be-ing force-pushed the icon_name branch 3 times, most recently from 6a71a2b to a31ad86 Compare August 15, 2021 15:51
@Be-ing Be-ing changed the title rename icon file to org.mixxx.Mixxx.svg for Freedesktop standards rename icon file to org.mixxx.Mixxx.svg and install multiple sizes of PNGs for Freedesktop standards Aug 15, 2021
@Holzhaus
Copy link
Member

Holzhaus commented Aug 15, 2021

Two points:

  1. We shouldn't rename our icon to match some ID used by some other application (flatpak) that we do not control.
  2. This violates the freedesktop icon naming specification, which states:

An application must either use a generic application icon name provided by this specification, or install an icon named the same as the executable for running the application.

I'm not willing to rename the mixxx executable to org.mixxx.Mixxx, so I don't think we should merge this.

@uklotzde
Copy link
Contributor

I was assuming that this renaming was due to some freedesktop conventions according to the other PRs, but missed to actually verify it myself. If this is not the case and the naming is only a custom convention of flatpak then we should not follow it blindly.

In this respect I agree with @Holzhaus.

@Be-ing Next time please add the rationale in the description of the PR to prevent such misunderstandings.

@uklotzde
Copy link
Contributor

@Holzhaus I wonder why the icon naming convention doesn't follow that of the .desktop file and ID naming convention? Doesn't make any sense to me, because it will result in naming conflicts.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

I am confused as well. I don't understand why Flathub wants it to be reverse DNS in conflict with the icon naming specification.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

Regardless we should install rasterized versions though.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

Let's see what happens with mixxx.svg and mixxx.png... flathub/org.mixxx.Mixxx#35

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

Yeah Flathub builds break with mixxx.svg and mixxx.png:

Running appstream-compose
FB: Running: flatpak build --die-with-parent --nofilesystem=host /srv/buildbot/worker/build-x86_64-2/build/.flatpak-builder/rofiles/rofiles-FrBg2z appstream-compose --prefix=/app --origin=flatpak --basename=org.mixxx.Mixxx org.mixxx.Mixxx
Processing application org.mixxx.Mixxx
Error loading desktop file: Failed to find icon: Failed to find icon org.mixxx.Mixxx
Error: ERROR: appstream-compose failed: Child process exited with code 1

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

I opened an issue on Flathub... let's see what the response is.

@Holzhaus
Copy link
Member

For now, I suggest to just leave the name unchanged and take care of the renaming in the flatpak packaging code. Then we can at least merge the other stuff in this PR.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

It's not just Flathub, building the Flatpak locally has the same issue:

Finishing app
Exporting share/applications/org.mixxx.Mixxx.desktop
Exporting share/icons/hicolor/scalable/apps/org.mixxx.Mixxx.svg
Not exporting share/icons/hicolor/32x32/apps/mixxx.png, non-allowed export filename
Exporting share/icons/hicolor/32x32/apps/org.mixxx.Mixxx.png
Exporting share/icons/hicolor/64x64/apps/org.mixxx.Mixxx.png
Exporting share/icons/hicolor/128x128/apps/org.mixxx.Mixxx.png
Exporting share/icons/hicolor/256x256/apps/org.mixxx.Mixxx.png
Exporting share/icons/hicolor/512x512/apps/org.mixxx.Mixxx.png

@Be-ing Be-ing force-pushed the icon_name branch 3 times, most recently from bf4fa70 to 72c4b8a Compare August 15, 2021 23:13
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 16, 2021

I reported the issue to Flatpak.

@Holzhaus
Copy link
Member

Does installing a flatpak package install an executable? If not, the executable would be flatpak with the app name org.mixxx.Mixxx passed on the command line. If you strictly adhered to the icon naming spec, you'd have to name the icon flatpak.svg which would be nonsensical and cause conflicts if multiple flatpak apps are installed. So I guess using the reverse DNS name is the next best thing in this specific case. But generally, the icon name should stay mixxx.svg and the flatpak packager needs to take care of the renaming.

@Be-ing Be-ing changed the title rename icon file to org.mixxx.Mixxx.svg and install multiple sizes of PNGs for Freedesktop standards install multiple sizes of rasterized icons Aug 29, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 29, 2021

ping

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@uklotzde uklotzde merged commit 0b72fc6 into mixxxdj:2.3 Aug 29, 2021
@ronso0 ronso0 mentioned this pull request Aug 30, 2021
@Be-ing Be-ing deleted the icon_name branch February 11, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build changelog This PR should be included in the changelog library ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants