Issue/#219

[poisdeux]

This PR includes PR #237. You probably want to review and merge that first.

This should fix issue #219 for music items. It should be implemented for other media items as well. But I want to show/discuss my proposed solution first.

The problem with the current way of determining if there are more items available on Kodi, is that it uses the size of the list of returned elements. When requesting media items from Kodi through JSON RPCs the start and end index are given for some media. To determine if there are more items available on Kodi, the size of the returned list is used (which should equal end - start). If Kodi returns less items than the requested size, it is assumed that all items have been retrieved.
I noticed that with Jarvis, Kodi may return a list smaller than end - start. This will make Kore stop recursing and thus only retrieve a small part of the available items.
Fortunately, Kodi returns the limit with the actual returned start and end indices. My solution uses this, to determine if we should recurse and get more items. I have tested my solution with Isengard and Jarvis and AFAICT it works.
(I'm thinking of adding a unit test for this in the future)

[SyncedSynapse]

Lot of changes... I wish you would have run this by me before, because i don't think that introducing JsonResult this way is the way to do it...

So, let's take it step by step:

1. Regarding Refactored LibrarySyncService #237 i haven't reviewed it, but i'm assuming that this is just spliting the various inner classes on the LibrarySyncService to different files, right? If so i agree.
2. Regarding introducing JsonResult, it shouldn't be introduced as the base return type of all json-rpc methods. The limits info is only returned on responses that have lists, so it should only be available on those methods.
   So, assuming we need the LimitsReturned info from Kodi (i'll get to this later), and taking for instance GetArtists, we currently have:
   public static class GetArtists extends ApiMethod<List<AudioType.DetailsArtist>>
   We should only substitute these type of classes with something like:
   public static class GetArtists extends ApiMethod<ApiList<AudioType.DetailsArtist>>
ApiList (maybe not the best name) should have the LimitsReturned and Items. It could probably be a subclass of List, adding the LimitsReturned (haven't thought this through, so this might not be the best way, but at first sight makes sense).
   Now, this is more work than just introducing JsonResult to all methods/classes, but it's the right way to handle it, and it's consistent with the way the API is constructed. Ideally it should be done for all methods, but it doesn't need to be so, we could do it on a needed basis.
3. You've added a total member to the List.Limits class, but it shouldn't be done there. This might be confusing, but the API is mimicking the json specification (available on the introspect files), and there's List.Limits, which is used to specify the limits we want returned on a call, and there's List.LimitsReturned, which is used to identify which limits are returned. List.LimitsReturned isn't yet available on the ListType class, but it's the one we want to implement and use on the return side (with the start, end and total members).
   Not sure if this is clear. One of the design principles of this api layer is to be as close and consistent with the json-rpc specification as possible, and it's important to always check the specification before creating new types. It's a little confusing at first but it's the only way to keep it consistent.
4. Now, regarding the original issue, i agree with the solution, but i want to confirm with other team members that this started happening on Jarvis. Pinging @Montellese (just read from this point on): Can you confirm that from Jarvis on, a call to for instance GetArtists that specify Limits can return less than the requested number, even though there are more items than those (e.g request 300 results, get back 250 even though there are 500 results)? Is this related to a webserver buffer size limit or something similar that i saw mentioned somewhere?

[poisdeux]

On Thu, Apr 28, 2016 at 8:30 PM, Synced Synapse notifications@github.com
wrote:

Lot of changes... I wish you would have run this by me before, because i
don't think that introducing JsonResult this way is the way to do it...

I believe you mean "run this by me first"? If so, I had to write some kind
of code to test my solution. This was the quickest way for me to add the
limits I needed.
I believe PRs are meant to discuss the solution/code. I prefer to just
write the code and have something to actually talk about. Instead of
discussing it up-front when
I don't have a clear view of what the problem is and what might be a
solution.

So, let's take it step by step:

Regarding #237 #237 i haven't
reviewed it, but i'm assuming that this is just spliting the various inner
classes on the LibrarySyncService to different files, right? If so i
agree.

Yup, that's it. It made it easier for me to get an overview and get a
glimpse of the code design.

Regarding introducing JsonResult, it shouldn't be introduced as the
base return type of all json-rpc methods. The limits info is only
returned on responses that have lists, so it should only be available on
those methods.
So, assuming we need the LimitsReturned info from Kodi (i'll get to
this later), and taking for instance GetArtists, we currently have:
public static class GetArtists extends
ApiMethod<List<AudioType.DetailsArtist>>
We should only substitute these type of classes with something like:
public static class GetArtists extends
ApiMethod<ApiList<AudioType.DetailsArtist>>
ApiList (maybe not the best name) should have the LimitsReturned and
Items. It could probably be a subclass of List, adding the
LimitsReturned (haven't thought this through, so this might not be the
best way, but at first sight makes sense).
Now, this is more work than just introducing JsonResult to all
methods/classes, but it's the right way to handle it, and it's consistent
with the way the API is constructed. Ideally it should be done for all
methods, but it doesn't need to be so, we could do it on a needed basis.

Thanks for explaining. I don't mind the extra work, I just didn't see
where else to put the returned limits

You've added a total member to the List.Limits class, but it shouldn't
be done there. This might be confusing, but the API is mimicking the json
specification (available on the introspect files), and there's
List.Limits, which is used to specify the limits we want returned on a
call, and there's List.LimitsReturned, which is used to identify which
limits are returned. List.LimitsReturned isn't yet available on the
ListType class, but it's the one we want to implement and use on the
return side (with the start, end and total members).
Not sure if this is clear. One of the design principles of this api
layer is to be as close and consistent with the json-rpc specification as
possible, and it's important to always check the specification before
creating new types. It's a little confusing at first but it's the only way
to keep it consistent.

Indeed, this wasn't clear to me. I will update the code, probably
somewhere next week.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#236 (comment)

[SyncedSynapse]

Ok, if you need some opinions or have any doubts post them here.

[poisdeux]

Using your suggestions I refactored the code. See
poisdeux/Kore@refactor/librarysyncservice...poisdeux:issue/#219
for a quick diff between the refactoring of LibrarySyncService and the fix for #219

(btw I'll squash the commits into a single commit when we have something that can be merged)

[SyncedSynapse]

I can't comment on the diff, so i'll comment here. Hopefully it will be clear from context what i'm commenting on.

1. Class ApiList looks good, though i wasn't thinking on using it the way you do.
2. Class ApiListMethod. Why do you need this? Just use ApiMethod. A method that returns a list is currently instantiated as ApiMethod<List<Something>>, with this it should be ApiMethod<ApiList<Something>> and the ApiList result should be constructed on his resultFromJson. For instance GetArtists should extend ApiMethod<ApiList<AudioType.DetailsArtist>> and have a public ApiList<AudioType.DetailsArtist> resultFromJson(ObjectNode jsonObject) that constructs the ApiList (just calling the LimitsReturned constructor with the correct json node, and using the current code for the rest of the list). Goal: take a look at the SyncMusic class, line 104. This shouldn't be public void onSuccess(ApiList<List<AudioType.DetailsArtist>> result), it should be public void onSuccess(ApiList<AudioType.DetailsArtist> result).
3. You don't need class ApiReturnParameter. All classes on jsonrpc/type are return parameters, and LimitsReturned is just one more that should follow existing conventions. LimitsReturned should have a constructor that does what you did in its parseJsonNode. Note: i get that you looked at ApiParameter and noticed that it inherited from ApiParameter and replicated the logic to this one, but the way the API is constructed classes that are possible inputs to a json method should implement this, but outputs don't need to implement anything, just rely on its constructor. I admit that it might make sense doing what you did, but to keep it consistent, it would have to be done to all the existing types...

Logic on SyncMusic looks good. Maybe the function moreItemsAvailable should be a member function on LimitsReturned so that it could be used on other places (the name should be changed though, to something like lastChunk or something).

[poisdeux]

1. Ok, what was your intention with ApiList?
2. I used inheritance to minimize code refactoring and reduce code duplication. I changed it and use composition now.
3. Changed.

I'm not sure about putting moreItemsAvailable in LimitsReturned. I think the current setup of ListType seems to be converting data to/from JSON. I think moving it to a class in the utils package would be more logical.

[SyncedSynapse]

Please check the latest line comments. ApiList is meant to substitute the use of List on methods that returns lists.

[SyncedSynapse]

I merged the songs PR first, so now this doesn't merge cleanly. SongsListFragment.java uses org.xbmc.kore.service.LibrarySyncService which is changed in this PR.

Can you check it.

[SyncedSynapse]

And by the way, #219 isn't needed anymore, right?

[poisdeux]

Fixed and squashed.

Indeed #219 can be closed.