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
