Add thumbnail in the video directory

deldesir commented 3 months ago

Description

This PR ensures thumbnails are downloaded inside the video directory.

Motivation and Context

This makes it consistent and easy to find.

For https://github.com/iiab/calibre-web/issues/161

How has this been tested?

by running lb tubeadd test.db https://www.youtube.com/watch?v=WcLlpWmEpQ8 then lb dl test.db --video https://www.youtube.com/watch?v=WcLlpWmEpQ8 --write-thumbnail --subs

Tested on Ubuntu 24.04 with xklb 2.8.0.49

Screenshots (if appropriate):

root@box:/home/ubuntu# lb tubeadd test.db https://www.youtube.com/watch?v=WcLlpWmEpQ8
Importing playlist-less media https://www.youtube.com/watch?v=WcLlpWmEpQ8

root@box:/home/ubuntu# lb dl test.db --video https://www.youtube.com/watch?v=WcLlpWmEpQ8 --write-thumbnail --subs
root@box:/home/ubuntu# tree Youtube/
Youtube/
└── TED-Ed
    ├── WcLlpWmEpQ8.webp
    └── Why dont we cover the desert with solar panels Dan Kwartler_4.74M_[WcLlpWmEpQ8].webm

1 directory, 2 files

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

holta commented 3 months ago

Question: Is this PR a companion needed alongside... ?

https://github.com/chapmanjacobd/library/commit/bb97e2401db3477bb76790233e941fb8f60b041e (commit merged 5h ago!)

deldesir commented 3 months ago

No, I have yet to test it.

deldesir commented 3 months ago

Just tested with and without https://github.com/chapmanjacobd/library/commit/bb97e2401db3477bb76790233e941fb8f60b041e. I cannot spot a difference.

chapmanjacobd commented 3 months ago

it's probably because you already had https://github.com/chapmanjacobd/library/commit/bb97e2401db3477bb76790233e941fb8f60b041e before you wrote 86e33a2231bdf6dcbc70cd9d22b5ae7d51e74808

This PR is modifying the behavior of lb tubeadd --extra and not lb dl

This is why it's better to start with a failing test so that you can know if you are changing the right function 😁 https://en.wikipedia.org/wiki/Falsifiability

deldesir commented 3 months ago

I was not clear by saying I spot no difference, I meant your commit (https://github.com/chapmanjacobd/library/commit/bb97e2401db3477bb76790233e941fb8f60b041e) doesn't affect PR #38 behavior. Isn't --subs using the same --extra mechanism?

Anyway, I'd like to add a proper test to support this claim. Is test_lb_fs a good hint to do this https://github.com/chapmanjacobd/library/blob/6a99a76efb2fed38cef3290f70ba3bb76c518a8a/tests/createdb/test_tube_add.py#L31?

deldesir commented 3 months ago

You are right. I unnecessarily modified tubeadd --extra behavior and your commit https://github.com/chapmanjacobd/library/commit/bb97e2401db3477bb76790233e941fb8f60b041e fixed the issue. I'll close this PR.

chapmanjacobd commented 3 months ago

Is test_lb_fs a good hint to do this?

To test --write-thumbnail you'd need to go through the full download process. I think this is a good example:

https://github.com/chapmanjacobd/library/blob/6a99a76efb2fed38cef3290f70ba3bb76c518a8a/tests/mediadb/test_download.py

More tests like these can be committed to the repo but because these tests use the network I'd probably add @skip("network") before merging. However, I think it is good to have at least one full download test. I'd accept a PR that changes this test to use write-thumbnail instead of -no, remove the TODO: notice and assert that the thumbnail file exists

deldesir commented 3 months ago

Thanks. Easy when I have a good example to start with. Done in https://github.com/chapmanjacobd/library/pull/40.

holta commented 3 months ago

https://github.com/chapmanjacobd/library/blob/6a99a76efb2fed38cef3290f70ba3bb76c518a8a/tests/mediadb/test_download.py

@chapmanjacobd can you clarify how often tests like the above are run?

(e.g. is this run automatically by GitHub Actions?)

good to have at least one full download test. I'd accept a PR that changes this test to use write-thumbnail instead of -no, remove the TODO: notice and assert that the thumbnail file exists

👍

chapmanjacobd commented 3 months ago

is this run automatically by GitHub Actions

Yes. They are run at least three times (Linux, Mac OS, and Windows) before each release. If any fail then the release is not published.

Tests might be run many times manually. Because of this it is better if tests are idempotent. For example, in this case, it is true that it would be more safe for this test if we delete the Youtube folder after the test.

However, this is not that important given that tests are usually run in GitHub Actions where the test environment is reset each time. Additionally, the current behavior of not deleting output files is desired for easier file inspection.

You can launch the python debugger on error when using the --pdb flag:

pytest tests/test_lb.py --pdb -vvv

chapmanjacobd / library