Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.51k stars 43 forks source link

Appstream issues: The need for plasmoid-specific metadata, three warnings, plus broken screenshot display #158

Closed sten0 closed 1 year ago

sten0 commented 1 year ago

Hi Marchus! Thank you for your continued work and support. Here is the current state of Syncthing Tray, as seen in an app store. Please note how broken URLs in the appdata result in no screenshots being presented to users.

Screenshot_20221008_142740 Screenshot_20221008_142821 Screenshot_20221008_142910

My top priority item is fulfilling metadata for the plasmoid, because currently KDE Plasma's "Discover" app store is only capable of discovering the Qt variant. The package is split in Debian (and derivatives), because users shouldn't have to install a tonne of KDE-related libraries just to use the Qt variant of Syncthing Tray :) Sure, I could create my own file, but Appstream changes must always be coordinated with upstream projects. It seems like it would be better maintain the file here, so that the appdata will be consistent between distributions that split the package for similar reasons. As a bonus, having two files means that one can clearly differentiate how the Qt and the Plasmoid variants look different by using nonoverlapping screenshot sets for each. Before doing this, I think it would be a good idea to at least take care of the rdns issue.

asv-cid-desktopapp-is-not-rdns needs to be fixed. While Plasma Discover is able to find Syncthing Tray (Qt variant), noncompliance with this spec means that GNOME Software and other app stores may not be able to find it.

asv-release-time-missing. IIRC appstream-util can be used to inject date, which is preferred over the timestamp property. Alternatively, there's almost certainly a CMake method to get an ISO 8601 format date, which is the preferred format.

asv-content-rating-missing is allegedly required for GNOME Software, which is what I think Ubuntu is using these days.

https://appstream.debian.org/sid/main/issues/syncthingtray.html https://freedesktop.org/software/appstream/docs/chap-Quickstart.html

appstream-util validate can be used locally to check one's work, and it also has some nice convenience/error-prevention functions.

Martchus commented 1 year ago

To summarize, the following needs to be done:


Sure, I could create my own file, but Appstream changes must always be coordinated with upstream projects.

That makes sense. If you want to make changes to the appstream file upstream, note that all meta-data is defined at the beginning of the CMake files of my projects and then used in various places (about dialogs, --help, *.desktop files, Windows-executable metadata and so on). The appstream file is one of those places and generated from a template found in cpp-utilities to avoid having the same boilerplate in all of my projects. Implementing most of these points likely requires refactoring/adjusting code cpp-utilities (which should not be a big deal, I'm just mentioning it).

sten0 commented 1 year ago

That's a fair summary.

To summarize, the following needs to be done:

  • [ ] Make URLs compatible with whatever format is required by Discover and GNOME Software

This isn't Discover and GNOME Software-specific. For anything not on Github, and anything besides a web browser, a screenshot URL should return type=image, not a web page.

eg: https://github.com/Martchus/syncthingtray/blob/master/tray/resources/screenshots/webview-dark.png to https://raw.githubusercontent.com/Martchus/syncthingtray/master/tray/resources/screenshots/webview-dark.png

Were it supported, one wouldn't want to load the Github website to show a screenshot in any app store... Think about this on your phone, or for the average desktop user. The thumbnail should never show Github UI.

  • [ ] Add appstream file for Plasmoid as well
  • [ ] Figure out what asv-cid-desktopapp-is-not-rdns means and update appstream metadata accordingly

Here's an example https://code.videolan.org/videolan/vlc/-/commit/f0a4dd5cd3f4927e947c4f9ecbe03868966ebd4a

FYI, these RDNS names are not without precedent in other OS: https://digitalai-dev.zoominsoftware.io/en-US/bundle/app-management/page/Manage-App-Identifiers_10403643600.html

Syncthing Tray should be "io.github.martchus.syncthing_tray"

  • [ ] Figure out a way to add a release date to avoid asv-release-time-missing without hard-coding any dates within the sources

The problem is where to source the release date for the project. I'd be inclined to use a Github pre-tag hook to insert the current date in ISO 8601 format somewhere into the sources--probably into a per-project CMake file, for any projects that need this. I'm guessing you're prefer that to running a tagged-release script.

  • [ ] Figure out what asv-content-rating-missing means and update appstream metadata accordingly

Please consult the appstream.debian.org link I provided, read to the bottom, and follow the link there; it explains everything. Also, this functionality does nothing until registering there.

  • [ ] Add a check within the buildsystem/testsuite to invoke appstream-util validate for validating appstream files

:)

Sure, I could create my own file, but Appstream changes must always be coordinated with upstream projects.

That makes sense. If you want to make changes to the appstream file upstream, note that all meta-data is defined at the beginning of the CMake files of my projects and then used in various places (about dialogs, --help, *.desktop files, Windows-executable metadata and so on). The appstream file is one of those places and generated from a template found in cpp-utilities to avoid having the same boilerplate in all of my projects. Implementing most of these points likely requires refactoring/adjusting code cpp-utilities (which should not be a big deal, I'm just mentioning it).

Yes, I noticed that ;) I guess cpp-utilities is arguably the true source of the bug because @META_APP_URL@ evaluates to a URL consumable exclusively by embedded web engines and browsers.

For another approach, here's a website you can use to generate correct app data files that you can compare the output of your custom solution to:

https://www.freedesktop.org/software/appstream/metainfocreator

sten0 commented 1 year ago

Nicholas D Steeves @.***> writes:

  • [ ] Figure out what asv-cid-desktopapp-is-not-rdns means and update appstream metadata accordingly

Here's an example https://code.videolan.org/videolan/vlc/-/commit/f0a4dd5cd3f4927e947c4f9ecbe03868966ebd4a

FYI, these RDNS names are not without precedent in other OS: https://digitalai-dev.zoominsoftware.io/en-US/bundle/app-management/page/Manage-App-Identifiers_10403643600.html

Syncthing Tray should be "io.github.martchus.syncthing_tray"

https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#spec-component-filespec

Martchus commented 1 year ago

Oh, right. I wasn't really aware that these valid URLs aren't pointing to just the image so I thought this complaint about broken URLs must be about the URLs itself (and not the content they're referring to). Of course pointing to a web page doesn't make any sense.

I'll think about the other points later.

Martchus commented 1 year ago

Syncthing Tray should be "io.github.martchus.syncthing_tray"

Is the ID really supposed to change if I would change the source code host? Note that this project has no real website and no registered domain. For an ID I'd probably rather make something up than using something that's maybe going to change.

sten0 commented 1 year ago

Martchus @.***> writes:

Syncthing Tray should be "io.github.martchus.syncthing_tray"

Is the ID really supposed to change if I would change the source code host?

Yes. It's a similar concept to FQDNs. The user-facing token is of course "name" and not "id", so it's not like this effects branding. This is a system integration bug.

Note that this project has no real website and no registered domain. For an ID I'd probably rather make something up than using something that's maybe going to change.

"The ID must follow a reverse-DNS scheme, consisting of {tld}.{vendor}.{product}, for example org.kde.gwenview or com.hugski.colorhug2. Ownership of {vendor}.{tld} in the domain name system guarantees uniqueness of IDs."

https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-id-generic https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Addon.html (which refers to tag-id-generic)

I suspect that the Qt tray application will end up being type desktop, while the plasmoid will be type addon. There is currently a small amount of ambiguity in the "desktop" definition, due to software that uses an older spec of Appstream.

If the site of the project changes, the RDNS changes, thus the id must change. There is a mechanism for maintaining continuity with the old id that you will discover when you read the docs.

Martchus commented 1 year ago

I implemented certain changes. Now appstreamcli has nothing to complain about anymore. If the tool is available at build time the build system will now invoke it to validate the appstream files as part of the test suite.

That doesn't mean this issue is completely resolved, though. For instance, the Plasmoid is still treated as a normal application but as you've already mentioned it should likely be an add-on. I suppose also the license isn't as accurate as it could be (considering it is actually GPL-2-or-later). Maybe the Dolphin integration also deserves its own appstream file but I'm not sure how this will work considering it is not an extra split package (in Debian packaging at least).

sten0 commented 1 year ago

Hi @Martchus, Thanks for you work!

The appstream stuff is pretty cool, I think, because no other app store system uses a federated model. An end-user user can click on a appstream://foo URI from an article they read about on the web (or from your Github project page), the mimetype is resolved by one of several handlers, which downloads a copy of the package in the OS's native format (deb, RPM, etc), from the distribution's repositories. Assuming the distribution is doing its job, the user gets a cryptographically authenticated, well-tested version that doesn't have any integration bugs. Once installed, the package automatically upgrades as new versions become available.

This is why an RDNS id is important. Also note that all KDE Plasma applications use an RDNS id--it's not just a GNOME thing.

I implemented certain changes. Now appstreamcli has nothing to complain about anymore. If the tool is available at build time the build system will now invoke it to validate the appstream files as part of the test suite.

Cool! Please note that appstreamcli may not reveal GNOME problems that appstream-util validate will identify; this is likely to only affect the Qt tray app.

That doesn't mean this issue is completely resolved, though.

Agreed, because https://github.com/Martchus/cpp-utilities/commit/d05677e3b5cb956086d8d0ab99f9830a8b71eadc doesn't provide a release date; although, this would be a nice solution for use in a release script and/or a "git tag" pre-hook, that then dumps the output into a file that is then committed and included in the tree :) When called during the build, cpp-utilities would then parse that project-specific file.

Unfortunately using an mtime-based approach that is called at build-time guaranties non-reproducibility.

Building, will update the mtime for ./ (even after cleaning up), and then building a 2nd time will claim date -I is the release date. This is wrong, because this field is for the upstream release date, or even snapshot date, not build timestamp.

Merging an upstream tag tags, then making a packaging-related change to debian/*, PKGBUILD, syncthingtray.spec, etc. will update the "release date". Further packaging-related changes will do the same thing. This is wrong, because this field is for the upstream release date, not packaging date.

On the upside, I could solve the reproducibility issue by taking the entry point you created, replacing your mtime-based approach with a the date from the current Debian changelog footer (static release date)... but this is wrong, because this field is for the upstream release date, not Debian version date.

For instance, the Plasmoid is still treated as a normal application but as you've already mentioned it should likely be an add-on.

From what I can tell this would make Syncthing Tray additionally discoverable from the "Download New Plasma Widgets" interface :)

I suppose also the license isn't as accurate as it could be (considering it is actually GPL-2-or-later).

Indeed. GPL-2 -> GPL-2-or-later is important for Syncthing Tray, because Syncthing Tray is only distribuable under the GPL-3 due to the combination of licenses in effect.

I think you were right to choose a maximally permissive license for the app data file itself, by the way; although, I confess that I don't fully understand why this convention is so useful... My guess is maximally permissive licences remove legal headaches for appstream data scraping, processing, caching, syndicating, etc.

Maybe the Dolphin integration also deserves its own appstream file but I'm not sure how this will work considering it is not an extra split package (in Debian packaging at least).

I can't find a "Download New Dolphin Plugins" interface, so I'm not sure how much value it would add. My interpretation is that KDE Plasma users "get a button" for the KDE Plasma version, non-KDE Plasma users "get a button" for the Qt version they'll be happy with, as well as that appstream is made for less technical users, and less technical users are confused by micropackaging.

Martchus commented 1 year ago

I implemented some further changes:


Unfortunately using an mtime-based approach that is called at build-time guaranties non-reproducibility.

It would generally do; unless one is tampering with the sources (or at least their timestamps).

Building, will update the mtime for ./ (even after cleaning up), and then building a 2nd time will claim date -I is the release date. This is wrong, because this field is for the upstream release date, or even snapshot date, not build timestamp.

Since I ditched the mtime-based approach this shouldn't be an immediate issue - however, I strongly recommend doing an out-of-source-tree-build. That is the only tested build configuration.

I don't fully understand why this convention is so useful..

Me neither; I was indeed merely following a convention.

I can't find a "Download New Dolphin Plugins" interface, so I'm not sure how much value it would add.

That's true. If it'll just show up in Discover when one searches for Syncthing that would be some value. (Right now it'll supposedly be pulled in automatically when one is installing the Plasmoid's Debian package. That should be ok although maybe not expected by some users.)

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Martchus commented 1 year ago

Not sure what's still missing.

sten0 commented 1 year ago

Looks good to me now :) Currently Apper is in a bad state, so I can't confirm with a GUI, but that should resolve within the month and I'll retest then. If necessary I'll reopen if there are any issues, but I don't think there will be any.