[Video] Fix for videoversions and bluray:// paths (fix 2 for #24720).

[78andyp]

Description

Following on from discussion in #24720.

Fixes:

• bluray:// path not updated if more than one videoversion
• videoversions would be readded after library scan after bluray:// path assigned

Changes:

• bluray:// path is now not stored in movies table at c22 and episodes table at c18 (for movies this base path can be used to look for external subtitles and audio).
• path table is not changed
• filename is stored as bluray:// in files table (hence changes to SplitPath and ConstructPath)

Motivation and context

How has this been tested?

What is the effect on users?

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)
• Student submission (PR was done for educational purposes and will be treated as such)
• 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

[78andyp]

@CrystalP - grateful for your thoughts, thanks.

[78andyp]

I've played around with versions and extras and can't find a difference between v21.

I do note a couple of ? bugs that are also present in v21

• the artwork for one of the movies is blank in the manage versions dialog (? the selected or first one - I haven't explored further) - it's OK in the select version to play dialog
• when you add a movie that's already been (part) played as a version the movie is deleted (by design) but so are the streamdetails (but not any bookmark) - so the stream details aren't shown but you are prompted to resume when you play
• if you add a movie with extras as a version and then delete the movie, the extras specific to the added and then deleted version are still listed when the extras button is pressed.

[CrystalP]

Thanks for working on a followup.
It will take me a bit of time to do a full review of previous PR + this, but I already have a few comments, not related to the versions yet.

• in CSaveFileState::DoWork() I don't think it's a good idea to systematically call SetFileForMovie/SetFileForEpisode. That should only be done when the idFile changes.
• I don't think that SetStreamDetailsForFile() should be bent into a function returning the idFile of the path. That's not its purpose. Please do the const int idFile = AddFile(path) in CSaveFileState::DoWork and then call SetStreamDetailsForFileId() to avoid a double-lookup of the id.
• updating c22 was probably correct, but a search of the rest of the code to check how it's usually populated and used would be needed. There may be some special cases and maybe it's not just a denormalization of the path table.

[78andyp]

Thanks for working on a followup. It will take me a bit of time to do a full review of previous PR + this, but I already have a few comments, not related to the versions yet.

• in CSaveFileState::DoWork() I don't think it's a good idea to systematically call SetFileForMovie/SetFileForEpisode. That should only be done when the idFile changes.
• I don't think that SetStreamDetailsForFile() should be bent into a function returning the idFile of the path. That's not its purpose. Please do the const int idFile = AddFile(path) in CSaveFileState::DoWork and then call SetStreamDetailsForFileId() to avoid a double-lookup of the id.
• updating c22 was probably correct, but a search of the rest of the code to check how it's usually populated and used would be needed. There may be some special cases and maybe it's not just a denormalization of the path table.

OK, fixed the first two.
It looks like c22 should be left alone - seems to be used to get base path for subtitle and external audio scan, if I'm following the code right.

[CrystalP]

You may have pushed the wrong commits, I don't see how the first two points are addressed.

[78andyp]

You may have pushed the wrong commits, I don't see how the first two points are addressed.

Sorry. I fixed it and then tested it and it didn't work.

Regarding the first point.

The new fileId can get generated in multiple places - (eg.update last played, streamdetails etc..) so it's not really possible to bring it into SaveFileStateJob.

The FileId routines are not being called every time, only if the streamdetails have changed.

I have addressed the second point - I've reverted SetStreamDetails back to a void function. That's how it is in v21.

[78andyp]

@CrystalP, was this OK??