Add support for Attica tags support
ClosedPublic

Authored by leinir on Jul 5 2017, 12:31 PM.

Details

Summary

This is to add support for the new tags support in Attica found in D6512, which is based on the proposal in T6133 to add tags support in the next version of OCS.

This introduces filtering which is applied onto any entry before handing it to the framework user. Filters can be set either via knsrc files, or through the Engine API. A default filter is set to ensure that if the server suggests certain items should not be shown, those entries will be hidden from the user (this is already supported by the KDE Store, and this ensures we support that in our client side).

It further adds a test tool to more easily perform testing on KNewStuff's functionality (originally based on the old knewstuff2 tester, but now with more functionality, and better visual feedback).

As it depends on D6512, this patch should consequently not be merged before that one is.

Test Plan

Run the test tool and ensure filters are applied correctly (the test entry data requests two of the entries be filtered out).

Diff Detail

Repository
R304 KNewStuff
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes

@since 5.50 is missing on any new *public* API (methods or classes).

(This won't get into 5.49 which I just tagged)

src/core/engine.cpp
132

use isEmpty()

133

That's out of bounds, if tagFilter is empty!! It will assert.

You meant append or push_back, I think.

137

isEmpty()

138

!? What's the point in clearing a list that is already empty?

This revision now requires changes to proceed.Aug 4 2018, 11:20 AM
leinir marked 17 inline comments as done.Aug 6 2018, 9:59 AM
leinir added inline comments.
src/core/engine.cpp
133

i did indeed!

138

None at all - that would be a bit of leftovers, thanks for spotting that :)

tests/khotnewstuff_test.knsrc.in
2

No, i guess we don't, really... gone :)

leinir updated this revision to Diff 39174.Aug 6 2018, 10:00 AM
leinir marked 3 inline comments as done.

Address the various comments by dfaure, cfeck and ngraham - with thanks!

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

cfeck requested changes to this revision.Sep 4 2018, 12:04 PM

I still see many coding style issues. If you cannot run it through the formatting scripts, I will review it later.

This revision now requires changes to proceed.Sep 4 2018, 12:04 PM
leinir updated this revision to Diff 40981.Sep 4 2018, 12:47 PM

Uncrustify-kf5 things as requested, hope this is better!

cfeck added inline comments.Sep 4 2018, 9:33 PM
src/attica/atticaprovider.cpp
282

Missing Space after if

296

Coding style:

if (...) {
    ...
} else {
    ...
}
src/core/engine.h
226

-> 5.51 (everywhere)

src/staticxml/staticxmlprovider.cpp
266

again

270

again

tests/khotnewstuff_test.cpp
42

<cstdio>

49

nullptr

66

Any rationale for using fromLocal8Bit() for fixed strings? If, for whatever reason, you do not want to use QStringLiteral or QLatin1String, please use fromUtf8(). This is what we ship for source files.

190

& -> &

202

* -> *

211

Missing space

222

* -> *

231

again

leinir marked 16 inline comments as done.Sep 5 2018, 9:51 AM

i guess uncrustify isn't a magic bullet either, eh? ;) Thanks for the findings!

tests/khotnewstuff_test.cpp
66

None apart from this being a modified version of an old test which used that function rather than the proper one. Fixed :)

leinir updated this revision to Diff 41034.Sep 5 2018, 9:57 AM
leinir marked an inline comment as done.

More codestyle fixes. I also notice some which were there before this work, but fixing that seems distinctly out of scope for this patch...

mlaurent accepted this revision.Sep 6 2018, 5:32 AM
dfaure requested changes to this revision.Sep 6 2018, 8:07 PM
dfaure added inline comments.
src/core/tagsfilterchecker.cpp
34 ↗(On Diff #39174)

Unnecessarily slow. Remove the .values() call.

47 ↗(On Diff #39174)

!value.isEmpty()

Or isNull()?

103 ↗(On Diff #39174)

Consider QString::null deprecated. Use QString().

104 ↗(On Diff #39174)

.insert(tag, val) is raster, see Effective STL

114 ↗(On Diff #39174)

Insert

tests/CMakeLists.txt
4–6 ↗(On Diff #39174)

Qick ? :)

This revision now requires changes to proceed.Sep 6 2018, 8:07 PM
leinir marked 6 inline comments as done.Sep 7 2018, 7:08 AM
leinir added inline comments.
src/core/tagsfilterchecker.cpp
104 ↗(On Diff #39174)

Scott Meyers' book? (might need to grab a copy of that, then :) )

tests/CMakeLists.txt
4–6 ↗(On Diff #39174)

Oups, yes, quite ;)

leinir updated this revision to Diff 41131.Sep 7 2018, 7:10 AM
leinir marked 2 inline comments as done.

Address @dfaure's comments

dfaure accepted this revision.Sep 7 2018, 7:43 AM
dfaure added inline comments.
src/core/tagsfilterchecker.cpp
104 ↗(On Diff #39174)

Yes.

This revision is now accepted and ready to land.Sep 7 2018, 7:43 AM
This revision was automatically updated to reflect the committed changes.
leinir added inline comments.Sep 7 2018, 7:52 AM
src/core/tagsfilterchecker.cpp
104 ↗(On Diff #39174)

Great, thank you :) Yeah, going to have to nab a copy of that for a readthrough :)

So is this enough for people to start tagging KDE4 content as such? Or is anything else still required before that capability lands?

leinir added a comment.Sep 8 2018, 9:54 AM

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 people run KNewStuff with this patch, any content which has been marked as ghns_exclude (that tick box you and the other moderators have on the store) will be hidden from the user :) For doing more "proper" filtering, changes will want to be done to either just the knsrc files, or to the clients themselves (which would be for things like "don't show wallpapers with incorrect resolutions" or "only show things with x86 binaries" or that sort of stuff).

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 people run KNewStuff with this patch, any content which has been marked as ghns_exclude (that tick box you and the other moderators have on the store) will be hidden from the user :) For doing more "proper" filtering, changes will want to be done to either just the knsrc files, or to the clients themselves (which would be for things like "don't show wallpapers with incorrect resolutions" or "only show things with x86 binaries" or that sort of stuff).

So I see ghns_exclude over at store.kde.org, but it doesn't feel quite right to check that box next to everything KDE4. Is that what I should be doing?

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 people run KNewStuff with this patch, any content which has been marked as ghns_exclude (that tick box you and the other moderators have on the store) will be hidden from the user :) For doing more "proper" filtering, changes will want to be done to either just the knsrc files, or to the clients themselves (which would be for things like "don't show wallpapers with incorrect resolutions" or "only show things with x86 binaries" or that sort of stuff).

So I see ghns_exclude over at store.kde.org, but it doesn't feel quite right to check that box next to everything KDE4. Is that what I should be doing?

That is precisely what you should be doing, yes :) You are right, it doesn't quite feel right to just outright exclude everything KDE4 from GHNS, but the reason it works is that the filtering happens clientside, and it will still show up for anybody who doesn't have this patch (or, in other words, anybody who has a less than version 5.51 Frameworks).

We should, however, also be talking about which other tags we want to have - it seems like we might well want to have some more capable filtering in our various dialogues. That, however, is not really something that'd make sense to discuss in a comment thread under some differential revision ;) Perhaps we should take it up at a Plasma meeting, sort of air the idea that it's now a thing that can be done, and then collect ideas in a Task?

So I see ghns_exclude over at store.kde.org, but it doesn't feel quite right to check that box next to everything KDE4. Is that what I should be doing?

That is precisely what you should be doing, yes :) You are right, it doesn't quite feel right to just outright exclude everything KDE4 from GHNS, but the reason it works is that the filtering happens clientside, and it will still show up for anybody who doesn't have this patch (or, in other words, anybody who has a less than version 5.51 Frameworks).

OK, got it! Will commence that work and then document it with some instructions in https://community.kde.org/Get_Involved. :)

We should, however, also be talking about which other tags we want to have - it seems like we might well want to have some more capable filtering in our various dialogues. That, however, is not really something that'd make sense to discuss in a comment thread under some differential revision ;) Perhaps we should take it up at a Plasma meeting, sort of air the idea that it's now a thing that can be done, and then collect ideas in a Task?

Sounds good to me. Can I get an invite/mention/whatever when that happens?

So I see ghns_exclude over at store.kde.org, but it doesn't feel quite right to check that box next to everything KDE4. Is that what I should be doing?

That is precisely what you should be doing, yes :) You are right, it doesn't quite feel right to just outright exclude everything KDE4 from GHNS, but the reason it works is that the filtering happens clientside, and it will still show up for anybody who doesn't have this patch (or, in other words, anybody who has a less than version 5.51 Frameworks).

OK, got it! Will commence that work and then document it with some instructions in https://community.kde.org/Get_Involved. :)

Great, thank you! :)

We should, however, also be talking about which other tags we want to have - it seems like we might well want to have some more capable filtering in our various dialogues. That, however, is not really something that'd make sense to discuss in a comment thread under some differential revision ;) Perhaps we should take it up at a Plasma meeting, sort of air the idea that it's now a thing that can be done, and then collect ideas in a Task?

Sounds good to me. Can I get an invite/mention/whatever when that happens?

They're every Monday on the Plasma IRC channel, so i'd say sooner is better, kick it off this Monday and see where we go from there :)

FYI, this included an ABI change to SearchRequest that broke Discover: https://bugs.kde.org/show_bug.cgi?id=398412

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 293ae2448f54fd1b1f7cacc86cd40b30a3fb087d

leinir reopened this revision.Sep 15 2018, 5:14 PM

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 293ae2448f54fd1b1f7cacc86cd40b30a3fb087d

It has. I am currently (bar the odd break for a cup of tea) knee deep in house renovations, but i will be reopening this when i work out what to do to make this not ABI incompatible. In the meantime, suggestions for achieving the least hacky approach would be appreciated.

This revision is now accepted and ready to land.Sep 15 2018, 5:14 PM
leinir updated this revision to Diff 41815.Sep 17 2018, 11:00 AM

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 so myself). The patch here attempts to fix the BIC issue, while also readjusting the logic for how the tag filters reach the provider. It is now done directly by the provider itself, rather than through each search request. While this does introduce that nasty d-pointer hack, it also makes more logical sense that the Provider itself holds the information (as that's where those filters are directly relevant). So, nasty surprise to find out i'd caused things to break, but the end result is, i think, kind of better anyway (even though it introduces that todo for kf-next).

leinir requested review of this revision.Sep 17 2018, 11:01 AM

Previous patch had BIC issues - new patch attempts to address this, but as a result (of course) requires another round of review.

leinir updated this revision to Diff 41886.Sep 18 2018, 9:20 AM

Forgot @since 5.51 in the new api in Provider.h

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 useful tool ;) )

Sorry for pinging again, but i'd quite like this to get in, bic fixed and all that, before the next release...

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.

src/core/provider.cpp
44 ↗(On Diff #41886)

Frameworks coding style:

if (...) {
    ...
}
51 ↗(On Diff #41886)

* -> *

54 ↗(On Diff #41886)

again

leinir marked 3 inline comments as done.Sep 26 2018, 11:53 AM
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 having only pointers in the hash will never free the referenced values.

It's the standard "oh dear, no pimpl" solution suggested on the BCI page on Community - https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#Adding_new_data_members_to_classes_without_d-pointer :) The potential memory leak should be taken care of through the delete_d(this) call in Provider's destructor. Thanks for the style bits, thought i'd caught them all...

leinir updated this revision to Diff 42360.Sep 26 2018, 11:54 AM

Address @cfeck's comments

Sorry for pinging again, but i'd really quite like to not have to wait (yet) another cycle for this to get out...

cfeck accepted this revision.Sep 30 2018, 1:12 PM
This revision is now accepted and ready to land.Sep 30 2018, 1:12 PM
This revision was automatically updated to reflect the committed changes.