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=42657 REVISION DETAIL

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,

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,

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=42360 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECTED FILES

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

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,

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=41886=ignore-most#toc (thank you very much differential for being a useful tool

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=41886 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECTED FILES

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,

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

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

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

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,

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

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

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

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

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

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,

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,

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=41133 REVISION DETAIL

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

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=41131 REVISION DETAIL

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. >

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,

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,

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,

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

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 >

D6513: Add support for Attica tags support

cfeck added inline comments. INLINE COMMENTS > atticaprovider.cpp:282 > +for (const Attica::DownloadDescription : > 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=40981 REVISION DETAIL https://phabricator.kde.org/D6513 AFFECTED

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

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

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=39174

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

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 =

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

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

D6513: Add support for Attica tags support

cfeck added inline comments. INLINE COMMENTS > atticaprovider.cpp:276 > Q_FOREACH (const 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,

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=38506 REVISION DETAIL

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

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"; > +

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=38489 REVISION DETAIL

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

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())) { > +

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

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,

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

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,