appstream icon indicating copy to clipboard operation
appstream copied to clipboard

Add an <internet/> kind to requires/recommends/supports

Open pwithnall opened this issue 3 years ago • 3 comments

See the commit messages. This adds a new <internet/> kind to requires/recommends/supports, both in the spec and in libappstream. It includes unit tests.

Fixes: #343

pwithnall avatar Jul 23 '22 19:07 pwithnall

I’m going to be away for a few months from basically now, so won’t be able to respond to review feedback on this after tomorrow or so. Please feel free to update/change/drop/close/adapt the MR in whatever way seems most appropriate.

One thing you might want to think about in particular is the bandwidth_mbitps attribute, which was more of a question mark in the discussion on #343 than the rest of the new bits in the spec. It should be fairly straightforward to drop bandwidth_mbitps from this MR if you don’t think it’s ready to add that now. It can always be added to the spec in future if people end up thinking that’s a good idea.

pwithnall avatar Jul 23 '22 19:07 pwithnall

Looks really good, all the comments I had are very minor. The only thing I think we need to answer is the "app that can use internet but doesn't need to" case. I initially thought of dropping the bandwidth_mbitps stuff, but tbh I think it looks fine and I would be okay with adding it now if you/your apps/your client would use it. If it has zero users, then I would be a lot more hesitant with including it.

All in all, thank you a lot for the really good patch and sorry for the delayed reply as well as causing a merge conflict by converting the AsRelation code to using the XML helpers ;-) (they don't do much except for making the C code easier to read).

ximion avatar Aug 10 '22 15:08 ximion

You should probably explicit mention that using it multiple times is supported. Something like this:

<requires>
  <internet>first-run</internet>
</requires>
<recommends>
  <internet>always</internet>
</recommends>

This is to avoid confusion in the implementations.

JakobDev avatar Aug 10 '22 21:08 JakobDev

Closing in favour of https://github.com/ximion/appstream/pull/425

ximion avatar Aug 20 '22 21:08 ximion