Movie Editions Feature #21469
Movie Editions Feature #21469
Conversation
| @@ -2502,8 +2503,60 @@ bool CApplication::PlayStack(CFileItem& item, bool bRestart) | |||
| return PlayFile(selectedStackPart, "", true); | |||
| } | |||
|
|
|||
| void CApplication::SelectMovieVersion(CFileItem& item) | |||
There was a problem hiding this comment.
I don't think dumping even more GUI stuff into CApplication is the way to go unfortunately. We need some sort of mechanism to bridge this into GUI via messaging, I am not sure if we already have any pattern like this in the codebase.
There was a problem hiding this comment.
I don't disagree, though I'm not very familiar with this codebase and my C++ skills are mediocre at best. The only thing I can think of is maybe mimicking the resume/restart logic? But after doing some skimming it looks like maybe too much of that is stashed in the PlayerCore, and this definitely doesn't belong there either.
There was a problem hiding this comment.
It's been a while since I have messed with this part of the code when I implemented the StereoscopicsManager, but the core should fire a onBeforePlaybackStarted kinda message which you might be able to hook into and display the select dialog. StereoscopicsManager is doing a similar thing, albeit IIRC one step further down the playback pipeline.
My C++ skills are also mediocre at best (didn't know C++ at all before I started poking around in KODI), so if I was able to pull off the StereoscopicsManager in a non-evasive way, you should be able to do something similar for editions.
| @@ -20,6 +20,8 @@ | |||
| #include "cores/VideoPlayer/DVDFileInfo.h" | |||
| #include "dialogs/GUIDialogExtendedProgressBar.h" | |||
| #include "dialogs/GUIDialogProgress.h" | |||
| #include "dialogs/GUIDialogSelect.h" | |||
There was a problem hiding this comment.
another component that was not supposed to be filled with gui includes
With this feature, it's possible to connect more than one video file with one movie entry in library. For example, Kodi will support both STANDARD version and EXTENDED version of a movie live under one movie item. A context menu item "Manage movie version" under "Manage" menu item has been added to the GUI. A "Version" button has also been added to the video info dialog for movie, which allows user change the default video file for the movie. The change will be saved to DB. Added common movie versions Change movie verson dialogs tweak (change fouchs, etc.) Close video info dialog after version changed Moved the type_link table cleanup to movie delete trigger Fixed the videodb://movies/types opening failure issue Optimized version adding logic, giving user warning when a duplicated version is selected Fixed staled video info tag after version changed Fixed ListItem.FileNameAndPath label issue Fix the default section issue Added the version selection dialog when playing movie with multiple versions based on feedback and enabled by default. Added a setting in player settings to give user option to turn the dialog off Fixed scanning failure for versioned movie Fixed issue for version selection dialog missing when playing from the context menu Fixed issue for staled file item info in media window after version change Fixed issue for GUI_MSG_UPDATE_ITEM to update file item in a inactive media window Fixed issue for staled strBasePath in movie DB table Merge movie versions at scanning time Add duplicate movie checking in scanning and ask user to convert it to an additional version of the original movie Added setting for the duplicate video scanning Report video file name in duplicate movie found dialog to give user more informaiton Rebased with updates by ccope
|
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. |
| @@ -1186,7 +1186,7 @@ bool CGUIWindowVideoBase::OnPlayAndQueueMedia(const CFileItemPtr& item, const st | |||
|
|
|||
| void CGUIWindowVideoBase::PlayMovie(const CFileItem *item, const std::string &player) | |||
| { | |||
| if(m_thumbLoader.IsLoading()) | |||
| if (m_thumbLoader.IsLoading()) | |||
There was a problem hiding this comment.
Unrelated change (but still welcome). Don't worry about this now, but as the PR progresses you'll incorporate feedback and combine the commits into a clean history, and at that point you'll want to split this out into a second commit.
Keep an eye on those "lines added" and "lines deleted" metrics. The main commit should ideally have 0 lines deleted. This indicates to me that no existing code is touched, making the feature much safer and more desirable. By splitting out unrelated changes you can probably get the main commit to be 950 lines added, 0 lines deleted, some most impressive metrics.
|
I'll do a thorough review when the PR is marked as ready status. I really like this feature though. Question, how do you scrape the movie edition? My current approaches uses a script that looks for e.g. "directors_cut" in the filename and modifies the nfo file to modify the title shown in the GUI
However, this has the downside that if I've ever renamed a file in my ripping history, then I could lose the edition modifier. |
Currently, there is no automatic scraping. Creating edition types and categorizing files is currently a manual process left to the user. IMO this feature is most useful for cases where a user would want to keep multiple versions of the same film in their library. I think there are a few versions that might be broadly used, but I think in most cases users won't keep multiple versions of those films anyway. There are also a lot of versions that are one-off's (typically special cuts) that will never be scrape-able unless some kind of standard metadata field is created. I don't think I'd use this for different resolutions or 3D since that should already be visible from the codec metadata. |
|
In a library of 1626 movies, these are the 115 marked editions I have: |
| if (selectedItem) | ||
| { | ||
| std::string path = selectedItem->GetLabel2(); | ||
| item.SetPath(path); |
There was a problem hiding this comment.
Please check if possible if this is needed. I assume it's not.
CFileItem has 2 different paths: the path (GetPath) and the dynamic path (GetDynPath). The path works as an identifier for the item and should not change throughout the item lifecycle, if you want to set a new path after selecting a version the way to go is to change only the dynamic path. At this point a path for the item likely already exists judging by the code above.
| @@ -1706,6 +1706,7 @@ void CFileItem::UpdateInfo(const CFileItem &item, bool replaceLabels /*=true*/) | |||
| m_videoInfoTag = new CVideoInfoTag; | |||
| } | |||
|
|
|||
| SetDynPath(item.GetDynPath()); | |||
There was a problem hiding this comment.
please also check if this is needed. GetDynpath will fallback to the path if dynpath is not set. I don't see a reason why you need to set a dynpath to the already defined dynpath. The only reason I see is to force the path to be the dynpath in cases where the dynpath is not set. But then, getdynpath will always fallback to the path which makes me question the need here
jenkins4kodi
added
the
Rebase needed
|
@ccope this needs a rebase |
|
After the rebase, if someone could kick off a build, I can then test this. |
|
Both jellyfin and emby support a scheme, where the resolution/version is stored as part of the file name: https://jellyfin.org/docs/general/server/media/movies.html#multiple-versions-of-a-movie Example: In order for kodi to co-exist nicely with them, it might be an idea to support a similar scheme. It's also an awful lot nicer to simply name files a certain way rather than having to manage the versions as part of the kodi UI. |
|
You make a good point. The first thing is to gather all existing standards for editions. Then we choose the one that makes the most sense, or define our own. I personally don't like the example, because I have a requirement - the file name has to match the folder name. Here is how my library is laid out: |
|
There are 2 different things at play here:
The use cases are different (and so are the requirements as a consequence). In case of the first one, you would probably only want a single entry for the movie, but have an option to choose the resolution on launch. In case of the latter, you would want to see each version as a separate entry as they are technically (or at least can be) different movies. |
|
I'm very interested in this update. My main use is going to be for different resolutions (1080p/2160p), but my main hope is that I can use it to properly handle my 3D movies. Is this going to make it into Nexus? I don't see it on the list of features (but I may well have missed it of course). |
|
I don't think [720] or [1080p] would have to be an "edition", as we can get this info directly from the stream details. So we could automatically group those kind of videos without the necessity to add some special "edition" parsing logic for these. So I would suggest that whatever is inside |
|
Instead of having special handling to read the stream details which would in any case require you to have some mechanism to say "these files are actually all the same movie, but just different editions", wouldn't it be far simpler to only deal with edition through the square brackets (or whatever mechanism is made)? You could easily foresee that some people for the sake of keeping things easy on the home-front would label them as |
|
Kodi really doesn't have to care about the editions other than "here's a bunch of files that all resolve to the same movie so prompt to choose edition on play". This (to me) seems like the most intuitive and simplest solution overall. |
I agree with these sentiments, as it allows for non-technical users to get multiple versions of movies without much hassle. |
|
Looks like we missed this being merged into v20. What else needs to be done before it can be merged? I am willing to test if that is needed. |
|
Testing is not the issue here.
There seems to be consensus that a more simplified approach (let’s call it approach “B” which is outlined in my earlier comment) is the way to go rather than the approach taken here (approach A).
*But* and this is pretty much as big a but as it gets - I have written exactly 0 lines of code to support approach B whereas @ccope actually wrote a functional prototype using approach A. So what I would like accounts for nothing in this conversation.
I can only appeal to @ccope to follow approach B instead which he may or may not choose to do. Or I can just shut up and write a PR using approach B but that’s realistically not going to happen any time soon.
So what is needed:
1. someone needs to decide on the correct/best approach
2. someone then needs to either write that from scratch or make this PR mergeable
3. then it needs to be tested
4. merge time!
Using approach B, people would be able to drop the “Extras” addon as that functionality could be covered by simply renaming the files accordingly.
If you are looking at a temporary workaround, the “Extras” addon will be able to help. It’s not super user friendly, but it works.
|
Description
I'm reviving #14972, I'm hoping this can make it into Kodi 20. Huge thanks to @xodidox for getting this so close to the finish line!
Remaining TODOs from the previous PR:
Copying most of the rest of this template from the original PR.
Added movie version feature
With this feature, it's possible to connect more than one video file with one movie entry in library. For example, Kodi will support both STANDARD version and EXTENDED version of a movie live under one movie item. A context menu item "Manage movie version" under "Manage" menu item has been added to the GUI. A "Version" button has also been added to the video info dialog for movie, which allows user change the default video file for the movie. The change will be saved to DB.
Step by step instruction for usage:
https://forum.kodi.tv/showthread.php?tid=337992
Motivation and context
Adding movie version support to Kodi has been asked many times in forum and I personally also want this feature for a long time
How has this been tested?
This was last tested before I rebased it. I need to get my Kodi test environment working again, and I'll see if anyone else in the forum thread can test as well. I just want to get the bikeshedding done in parallel.
What is the effect on users?
This will allow users to add extra release types (Director's Cut, Extended Edition, RiffTrax, etc), import extra copies of movies with those release types, and choose which type of the film to play.
Screenshots (if appropriate):
Types of change
Checklist: