D6513: Add support for Attica tags support

[Dan Leinir Turthra Jensen]

leinir marked 7 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in atticaprovider.cpp:283
> Wouldn't it be simpler to first check if the content should be considered at all, and then check if one of the downloads is valid? So something like:
> 
>   if(!checker.filterAccepts(content.tags()) {
>     continue
>   }
>   
>   foreach download {
>    if(downloadsChecker.filterAccepts(download.tags) {
>      add download
>      break
>    }
>   }

Hmm... It's not quite so simple as that (it's just just a straight foreach, there's also the preselection logic, which is what that ternary is for), but the fact that wasn't clear to you while reading does suggest it was too convoluted. I've remodelled it somewhat, and i think the new logic is perhaps more easily readable as well, which is not a bad thing. Thanks :)

> ahiemstra wrote in engine.cpp:131
> Considering KConfig has methods to read and write lists, wouldn't it be better to use those?

It would indeed, and also removes the frankly distasteful jiggling about on the next line because QString::split returns a list with a single empty item if the string you're trying to split is zero length ;)

> ahiemstra wrote in engine.h:190
> Would it make sense to add an `addTagFilter` method or similar for convenience that appends instead of replaces? Or maybe make the setter a bit more intelligent so it is harder to forget adding the `ghns_exclude!=1` filter? Right now, if I need to do anything with the tag filters, I will constantly need to remember to add the ghns_exclude filter.
> 
> Maybe even completely separate the ghns_exclude filter from this setter and make a separate "also show excluded content" switch that removes the filter?

Hmm... Yes, when using this from code, rather than just using it with knsrc files, that's not a bad idea at all. Incidentally, something we'll be needing when implementing e.g. support for filtering AppImages by architecture (which can't really be put into the knsrc files, unless we come up with some kind of placeholder for that, which i'm not against, but feel is perhaps a little overly magic).

> ahiemstra wrote in tagsfilterchecker.cpp:98
> Wouldn't this be simpler?
> 
>   if(!validators.contains(tag)) {
>     validators[tag] = new EqualityValidator(tag, "");
>   }
>   validators.value(tag)->m_acceptedValues << value;

Hmm, not quite, that would create a ghost entry in the list... However, allowing the validator to be created without values to work on would simplify things, so yeah, just going to do that :)

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D6513

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra
Cc: ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180726/5b4eed93/attachment-0001.html>