Differential D6513 Diff 41815 src/attica/atticaprovider.cpp

Changeset View

Standalone View

View Options

src/attica/atticaprovider.cpp

Context not available.
18#include "atticaprovider_p.h" 18​#include "atticaprovider_p.h"
19 19​
20#include "question.h" 20​#include "question.h"
21​#include "tagsfilterchecker.h"
21 22​
22#include <QCollator> 23​#include <QCollator>
23#include <knewstuffcore_debug.h> 24​#include <knewstuffcore_debug.h>
Context not available.
270 Content::List contents = listJob->itemList(); 271​ Content::List contents = listJob->itemList();
271 272​
272 EntryInternal::List entries; 273​ EntryInternal::List entries;
274​ TagsFilterChecker checker(tagFilter());
275​ TagsFilterChecker downloadschecker(downloadTagFilter());
273 Q_FOREACH (const Content &content, contents) { 276​ Q_FOREACH (const Content &content, contents) {
274 mCachedContent.insert(content.id(), content); 277​ if (checker.filterAccepts(content.tags())) {
cfeckUnsubmitted
Done

Coding style: Space after if (multiple occurences).

cfeck: Coding style: Space after `if` (multiple occurences).
275 entries.append(entryFromAtticaContent(content)); 278​ bool filterAcceptsDownloads = true;
279​ if (content.downloads() > 0) {
280​ filterAcceptsDownloads = false;
cfeckUnsubmitted
Done

Space after foreach. No space after &.

cfeck: Space after `foreach`. No space after `&`.
ngrahamUnsubmitted
Done

Also isn't Qt foreach deprecated anyway? https://www.kdab.com/goodbye-q_foreach/

ngraham: Also isn't Qt `foreach` deprecated anyway? https://www.kdab.com/goodbye-q_foreach/
cfeckUnsubmitted
Done

Ah, yes. Q_FOREACH, or C++11 for (x : y) if you see it does not cause unneeded detaches.

cfeck: Ah, yes. `Q_FOREACH`, or C++11 `for (x : y)` if you see it does not cause unneeded detaches.
281​ for (const Attica::DownloadDescription &dli : content.downloadUrlDescriptions()) {
282​ if (downloadschecker.filterAccepts(dli.tags())) {
cfeckUnsubmitted
Done

Missing Space after if

cfeck: Missing Space after `if`
283​ filterAcceptsDownloads = true;
ahiemstraUnsubmitted
Done

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
 }
}
ahiemstra: Wouldn't it be simpler to first check if the content should be considered at all, and then…
leinirAuthorUnsubmitted
Done

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 :)

leinir: Hmm... It's not quite so simple as that (it's just just a straight foreach, there's also the…
284​ break;
285​ }
286​ }
287​ }
288​ if (filterAcceptsDownloads) {
289​ mCachedContent.insert(content.id(), content);
290​ entries.append(entryFromAtticaContent(content));
291​ } else {
cfeckUnsubmitted
Done

Frameworks coding style:

if (...) {
    ...
} else {
    ...
}
cfeck: Frameworks coding style: if (...) { ... } else { ... }
292​ qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << content.name() << "on download filter" << downloadTagFilter();
293​ }
294​ } else {
295​ qCDebug(KNEWSTUFFCORE) << "Filter has excluded" << content.name() << "on entry filter" << tagFilter();
296​ }
cfeckUnsubmitted
Done

Coding style:

if (...) {
    ...
} else {
    ...
}
cfeck: Coding style: if (...) { ... } else { ... }
276 } 297​ }
277 298​
278 qCDebug(KNEWSTUFFCORE) << "loaded: " << mCurrentRequest.hashForRequest() << " count: " << entries.size(); 299​ qCDebug(KNEWSTUFFCORE) << "loaded: " << mCurrentRequest.hashForRequest() << " count: " << entries.size();
Context not available.
482 entry.setSummary(content.description()); 503​ entry.setSummary(content.description());
483 entry.setShortSummary(content.summary()); 504​ entry.setShortSummary(content.summary());
484 entry.setChangelog(content.changelog()); 505​ entry.setChangelog(content.changelog());
506​ entry.setTags(content.tags());
485 507​
486 entry.clearDownloadLinkInformation(); 508​ entry.clearDownloadLinkInformation();
487 QList<Attica::DownloadDescription> descs = content.downloadUrlDescriptions(); 509​ QList<Attica::DownloadDescription> descs = content.downloadUrlDescriptions();
Context not available.
494 info.id = desc.id(); 516​ info.id = desc.id();
495 info.size = desc.size(); 517​ info.size = desc.size();
496 info.isDownloadtypeLink = desc.type() == Attica::DownloadDescription::LinkDownload; 518​ info.isDownloadtypeLink = desc.type() == Attica::DownloadDescription::LinkDownload;
519​ info.tags = desc.tags();
497 entry.appendDownloadLinkInformation(info); 520​ entry.appendDownloadLinkInformation(info);
498 } 521​ }
499 522​
Context not available.