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

Build: Include jamulus-server.desktop in Debian/Ubuntu builds and fix Linux SVG icon installation location #2460

Merged

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Mar 4, 2022

  • Moves the existing .svg icons to /usr/share/icons/hicolor/scalable/apps/ (from /usr/share/icons/hicolor/512x512/apps, which sounds wrong for SVGs). This affects all Linux installations.
  • Adds the existing jamulus-server.desktop file to the .deb packaging (but not to headless). This is Debian-only.

Both combined should the effect of having a working Jamulus Server desktop icon in the .deb package.

Short description of changes

CHANGELOG: Linux: Added the Jamulus Server desktop icon to the Debian/Ubuntu build and fixed SVG icon installation location.

Context: Fixes an issue?

Related: #2451 (comment)
Fixes: #1879
Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready, resulting .deb has been checked if the file is there -- it is. I can't test the UI (icon rendering) myself though.

What is missing until this pull request can be merged?

Ready.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.9.0 milestone Mar 4, 2022
@hoffie hoffie added this to Triage in Tracking (old) via automation Mar 4, 2022
@hoffie hoffie moved this from Triage to Waiting on Team in Tracking (old) Mar 4, 2022
@@ -1,3 +1,4 @@
usr/bin/jamulus
usr/share/applications/jamulus.desktop
usr/share/applications/jamulus-server.desktop
Copy link
Member

@ann0see ann0see Mar 4, 2022

Choose a reason for hiding this comment

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

Unfortunately it doesn't show the server icon for me. Probably the icon is not set correctly?
Edit: Yes. It's not even in the data file of the .deb

Copy link
Member Author

@hoffie hoffie Mar 4, 2022

Choose a reason for hiding this comment

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

There's no .png in the tree, I'll check whether the .svgs can be used.

Edit: /usr/share/pixmaps seems to work.

https://specifications.freedesktop.org/icon-theme-spec/latest/ar01s07.html

installing a svg icon in $prefix/share/icons/hicolor/scalable/apps means most desktops will have one icon that works for all sizes

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes. .svg's would be even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

The .svgs were already installed by the Makefile, but to the wrong location (IMO). I've fixed that as well and adjusted the debian .install file. Can you test again once the build is done?

Tracking (old) automation moved this from Waiting on Team to Waiting Externally Mar 4, 2022
@hoffie hoffie marked this pull request as draft March 4, 2022 21:54
@hoffie hoffie force-pushed the debian-install-server.desktop branch from a650515 to f2d2841 Compare March 4, 2022 22:14
Comment on lines +415 to +417
isEmpty(ICONSDIR_SVG) {
ICONSDIR_SVG = share/icons/hicolor/scalable/apps/
}
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've no idea why we are checking ICONSDIR (and others) for emptiness. Those variables do not seem magical (i.e. they are not pre-set by qmake, as far as I can see). Therefore the intention was probably to make them overridable. Anyway, I'm following this path and I'm using the same pattern (probably same reasoning what @atsampson did in 69e3286 when first adding it for ICONSDIR).

Fear of breakage is what prevents me from removing that. :)

@hoffie hoffie marked this pull request as ready for review March 4, 2022 22:28
@hoffie hoffie changed the title Build: Include jamulus-server.desktop in Debian/Ubuntu builds Build: Include jamulus-server.desktop in Debian/Ubuntu builds and fix Linux SVG icon installation location Mar 4, 2022
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Ok. Works now.

@hoffie hoffie requested a review from pljones March 4, 2022 23:01
Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

Based on @ann0see's OK.

Tracking (old) automation moved this from Waiting Externally to In Progress Mar 4, 2022
@ann0see ann0see merged commit 7f9f039 into jamulussoftware:master Mar 5, 2022
Tracking (old) automation moved this from In Progress to Done Mar 5, 2022
@hoffie hoffie deleted the debian-install-server.desktop branch March 9, 2022 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Linux deb package not installing server icon
3 participants