Simplify KNSBackend fetch logic
ClosedPublic

Authored by leinir on Jun 12 2017, 12:46 PM.

Details

Summary

This code removes the custom pagination in KNSBackend in favour of using the internal pagination in KNSCore::Engine. It further removes the explicit call to request data, as this is already done by setting the search term, which caused duplicated results to be returned. Further, remove results already returned when KNS requests the view to be cleared, further reducing duplicate view entries.

nb: The fact setting the search term starts a new search is undocumented, which will need fixing (some thorough documentation work is ongoing in kns as part of a gsoc project)

Test Plan

Start Discover without patch, notice duplicates for most KNS sources
Start discover with patch, notice no duplicate entries for KNS sources

Diff Detail

Repository
R134 Discover Software Store
Lint
Lint Skipped
Unit
Unit Tests Skipped
leinir created this revision.Jun 12 2017, 12:46 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 12 2017, 12:46 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol edited edge metadata.Jun 12 2017, 4:56 PM

Can you maybe extend the knsbackendtest to make sure we won't regress this?

Thanks a lot!!!

libdiscover/backends/KNSBackend/KNSBackend.cpp
109

Add an #if KNEWSTUFF_VERSION_MAJOR==5 && KNEWSTUFF_VERSION_MINOR==36

We always end up forgetting these fixme

130

Good, maybe we should also delete them?
Can you see if the StandardUpdater is listening to resourceRemoved?

leinir marked 2 inline comments as done.Jun 13 2017, 12:09 PM
leinir added inline comments.
libdiscover/backends/KNSBackend/KNSBackend.cpp
109

Very true, yes - to be quite frank, i forgot about the preprocessor for a moment, there ;)

130

Yes, that does seem like a good idea, if we're reasonably certain they're not accessed in other places...
StandardUpdater did not, but it does now :)

leinir updated this revision to Diff 15419.Jun 13 2017, 12:10 PM
leinir marked 2 inline comments as done.

Adapted code as suggested. Note that this now depends on D6190 getting merged (as that contains the code that's iffed out).

apol accepted this revision.Jun 13 2017, 12:42 PM

Ship when the dependency is in.

This revision is now accepted and ready to land.Jun 13 2017, 12:42 PM
leinir updated this revision to Diff 15444.Jun 14 2017, 10:51 AM

Upon reviewing prior to merging, i realised that a file was missing from the most recent diff (StandardBackendUpdater did, as suggested, not listen to the resourceRemoved signal, and now does).

apol accepted this revision.Jun 14 2017, 10:06 PM

This will go to Plasma/5.10, right?

In D6191#116480, @apol wrote:

This will go to Plasma/5.10, right?

As in, merge this patch into Plasma/5.10 as well as master? I don't see why it would fail to merge, yeah, i'll do that :)

This revision was automatically updated to reflect the committed changes.
apol added a comment.Jun 15 2017, 5:45 PM
In D6191#116480, @apol wrote:

This will go to Plasma/5.10, right?

As in, merge this patch into Plasma/5.10 as well as master? I don't see why it would fail to merge, yeah, i'll do that :)

It went to master, right? Also for some reason it didn't close the bug...

In D6191#116635, @apol wrote:
In D6191#116480, @apol wrote:

This will go to Plasma/5.10, right?

As in, merge this patch into Plasma/5.10 as well as master? I don't see why it would fail to merge, yeah, i'll do that :)

It went to master, right? Also for some reason it didn't close the bug...

It went to master, yeah - as for not closing the bug, it turns out there's an issue matching my commit account to bugzilla... long standing and apparently unsolvable, because... reasons, i guess, tried to get it sorted a while ago. However, i am also now not entirely certain this patch fixes that particular issue - i'll have to have a look, but yeah, not sure. Those particular duplicates aren't KNS entries, so if this patch were to fix it, it would need to go through the resourceRemoved signal catch here.