D6513: Add support for Attica tags support

[Arjen Hiemstra]

ahiemstra requested changes to this revision.
ahiemstra added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> atticaprovider.cpp:283
> +        }
> +        if(filterAcceptsDownloads && checker.filterAccepts(content.tags())) {
> +            mCachedContent.insert(content.id(), content);

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
   }
  }

> engine.cpp:131
>  
> +    d->tagFilter = group.readEntry("TagFilter").split(QStringLiteral(","));
> +    if(d->tagFilter.length() == 1 && d->tagFilter.at(0).isEmpty()) {

Considering KConfig has methods to read and write lists, wouldn't it be better to use those?

> engine.h:190
> +     * out entries marked as ghns_exclude=1. To retain this when setting a custom
> +     * filter, add "ghns_exclude!=1" as one of the filters.
> +     *

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?

> engine.h:227
> +     */
> +    void setTagFilter(QStringList filter);
> +    /**

`const QStringList&`

(Also applies to setDownloadTagFilter below)

> entryinternal.cpp:165
> +
> +void KNSCore::EntryInternal::setTags(const QStringList& tags)
> +{

Nitpick, but this doesn't match with the coding style of the declaration (`const QStringList& tags`  vs `const QStringList &tags`)

> tagsfilterchecker.cpp:43
> +    public:
> +        Validator(const QString& tag, const QString value) {
> +            m_tag = tag;

I think you mean `const QString& value` ?

> tagsfilterchecker.cpp:98
> +            QString value = filter.mid(tag.length() + 2);
> +            Validator* val = validators.value(tag, nullptr);
> +            if(val)

Wouldn't this be simpler?

  if(!validators.contains(tag)) {
    validators[tag] = new EqualityValidator(tag, "");
  }
  validators.value(tag)->m_acceptedValues << value;

REPOSITORY
  R304 KNewStuff

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

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