D6513: Add support for Attica tags support

This revision was automatically updated to reflect the committed changes. Closed by commit R304:158ba4e2420b: Add support for Attica tags support (authored by leinir). REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=42360&id=42657 REVISION DETAIL h

D6513: Add support for Attica tags support

cfeck accepted this revision. This revision is now accepted and ready to land. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, cfeck Cc: rikmills, dfaure, cfeck, mlaurent, ngraham, ah

D6513: Add support for Attica tags support

leinir added a comment. Sorry for pinging again, but i'd really quite like to not have to wait (yet) another cycle for this to get out... REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, d

D6513: Add support for Attica tags support

leinir updated this revision to Diff 42360. leinir added a comment. Address @cfeck's comments REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=41886&id=42360 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECTED FILES autotests/knewstuffe

D6513: Add support for Attica tags support

leinir marked 3 inline comments as done. leinir added a comment. In D6513#332106 , @cfeck wrote: > Using a hash to map public to private class is a good idea. The only thing I'm not sure about is if there is a memory leak. I would think that havin

D6513: Add support for Attica tags support

cfeck added a comment. Using a hash to map public to private class is a good idea. The only thing I'm not sure about is if there is a memory leak. I would think that having only pointers in the hash will never free the referenced values. INLINE COMMENTS > provider.cpp:44 > +if (!ret) >

D6513: Add support for Attica tags support

leinir added a comment. Sorry for pinging again, but i'd quite like this to get in, bic fixed and all that, before the next release... REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfau

D6513: Add support for Attica tags support

leinir added a comment. For ease of re-reviewing, the relevant bits of the patch (the bits that didn't already get the tickmark) can be seen on their own like so: https://phabricator.kde.org/D6513?vs=41133&id=41886&whitespace=ignore-most#toc (thank you very much differential for being a

D6513: Add support for Attica tags support

leinir updated this revision to Diff 41886. leinir added a comment. Forgot `@since 5.51` in the new api in Provider.h REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=41815&id=41886 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECTED FILE

D6513: Add support for Attica tags support

leinir requested review of this revision. leinir added a comment. Previous patch had BIC issues - new patch attempts to address this, but as a result (of course) requires another round of review. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #kn

D6513: Add support for Attica tags support

leinir updated this revision to Diff 41815. leinir added a comment. After discovering that the previous version of this patch had introduced a binary incompatibility, and panicking momentarily that this had been done in a release, Jonathan reverted it (as i was away for a bit and unable to do

D6513: Add support for Attica tags support

leinir reopened this revision. leinir added a comment. This revision is now accepted and ready to land. In D6513#326519 , @rikmills wrote: > In D6513#326071 , @ngraham wrote: > > > FYI, this included an

D6513: Add support for Attica tags support

rikmills added a comment. In D6513#326071 , @ngraham wrote: > FYI, this included an ABI change to `SearchRequest` that broke Discover: https://bugs.kde.org/show_bug.cgi?id=398412 I see this revision has been reverted in master in 293ae244

D6513: Add support for Attica tags support

ngraham added a comment. FYI, this included an ABI change to `SearchRequest` that broke Discover: https://bugs.kde.org/show_bug.cgi?id=398412 REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlauren

D6513: Add support for Attica tags support

leinir added a comment. In D6513#324915 , @ngraham wrote: > In D6513#324458 , @leinir wrote: > > > In D6513#324419 , @ngraham wrote: > > > > > So I see `gh

D6513: Add support for Attica tags support

ngraham added a comment. In D6513#324458 , @leinir wrote: > In D6513#324419 , @ngraham wrote: > > > So I see `ghns_exclude` over at store.kde.org, but it doesn't feel quite right to check that box next

D6513: Add support for Attica tags support

leinir added a comment. In D6513#324419 , @ngraham wrote: > In D6513#322167 , @leinir wrote: > > > In D6513#322078 , @ngraham wrote: > > > > > So is this e

D6513: Add support for Attica tags support

ngraham added a comment. In D6513#322167 , @leinir wrote: > In D6513#322078 , @ngraham wrote: > > > So is this enough for people to start tagging KDE4 content as such? Or is anything else still require

D6513: Add support for Attica tags support

leinir added a comment. In D6513#322078 , @ngraham wrote: > So is this enough for people to start tagging KDE4 content as such? Or is anything else still required before that capability lands? In short, this is basically enough :) When peo

D6513: Add support for Attica tags support

ngraham added a comment. So is this enough for people to start tagging KDE4 content as such? Or is anything else still required before that capability lands? REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ah

D6513: Add support for Attica tags support

leinir added inline comments. INLINE COMMENTS > dfaure wrote in tagsfilterchecker.cpp:103 > Yes. Great, thank you :) Yeah, going to have to nab a copy of that for a readthrough :) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #k

D6513: Add support for Attica tags support

This revision was automatically updated to reflect the committed changes. Closed by commit R304:2ad3e66d81b6: Add support for Attica tags support (authored by leinir). REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=41131&id=41133 REVISION DETAIL h

D6513: Add support for Attica tags support

dfaure accepted this revision. dfaure added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > leinir wrote in tagsfilterchecker.cpp:103 > Scott Meyers' book? (might need to grab a copy of that, then :) ) Yes. REPOSITORY R304 KNewStuff REVISION DETAIL https

D6513: Add support for Attica tags support

leinir updated this revision to Diff 41131. leinir marked 2 inline comments as done. leinir added a comment. Address @dfaure's comments REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=41034&id=41131 REVISION DETAIL https://phabricator.kde.org/D6

D6513: Add support for Attica tags support

leinir marked 6 inline comments as done. leinir added inline comments. INLINE COMMENTS > dfaure wrote in tagsfilterchecker.cpp:103 > .insert(tag, val) is raster, see Effective STL Scott Meyers' book? (might need to grab a copy of that, then :) ) > dfaure wrote in CMakeLists.txt:4 > Qick ? :) O

D6513: Add support for Attica tags support

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > tagsfilterchecker.cpp:33 > +{ > +qDeleteAll(validators.values()); > +} Unnecessarily slow. Remove the .values() call. > tagsfilterchecker.cpp

D6513: Add support for Attica tags support

mlaurent accepted this revision. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, cfeck Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, Zr

D6513: Add support for Attica tags support

cfeck added a comment. @dfaure, @mlaurent okey? REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, cfeck Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knew

D6513: Add support for Attica tags support

cfeck accepted this revision. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent, dfaure, cfeck Cc: dfaure, cfeck, mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenB

D6513: Add support for Attica tags support

leinir updated this revision to Diff 41034. leinir marked an inline comment as done. leinir added a comment. More codestyle fixes. I also notice some which were there before this work, but fixing that seems distinctly out of scope for this patch... REPOSITORY R304 KNewStuff CHANGES SINCE L

D6513: Add support for Attica tags support

leinir marked 16 inline comments as done. leinir added a comment. i guess uncrustify isn't a magic bullet either, eh? ;) Thanks for the findings! INLINE COMMENTS > cfeck wrote in khotnewstuff_test.cpp:66 > Any rationale for using `fromLocal8Bit()` for fixed strings? If, for whatever > reaso

D6513: Add support for Attica tags support

cfeck added inline comments. INLINE COMMENTS > atticaprovider.cpp:282 > +for (const Attica::DownloadDescription &dli : > content.downloadUrlDescriptions()) { > +if(downloadschecker.filterAccepts(dli.tags())) { > +filterAcceptsDownloads

D6513: Add support for Attica tags support

leinir updated this revision to Diff 40981. leinir added a comment. Uncrustify-kf5 things as requested, hope this is better! REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=39174&id=40981 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECT

D6513: Add support for Attica tags support

cfeck requested changes to this revision. cfeck added a comment. This revision now requires changes to proceed. I still see many coding style issues. If you cannot run it through the formatting scripts, I will review it later. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator

D6513: Add support for Attica tags support

leinir added a comment. Welcome back from Akademy - sorry for being a pest, but it'd be really nifty if we could get this and it's dependent in D6512 through before 5.50... :) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513

D6513: Add support for Attica tags support

leinir updated this revision to Diff 39174. leinir marked 3 inline comments as done. leinir added a comment. Address the various comments by dfaure, cfeck and ngraham - with thanks! REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=38506&id=39174 RE

D6513: Add support for Attica tags support

leinir marked 17 inline comments as done. leinir added inline comments. INLINE COMMENTS > dfaure wrote in engine.cpp:133 > That's out of bounds, if tagFilter is empty!! It will assert. > > You meant append or push_back, I think. i did indeed! > dfaure wrote in engine.cpp:138 > !? What's the po

D6513: Add support for Attica tags support

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. @since 5.50 is missing on any new *public* API (methods or classes). (This won't get into 5.49 which I just tagged) INLINE COMMENTS > engine.cpp:132 > +d->tagFilter = gro

D6513: Add support for Attica tags support

cfeck added inline comments. INLINE COMMENTS > ngraham wrote in atticaprovider.cpp:280 > Also isn't Qt `foreach` deprecated anyway? > https://www.kdab.com/goodbye-q_foreach/ Ah, yes. `Q_FOREACH`, or C++11 `for (x : y)` if you see it does not cause unneeded detaches. REPOSITORY R304 KNewStuf

D6513: Add support for Attica tags support

ngraham added inline comments. INLINE COMMENTS > cfeck wrote in atticaprovider.cpp:280 > Space after `foreach`. No space after `&`. Also isn't Qt `foreach` deprecated anyway? https://www.kdab.com/goodbye-q_foreach/ REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D651

D6513: Add support for Attica tags support

cfeck added inline comments. INLINE COMMENTS > atticaprovider.cpp:276 > Q_FOREACH (const Content &content, contents) { > -mCachedContent.insert(content.id(), content); > -entries.append(entryFromAtticaContent(content)); > +if(checker.filterAccepts(content.tags())) { >

D6513: Add support for Attica tags support

leinir added a comment. @mlaurent quick ping? :) Also in general, sorry, would really like to get this merged in asap - i realise it's the summer holidays for a lot of people ;) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol

D6513: Add support for Attica tags support

ahiemstra accepted this revision. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, bruns

D6513: Add support for Attica tags support

leinir updated this revision to Diff 38506. leinir marked an inline comment as done. leinir added a comment. Address mlaurent's comments REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=38489&id=38506 REVISION DETAIL https://phabricator.kde.org/D

D6513: Add support for Attica tags support

leinir marked 3 inline comments as done. leinir added inline comments. INLINE COMMENTS > mlaurent wrote in entryinternal.cpp:495 > qCWarning(KNEWSTUFF_CORE) ? I was just adding in a bit of verbosity to the output... but yes, makes sense to fix that while i'm there anyway :) REPOSITORY R304 K

D6513: Add support for Attica tags support

mlaurent requested changes to this revision. mlaurent added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > entryinternal.cpp:495 > if (reader.name() != QLatin1String("stuff")) { > -qWarning() << "Parsing Entry from invalid XML"; > +qWarning(

D6513: Add support for Attica tags support

leinir updated this revision to Diff 38489. leinir marked 4 inline comments as done. leinir added a comment. Address ahiemstra's comments REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6513?vs=37047&id=38489 REVISION DETAIL https://phabricator.kde.org/

D6513: Add support for Attica tags support

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(!

D6513: Add support for Attica tags support

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

D6513: Add support for Attica tags support

leinir added a comment. Pingaling, it would be super handy if we could get this merged soon :) It's needed to support the moderation happening on the KDE Store, and also needed for better handling of specific types of content like e.g. comic books in Peruse. Thanks! :D REPOSITORY R304 KNe

D6513: Add support for Attica tags support

leinir added a comment. Test tool using the default settings, and with QT__LOGGING_RULES="org.kde.knewstuff.*=true" F5998853: image.png REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D6513 To: leinir, #knewstuff, apol, #kde

D6513: Add support for Attica tags support

leinir updated this revision to Diff 37047. leinir edited the summary of this revision. leinir edited the test plan for this revision. leinir added a comment. Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. - add test tool - add support for filter

D6513: Add support for Attica tags support

leinir created this revision. leinir added a project: KNewStuff. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY This is to add support for the new tags support in Attica found in https://phabricator.kde.org/D6512, whic