xbmc / Official-Kodi-Remote-iOS

Full-featured remote control for XBMC Media Center. It features library browsing, now playing informations and a direct remote control.
Other
221 stars 104 forks source link

Feature request: honour the sorttokens in the Title List #97

Closed heitbaum closed 3 years ago

heitbaum commented 4 years ago

It would be nice if the iOS application could honour the sorttokens in the Title list, the same as the kodi application does.

wutschel commented 3 years ago

When looking at https://github.com/xbmc/xbmc/pull/14227 (merged into Kodi 18) I am not 100% clear, if this can be achieved via simply using "ignorearticle": true instead of "ignorearticle": false. The Remote App is currently hardcoding this to false. If you are able to build the App this can be tested quickly.

howie-f commented 3 years ago

tested with current master branch. the app can query the current setting in kodi with:

{"method":"Settings.GetSettingValue","id":1,"jsonrpc":"2.0","params":{"setting":"filelists.ignorethewhensorting"}}

this should return "result":{"value":true / false} and then adjust the "ignorearticle" param accordingly instead of hardcoding

wutschel commented 3 years ago

What happens if you request ignoreartice: true and in Kodi server this is not enabled? Will this result in an error?

If not, we could simply request ignoreartice: true always. If sorttokens are enabled on server, the sorttokens will be respected. If not, nothing changes.

howie-f commented 3 years ago

Will this result in an error?

no, it won‘t. just means you‘re hardcoding ignorearticle: true.. and the kodi setting doesn‘t matter then (for the app)..

correct (but not really mandatory to me) would be to query the setting from kodi and let the remote act accordingly. maybe a good idea to do this also where the json-api-version is requested and then AppDelegate?

wutschel commented 3 years ago

Hmm, still confused. Are you saying the server can have the sorttokens defined but still disabled? And when we would call ignorearticle: true always from the App, then we might use the sorttokens which were defined for the server but not enabled there?

maybe a good idea to do this also where the json-api-version is requested and then AppDelegate?

Yes, that´s the easy part. The work lies within adapting the sort commands everywhere. This is not done in a centralized way.

howie-f commented 3 years ago

Are you saying the server can have the sorttokens defined but still disabled?

the sorttoken is always defined, as the setting always exists. it is either set to true or false.

you can also modify this setting in the remote: (in the gui as well) so now if you're hardcoding ignorearticle: true the remote would ignore the articles despite of the setting. it would better the app would do what the user requests in the gui setting or via app. then listings inside the kodi gui and inside the remote control are consistent.

understandable what i'm trying to say? :)

IMG_3820

howie-f commented 3 years ago

The work lies within adapting the sort commands everywhere. This is not done in a centralized way.

as said, it's not mandatory imo. but the complete solution to the problem.

joethefox commented 3 years ago

This is a old request and despite the appearance not a simple task to achieve.

Let me explain how the data received from kodi is treated.

When the app request to kodi the artists list, this list is sectioned per initial letter (to allow you to scroll by letter with the right side section) so the initial sort applied by Kodi is simply ignored.

With an example, you have ignorearticle enabled and when kodi sends the list you will have something like this:

.... Donna Summer The Doors Doris Day ....

wonderful! the ignorearticle flag is honored!

BUT when the app start to section the items retrieved from kodi, will check:

"Donna Summer" starts with "D", let's put it in the letter D section "The Doors" starts with "T", let's put it in the letter T section "Doris Day" starts with "D", let's put it in the letter D section

so the ignorearticle was lost :(

The solution I see are two:

  1. Implement the ignorearticles feature into the app (for every language!!) and duplicate a logic that is already done
  2. Kodi should send "Doors, The" to the remote app if the ignorearticle is enabled

The number 2 was alway my prefered path but I never got feedback from kodi side to think about this feature.

howie-f commented 3 years ago

mhmrf, there a whole bunch of 52 [NSNumber numberWithBool:FALSE],@"ignorearticle" across the code. you would have to adjust any FALSE to TRUE, right? may then adjust FALSE to MyNewDelegatedVariable

joethefox commented 3 years ago

mhmrf, there a whole bunch of 52 [NSNumber numberWithBool:FALSE],@"ignorearticle" across the code. you would have to adjust any FALSE to TRUE, right? may then adjust FALSE to MyNewDelegatedVariable

I explained why the ignorearticle is just ignored in any case

howie-f commented 3 years ago

ah, ok that is my lesser knowledge, thanks for the explanation @joethefox

joethefox commented 3 years ago

you're welcome :)

howie-f commented 3 years ago
  1. Kodi should send "Doors, The" to the remote app if the ignorearticle is enabled

will see what i can do here and report back

howie-f commented 3 years ago

ok, kodi is sending Ärzte, Die to my app. (category Ä isn't handled correctly yet, shows /, but this is a different story): IMG_3829

IMO, to keep consistency with the kodi gui, we still need to display Die Ärzte, but sort into Cat. Ä. later we should still see:

Donna Summer
The Doors
Doris Day

so what if the json-response for GetArtists looks like:

{"artist":"Die Ärzte","artistid":367,"label":"Die Ärzte","SORTINTO (however we call it)":"Ärzte"}

or even

{"artist":"Die Ärzte","artistid":367,"label":"Die Ärzte","SORTINTO (however we call it)":"Ä"}

@wutschel @joethefox what do you think? not sure if a change like this will be accepted in core, let's try

wutschel commented 3 years ago

I would not go for the 2nd option as this does not allow to sort within a section. To give the App the freedom for implementation either API sends the complete name w/o the ignored article (your option 1), or the API tells the exact ignored text segment ("Die" or "The" in your case).

DaveTBlake commented 3 years ago

so the initial sort applied by Kodi is simply ignored.

Sticking my nose in because I think you may want some SQL level changes to the JSON GetArtists requests rather than the approach current proposed by @howie-f PR.

Does the remote fetch all artists and do its own sorting or fetch a limited numbers of artists, in which case using the sorting applied by Kodi matters.

Also Kodi UI and API sorting by artist name allows for ignoring articles and artist sortname (which could be surname first, or something bespoke), I think that needs to be taken into account too if you want to reproduce the sort order that Kodi offers.

Tell me more about what you are trying to achieve :)

howie-f commented 3 years ago

app fetches all artists and does its own thing then. i think the result, say what my PR produces, is correct and @wutschel can deal with: received new sortlabel ("Doors", without article), put that in the letter D section, sort the D section by this new label. just the way the PR does it is, er... sigh :) that's my pov, can you confirm @wutschel ?

DaveTBlake commented 3 years ago

I am probably wrong or at least well out of date, but I had it in my head that the iOS remote app only requested a screen full of items (artists, albums, songs etc.) at a time, specifiying the sort order and limits in the requests. But no, all artists are fetched and then resorted by the app?

howie-f commented 3 years ago

all artists are fetched and then resorted by the app?

yes i think so, i just logged the requests from the app to kodi. and then started digging with that. responses were always full blown datasets.. all artists.. all albums

wutschel commented 3 years ago

Well, the app requests all at once as far as I can see (I can confirm the app gets all my 431 albums in one shot). The app will request all items with a default sorting (e.g. sort by "label") and additional data like "year", "playcount" and such. The additional data is used to sort locally without requesting the data again via API.

The more I am thinking about it, the more I am in doubt if the sorttokens can really be implemented easily in the app. There is one catch: Local sorting is based on the additional data. Let us assume we ask for albums sorted with "ignorearticle: true". In addition for each album we request "year", "playcount" and "artist". When the app now locally sorts for "artist", there is no "sortlabel" provided for this (it was provided only for the initial sort-request for the album name). As far as i understand this can be resolved only when the app is always newly requesting the data instead of performing sort locally. Quite a performance drop.

howie-f commented 3 years ago

you're right, had focus on GetArtists only... for sure we need the new "sortlabel" for the GetAlbums-call then, too. and sortlabel in the case of albums must return the cut ArtistLabel not the Album-Label.. the rabbit hole opens up ;)

IMG_3830

just to explain @DaveTBlake my "The Donnas" Albums are in section T, we need them in D but still displaying "The Donnas" :+1:

DaveTBlake commented 3 years ago

Well, the app requests all at once as far as I can see

OK, so I was mis-remembering conversations I had long ago, maybe even some other app. You are fetching everything, storing and sorting locally. Other apps that store data deffinitely do process sort tokens themselves, I added JSON to return what the current token settings are to support it.

Artists is really a special case because there is little other than name (with options for articles and using sort name) you would sort by. With albums it can become more complex e.g. title (ignoring articles) + year + artist (ignoring articles + sortname). Kodi handles it for both UI and JSON requests.

While it could be possible to return artist name and album title sort labels even when not sorting by those fields, I'm not sure that is right use of the API. Convince me.

howie-f commented 3 years ago

could be - i really don‘t know - the effort is manageable to move from local sorting to using proper json-requests and kodi‘s token settings. but i think it‘s not possible to implement the sections correctly without an additional property.. or there‘s something i don‘t see yet 🤔 sorry

wutschel commented 3 years ago

but i think it‘s not possible to implement the sections correctly without an additional property..

Yes, that´s true. If we would request the data freshly each time we still need the cut label to sort into the sections properly. Still I do not like this approach as this impacts the performance -- especially with huge databases.

While it could be possible to return artist name and album title sort labels even when not sorting by those fields, I'm not sure that is right use of the API.

To me this is also a kind of overkill for the API.

Overall I tend to either:

  1. Keep it like it is. Personally I can live very well without this feature, especially after looking into this now.
  2. Implement the sort tokens locally (then sort results might of course still differ from Kodi).
heitbaum commented 3 years ago

My biggest ask would be the Movies. I have 738 “The” files that would be nice if they were A-Z 0-9.

DaveTBlake commented 3 years ago

Implement the sort tokens locally (then sort results might of course still differ from Kodi).

I don't know how the app does sorting (until yesterday I thought it had Kodi do it all), but if it is able to sort using multiple fields e.g. title + artist + year then I would hope that ignoring articles would not be impossible. The API can return what the sort tokens are, it is just a matter of applying them (and at the same time for music you could also make use of "sortname" and "sortartist", that would match Kodi better).

wutschel commented 3 years ago

I now did a quick-and-dirty approach to let the App handle the sort tokens. First, the App reads the tokens from Kodi after connecting ({"jsonrpc":"2.0","id":1,"method":"Application.GetProperties","params":{"properties":["sorttokens"]}}). Then I am removing any such leading text segments from the text items which were are sorting for before grouping into sections. This works quite generic for different sort methods and applies for artists/albums/movie titles and such. Screen shots:

https://abload.de/img/simulatorscreenshot-i0eknj.png https://abload.de/img/simulatorscreenshot-i2bkfx.png https://abload.de/img/simulatorscreenshot-iipkje.png

Next step will be to look at sorting within the sections. Currently the sections are still sorted by the full name -- incl. the leading "The". Personally I would prefer to somehow find a way to make the "The" visibly less prominent as it really disturbs me when looking at the sorted items. I am thinking of using a different gray color for these text segments.

Question: Does Kodi apply sorttokens also to all items like movie titles, PVR recordings and such?

DaveTBlake commented 3 years ago

Does Kodi apply sorttokens also to all items like movie titles, PVR recordings and such?

Yes. To check see which map entries use SortAttributeIgnoreArticle

https://github.com/xbmc/xbmc/blob/281295c570bd33fb90e8975172a2c9c98207b212/xbmc/utils/SortUtils.cpp#L1097-L1166

wutschel commented 3 years ago

A bit of further rework done to also sort within the sections. I am adding a new key named "sortby" and add the pre-processed label/artist/whatever there -- e.g. "Commitments". All items are sorted by this, and sections are created by this, but the displayed text stays unprocessed - e.g. "The Commitments".

https://abload.de/img/simulatorscreenshot-in0jyx.png ("Das Boot" sorted by "Boot") https://abload.de/img/simulatorscreenshot-i34j1u.png (Look at "The Classic Collection" and "The Commitments") https://abload.de/img/simulatorscreenshot-iqkktk.png ("The Commitments" sorted by "Commitments")

Now all items are sorted by this, even the sorting order by Kodi is overruled.

The code is not really clean yet, and I also would like to check if Kodi server has ignorearticles enabled before applying this. Alternatively I could just enable this via a new switch for the App Settings.

howie-f commented 3 years ago

pretty nice, i like that it matches the kodi gui list sorting 👍🏻

wutschel commented 3 years ago

I won´t raise a PR for now, but if anyone is interested: https://github.com/wutschel/Official-Kodi-Remote-iOS/tree/sorttokens_2

The branch is based on the fix for the playcount index sorting. It still does apply the sort tokens always and (for testing) uses dummy sort tokens when Kodi server is not proving them. I will add a check, if Kodi has enabled "ignorearticles" or not and just follow this setting.

Let me know your thoughts.

howie-f commented 3 years ago

nice, at least i think i can follow. you can open a draft pull request if you like. that's easier for discussion.

I will add a check, if Kodi has enabled "ignorearticles" or not and just follow this setting.

i would go with this too, keep in mind that - when ignorearticles is switched in the kodi ui by using the remote control (not an action button or kodi settings in the app), the app cannot know about this AFAIK. still i wouldn't introduce a new setting for the app itself in this regard, because you would usually set this once and then never touch it again.

DaveTBlake commented 3 years ago

when ignorearticles is switched in the kodi ui by using the remote control (not an action button or kodi settings in the app), the app cannot know about this AFAIK.

No, all GUI settings can re retrieved via JSON, so the remote can read all the related settings and so completely reproduce the Kodi sorting.

Here is example JSON Get ignorearticles using

{"jsonrpc":"2.0","id": 1, "method":"Settings.GetSettingValue",
"params":{"setting":"filelists.ignorethewhensorting"}}

Get the sort tokens using

{"jsonrpc":"2.0","id":1,"method":"Application.GetProperties",
"params":{"properties":["sorttokens"]}}

Get the music only use artist sortname

{"jsonrpc":"2.0","id": 1, "method":"Settings.GetSettingValue",
"params":{"setting":"musiclibrary.useartistsortname"}}

Can I encourage you to include the latter music options too. JSON returns artist sortname (often a surname first, but could alias name etc.), properties "sortname" (GetArtists) and "artistsort" (GetAlbums). Hope that info is useful @wutschel

wutschel commented 3 years ago

I now added a check based on suggestions by @howie-f and @DaveTBlake. At connection the App now asks for ignorearticles and only applies the whole function if this feature is enabled and there are sort tokens provided by Kodi server. Reason for this condition: On my old Kodi 17.6 ignorearticles is enabled, but the JSON command to ask for sort tokens is not available. I do not want to define own tokens in the App.

I do not want to go down that path of also using the sortname. At least not for now. I feel this is adding too many special cases to handle...

DaveTBlake commented 3 years ago

Still using v17.6, come on @wutschel join the bleeding edge ;)

All JSON consumers have to adjust for what schema version is running. So you may want to use

{"jsonrpc":"2.0","id":1,"method":"JSONRPC.Version"}

and then check for of 9.5.0 or above when "sorttokens" were added. Or even use Kodi version

{"jsonrpc":"2.0","id":1,"method":"Application.GetProperties",
"params":{"properties":["version"]}}

Forgive me if you already knew this.

EDIT: Oops muddled up my JSON methods, some help that is. Corrected now.

I do not want to go down that path of also using the sortname. At least not for now.

Fair enough, but it is so nice not to have to remember the first name of classical conductors and composers.

howie-f commented 3 years ago

yup, you're already calling Application.GetProperties here https://github.com/xbmc/Official-Kodi-Remote-iOS/blob/74202266374bcfdfec99e5219bae6dce8e8830af/XBMC%20Remote/tcpJSONRPC.m#L183-L184

and then check for of 9.5.0 or above when "sorttokens" were added

...if we're on a lower version fallback to "The ", "The_", "The." that motivates users to wipe 17.6 and use the Matrix :)

EDIT: whoops, the correct call is to JSONRPC.Version not Application.GetProperties

wutschel commented 3 years ago

Still using v17.6, come on @wutschel join the bleeding edge ;)

Don´t worry, I am using Kodi 20 Alpha for many of my test already. I am just reluctant to migrate my music PC though, I would need to set it up freshly (new OS, recompile Kernel for RT, and many other stuff).

and then check for of 9.5.0 or above when "sorttokens" were added. Forgive me if you already knew this.

I didn´t yet check for the API version before sending the Application.GetProperties request which asks for sort tokens. I can of course do this, but wouldn´t the error response itself be also good enough to know there are no tokens provided? Or is there a possible problem on Kodi server side?

Fair enough, but it is so nice not to have to remember the first name of classical conductors and composers.

Then I am lucky, the app doesn´t support sorting for those. :)

...if we're on a lower version fallback to "The ", "The_", "The." that motivates users to wipe 17.6 and use the Matrix :)

In fact, I do something similar currently in my local version. If the request for sort tokens responds with an error I am faking a default set of tokens. I could of course keep it like this and even add localized defaults. But then the tokens might differ from Kodi servers sort tokens and a different behavior is seen. Not sure if this is a good idea.

DaveTBlake commented 3 years ago

but wouldn´t the error response itself be also good enough to know there are no tokens provided?

Yes I guess you could inspect the error code for invalid parameters. But if you already make a JSONRPC.Version call then why not use that data rather than make an additional request that will fail.

wutschel commented 3 years ago

Hmm, would need some guidance on the best way to implement this as the JSON results come asynchronous. Not sure if it is a good idea to nest the JSON calls into each other. But such change can for sure be done. For now it works via risking a JSON error response and take this as sign of Kodi server not providing tokens.

DaveTBlake commented 3 years ago

Not sure I understand your nesting JSON calls comment. I was thinking that the app would get the API version on start up, know what version is running, and then use wherever and whenever. Once you have version you know to request sort tokens, or not to bother. But honestly I have poked my noise in enough :)

howie-f commented 3 years ago

theory: i would store major, minor and patch of the jsonrpc.version call into variables before you ‚appdelegate‘ them. next would be

if ( jsonrpc >= 9.5.0) getsorttokens rpc call; else fake(the, the_....);

howie-f commented 3 years ago

we‘re gettin it together @DaveTBlake don‘t run away 😅

wutschel commented 3 years ago

But honestly I have poked my noise in enough :)

Thanks for poking. Helped a lot ;)

No worries, we can sort this out later. Anyway I would like to have the other PRs reviewed, reworked and merged before moving on. There are already too many loose ends.

howie-f commented 3 years ago

all good, i think @kambala-decapitator has an eye on it... if you want me to help a bit on your branches but not pr yet, feel free to invite me as collaborator on your fork

wutschel commented 3 years ago

What would be a good default for the tokens? Older versions of Kodi server (e.g. the 17.6 I still use for music playback) allow the enable sort tokens, but it is not possible to read them.

"The", "A" and "An" (each with variants ending with " ", "." and "_")?

howie-f commented 3 years ago

my personal vote is The, A, An + their variants as it's a fallback for older kodi versions. kodi's resource.language.en_gb only defines The right now but that shouldn't be an issue. https://github.com/xbmc/xbmc/blob/83387b985ea65596057e20ea3ba0548473b97076/addons/resource.language.en_gb/addon.xml#L17-L19

DaveTBlake commented 3 years ago

Butting in again :)

kodi's resource.language.en_gb only defines The right now but that shouldn't be an issue.

I have no problem with the remote diverging from Kodi English default, but have you considered it could be just the for a good reason? Perhaps English speaking people expect to see names and titles that start "A ..." or "An ..." under "A", I know I do.

wutschel commented 3 years ago

I am fine with only using "The". We just need to decide. :)

In my -- yes, I know, very old -- Kodi 17.6 obviously "A" is also still used.

DaveTBlake commented 3 years ago

Not an iPhone owner so I have no skin in the game, and no strong feelings about it, but I just thought I may be the only native English speaker in the conversation. :)

howie-f commented 3 years ago

hm, i asked dr. goo.. and found artists: :) i (and just me) wouldn't expect them in the A section. A DAY TO REMEMBER A FLOCK OF SEAGULLS A FOOT IN COLDWATER

so i think it's your free decision @wutschel what you like more.

In my -- yes, I know, very old -- Kodi 17.6 obviously "A" is also still used.

also only The in core for v17, and v16.... not sure where the 'A' comes from