Details

Reviewers

apol
whiting
ahiemstra
mlaurent
dfaure
cfeck

Group Reviewers

KNewStuff
KDE Store

Commits

R304:2ad3e66d81b6: Add support for Attica tags support
R304:158ba4e2420b: Add support for Attica tags support

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

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

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

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

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

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

@dfaure, @mlaurent okey?

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

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

Closed by commit R304:2ad3e66d81b6: Add support for Attica tags support (authored by leinir). · Explain WhySep 7 2018, 7:51 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?

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

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

In D6513#324419, @ngraham wrote:

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

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

In D6513#324915, @ngraham wrote:

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

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

In D6513#326519, @rikmills wrote:

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

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

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

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

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

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

Closed by commit R304:158ba4e2420b: Add support for Attica tags support (authored by leinir). · Explain WhyOct 1 2018, 11:49 AM

This revision was automatically updated to reflect the committed changes.

		Path
M		src/attica/atticaprovider.cpp (29 lines)
M		src/core/engine.h (96 lines)
M		src/core/engine.cpp (48 lines)
M		src/core/entryinternal.cpp (20 lines)
A	M	src/core/tagsfilterchecker.h (83 lines)
A	M	src/core/tagsfilterchecker.cpp (171 lines)
M		src/staticxml/staticxmlprovider.cpp (59 lines)
M		tests/CMakeLists.txt (14 lines)
A	M	tests/khotnewstuff_test.cpp (246 lines)
V		tests/{khotnewstuff_test.knsrc.in ← knewstuff2_test.knsrc} (3 lines)
V		tests/{knewstuff2_test.knsrc → khotnewstuff_test.knsrc.in}

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	16201			Jul 5 2017, 12:27 PM	★	★
Diff 2	37047			Jul 2 2018, 9:56 AM	★	★
Diff 3	38489			Jul 26 2018, 9:55 AM	★	★
Diff 4	38506			Jul 26 2018, 1:04 PM	★	★
Diff 5	39174			Aug 6 2018, 10:00 AM	★	★
Diff 6	40981			Sep 4 2018, 12:46 PM	★	★
Diff 7	41034			Sep 5 2018, 9:55 AM	★	★
Diff 8	41131			Sep 7 2018, 7:09 AM	★	★
Diff 9	41133	dbd6404	R304:2ad3e66d81b63495a59d012f673af7bd854b53d7	Sep 7 2018, 7:47 AM	★	★
Diff 10	41815			Sep 17 2018, 10:54 AM	★	★
Diff 11	41886			Sep 18 2018, 9:19 AM	★	★
Diff 12	42360			Sep 26 2018, 11:54 AM	★	★
Diff 13	42657	49533c5	R304:158ba4e2420b9500d25bb739971aa53ceb375b2e	Oct 1 2018, 11:38 AM	★	★

	Status	Author	Revision
	Closed	leinir	D6513 Add support for Attica tags support
	Closed	leinir	D6512 Add support for proposed tags addition in OCS 1.7

Add support for Attica tags support
ClosedPublic
Actions

Details

Diff Detail

Showing Only Differences

Revision Contents
Changeset List

Diff 40981

src/attica/atticaprovider.cpp

src/core/engine.h

src/core/engine.cpp

src/core/entryinternal.cpp

src/core/tagsfilterchecker.h

src/core/tagsfilterchecker.cpp

src/staticxml/staticxmlprovider.cpp

tests/CMakeLists.txt

tests/khotnewstuff_test.cpp

tests/khotnewstuff_test.knsrc.in

tests/knewstuff2_test.knsrc

Add support for Attica tags supportClosedPublicActions

Details

Diff Detail

Showing Only Differences

Revision ContentsChangeset List

Diff 40981

src/attica/atticaprovider.cpp

src/core/engine.h

src/core/engine.cpp

src/core/entryinternal.cpp

src/core/tagsfilterchecker.h

src/core/tagsfilterchecker.cpp

src/staticxml/staticxmlprovider.cpp

tests/CMakeLists.txt

tests/khotnewstuff_test.cpp

tests/khotnewstuff_test.knsrc.in

tests/knewstuff2_test.knsrc

Add support for Attica tags support
ClosedPublic
Actions

Revision Contents
Changeset List