Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSON]Add sort video items by original title - SortByOriginalTitle #19435

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

JimmyS83
Copy link
Contributor

@JimmyS83 JimmyS83 commented Mar 16, 2021

This is Matrix version of a (now closed) PR originally submitted for Leia - #19434

Description

Adds sort method, according originaltitle variable/infolabel, with fallback to standard sorttitle, title.

Motivation and Context

Most of (newer) scrapers (such as default tmdb) supports scraping item Title in user’s choice language. They are able to store also original title into originaltitle variable (infolabel).

It could be displayed by skin in lists, etc. User can choose, which title he wants in lists, dynamically, even both of them for example.

The problem is, that Kodi lacks function to sort list according originaltitle. It supports only Title, and Sorttitle parameters. Sorttitle is written to DB during scraping, so its not dynamic parameter at all.

How Has This Been Tested?

Screenshots (if appropriate):

Example on dummy data, where both titles are visible to make a point more easilly: https://streamable.com/kgvjj9

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@fuzzard
Copy link
Contributor

fuzzard commented Mar 16, 2021

You will actually need to PR this against master branch.

Edit: forget the above, this is against master.

Our process is anything new must be merged into master, and then a backport may be considered for the previous version after this (ie Matrix currently).
Having said that, we generally don't backport features, only fixes, but I'll let @DaveTBlake be the decider on this one

@wsnipex
Copy link
Member

wsnipex commented Mar 18, 2021

jenkins build this please

@phunkyfish phunkyfish added this to the N* 20.0 Alpha 1 milestone Mar 21, 2021
@phunkyfish phunkyfish added Type: Feature non-breaking change which adds functionality v20 Nexus labels Mar 21, 2021
Copy link
Member

@Montellese Montellese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good apart from the missing JSON-RPC API version bump.

And please fix your commit messages to use at least a sentence like add support to sort items by original title or something similar.

xbmc/interfaces/json-rpc/schema/types.json Show resolved Hide resolved
@Montellese Montellese self-assigned this May 25, 2021
@JimmyS83 JimmyS83 changed the title Sort function by Original title - SortByOriginalTitle JSON-RPC sort by original title - SortByOriginalTitle May 25, 2021
@DaveTBlake DaveTBlake changed the title JSON-RPC sort by original title - SortByOriginalTitle [JSON]Add sort video items by original title - SortByOriginalTitle May 25, 2021
@DaveTBlake
Copy link
Member

I think that the first commit could also do with a better description too. For example:
"Add sorting of video items by original title".

Copy link
Member

@Montellese Montellese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks.

@DaveTBlake do you want to hold this back as it poses the same issue as #19372 concerning JSON-RPC version bumps?

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jul 12, 2021
@DaveTBlake
Copy link
Member

do you want to hold this back as it poses the same issue as #19372 concerning JSON-RPC version bumps?

Yes, would be useful to hold merging this until after 19.2 point release it out.

@JimmyS83
Copy link
Contributor Author

@DaveTBlake: I dont know, why build failed, when I set actual JSON version,

Running Prebuild steps
ERROR: This PR contains 1 merge commit(s)
Build was aborted

but, 19.2 is out already with 19.3 even?

@Montellese
Copy link
Member

We don't allow merge commits in pull request. You need to git rebase upstream master your branch onto Kodi's master branch instead of git merge it.

@JimmyS83
Copy link
Contributor Author

We don't allow merge commits in pull request. You need to git rebase upstream master your branch onto Kodi's master branch instead of git merge it.

Thank you for the hint. I did it just now, lets see what Jenkins and you think about that :)

Copy link
Member

@Montellese Montellese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general but I found a few things that need to be addressed. It also looks like you lost the JSON-RPC version bump in your rebase.

xbmc/SortFileItem.h Outdated Show resolved Hide resolved
xbmc/SortFileItem.h Show resolved Hide resolved
xbmc/utils/SortUtils.h Show resolved Hide resolved
xbmc/filesystem/PluginDirectory.cpp Outdated Show resolved Hide resolved
xbmc/video/GUIViewStateVideo.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/GUIViewStateVideo.cpp Show resolved Hide resolved
@JimmyS83
Copy link
Contributor Author

I tried to adress your mentions, let me know how you see it, eventually I will rebase it again, when you will be happy.

@JimmyS83
Copy link
Contributor Author

JimmyS83 commented Oct 25, 2021

I did rebase, because I am leaving to week long holiday, just in case..

@phunkyfish
Copy link
Contributor

@phunkyfish Hello, yes, I started to be a bit tired of updating just JSON number over and over, when this doesnt seems to be any closer to the merge.. :(

Fair point. I’ve get that pain. Let’s see can we get this merged.

@DaveTBlake you ok with this being merged once the version is added?

@JimmyS83
Copy link
Contributor Author

JimmyS83 commented Jan 8, 2022

@phunkyfish Hello, yes, I started to be a bit tired of updating just JSON number over and over, when this doesnt seems to be any closer to the merge.. :(

Fair point. I’ve get that pain. Let’s see can we get this merged.

@DaveTBlake you ok with this being merged once the version is added?

I am fine with bump number of JSON. Current is I believe 12.7.0. If somebody really merge this after resolving all other possible things, just tell me wanted version number, I will add it before squash/rebase at the end.

JimmyS83 added a commit to JimmyS83/skin.confluence2 that referenced this pull request Jan 8, 2022
Very usefull switch, when you use parser able to parse both Original and Localised titles. When xbmc/xbmc#19435 will be merged (Nexus hopefully), sort method will be also available for OriginalTitle.  POC video in PR above.
@JimmyS83
Copy link
Contributor Author

I added requested comment for translators, I added again JSON version (which was changed again 2 days ago), but I dont know if anyone merge this before another JSON version bump...

@JimmyS83 JimmyS83 requested a review from ksooo January 10, 2022 11:15
@ksooo
Copy link
Member

ksooo commented Jan 10, 2022

@JimmyS83 please fix the clang-format issues, otherwise the PR wont get merged.

@JimmyS83
Copy link
Contributor Author

@JimmyS83 please fix the clang-format issues, otherwise the PR wont get merged.

I added formatting changes proposed by jenkins.

@lrusak lrusak closed this Jan 11, 2022
@lrusak lrusak reopened this Jan 11, 2022
@JimmyS83
Copy link
Contributor Author

I think that one machine has problems with disc space or smth, otherwise everything should be OK.

@phunkyfish
Copy link
Contributor

Jenkins build this please

@JimmyS83
Copy link
Contributor Author

Merge me please :-)

@phunkyfish phunkyfish merged commit f4f14b7 into xbmc:master Jan 12, 2022
@phunkyfish
Copy link
Contributor

Thanks for sticking with it @JimmyS83!

@JimmyS83 JimmyS83 deleted the sort_original_title_v19 branch January 12, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change: JSON-RPC Type: Feature non-breaking change which adds functionality v20 Nexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants