[json-rpc] add 'sort' parameter to methods PVR.GetRecordings / GetCha…

[howie-f]

…nnels / GetTimers

Description

adds the ability to order the result sets for the named json-rpc methods. i.e. remote control apps no longer need to do sorting on their own. they can adjust their requests.

refs:
xbmc/Official-Kodi-Remote-iOS#134
https://forum.kodi.tv/showthread.php?tid=360315

no problem if postponed as this is only an improvement

Motivation and Context

Official iOS Remote returns an unordered list of recordings

How Has This Been Tested?

{"jsonrpc":"2.0","method":"PVR.GetRecordings","id":-1,
  "params":{"properties":["title"],
            "sort":{"order":"descending","method":"date"}}}

returns recordings in the desired order.
This is the shortened request that iOS remote sends out, extended with the new parameter.
retrieval via Official iOS Remote (AppStore and Beta) is working like before.

Types of change

• Improvement (non-breaking change which improves existing functionality)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have read the Contributing document

fyi @wutschel

[ksooo]

Looks good to me. Thanks.

[howie-f]

one problem i see here. using this new param will break the functionality against v18 and lower, where it is not known. how are such things handled usually?

[ksooo]

how are such things handled usually?

I'd say, the using app must check json-RPC version before calling.

[Tolriq]

This is strange that the call to ParseLimits is added and was not present before.
It probably means that limits previously where ignored for PVR and would now behave differently (either don't work or properly apply them). That would be a major change in both cases.

This needs a tons of tests about limits or remove that call to add the feature without changing actual usage of current params that would make the scope of this PR a lot bigger.

[howie-f]

the call to ParseLimits(parameterObject, **sorting**.limitStart, **sorting**.limitEnd); sets the limit parameters (if there are any passed) for the SortDescription. limits were respected by PVR method calls previously. we stumbled over it yesterday and tested

[Tolriq]

Ok fine then if it's tested and not oversight

[ksooo]

The limits were already parsed before in HandleLimits. In fact with ParseLimits call added this is done twice now. Interestingly, the double parse will also be done for VideoLibrary and other listings. So, we thought there must be a reason for that and simply copied the existing logics for PVR.

[Tolriq]

This is indeed strange, the sort does apply the limits and actively remove items so if it worked before there's now double removal and counts for the return would be impacted.

Are you sure limits are handled the same before and after the PR? Like GetChannels with same start and limits in the middle of the list with and without sort order and returning the proper start/end/total ?

I lack time but from a quick look the way videolibrary handles it differently by passing those to the underlying query and resetting the values when needed.

[howie-f]

current master:
request "method":"PVR.GetRecordings","params":{"properties":["title"],"limits":{"start":3,"end":6}}}
response "result":{"limits":{"end":6,"start":3,"total":28}

this pr applied:
request "method":"PVR.GetRecordings","params":{"properties":["title"],"limits":{"start":3,"end":6}}}
result "result":{"limits":{"end":6,"start":3,"total":28}

request {"jsonrpc":"2.0","method":"PVR.GetRecordings","id":-223078810,"params":{"properties":["title"],"limits":{"start":3,"end":6},"sort":{"order":"ascending","method":"date"}}}
result "result":{"limits":{"end":3,"start":3,"total":3} 👎 and an empty set returned

@Tolriq you're right, nice catch

[ksooo]

Things are getting more and more strange. GetChannels and friends are calling HandleFileItemlist, which has a parameter bool sortLimit, which is set to true by GetChannels even before this PR. Looking at HandleFileItemList:

void CFileItemHandler::HandleFileItemList(const char *ID, bool allowFile, const char *resultname, CFileItemList &items, const CVariant &parameterObject, CVariant &result, int size, bool sortLimit /* = true */)
{
  int start, end;
  HandleLimits(parameterObject, result, size, start, end);

  if (sortLimit)
    Sort(items, parameterObject);

Seems, that it sorts the list, if the respective paramter is set to true.

Question is whether this PR is actually needed as it seems as both sort and limits support should already work?! Is there just a bug we need to fix to get it work?

[Tolriq]

In all cases there's need for a PR to expose the fields to the schema.

As a reminded for JSON:

• There's a very strict schema validation for the inputs, so if the schema does not say you can pass sort parameters, the request will fail for invalid data.
• There's 0, niet, nada, none validation for the outputs, meaning that anything inside Kodi can serialize wrong data to the output that will break the schema (This is the problematic part that happens frequently).

[howie-f]

Question is whether this PR is actually needed as it seems as both sort and limits support should already work?! Is there just a bug we need to fix to get it work?

yes could be this is the wrong approach, channel list on my remote is sorted but no idea if the app does that or the request returns correctly, i'll dig into there.

[ksooo]

In all cases there's need for a PR to expose the fields to the schema.

Yep. And maybe this is all what is currently missing. No code change needed.

[ksooo]

Just did a quick test and applied only the methods.json changes (which add the sort parameter to the methods) of the PR to my personal build, none of the code changes ... and it works. :-)

[howie-f]

only the methods.json changes - and it works. :-)

nice, thanks for the test. everybody agrees with that?

[Tolriq]

The schema changes are ok for sure.

[DaveTBlake]

jenkins build this please