[jsonrpc] [breakingchange]remove ambiguous and duplicate Player.Seek options

[rmrector]

Description

Remove implicit duplicate options

value: 25
value: {"minutes":10,"seconds":15}
value: "bigforward"

Leaving only the explicitly parameterized options (available since Kodi 15 Isengard)

// Percentage value to seek to
value: {"percentage":25}
// Time to seek to
value: {"time": {"minutes":10,"seconds":15}}

// Seek by predefined jumps
value: {"step":"bigforward"}

// Seek by the given number of seconds
value: {"seconds":30}
// backward
value: {"seconds":-30}

Motivation and Context

This method's parameters are ambiguous with one of the duplicates. Removing it clears up the ambiguity and solves a reported issue #15914.

Removing the rest of the duplicates simplifies the schema and the code, slightly, with no loss of functionality.

How Has This Been Tested?

Manually.

Types of change

• Breaking change any JSON-RPC consumers that use the removed options will receive an error response "Invalid params.".

Checklist

• will need an update to the forum thread tracking JSON-RPC changes

[DaveTBlake]

It's really early to bump the JSON-RPC version for this little guy, but here's the code.

Yes it is, and such a bump would make any fixes that need backporting to Leia tricky, so can we hold off breaking changes if possible until v18 is frozen. As a reminder when the first breaking change happens it will be a bump to 11.0.0 (with v11 covering all breaking changes during development phase rather than run-away number), and finally to 12.0.0 on release of v19.

[rmrector]

Yup, it can definitely wait some time. My thought was to wait for something more substantial to be the first breaking change.

[rmrector]

With the JSON-RPC version bump its about time for this to go in.

[rmrector]

Looking through the existing remote implementations, Kore, Yatse, and Chorus2 all use implicit options that are removed by this PR and will break on seeking with the seekbar.

Kore and Yatse both seek by time with "params":{"playerid:"0,value: {"hours":0,"milliseconds":0,"minutes":1,"seconds":18}} - the explicit replacement is "params":{"playerid:"0,value: {"time": {"hours":0,"milliseconds":0,"minutes":1,"seconds":18}}}.

Chorus2 seeks by percentage with "params":[0,74], the explicit replacement is "params":[0,{"percentage":74}].

[phunkyfish]

@rmrector how should this be progressed?

[rmrector]

It can be merged when it's green. I spaced, I didn't mean to hold it past the new year.

[phunkyfish]

@DaveTBlake how are breaking changes for jsonrpc usually handled?

[DaveTBlake]

Strictly (by definition in the schema itself)

xbmc/xbmc/interfaces/json-rpc/schema/methods.json

Lines 32 to 34 in 3a9c94d

	"major": { "type": "integer", "minimum": 0, "required": true, "description": "Bumped on backwards incompatible changes to the API definition" },
	"minor": { "type": "integer", "minimum": 0, "required": true, "description": "Bumped on backwards compatible additions/changes to the API definition" },
	"patch": { "type": "integer", "minimum": 0, "required": true, "description": "Bumped on any changes to the internal implementation but not to the API definition" }

a breaking change to the API requires a bump to the major version, however wanting to avoid version runaway during alpha only the first breaking change does that. Hence although this is a breaking change only the minor gets bumped as it is not the first breaking change. The major will then be bumped again to v12.0.0 for release of v19 - so releases are even numbers. Obviously we try to do braking changes early in the dev cycle.

To help JSON consumers spot changes in the commit history of the build it is also good to include [JSON] and [Breaking Change] in the commit title of breaking commits. Github labelling, although usefull during PR processing, is only visible when looking at Github.

For Leia I also maintained a forum thread that summarized changes, but I have been a bit lax for v19 so far.

[phunkyfish]

So once this gets reviewed and approved it’s ok to go in.

[DaveTBlake]

Rebase the JSON version number @rmrector then the button is yours

[howie-f]

@rmrector merging this pullrequest broke seeking with offical-kodi-remote (ios). for my personal branch i can revert it without (to me) obvious drawbacks, but was this intended and will be fixed later? (i doubt official-kodi-remote is under maintenance atm.)

[phunkyfish]

@howie-f is this the correct repo for the remote you are referring to: https://github.com/xbmc/Official-Kodi-Remote-iOS ?

[DaveTBlake]

It was an intentional breaking change, and so it is up to JSON API consumers to adapt. This is problem for the iOS remote or any other unmaintained apps.

You could question the wisdom of making a breaking chance to primarily simplify the schema and remove duplicated parameters, but that is what has happened. It also claims to fix issue #15914.

[howie-f]

@phunkyfish yeah, correct. installed through app-store.
@DaveTBlake i'm fine with that and don't question any wisdom. just telling i noticed this behaviour. that's it. sorry for the noise

[nkichukov]

For now, I will also have to revert this patch on my Matix builds, as the kodi remote ios app seek functionality is something I use on daily basis.

[basilgello]

We also need to ping @Tolriq as Yatse users will soon complain about this change a lot.

[Tolriq]

It's already fixed in Yatse beta, just not released due to Google issues ;)
But I also wonder why the need to break all the apps and millions users to try to fix the issue of one reported 5 years later.

[howie-f]

i think it‘s a legit change and let‘s keep in mind that matrix isn‘t even alpha yet. but i’d just rather test alpha in the living-room with my ios-remote.

[Tolriq]

Not 5 years later (#6801) to avoid fixing a rare issue only one person have at the expanse of breaking nearly everyone using this, and without announcing a deprecation before. This is usually not the way to maintain a public API.

I care about Kodi users and it's not a nice move, from Yatse point of view this is nice as it will make all users of no more supported remotes search alternatives that works.

[graysky2]

Given xbmc/Official-Kodi-Remote-iOS#95, would it be sane for end-users wishing to have arrow key functionality within the official iOS app to simply revert this commit when compiling Matrix? Any unforeseen breakage by doing so?

[howie-f]

@graysky2 i‘m doing the revert on my living room branch and have not seen any breakage. but the correct fix needs to go to (unmaintained) official ios remote.