Fix semantics for ghns_exclude
ClosedPublic

Authored by leinir on Mon, Jan 7, 1:30 PM.

Details

Summary

After much discussion on the topic, it came to light that the specific grammar used for the default tag in KNSCore was incorrect, as it was an action verb (rather than an adjective the way the metadata tags are supposed to be). While this does mean that previous versions of Frameworks will not have the filtering functionality by default, it is otherwise non-invasive and causes no side effects apart from the server-suggested filtering working.

BUG: 402888

Test Plan

Run the test tool without this patch: Items which are supposed to be excluded are not excluded
Run the test tool with this patch: Items which are supposed to be excluded from listings are excluded.

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Mon, Jan 7, 1:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Jan 7, 1:30 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
leinir requested review of this revision.Mon, Jan 7, 1:30 PM

What is the test tool? Can you help a total n00b like me learn how to test KNewStuff patches like these?

Also, the correct formatting is CCBUG: 402888. See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

What is the test tool? Can you help a total n00b like me learn how to test KNewStuff patches like these?

The test tool is khotnewstuff_test in tests/. It's not installed, but once compiled it's in (build)/bin/khotnewstuff_test and can be run from there once the libraries are installed. For the most useful output, be sure to turn on all of the KNewStuff debug output, and then pass it a knsrc file. For example, what i use is

QT_LOGGING_RULES="org.kde.knewstuff.*=true" ./bin/khotnewstuff_test /etc/xdg/peruse.knsrc

You then get a nice little UI which lets you run a couple of tests (engine test initialises the engine and lists the first page of content in the requested categories using the default sort order, and if you turn on the option underneath that, it will also attempt to download them). If you don't give it a knsrc file, it'll just use the static data found in tests/testdata.

As you can tell, it's a super simple tool, and could of course be improved :) Haven't really had the need to do so, since the more advanced features seem better tested on live installs, and this one is more for testing out the very most basic functionality (which is just sort of expected to work in proper applications).

Also, the correct formatting is CCBUG: 402888. See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

Ah yes, quite, i've only been using those hooks for about a decade ;) You'd imagine i would be able to remember them at some point. Not that the bug hooks work for me, mind, but still :P

ngraham accepted this revision.Fri, Jan 11, 10:22 PM

Thanks, in addition to the testing tool working, this patch seems to actually fix the issue in production (e.g. "Tree on Island" is no longer visible in the wallpaper downloader), and as far as I can tell the code is sane. Thanks for the additional documentation and commenting too.

Should this be marked as actually fixing 402888? If so, it should be BUG: 402888

This revision is now accepted and ready to land.Fri, Jan 11, 10:22 PM

Thanks, in addition to the testing tool working, this patch seems to actually fix the issue in production (e.g. "Tree on Island" is no longer visible in the wallpaper downloader), and as far as I can tell the code is sane. Thanks for the additional documentation and commenting too.

Should this be marked as actually fixing 402888? If so, it should be BUG: 402888

Yay! :D i'm quite keen on documentation being solid, i know what it's like to arrive at something which is... less than well documented ;) Great stuff, nice to know it works for people not me :)

I think it should probably fix said bug (which also will show you what i mean when i say it doesn't work for me ;) ).

leinir edited the summary of this revision. (Show Details)Sat, Jan 12, 3:48 PM
This revision was automatically updated to reflect the committed changes.