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.

Sign up for GitHub

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

Jump to bottom

[videodb] Movie Deletion/Conversion to Version Refactor and Optimization #25022

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[videodb] Movie Deletion/Conversion to Version Refactor and Optimization #25022

wants to merge 3 commits into from
+41 −39

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented Apr 19, 2024 •
edited

Description

Minor refactor and optimization in movie deletion and conversion of a movie into a version.

See motivation and context for the "why".

One commit per logical step, as follows:

[refactors]

  • Removed the KeepId parameter of DeleteMovie. It was complicating the flow and reducing the function to a stream deletion. Have the only caller delete the streams directly instead.
  • The stream deletion by the caller is redundant because of an unconditional call a bit later of "SetStreamDetailsForFileId" which unconditionally deletes all streams first, before adding the new ones (if any) > removed the duplicate stream deletion
  • Added doxygen to SetStreamDetailsForFileId

[small optimization]

  • Added parameter to DeleteMovie to skip the hash deletion for the case of movie conversion into a version. That doesn't disrupt the flow of the function and is OK I think

No backport to v21, there is nothing broken to fix.

Motivation and context

Prompted by an investigation of what happens after a movie is turned into a version, and the user refreshes the library or adds the movie to library again from Videos node.

There is no crash and things work fine, but a small optimization can be made to the hash: it's deleted when converting a movie into a version, but recreated (even though there is no file/folder change) in the next library scan / explicit scan of the movie.

Instead, the hash could be preserved and the next scan will be a little faster (detect the folder as "no change" instead of "new")

Then noticed that the parameters of the DeleteMovie are a bit strange and could use a bit of refactoring.

How has this been tested?

Deleted movies, converted movies to versions
Tried different scraper settings (single movie in folder named after the movie or not, recurse or not)
"Library Update" function, "Scan to library", "Scan source"

What is the effect on users?

Barely noticeable - slightly faster library scan after converting movies to versions

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

@CrystalP CrystalP added this to the "P" 22.0 Alpha 1 milestone Apr 19, 2024
@CrystalP
Copy link
Contributor Author

jenkins please build this

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 26, 2024
@CrystalP CrystalP mentioned this pull request Apr 28, 2024
Merged
14 tasks
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label May 5, 2024
@CrystalP
[videodb] Remove KeepId from DeleteMovie
98bfc75 
DeleteMovie behaves very differently with KeepId=true and turns it into a "DeleteStreams" function.
To simplify the logic, remove the parameter and make the only caller do the stream deletion directly.
@CrystalP
[videodb] Remove redundant deletion of stream details when setting mo…
a7f0f25 
…vie details

SetStreamDetailsForFileId clears existing details unconditionnally, then adds the new ones.
Added doxygen.
@CrystalP
[videodb] Preserve path hash when turning a movie into a version
c56f900 
This is a small optimization: the path hash is deleted when a movie is turned into a version, and
during the next libray scan, the folder of the movie is scanned (unnecessary) and the same hash
is recreated.
The file of the version hasn't moved or changed, the hash is still valid, might as well not delete
it in the first place and save the directory scan.
@CrystalP CrystalP changed the title [videodb] Movie Deletion Refactor and Movie to Version Conversion Hash Optimization [videodb] Movie Deletion/Conversion to Version Refactor and Optimization May 5, 2024
@CrystalP CrystalP requested a review from ksooo May 5, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksooo ksooo Awaiting requested review from ksooo

Assignees
No one assigned
Labels
Component: Database Feature: Video Versions/Extras v22 "P"
Projects
None yet
Milestone
"P" 22.0 Alpha 1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@CrystalP @jenkins4kodi