[video][Estuary] Video Versions: Improvements and Fixes #24430

ksooo · 2024-01-04T09:05:25Z

Here comes a bunch of improvements and fixes for the video versions feature:

This PR is not heading for fixing the (known) refresh problems after changing artwork. We have open issues for some of them.

For skinners:

new GUI infolabel Listitem.VideoVersionName- does what it says. Filled for all movies, empty for all other media types.

For users:

Movie window views, video info dialog, Home Screen widgets now should properly show version specific art.

* version specific art can now be edited also via context menu "Manage" -> "Choose art" and video info dialog's button "Choose art".

"Show videos with multiple versions as folders" improvement. Movie metadata (plot etc.) are now displayed in the movies window

Before:

After:

Runtime-tested on macOS, latest Kodi master.

@enen92 @CrystalP I tried to split commits into logical chunks. Hope that helps for the review.

CrystalP · 2024-01-04T23:18:21Z

It will take some time to absorb all the changes and comment, except for the artwork, that I just worked on and am more familiar with.

The description doesn't say what you tried to achieve so I can only describe what seem to be problems, not knowing for sure what is supposed to happen.

1/ Movie with single version and proper artwork: open manage versions. A thumb is displayed instead of the art of the movie. Also, (pre-existing bug) going to Choose Art shows no current art, not even the thumb.
2/ Add version with art to a movie that already has art. Set new version as default. Exit manage versions / info. Play the movie / navigate into the folder: the added movie has normal art, the Standard Edition does not, just the thumb.
3/ Taking this one step further. Add version with art to a movie that already has art. Set new version as default, then set the original "Standard Edition" as default again. The original art of the movie is lost, only the thumb remains. The poster of the additional version is still used. Remove the added version > the poster of the removed version remains the art of the movie
4/ Artwork change propagation not happening from Manage version to Info to Library list, and they don't display the new art until leaving and returning to the library (choose art, set as default buttons). Easy to fix with a GUI_MSG_UPDATE_ITEM.

Skinning / new infotag: of course it's just a preview, and actual appearance is subject to change, but I don't know if skins will have enough information to display the version name only when appropriate (different look for different contexts, like library list, versions folder, maybe more?).
Some users will have different preferences, but for example I don't want it in my face all the time, especially for movies that have a single version and that version is the default "Standard Edition". In the "versions folder" navigation the version name definitely has its place, but the movie name as prefix may not be needed (as it was before PR).

ksooo · 2024-01-05T02:34:45Z

It will take some time to absorb all the changes and comment, except for the artwork, that I just worked on and am more familiar with.

Shall I remove the two commits from this PR? Next time we definitely should sync before both of us put effort into the same things. I assigned the respective issue to myself and even took a note that I plan to work on this. S**t happens, but it would have been good if you would have communicated earlier that you are familiar with this code and that you want to fix this. :-/

Re skinning, as I said this is not more than a proof of concept. An experienced skinner needs to pick this. At some places the version name definitely should be displayed (Video info dialog, folder view of versions, ...)

1/ Movie with single version and proper artwork: open manage versions. A thumb is displayed instead of the art of the movie. Also, (pre-existing bug) going to Choose Art shows no current art, not even the thumb.

I can reproduce this. Both bugs are also in master. Nothing supposed to be fixed in this PR.

2/ Add version with art to a movie that already has art. Set new version as default. Exit manage versions / info. Play the movie / navigate into the folder: the added movie has normal art, the Standard Edition does not, just the thumb.

Oh, nothing I tested. Maybe a bug, maybe something also present in master. Must be fixed (most probably by you in your PR or already working there)?

3/ Taking this one step further. Add version with art to a movie that already has art. Set new version as default, then set the original "Standard Edition" as default again. The original art of the movie is lost, only the thumb remains. The poster of the additional version is still used. Remove the added version > the poster of the removed version remains the art of the movie

See 2)

4/ Artwork change propagation not happening from Manage version to Info to Library list, and they don't display the new art until leaving and returning to the library (choose art, set as default buttons). Easy to fix with a GUI_MSG_UPDATE_ITEM.

Nothing I wanted to fix in this PR. I have seen you asked somebody else to deal with the refresh problems and so I refrained from looking into this.

ksooo · 2024-01-05T03:13:56Z

@CrystalP I removed the artwork get/set changes from this PR. You said you can do better. Go ahead. And don't get me wrong please, no offence taken. I'm honestly happy that I don't have to deal with this super complex stuff myself...

garbear · 2024-01-05T03:34:24Z

Sorry I can't review, I don't have a good mental model of versions yet, but the least I can do is go through commit by commit and I saw no code issues.

ksooo · 2024-01-05T03:46:20Z

@Hitcher @jjd-uk maybe one of you guys can find some time to polish usage of the new listitem.videoversionname in Estuary? The current two commits are not more than an “inspiration ”. We easily could add the respective commits from you to this PR before merge.

ksooo · 2024-01-05T03:51:53Z

In the "versions folder" navigation the version name definitely has its place, but the movie name as prefix may not be needed (as it was before PR).

I’m not sure whether the movie title of all versions of a “parent” movie will always be the same as the title of the parent movie. If this is a given, then in this view the version name would be enough, yes.

Question is whether this is something that should to be implemented by the skin or the core.

ksooo · 2024-01-05T04:05:08Z

@CrystalP I put the two artwork commits back, as you said internally you want to add artwork fixes on top of this PR. This is much appreciated.

addons/skin.estuary/xml/Variables.xml

Hitcher · 2024-01-05T08:59:44Z

Some users will have different preferences, but for example I don't want it in my face all the time, especially for movies that have a single version and that version is the default "Standard Edition". In the "versions folder" navigation the version name definitely has its place, but the movie name as prefix may not be needed (as it was before PR).

See my comment: https://github.com/xbmc/xbmc/pull/24430/files#r1442634920

ksooo · 2024-01-05T11:40:18Z

@CrystalP

1/ Movie with single version and proper artwork: open manage versions. A thumb is displayed instead of the art of the movie.

Fixed with latest force-push. (CVideoDatabase::GetArtForItem fix to handle MediaTypeVideoVersion correctly - needs special lookup in art table)

Also, (pre-existing bug) going to Choose Art shows no current art, not even the thumb.

Fixed with latest force-push. (CGUIDialogVideoManagerVersions init + update fix)

ksooo · 2024-01-05T12:05:24Z

Sorry guys that this PR gets bigger and bigger, but all the changes build on top of each other.

ksooo · 2024-01-06T15:09:08Z

Squashed. No code changes. Ready to go in.

CrystalP · 2024-01-06T15:50:34Z

I don't know how the navigation / filtering for gui works. Since movie_view now returns one row per movie and version combination, can you explain where/how the filtering is done to show the library list as usual without the additional versions? Hopefully it's done by the db to avoid transferring extra rows that will be hidden most of the time and will take additional time to send for external db.

ksooo · 2024-01-06T16:21:29Z

I don't know how the navigation / filtering for gui works.

@CrystalP :

ksooo · 2024-01-06T16:27:10Z

Heads-up: I removed the commit that makes setting art for video versions using context menu "choose art" and video info dialog button "choose art" working. We will fix this in another PR. @CrystalP fyi

ksooo · 2024-01-06T16:59:58Z

Final cleanup of code (no functional changes).

ksooo · 2024-01-06T17:02:43Z

Need to trick Jenkins...

…now the movies with the respective versions, no longer video version items.

…alog.

Ch1llb0 · 2024-01-17T11:53:10Z

This change introduced that the ListItem.HasVideoVersions boolean is set to true even when browsing a versions folder... IMHO, this doesn't make much sense as it leads to all versions in that folder to be tagged as "having versions" - information we already have and that's frankly redundant. Using Estuary this leads to missing watched information and in my skin it just looks weird (only possible to prevent with very complicated visible conditions):

Wouldn't it be more elegant to set the boolean to false when browsing a version folder?

Hitcher · 2024-01-17T12:03:35Z

Using Estuary this leads to missing watched information

That will be fixed with this PR.

in my skin it just looks weird (only possible to prevent with very complicated visible conditions):

Not that complicated. I just added a !String.Contains(Container.FolderPath,videoversion) condition in mine.

Ch1llb0 · 2024-01-17T12:15:36Z

Not that complicated. I just added a !String.Contains(Container.FolderPath,videoversion) condition in mine.

Unfortunately, it is: My skin shows multiple icons in that poster overlay bar and I have to move icons depending on visibility of the other icons. Therefore, the visibility conditions are complex as is, but they'd be even more complicated, if I had to add another condition.

But the point was rather: It doesn't serve any purpose to turn this boolean true in a subfolder where this type of information would never make any sense (also not using Estuary). In a folder that shows versions, why return true that a version has versions? 😆

Hitcher · 2024-01-17T12:33:24Z

But the point was rather: It doesn't serve any purpose to turn this boolean true in a subfolder where this type of information would never make any sense (also not using Estuary). In a folder that shows versions, why return true that a version has versions?

I agree but don't know how trivial it is to do in core.

ksooo · 2024-01-17T15:01:53Z

The whole concept and implementation of "show movies with multiple versions as folder" needs to be rethought / redone. There are more things that make not really sense, but I'd rather go for think about all this in details before start band-aiding stuff and need to revise later.

CrystalP · 2024-01-18T23:27:34Z

xbmc/video/VideoDatabase.cpp

@@ -8029,6 +8048,9 @@ bool CVideoDatabase::GetMoviesByWhere(const std::string& strBaseDir, const Filte
    if (!videoUrl.FromString(strBaseDir) || !GetFilter(videoUrl, extFilter, sorting))
      return false;

+    if (!videoUrl.HasOption("videoversionid"))
+      extFilter.AppendWhere("isDefaultVersion = 1");


I'm reviewing how the version folder works and I know it's a bit far already,.. Do you remember if there is a reason for this block not to be integrated inside GetFilter() just above, with other videoversionid option logic?
I'm thinking about moving it in upcoming PR.

@CrystalP sorry, don't remember.

ksooo added this to the Omega 21.0 Beta 3 milestone Jan 4, 2024

ksooo requested review from CrystalP and enen92 January 4, 2024 09:05

ksooo mentioned this pull request Jan 4, 2024

Movie version artwork sync #24429

Closed

14 tasks

ksooo force-pushed the video-info-tags-asset-info branch from a4d1c21 to df9efb7 Compare January 4, 2024 11:39

ksooo force-pushed the video-info-tags-asset-info branch from a5a13cc to c9fd6c4 Compare January 5, 2024 03:12

ksooo force-pushed the video-info-tags-asset-info branch 2 times, most recently from a9f0592 to 671683f Compare January 5, 2024 03:20

Hitcher reviewed Jan 5, 2024

View reviewed changes

addons/skin.estuary/xml/Variables.xml Outdated Show resolved Hide resolved

Hitcher reviewed Jan 5, 2024

View reviewed changes

addons/skin.estuary/xml/Variables.xml Outdated Show resolved Hide resolved

ksooo force-pushed the video-info-tags-asset-info branch from 5347dd2 to c42fa78 Compare January 5, 2024 11:35

ksooo force-pushed the video-info-tags-asset-info branch from c42fa78 to 4fc2960 Compare January 5, 2024 12:50

[guilib] Add GUI info label ListItem.VideoVersionName.

000b9fd

ksooo force-pushed the video-info-tags-asset-info branch from c343276 to cffd7a5 Compare January 6, 2024 15:07

ksooo force-pushed the video-info-tags-asset-info branch from cffd7a5 to c06043d Compare January 6, 2024 16:23

ksooo mentioned this pull request Jan 6, 2024

Artwork different from default version in library, info, etc #24371

Open

ksooo force-pushed the video-info-tags-asset-info branch from c06043d to 74f6b96 Compare January 6, 2024 16:59

ksooo closed this Jan 6, 2024

ksooo reopened this Jan 6, 2024

ksooo force-pushed the video-info-tags-asset-info branch from 74f6b96 to ff38486 Compare January 6, 2024 18:10

ksooo added 7 commits January 6, 2024 19:47

[video] 'Show movies with multiple versions as folder': contents are …

c84fdfa

…now the movies with the respective versions, no longer video version items.

[Estuary] Show video version name in the Video info dialog.

01b425e

[video] Show correct video version artwork for movies.

1191df1

[video] Video Versions Manager: Fix no art available in choose art di…

b0eb5fb

…alog.

[video] Remove unused method CVideoDatabase::GetFilenameAndPathById.

3343d60

[video] Remove unused method CVideoDatabase::GetVideoVersionFile.

7224f09

[FileItem] CFileItem::IsSamePath: Small optimization.

5ebfe93

ksooo force-pushed the video-info-tags-asset-info branch from ff38486 to 5ebfe93 Compare January 6, 2024 18:48

ksooo merged commit 00216dc into xbmc:master Jan 6, 2024
2 checks passed

ksooo deleted the video-info-tags-asset-info branch January 6, 2024 20:45

CrystalP reviewed Jan 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video][Estuary] Video Versions: Improvements and Fixes #24430

[video][Estuary] Video Versions: Improvements and Fixes #24430

ksooo commented Jan 4, 2024 •

edited

CrystalP commented Jan 4, 2024 •

edited

ksooo commented Jan 5, 2024

ksooo commented Jan 5, 2024 •

edited

garbear commented Jan 5, 2024

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024

Hitcher commented Jan 5, 2024

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 6, 2024

CrystalP commented Jan 6, 2024 •

edited

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

Ch1llb0 commented Jan 17, 2024 •

edited

Hitcher commented Jan 17, 2024 •

edited

Ch1llb0 commented Jan 17, 2024 •

edited

Hitcher commented Jan 17, 2024

ksooo commented Jan 17, 2024 •

edited

CrystalP Jan 18, 2024 •

edited

ksooo Jan 19, 2024

[video][Estuary] Video Versions: Improvements and Fixes #24430

[video][Estuary] Video Versions: Improvements and Fixes #24430

Conversation

ksooo commented Jan 4, 2024 • edited

CrystalP commented Jan 4, 2024 • edited

ksooo commented Jan 5, 2024

ksooo commented Jan 5, 2024 • edited

garbear commented Jan 5, 2024

ksooo commented Jan 5, 2024 • edited

ksooo commented Jan 5, 2024 • edited

ksooo commented Jan 5, 2024

Hitcher commented Jan 5, 2024

ksooo commented Jan 5, 2024 • edited

ksooo commented Jan 5, 2024 • edited

ksooo commented Jan 6, 2024

CrystalP commented Jan 6, 2024 • edited

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

ksooo commented Jan 6, 2024

Ch1llb0 commented Jan 17, 2024 • edited

Hitcher commented Jan 17, 2024 • edited

Ch1llb0 commented Jan 17, 2024 • edited

Hitcher commented Jan 17, 2024

ksooo commented Jan 17, 2024 • edited

CrystalP Jan 18, 2024 • edited

Choose a reason for hiding this comment

ksooo Jan 19, 2024

Choose a reason for hiding this comment

ksooo commented Jan 4, 2024 •

edited

CrystalP commented Jan 4, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

ksooo commented Jan 5, 2024 •

edited

CrystalP commented Jan 6, 2024 •

edited

Ch1llb0 commented Jan 17, 2024 •

edited

Hitcher commented Jan 17, 2024 •

edited

Ch1llb0 commented Jan 17, 2024 •

edited

ksooo commented Jan 17, 2024 •

edited

CrystalP Jan 18, 2024 •

edited