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/RPC Support adding new videos #20797

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hupfdule
Copy link

@hupfdule hupfdule commented Jan 5, 2022

Description

This PR adds support for adding new movies to the library.

I plan to extend it to support adding TV shows, TV show series and TV show episodes as well as Music Videos, but want to get these changes for the addition of movies into shape first.

Motivation and context

This PR extends the utility of the JSON/RPC interface by adding support for the creation of new entries in the library.

At the moment it is possible to set all details for movies (and TV shows, etc.) via JSON/RPC, but only for files that are already existing in the library. This requires a successful scan of those videos into the library.

In the case the scan does not import those files or using the scan functionality is not desired, it is not possible to add those items to the library via JSON/RPC.

How has this been tested?

I tested it by executing the corresponding JSON/RPC function. The files were correctly inserted into the database and are accessible from the Kodi UI then. Calling VideoLibrary.SetVideoDetails afterwards does also work as expected.

What is the effect on users?

It increases the utility of the JSON/RPC interface by offering users a way to create entries for movies (TV shows, etc.) in the library.

Screenshots (if appropriate):

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 (new functions in the JSON/RPC documentation in the Wiki)
  • I have updated the documentation accordingly (I didn't change the wiki, as this PR is not yet integrated)
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

Open questions:

The PR is (apart from the missing functionality to add other types of video than movies) ready.

However I have a few questions about possible changes to the PR.

  1. To be able to call VideoDatabase::AddNewMovie from the VideoLibrary I had to change the visiblity of it.
    If this is not desired, how should I handle his? Add another method in VideoDtabase that delegates to VideoDatabase::AddNewMovie? This would not provide any benefit in my opinion.

    hupfdule@8c265cd#diff-1c9dd1b7b484ba7a64c670d64a855bfe5ff9e8ba31da75a3be351223a3b61815R915-R917

  2. At the moment this new function only creates the entry in the library and sets the title to the filename. All remaining information has to be set afterwards by calling VideoLibrary.SetMovieDetails. It would be possible to allow setting all the same fields that can be modified via VideoLibrary.SetMovieDetails in VideoLibrary.AddMovie as well. This would remove the need for calling a second function by providing the ability to create the entry and fill it with data in the same call.
    Is there any guideline/convention on whether to provide this functionality?

@jenkins4kodi
Copy link
Contributor

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

For more information please see our current code style guidelines.

@keithah keithah added v18 Leia and removed v18 Leia labels Jan 8, 2022
@DaVukovic
Copy link
Member

DaVukovic commented Jan 12, 2022

Can't comment on the code and if it's correct or not.

But could you please explain a usecase where a user is in need of using the JSON RPC to add a movie which haven't been added to the library by scraping? Most likely, if scraping fails, it's a naming problem of files/folders and users should probably rename their files in order to be scraped correctly. So, what's the usecase to forcefully add a movie to the library if the scraper fails to scrape?
A partially answer might be, homemade movies (from holidays and such). Most likely users are using NFO files for that. So I still don't see the need to use the JSON RPC for that. As creating NFO files for those entries is way easier and will be future proof if the library needs a rebuild.

Also not too sure if using the filename as the title is a good idea. Tools like MakeMKV for example are often using default filesnames such as title01.mkv if not changed afterwards. You might end up with multiple entries with the same title. If changed afterwards, it should have been changed to something the scraper is able to deal with.

@hupfdule
Copy link
Author

Spontaneously the following reasons spring to mind:

  • the file is strangely named
  • the file is in an unusual location of the file system
  • the file is not in a database to be scraped from
  • the file is a homemade video
  • scraping is not working for any reason

and

  • the user does not want to change the name / location of the file
  • the user does not want to clutter the filesystem with .nfo files
  • the user does not want to use scraping at all

for whatever reason.

You mention that .nfo files would allow getting files into the library that
cannot or should not be scanned otherwise. The same is true for setting
data on files that already exists in the library. Yet,
VideoLibrary.SetMovieDetails (and the same for the other types of files)
exists in the JSON/RPC interface. Why does that method exist when an nfo
file would also allow setting this data?

It is the same reason: It provides a way to fill the library with
information via JSON/RPC.

The JSON/RPC API provides functionality to be performed via
HTTP/Socket/etc. There is no reason to reduce the API to functionality that
cannot be accessed otherwise. In fact nearly all functionality provided by
the JSON/RPC API is also accessible via other means. But the existance of
the API allows for a wide area of uses cases that the authors of the API
don't even think of. That's the main reason for a standardised generic API.

In fact users of the JSON/RPC API are not the same users as those generating
.nfo files (they may be the same users, but they use them for different uses).

Regarding this concrete PR: The functionality provided by it is very much
in line with the already existing functionality of
VideoLibrary.SetMovieDetails, etc. In my opinion there is no reason to
include one, but exclude the other.

@hupfdule
Copy link
Author

Also not too sure if using the filename as the title is a good idea. Tools like MakeMKV for example are often using default filesnames such as title01.mkv if not changed afterwards. You might end up with multiple entries with the same title.

The reason the file name is set as the default title is that otherwise the title would be blank. Having multiple entries with a title of "title01" is better than having multiple entries with blank titles. It is expected of course that the user of the API uses the VideoLibrary.SetMovieDetails method afterwards to set a better title (and fill in any additional information).

Therefore my question above:

At the moment this new function only creates the entry in the library and sets the title to the filename. All remaining information has to be set afterwards by calling VideoLibrary.SetMovieDetails. It would be possible to allow setting all the same fields that can be modified via VideoLibrary.SetMovieDetails in VideoLibrary.AddMovie as well. This would remove the need for calling a second function by providing the ability to create the entry and fill it with data in the same call.
Is there any guideline/convention on whether to provide this functionality?

This would allow for avoiding the "auto-generated" title at all by directly providing it in the same method call.

However, calling VideoLibrary.SetMovieDetails after VideoLibrary.AddMovie is already possible. As users of the JSON/RPC API are very likely not doing this manually, but instead from a script or a 3rd party application, calling both methods one after the other is not a big hurdle.

If changed afterwards, it should have been changed to something the scraper is able to deal with.

I am not sure what you mean here. What does the scraper use the title for? Actually a scraper would fill in the title as this method does. Also by using the JSON/RPC API it would not be necessary (but still possible) to use the scraper to override the title. The API is used to override it.

@a1rwulf
Copy link
Member

a1rwulf commented Jan 13, 2022

Afaik there is no way in Kodi to just add a single movie to the database, without either scraping or supplying an NFO file.
I don't see why you would add this as part of an API without having any other way of doing the same thing in Kodi's UI.

You can play video files on your system with existing calls already.
And scraping/nfo is the way to scan it into your library, which is also there already.

I can't see the benefit of adding movies to the database that contain zero metadata.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants