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

[Feature][video]Added Video Versions feature #14972

Merged
merged 1 commit into from
Nov 22, 2023
Merged

[Feature][video]Added Video Versions feature #14972

merged 1 commit into from
Nov 22, 2023

Conversation

xodidox
Copy link
Contributor

@xodidox xodidox commented Nov 29, 2018

Description

Added Video Versions feature

Video Versions feature support different video versions for library video items, which can be used to add associated videos to a video item in library.

Based on this feature, the movie versions and video extras support have been added to Kodi with this PR. A special designed video versions dialog has been added to represent versions information.

(OLD) Step by step instruction for usage:
https://forum.kodi.tv/showthread.php?tid=337992

A picture is worth a thousand words

dialog

Motivation and Context

Video versions and extras support is a long time requested feature

How Has This Been Tested?

This has been tested with Kodi running on MacBook

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
  • 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

@MartijnKaijser
Copy link
Member

Very nice and a long wanted addition for such a feature. I'll leave the technical review over to the developers.
👍

@DaVukovic
Copy link
Member

DaVukovic commented Nov 29, 2018

I ran a first test.

A question I have is...let's say if I have 2 movies in different versions in my library. For example "Harry Potter and the chamber of secrets" and "Harry Potter and the chamber of secrets Extended".

I had different sets for those movies

I navigated to the "normal" movies, managed it, and let it point to the extended version of that movie. As a result, the selected movie is completely gone from the library and only the other movie I linked it to exists (I guess that's expected).

How do I undo that change?

If I navigate to the movie that still exists and use "manage" and "manage movie version", I only get a "there's more than one version...." notice

xbmc/dialogs/GUIDialogContextMenu.h Outdated Show resolved Hide resolved
xbmc/filesystem/VideoDatabaseDirectory/DirectoryNode.h Outdated Show resolved Hide resolved
xbmc/filesystem/VideoDatabaseDirectory/QueryParams.h Outdated Show resolved Hide resolved
xbmc/interfaces/legacy/ListItem.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoInfoTag.cpp Outdated Show resolved Hide resolved
@yol yol added Type: Feature non-breaking change which adds functionality Component: Video v19 Matrix labels Nov 29, 2018
@xodidox
Copy link
Contributor Author

xodidox commented Nov 29, 2018

@DaVukovic That's expected and by design. The library entries for alternate version of movies are deleted. Only the main movie entry left. There is no way to undo this currently unfortunately. You have to remove the movie from library and rescrap them.

@xodidox
Copy link
Contributor Author

xodidox commented Nov 29, 2018

Just noticed that when I change the version in video info dialog for movie, the information at the bottom (video resolution, duration, etc.) didn't change. I have to exit the dialog and reenter to see the updated data. Seems a little lame. Is there any way to show the updated data without exiting the dialog?

Update: Fixed

@DaVukovic
Copy link
Member

@DaVukovic That's expected and by design. The library entries for alternate version of movies are deleted. Only the main movie entry left. There is no way to undo this currently unfortunately. You have to remove the movie from library and rescrap them.

Ok, so if there´s no way to undo that (beside removing and re-adding) I didn´t see an option to play the 'other' (the removed) version. If I enter the movie information dialog and hit the "Version" button, I saw 2 times "STANDARD" listed. And not something like "EXTENDED".

Is there an option to play the 'other' movie (the one that was removed from the library)?

If not, it would be neat if you could add that functionality as I guess users would love to access both versions of the movie if they have it on their storage. As a suggestion...there could be some kind of menu, if you hit "Play" and the selected movie has 2 versions, there could be a dialog where you choose the version you would like to play.

@xodidox
Copy link
Contributor Author

xodidox commented Nov 30, 2018

@DaVukovic The movie version type (STANDARD/EXTENDED, etc.) is set by user when you convert the movie as an additional version for another movie. In the select movie version dialog (the 2nd dialog for "Mange movie verison" menu), there is a button "Add movie to a new version" at the right side, this is similar with the adding movie to set dialog. With this button, you can name the movie version as "EXTENDED", if you didn't do that, the default is "STANDARD". That's why you see two "STANDARD".

This feature itself doesn't try to identify the movie version as EXTENDED or not as there is no standard way for that, the only information we can get may be from the video file name, but that shouldn't be done in core, may be from scrapper is more appropriate. I think set/choose by user should be more suitable in this case.

When you click the "Version" button and choose a different "STANDARD", you can see its a different version of movie by checking movie duration data at the bottom. Selecting movie version is done with "Version" button. When you select one version, that version will be the default version for this movie until you change it again, this states is saved in database. So anytime you click the play button, it will play the version you just chose by the "Version" button. I think this is better than asking user to choose the version each time when they click the "Play" button.

The feature also support to name the movie itself as an specific version. For example, if you only have the EXTENDED version of a movie, you can choose the movie itself when manage the version, and select/add the specific version. When you click the "Version" button after that, you will see there is only the EXTENDED version available.

@xodidox
Copy link
Contributor Author

xodidox commented Nov 30, 2018

@Rechi @DaVukovic @tadly All concerns have been addressed with the new commit and added/fixed several things. Please check the update.

xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoDatabase.cpp Outdated Show resolved Hide resolved
xbmc/video/VideoInfoTag.cpp Outdated Show resolved Hide resolved
@da-anda
Copy link
Member

da-anda commented Nov 22, 2023

hi. Sorry for not finding it, but I was just browsing the code here on Github and wondered how the video versions are detected? We usually have detection patterns configurable by users via advancedsettings.xml while providing a sane default, like a nifty regex that finds movie name (year) - version.ext and movie name (year) [version].ext and could extract the name of the version from there (either file or folder name). It is no problem if this is not implemented (yet), was just curious if I just haven't spotted the correct place where the detection is taking place or if there is no automation for this yet (apart from asking if the same movie is found multiple times). If there is no automatic detection of the version type, see this comment as a feature request for a follow up PR 😉

Thanks for the great work on this - looking forward to see it in action 👍

@xodidox
Copy link
Contributor Author

xodidox commented Nov 22, 2023

Hi @da-anda , the version auto detection is not implemented yet. A good discussion is needed for this before implementing. Currently, only the names for video extras are generated automatically based on file names (non useful characters are removed).

@ksooo ksooo merged commit 8f075c5 into xbmc:master Nov 22, 2023
2 checks passed
@xodidox xodidox deleted the movieversion branch November 22, 2023 14:50
@da-anda
Copy link
Member

da-anda commented Dec 5, 2023

@xodidox one thing I just noticed is, that my videoversiontype table in the DB is starting with ID 40400 and also contains a lot of empty values (name empty and owneris 0). Just mentioning it as there could be an issue, but haven't investigated myself yet

@Ch1llb0
Copy link

Ch1llb0 commented Dec 15, 2023

@xodidox, testing this with v21 Beta 2 I've noticed that the rename button always opens a list of versions even when the extras list is highlighted. Shouldn't this open a list of extras titles instead of versions titles?

Another thing: The rename and the set default buttons (ID 25 and 26) are newer enabled - they always remain in a disabled state.

Nice feature BTW!

@ksooo
Copy link
Member

ksooo commented Dec 15, 2023

Please use the forum to give feedback or even better open an issue here if you find a bug. Thanks.

@da-anda
Copy link
Member

da-anda commented Dec 15, 2023

just as a side note to the just mentioned issue regarding Extras names. IMHO the name of extras should also not end up in the version types DB like it currently seems to be the case. Their name should be defined by the filename and name changes should be handled via the infotag or alike but not as version type (like Director's cut etc), which seems to be the case atm (at least my DB column contains names of extras where they don't belong). So if somebody is looking into it and trying to fix the rename issue, the entire implementation should better be changed.

@Ch1llb0
Copy link

Ch1llb0 commented Dec 15, 2023

Please use the forum to give feedback or even better open an issue here if you find a bug. Thanks.

Sure, no problem: #24245

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

just as a side note to the just mentioned issue regarding Extras names. IMHO the name of extras should also not end up in the version types DB like it currently seems to be the case. Their name should be defined by the filename and name changes should be handled via the infotag or alike but not as version type (like Director's cut etc), which seems to be the case atm (at least my DB column contains names of extras where they don't belong). So if somebody is looking into it and trying to fix the rename issue, the entire implementation should better be changed.

The extras names ARE from file names (and folder names if exist), but it's needed for user to customize the extras names as auto generated names are not always good. Video extras are treated like a special type of video version, the "owner" column in the table is for this purpose. More values can be defined for this column in the future if we want to add more feature, for example, use a dedicated value to identify "Behind The Scenes" types of extras.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

@xodidox one thing I just noticed is, that my videoversiontype table in the DB is starting with ID 40400 and also contains a lot of empty values (name empty and owneris 0). Just mentioning it as there could be an issue, but haven't investigated myself yet

Missed this. The id is from 40400, as we're currently using the string id value in code directly, so we can use localized version types. All system defined version types will use localized ones, user defined version types will only be what they are defined.

@da-anda
Copy link
Member

da-anda commented Dec 15, 2023

@xodidox one thing I just noticed is, that my videoversiontype table in the DB is starting with ID 40400 and also contains a lot of empty values (name empty and owneris 0). Just mentioning it as there could be an issue, but haven't investigated myself yet

Missed this. The id is from 40400, as we're currently using the string id value in code directly, so we can use localized version types. All system defined version types will use localized ones, user defined version types will only be what they are defined.

this is IMHO a terrible design to use the string IDs also as DB IDs as this is subject cause issues on the long run (custom values will eventually have an ID that a new label ID has).
And I think that the names of Extras should not be in this DB table at all as extras are not unified enough in their naming, apart from a behind the scenes label that most have. Like when you have 10 deleted scenes in a movie, the name of those scenes end up in this table and would therefore be selectable as name for an Extra from another movie? This just doesn't make any sense. What you have in mind only makes sense to categorize extras, but not for their name. So in this table could be things like behind the scenes, deleted scenes, interviews etc, but certainly not the name of the particular extra. If such a category only has one extra, it could be played directly, if it has multiple, you could browse them. But for this, the extras logic has to be separated from the versions. That's IMHO the only sane way to handle extras (an extra is not a version after all).

edit: as for the label IDs in the DB ID column
it would be better if those IDs would be the label string in the DB, and when fetching the entries you check if the label is an integer, and if so, use our translation system to fetch the correct label. This way you also don't have to re-import/update the DB when the UI language changes

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

this is IMHO a terrible design to use the string IDs also as DB IDs as this is subject cause issues on the long run (custom values will eventually have an ID that a new label ID has).

This won't happen, the id range 40000 to 40800 are reserved for Video Versions feature. And only range 40400 to 40800 are used for system defined version types. Other ids can be freely used by other code.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

as for the label IDs in the DB ID column
it would be better if those IDs would be the label string in the DB, and when fetching the entries you check if the label is an integer, and if so, use our translation system to fetch the correct label. This way you also don't have to re-import/update the DB when the UI language changes

UI language change is a thing that only happens very rarely (only zero or once for most users), why involve the overhead to translate strings each time?

@da-anda
Copy link
Member

da-anda commented Dec 15, 2023

you have to update those strings with every new version of the language add-on as they might have changed. And the videodb most certainly should not have to know about add-on updates. We will still have to do DB migrations if new labels (IDs) get added, but at least don't have to keep track on language add-on versions/updates.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

this is IMHO a terrible design to use the string IDs also as DB IDs as this is subject cause issues on the long run (custom values will eventually have an ID that a new label ID has).
And I think that the names of Extras should not be in this DB table at all as extras are not unified enough in their naming, apart from a behind the scenes label that most have. Like when you have 10 deleted scenes in a movie, the name of those scenes end up in this table and would therefore be selectable as name for an Extra from another movie? This just doesn't make any sense. What you have in mind only makes sense to categorize extras, but not for their name. So in this table could be things like behind the scenes, deleted scenes, interviews etc, but certainly not the name of the particular extra. If such a category only has one extra, it could be played directly, if it has multiple, you could browse them. But for this, the extras logic has to be separated from the versions. That's IMHO the only sane way to handle extras (an extra is not a version after all).

The version types for extras one movie are not expected to be used by other movies, that why they are filtered out when selecting a version type. Only some common version types (Behind The Scenes, Deleted Scenes, etc.) can be shared. This is the fix I intended to do to fix the "rename" button behavior for extras.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

you have to update those strings with every new version of the language add-on as they might have changed. And the videodb most certainly should not have to know about add-on updates. We will still have to do DB migrations if new labels (IDs) get added, but at least don't have to keep track on language add-on versions/updates.

Not sure I understand this. Currently only a language switch will trigger the refresh of the localized strings. A new version of language add-on will switch language? This is the code to trigger the refresh https://github.com/xbmc/xbmc/blob/37865f4ab505fd697c1942363a2f347b7116aa79/xbmc/LangInfo.cpp#L794C64-L794C64

@da-anda
Copy link
Member

da-anda commented Dec 15, 2023

Not sure I understand this. Currently only a language switch will trigger the refresh of the localized strings. A new version of language add-on will switch language? This is the code to trigger the refresh https://github.com/xbmc/xbmc/blob/37865f4ab505fd697c1942363a2f347b7116aa79/xbmc/LangInfo.cpp#L794C64-L794C64

I don't think an add-on update will trigger a language change, but the updated add-on could have a changed string for one of the videoversion label IDs (like when it had a typo, inconsistent spellings etc) and thus the DB values would have to be updated when the language add-on of the currently used language is updated. I haven't checked which events/signals/messages are fired in this case, but it is one thing that needs to be taken care of. And to simplify things, my suggestion would be to translate on the fly. It's not like the version type labels are fetched/read frequently either.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 15, 2023

I don't think an add-on update will trigger a language change, but the updated add-on could have a changed string for one of the videoversion label IDs (like when it had a typo, inconsistent spellings etc) and thus the DB values would have to be updated when the language add-on of the currently used language is updated. I haven't checked which events/signals/messages are fired in this case, but it is one thing that needs to be taken care of. And to simplify things, my suggestion would be to translate on the fly. It's not like the version type labels are fetched/read frequently either.

I see your point. In this case, the DB won't be refreshed. User can just set the language to another one and set it back, which will trigger the refresh and got the corrected ones.

@xodidox
Copy link
Contributor Author

xodidox commented Dec 20, 2023

Kodi team, please keep your dignity, delete all of my code from your code base, and never use my code and ideas for this feature in your code base.

@ksooo
Copy link
Member

ksooo commented Dec 20, 2023

delete all of my code from your code base

Nothing you can request, sorry. By contributing code to this project you donated it to the oss community, it’s not your code anymore.

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

Successfully merging this pull request may close these issues.