<table><tr><td style="">leinir marked 7 inline comments as done.<br />leinir added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D6513">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6513#inline-75451">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">atticaprovider.cpp:283</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if(!checker.filterAccepts(content.tags()) {
  continue
}

foreach download {
 if(downloadsChecker.filterAccepts(download.tags) {
   add download
   break
 }
}</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6513#inline-75453">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">engine.cpp:131</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Considering KConfig has methods to read and write lists, wouldn't it be better to use those?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6513#inline-75457">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">engine.h:190</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Would it make sense to add an <tt style="background: #ebebeb; font-size: 13px;">addTagFilter</tt> 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 <tt style="background: #ebebeb; font-size: 13px;">ghns_exclude!=1</tt> 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.</p>

<p style="padding: 0; margin: 8px;">Maybe even completely separate the ghns_exclude filter from this setter and make a separate "also show excluded content" switch that removes the filter?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D6513#inline-75466">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">tagsfilterchecker.cpp:98</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Wouldn't this be simpler?</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if(!validators.contains(tag)) {
  validators[tag] = new EqualityValidator(tag, "");
}
validators.value(tag)->m_acceptedValues << value;</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R304 KNewStuff</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6513">https://phabricator.kde.org/D6513</a></div></div><br /><div><strong>To: </strong>leinir, KNewStuff, apol, KDE Store, whiting, ahiemstra<br /><strong>Cc: </strong>ngraham, ahiemstra, kde-frameworks-devel, KNewStuff, michaelh, ZrenBot, bruns<br /></div>