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 appdata.xml #128

Merged
merged 1 commit into from
Jun 26, 2021
Merged

Fix appdata.xml #128

merged 1 commit into from
Jun 26, 2021

Conversation

AsciiWolf
Copy link
Contributor

@AsciiWolf AsciiWolf commented May 26, 2021

Fix incorrect app id (and add correct component type) that is not correct rDNS and does not match desktop file name, see: https://bugzilla.redhat.com/show_bug.cgi?id=1819344#c6

@dave-theunsub Feel free to review and merge. :-)

@AsciiWolf
Copy link
Contributor Author

Also, consider adding an actual screenshot instead of https://raw.githubusercontent.com/dave-theunsub/clamtk/master/images/clamtk_300x300.png. :-) You can probably use one from Flathub.

Fix incorrect app id and add correct component type
@AsciiWolf
Copy link
Contributor Author

@dave-theunsub Feel free to let me know if there is any problem with this PR. :-)

@dave-theunsub
Copy link
Owner

Sorry I'm moving slow - I'll get to it. :)

@dave-theunsub
Copy link
Owner

Hi @AsciiWolf ,

I'm getting error messages with your inputs:

appstreamcli validate clamtk.appdata.xml
E: clamtk.desktop:82: metainfo-invalid-icon-type local
W: clamtk.desktop:4: cid-desktopapp-is-not-rdns clamtk.desktop

So I used this page for reference: https://freedesktop.org/software/appstream/docs/chap-Quickstart.html

https://gist.github.com/dave-theunsub/68faf2078253e5f832bd0e1df04c3b3d

Returns this:
Validation was successful.

What do you think?

@AsciiWolf
Copy link
Contributor Author

AsciiWolf commented Jun 5, 2021

Hi Dave,

yeah, I have noticed this too. It is not caused by my PR. The appstreamcli validator does not like the local icon that is specified in this AppData file. But the AppStream standard itself allows local icon types, so it should be fine. I think that most AppStream parsers will ignore this tag if they do not support it.

Ideally, there should not be this tag in the AppData file (it should not be needed if you specify a correct id matching the desktop file name in AppData - which is what exactly my PR does) and icons should be installed in /usr/share/icons/hicolor/*/apps/ since /usr/share/pixmaps/ is a legacy icon location, but I think (however I am not sure) that this legacy location is now also supported by most parsers/generators. Anyway, I would keep this tag in the AppData file, it hopefully should not do any harm and is probably good for legacy reasons (for older AppStream parsers that do not support the legacy icon location).

You can safely remove this tag if you change the default desktop icon installation directory to /usr/share/icons/hicolor/*/apps/ and merge my PR.

@AsciiWolf
Copy link
Contributor Author

AsciiWolf commented Jun 6, 2021

By the way, the gist you linked is also incorrect: https://gist.github.com/dave-theunsub/68faf2078253e5f832bd0e1df04c3b3d#file-clamtk-appdata-xml-L4 - You fixed the id to be a correct rDNS one, but 1. the appdata file should have the same (com.github.davetheunsub.clamtk.appdata.xml) name as rDNS id; 2. the desktop file should also have the same name or you can add <launchable type="desktop-id">clamtk.desktop</launchable> which will allow to keep the current desktop file name while using a rDNS AppData id.

@AsciiWolf
Copy link
Contributor Author

Any update?

@dave-theunsub
Copy link
Owner

Still working it, thanks.

@AsciiWolf
Copy link
Contributor Author

Please, let me know if there is anything (regarding the AppData) I could help with. Thanks!

@dave-theunsub
Copy link
Owner

Ok, in one post, let's put together everything that needs to be done. Can you put the entire file contents in one gist? And with what you wrote in #128 (comment) and #128 (comment)? I think those two parts confused me slightly. Thanks for the help.

@AsciiWolf
Copy link
Contributor Author

AsciiWolf commented Jun 25, 2021

Hi Dave,

as I said, there are two different approaches to fix the issue:

  1. The easiest way (that I personally recommend) would be merging this PR. It will just map the AppData file to the actual desktop file. The AppStream metadata won't contain any rDNS id, but it doesn't matter as most AppStream parsers/generators support this as long as a valid desktop file is found.

  2. The other possible solution as I mentioned is to use rDNS id, but fix it, so basically change <id>org.clamtk</id> to <id>com.github.davetheunsub.clamtk</id> in the AppData file and rename the file itself to com.github.davetheunsub.clamtk.appdata.xml. You will also have to add <launchable type="desktop-id">clamtk.desktop</launchable> to the AppData file to map the desktop file to AppStream metadata, otherwise you would need to rename the desktop file to com.github.davetheunsub.clamtk.desktop.

@AsciiWolf
Copy link
Contributor Author

AsciiWolf commented Jun 25, 2021

There is other small thing that I mentioned before: You are installing a desktop icon to a legacy /usr/share/pixmaps/ location instead of a proper /usr/share/icons/hicolor/128x128/apps/ one. It should work as long as you have <icon type="local" width="128" height="128">/usr/share/pixmaps/clamtk.png</icon> in the AppData file (although I really dislike hardcoded icon paths), so it's up to you if you want to fix this (by installing the icon in a non-legacy location and removing the tag from AppData file afterwards).

@dave-theunsub dave-theunsub merged commit 05af9c2 into dave-theunsub:master Jun 26, 2021
@dave-theunsub
Copy link
Owner

Ok, it's merged. I would like to keep the legacy path for now and change later.

One thing left to do: fix W: clamtk.desktop:4: cid-desktopapp-is-not-rdns clamtk.desktop, which you talked about here: #128 (comment)

@AsciiWolf
Copy link
Contributor Author

That warning is about the desktop id not being rDNS. That is what I talked about in the first point of my previous comment. It is just a warning and should not be a problem. If you would like to fix it, you would need to do all the things mentioned in the second point. But I also think that the legacy path should be good for now. Thanks!

@AsciiWolf AsciiWolf mentioned this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants