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
