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

[videodb] Fix TV shows cleaning after 5755573981ca706a82a7ecbbdf5de112a570377a. #8133

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@ghost
Copy link
Contributor

ghost commented Sep 25, 2015

A regression in 5755573 meant removing a
deleted TV show needed multiple library cleanings (i.e. TV shows only got
removed when filesToTestForDelete was empty). This is much more noticeable
after 5cae3b5.

After 5755573:

  • if we had files to be checked for deletion: CleanMediaType would return a
    decision for the TV show path (delete) and a decision for the parent path
    (keep), so the TV show wasn't removed from the database;
  • if no files needed to be checked for deletion: CleanMediaType would return
    earlier and pathsDeleteDecisions was empty, so we'd have checked if the
    path existed and there was a decision for the parent path (delete) or no
    decision at all.

This commit checks if there's a decision to delete the parent path or it
exists (we only end up there if there's a decision to delete the path or it
doesn't exist), so that you can delete a TV show with episodes (or other random
files).

See:

The 2 booleans are just to improve readability.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Oct 3, 2015

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Oct 3, 2015

No objections but i'd like to get @Montellese opinion as well.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Oct 19, 2015

Ping

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Oct 25, 2015

@Montellese could you please have a look at this?

"AND (exclude IS NULL OR exclude != 1))";
sql = "SELECT path.idPath, path.strPath, path.idParentPath, path2.strPath AS strParentPath "
"FROM path "
"LEFT JOIN path AS path2 ON path2.idPath = path.idParentPath "

This comment has been minimized.

@Montellese

Montellese Oct 25, 2015

Member

I would use parentPath instead of path2 which makes it much easier to read.

bool hasDecisionForParent = pathsDeleteDecisionByParent != pathsDeleteDecisions.end();
std::string parentPath = m_pDS->fv(3).get_asString();

if (((hasDecision && pathsDeleteDecision->second) ||

This comment has been minimized.

@Montellese

Montellese Oct 25, 2015

Member

I think it's time to add some comments what this is supposed to do:

// add the path to the list of paths to be deleted if
//   it has been handled before and should be deleted
//   OR
//   it hasn't been handled before and doesn't exist
// AND
//   its parent path has been handled before
//   AND
//     should be deleted
//     OR
//     doesn't exists
//   its parent path hasn't been handled before

Reading it like that I'm actually a bit confused about the AND combination of the path and the parentpath parts.

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Oct 25, 2015

Please wait. This breaks multiple movies in the same folder:
http://forum.kodi.tv/showthread.php?tid=231092&pid=2142689#pid2142689

Suggestions welcome.

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Oct 25, 2015

Looks like in that case pathsDeleteDecision->second is true and the added logic deletes it - why is it true (delete) if the path does exist?

Edit:

CleanDatabase: hasDecision=true pathsDeleteDecision->second=true
CleanDatabase: !CDirectory::Exists(/Users/anaconda/tmp/xbmc-testsource/Action/, false)=false
CleanDatabase: hasDecisionForParent=true pathsDeleteDecisionByParent->second=false
CleanDatabase: !parentPath.empty()=true CDirectory::Exists(/Users/anaconda/tmp/xbmc-testsource/, false)=true
@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Nov 18, 2015

@anaconda would this still something for Jarvis or too risky?

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Nov 21, 2015

Too risky for Jarvis unless CleanMediaType is fixed to return false.
After a path with multiple movies is incorrectly cleaned, a subsequent scan adds them again, but you get duplicates.

[videodb] Fix TV shows cleaning after 5755573.
A regression in 5755573 meant removing a
deleted TV show needed multiple library cleanings (i.e. TV shows only got
removed when filesToTestForDelete was empty).  This is much more noticeable
after 5cae3b5.

After 5755573:
  * if we had files to be checked for deletion: CleanMediaType would return a
    decision for the TV show path (delete) and a decision for the parent path
    (keep), so the TV show wasn't removed from the database;
  * if no files needed to be checked for deletion: CleanMediaType would return
    earlier and pathsDeleteDecisions was empty, so we'd have checked if the
    path existed and there was a decision for the parent path (delete) or no
    decision at all.

This commit checks if there's a decision to delete the parent path *or* it
exists (we only end up there if there's a decision to delete the path or it
doesn't exist), so that you can delete a TV show with episodes (or other random
files).

See:
  * http://forum.kodi.tv/showthread.php?tid=238881

@anaconda anaconda force-pushed the menakite:vc-remove-tvshows branch from 46ea4b6 to 5f4ef0c Jan 6, 2016

@vbat99

This comment has been minimized.

Copy link

vbat99 commented Feb 22, 2016

No chance of this getting into 16.1 then?

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Nov 14, 2016

@anaconda is this still wanted/needed?

@eusi

This comment has been minimized.

Copy link

eusi commented Apr 5, 2017

Actually your fix needs some improvements.

Example, I am using a movie structure like:
SourceVideoFolder ("Movies")
---QualityFolder (like "HD", "SD", "4k" etc.)
------YearFolder (like "1900-1999", "2000-2010" etc)
---------Movies

Following part of the if-statement (in VideoDatabase) wipes an entire folder (removes all movies in the folder from the database), when I delete only ONE movie in the folder and execute a "clean video library":
(pathsDeleteDecisionByParent != pathsDeleteDecisions.end() && pathsDeleteDecisionByParent->second)

This issue is super annoying for me and my folder structure. Because any time I delete a movie, like 200 movies getting removed as well from my kodi database.

It works correctly, if I change
if (scanSettings.parent_name) pathsDeleteDecisions.insert(std::make_pair(m_pDS2->fv(2).get_asInt(), del));
to
if (scanSettings.parent_name) pathsDeleteDecisions.insert(std::make_pair(m_pDS2->fv(2).get_asInt(), !CDirectory::Exists( parentPath, false ) && del));
Those line(s) can be found in CleanMediaType(). This method sets the pathsDeleteDecisionByParent-object.

@anaconda this could also be a fix for your last comment/worries on Oct 25, 2015.

I opened PR11937 that includes the fix and some of your/this improvements for better readability of this part.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Sep 16, 2017

@menakite will you revisit this?

@jenkins4kodi

This comment has been minimized.

Copy link
Contributor

jenkins4kodi commented Nov 19, 2017

@anaconda this needs a rebase

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jun 24, 2018

I will close this PR. If there's interest please open a new one (with required changes if needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.