D18813: Filter out invalid content in lists
This revision was automatically updated to reflect the committed changes. Closed by commit R304:1f4e1a6db756: Filter out invalid content in lists (authored by leinir). REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18813?vs=51169=51501 REVISION DETAIL https://phabricator.kde.org/D18813 AFFECTED FILES src/attica/atticaprovider.cpp To: leinir, #knewstuff, ngraham Cc: ngraham, apol, kde-frameworks-devel, michaelh, ZrenBot, bruns
D18813: Filter out invalid content in lists
ngraham accepted this revision. This revision is now accepted and ready to land. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D18813 To: leinir, #knewstuff, ngraham Cc: ngraham, apol, kde-frameworks-devel, michaelh, ZrenBot, bruns
D18813: Filter out invalid content in lists
ngraham added a comment. Conceptually this makes sense, provided we can rely on `content.isValid()` returning valid results. :) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D18813 To: leinir, #knewstuff Cc: ngraham, apol, kde-frameworks-devel, michaelh, ZrenBot, bruns
D18813: Filter out invalid content in lists
leinir updated this revision to Diff 51169. leinir marked an inline comment as done. leinir added a comment. Swap the logic around a bit, makes for an easier to read patch and whatnot. REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D18813?vs=51099=51169 REVISION DETAIL https://phabricator.kde.org/D18813 AFFECTED FILES src/attica/atticaprovider.cpp To: leinir, #knewstuff Cc: apol, kde-frameworks-devel, michaelh, ZrenBot, ngraham, bruns
D18813: Filter out invalid content in lists
leinir marked an inline comment as done. leinir added inline comments. INLINE COMMENTS > apol wrote in atticaprovider.cpp:277 > Maybe it would be easier to read if we had a `if (!content.isValid()) > continue; ...`. Hmm... the patch certainly would, i'll swap that around a bit. Generally don't like negations too much if i can avoid them, hunting exclamation marks just gets tiring after a while, but yeah, smaller patch is good anyway :) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D18813 To: leinir, #knewstuff Cc: apol, kde-frameworks-devel, michaelh, ZrenBot, ngraham, bruns
D18813: Filter out invalid content in lists
apol added a comment.
I guess the patch makes sense overall.
INLINE COMMENTS
> atticaprovider.cpp:277
> +for (const Content : contents) {
> +if (content.isValid()) {
> +if (checker.filterAccepts(content.tags())) {
Maybe it would be easier to read if we had a `if (!content.isValid()) continue;
...`.
REPOSITORY
R304 KNewStuff
REVISION DETAIL
https://phabricator.kde.org/D18813
To: leinir, #knewstuff
Cc: apol, kde-frameworks-devel, michaelh, ZrenBot, ngraham, bruns
D18813: Filter out invalid content in lists
leinir created this revision. leinir added a reviewer: KNewStuff. leinir added a project: KNewStuff. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. leinir requested review of this revision. REVISION SUMMARY Prior to this change, if the ocs server being queried returned a list of entries containing invalid entries (such as happened a few days ago for the otherwise empty Akonadi email providers category), the listing function would faithfully return that list of entries verbatim, which led consumers of that list to insist there were more entries to fetch, and then try again, and again, and again. With this patch, only valid entries get forwarded as results to the consumers (be they knewstuff's own dialogues, or others, such as Discover or Peruse). This change is in KNewStuff rather than Attica, because Attica by design does not apply heuristics of any sort to its functions. That, again by design, is what KNewStuff is for. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D18813 AFFECTED FILES src/attica/atticaprovider.cpp To: leinir, #knewstuff Cc: kde-frameworks-devel, michaelh, ZrenBot, ngraham, bruns
