search

dave-theunsub / clamtk

An easy to use, light-weight, on-demand virus scanner for Linux systems
https://gitlab.com/dave_m/clamtk/wikis/home
Other
349 stars 44 forks source link

Fix appdata.xml #128

Closed AsciiWolf closed 3 years ago

AsciiWolf commented 3 years ago

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 commented 3 years ago

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.

AsciiWolf commented 3 years ago

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

dave-theunsub commented 3 years ago

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

dave-theunsub commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Any update?

dave-theunsub commented 3 years ago

Still working it, thanks.

AsciiWolf commented 3 years ago

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

dave-theunsub commented 3 years ago

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 https://github.com/dave-theunsub/clamtk/pull/128#issuecomment-855388177 and https://github.com/dave-theunsub/clamtk/pull/128#issuecomment-855269396? I think those two parts confused me slightly. Thanks for the help.

AsciiWolf commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/dave-theunsub/clamtk/pull/128#issuecomment-855388177

AsciiWolf commented 3 years ago

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!