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
adding jamulus-server.desktop.in and new icons #1672
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to add these for future use by packagers.
Do the files jamulus.desktop
and jamulus.desktop.in
also need attention? They are the same except for the Exec
line, for which the latter has Exec=$$TARGET
You're right @softins ! Adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think two further things should be done:
- The image files should probably be added to
DISTFILES
inJamulus.pro
, similar tojamulus.png
jamulus-server.desktop.in
should probably be added toDISTFILES
andQMAKE_SUBSTITUES
. The resultingjamulus-server.desktop
should probably be added todekstop.files
inJamulus.pro
(Note: I don't know specifics either, I'm just looking for similarities and what seems to be needed to make this useful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good to me.
Two (hopefully last) nits:
- After building this PR, the generated
jamulus-server.desktop
file looks good, but I think it should also be added to.gitignore
similar tojamulus.desktop
? (I think someone else -- @softins? -- had already pointed that out) - One stray space, see inline comment
Done by 48e837d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Sorry for the forth-and-back, I was unsure myself what other parts needed to be touched, but I think this lead to a good state now.
I'll defer merging for some days in case anyone else wants to have another look (@softins's review was about an earlier state, for example). Besides that, this is ready to go into 3.8.0, I think.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now. Thanks!
Thanks ! |
In order to be in line with the comments made on #1605 , proposing this new pull request.
Fixes #1316