diff --git a/modules/videos/downloader.py b/modules/videos/downloader.py index 39636ab5..789febb7 100755 --- a/modules/videos/downloader.py +++ b/modules/videos/downloader.py @@ -887,6 +887,7 @@ class PreparedVideo: destination: Optional[pathlib.Path] = None info_json: Optional[dict] = None location: Optional[str] = None # existing video, for error reports + wrolpi: dict = field(default_factory=dict) # existing video's wrolpi section, to carry over @dataclass @@ -907,6 +908,7 @@ class ExecutedVideo: tag_names: List[str] settings: dict location: Optional[str] # for error reporting + wrolpi: dict = field(default_factory=dict) # existing video's wrolpi section, to carry over class VideoDownloader(Downloader, ABC): @@ -954,6 +956,8 @@ class VideoDownloader(Downloader, ABC): # can still tell the user where the previous attempt landed. existing = Video.get_by_url(session, url) location = existing.location if existing else None + # yt-dlp rewrites the whole info json; the user's edits (its wrolpi section) must survive. + wrolpi = existing.file_group.get_wrolpi_json() if existing else dict() return PreparedVideo( url=url, @@ -962,6 +966,7 @@ class VideoDownloader(Downloader, ABC): destination=download.destination, info_json=download.info_json, location=location, + wrolpi=wrolpi, ) async def execute_download(self, prepared: PreparedVideo, ctx: DownloadContext, @@ -1201,6 +1206,7 @@ class VideoDownloader(Downloader, ABC): tag_names=prepared.tag_names, settings=settings, location=location, + wrolpi=prepared.wrolpi, ) except BotBlockedDownloadError: @@ -1238,6 +1244,11 @@ class VideoDownloader(Downloader, ABC): video.channel_id = executed.channel_id if video.info_json_path and (new_info_json := video.clean_info_json()): video.replace_info_json(new_info_json, clean=False) + if executed.wrolpi and video.info_json_path: + # The re-downloaded info json lost the user's edits; put them back, then derive the + # title/description again so the custom ones win (from_paths validated without them). + video.file_group.update_wrolpi_json(executed.wrolpi) + video.validate(session) if parent_download_url := executed.settings.get('parent_download_url'): try: @@ -1368,9 +1379,12 @@ class VideoDownloader(Downloader, ABC): settings = download.settings or dict() with get_db_session(commit=True) as session: - # Find the existing video, replace its info json. + # Find the existing video, replace its info json; keep the user's edits (wrolpi section). video = Video.get_by_url(session, url) + wrolpi = video.file_group.get_wrolpi_json() video.replace_info_json(info_json) + if wrolpi: + video.file_group.update_wrolpi_json(wrolpi) if parent_download_url := settings.get('parent_download_url'): try: diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 0f16648e..072d1e5e 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -58,7 +58,9 @@ def extract_video_info_json(video: Video) -> VideoInfoJSON: """ video_info_json = VideoInfoJSON() if info_json := video.get_info_json(): - title = info_json.get('fulltitle') or info_json.get('title') + # A title the user set (Edit > Edit) is kept apart from yt-dlp's keys, and wins. + wrolpi_section = info_json.get('wrolpi') or dict() + title = wrolpi_section.get('custom_title') or info_json.get('fulltitle') or info_json.get('title') video_info_json.title = html.unescape(title) if title else None upload_date = dates.strpdate(i) if (i := info_json.get('upload_date')) else None diff --git a/modules/videos/models.py b/modules/videos/models.py index 22cff5a8..750a4d5b 100644 --- a/modules/videos/models.py +++ b/modules/videos/models.py @@ -141,10 +141,16 @@ class Video(ModelHelper, Base): def get_video_description(self) -> Optional[str]: """ - Get the Video description from the file system. + Get the Video description from the file system. A description the user set (Edit > Edit) + lives in the info json's `wrolpi` section and wins; an empty one means "none". """ - if (info_json := self.get_info_json()) and (description := info_json.get('description')): - return description + info_json = self.get_info_json() + if not info_json: + return None + wrolpi_section = info_json.get('wrolpi') or dict() + if 'custom_description' in wrolpi_section: + return wrolpi_section['custom_description'] or None + return info_json.get('description') or None def get_description(self) -> Optional[str]: """The indexed description (`FileGroup.c_text`), falling back to the info json on disk. diff --git a/modules/videos/test/test_downloader.py b/modules/videos/test/test_downloader.py index 31fa25aa..0af65528 100644 --- a/modules/videos/test/test_downloader.py +++ b/modules/videos/test/test_downloader.py @@ -1789,3 +1789,76 @@ async def test_enforce_codecs(): with pytest.raises(UnrecoverableDownloadError): await enforce_codecs(video_path, video_paths, effective) mock_transcode.assert_not_called() + + +@pytest.mark.asyncio +async def test_metadata_refresh_keeps_wrolpi_section(test_session, test_directory, video_factory): + """Refreshing a Video's metadata replaces yt-dlp's info json; the wrolpi section (the user's + custom title and description) is carried over and still wins.""" + source_id = 'video-source-id' + url = 'https://example.com/the-video' + video = video_factory(with_info_json={'id': source_id, 'title': 'Orig', + 'wrolpi': {'custom_title': 'Mine', 'custom_description': 'my words'}}, + source_id=source_id) + video.file_group.url = url + test_session.commit() + assert video.file_group.title == 'Mine' + + download = Download(url=url, info_json={'id': source_id}, settings={}) + with mock.patch('modules.videos.downloader.download_video_info_json') as mock_download: + mock_download.return_value = {'id': source_id, 'title': 'Orig v2', 'description': 'theirs', 'duration': 9} + result = await VideoDownloader.download_info_json(download, video.video_path) + assert result.success is True, result + + test_session.expire_all() + video = Video.find_by_file_group_id(test_session, video.file_group_id) + info_json = video.get_info_json() + assert info_json['title'] == 'Orig v2', 'yt-dlp data was refreshed' + assert info_json['wrolpi'] == {'custom_title': 'Mine', 'custom_description': 'my words'} + assert video.file_group.title == 'Mine' + assert video.get_description() == 'my words' + + +@pytest.mark.asyncio +async def test_redownload_keeps_wrolpi_section(test_session, test_directory, mock_video_extract_info, simple_channel, + await_switches, video_download_manager, mock_video_process_runner, + image_file): + """A full re-download has yt-dlp overwrite the info json; the wrolpi section is carried over.""" + simple_channel.source_id = example_video_json['channel_id'] + simple_channel.directory = test_directory / 'videos/channel name' + simple_channel.directory.mkdir(parents=True) + url = 'https://www.youtube.com/watch?v=31jPEBiAC3c' + + video_path = simple_channel.directory / 'a video.mp4' + shutil.copy(PROJECT_DIR / 'test/big_buck_bunny_720p_1mb.mp4', video_path) + info_json_path = video_path.with_suffix('.info.json') + info_json_path.write_text(json.dumps({'title': 'Orig', 'webpage_url': url, 'wrolpi': {'custom_title': 'Mine'}})) + from modules.videos.normalize_video_url import normalize_video_url + existing = Video.from_paths(test_session, video_path, info_json_path) + existing.validate(test_session) + test_session.commit() + assert existing.file_group.title == 'Mine' + assert Video.get_by_url(test_session, normalize_video_url(url)) is not None, 'lookup by normalized url' + assert existing.file_group.get_wrolpi_json() == {'custom_title': 'Mine'} + + from wrolpi.cmd import CommandResult + + async def yt_dlp_overwrites_info_json(*args, **kwargs): + # yt-dlp rewrites the whole info json; the wrolpi section is gone from disk. + info_json_path.write_text(json.dumps({'title': 'Orig v2', 'webpage_url': url, 'duration': 5})) + return CommandResult(return_code=0, cancelled=False, stdout=b'', stderr=b'', elapsed=0) + + mock_video_process_runner.side_effect = yt_dlp_overwrites_info_json + with mock.patch('modules.videos.downloader.prepare_video_filename') as mock_prepare_filename: + mock_video_extract_info.return_value = example_video_json + mock_prepare_filename.return_value = (video_path, {'id': 'foo'}) + video_download_manager.create_download(test_session, url, video_downloader.name) + await video_download_manager.wait_for_all_downloads() + mock_video_process_runner.assert_called_once() + + test_session.expire_all() + video: Video = test_session.query(Video).one() + info_json = video.get_info_json() + assert info_json['title'] == 'Orig v2' + assert info_json['wrolpi'] == {'custom_title': 'Mine'} + assert video.file_group.title == 'Mine' diff --git a/modules/videos/test/test_video_edit.py b/modules/videos/test/test_video_edit.py index 9a095a45..095b7db7 100644 --- a/modules/videos/test/test_video_edit.py +++ b/modules/videos/test/test_video_edit.py @@ -1,16 +1,20 @@ -"""Editing a Video's details from the video page.""" +"""Editing a Video's details from the video page. + +Edits never touch yt-dlp's own keys: they are written to the `wrolpi` section of the info json +(`custom_title`, `custom_description`), which the modeler prefers when it derives the Video.""" import json import pytest +from modules.videos.lib import extract_video_info_json from modules.videos.models import Video from modules.videos.video.lib import update_video from wrolpi.errors import ValidationError def test_update_video_title(test_session, test_directory, video_factory): - """The new title is written to the info json (title and fulltitle) and the Video re-validated, - so its title and search text change too. Other info json keys are kept.""" + """The new title lands in the wrolpi section; yt-dlp's title/fulltitle stay as they were. The + Video is re-validated, so its title and search text change.""" video = video_factory(with_info_json={'title': 'Old Title', 'fulltitle': 'Old Title', 'duration': 7, 'formats': ['kept, not cleaned']}) test_session.commit() @@ -24,12 +28,13 @@ def test_update_video_title(test_session, test_directory, video_factory): assert video.file_group.a_text == 'New Title' assert video.file_group.length == 7, 'Other details from the info json are unchanged' info_json = json.loads(video.info_json_path.read_text()) - assert info_json['title'] == 'New Title' and info_json['fulltitle'] == 'New Title' + assert info_json['title'] == 'Old Title' and info_json['fulltitle'] == 'Old Title', 'The original is kept' + assert info_json['wrolpi']['custom_title'] == 'New Title' assert info_json['formats'] == ['kept, not cleaned'] def test_update_video_title_creates_info_json(test_session, test_directory, video_factory): - """A Video without an info json gets one holding the title.""" + """A Video without an info json gets one holding only the wrolpi section.""" video = video_factory() test_session.commit() assert video.info_json_path is None @@ -40,13 +45,14 @@ def test_update_video_title_creates_info_json(test_session, test_directory, vide video = Video.find_by_file_group_id(test_session, video.file_group_id) assert video.file_group.title == 'Brand New' assert video.info_json_path and video.info_json_path.is_file() - assert json.loads(video.info_json_path.read_text())['title'] == 'Brand New' + info_json = json.loads(video.info_json_path.read_text()) + assert info_json == {'wrolpi': {'custom_title': 'Brand New'}} assert video.info_json_path.name in [i['path'] for i in video.file_group.files], 'The new file is tracked' def test_update_video_description(test_session, test_directory, video_factory): - """The description is written to the info json and indexed; the title is untouched when not - given. An empty description clears it.""" + """The description lands in the wrolpi section and is indexed; yt-dlp's description stays. + An empty custom description clears the shown description.""" video = video_factory(with_info_json={'title': 'Kept', 'description': 'old words'}) test_session.commit() assert video.file_group.c_text == 'old words' @@ -58,12 +64,15 @@ def test_update_video_description(test_session, test_directory, video_factory): assert video.file_group.title == 'Kept' assert video.file_group.c_text == 'new words\nsecond line' assert video.get_description() == 'new words\nsecond line' - assert json.loads(video.info_json_path.read_text())['description'] == 'new words\nsecond line' + info_json = json.loads(video.info_json_path.read_text()) + assert info_json['description'] == 'old words', 'The original is kept' + assert info_json['wrolpi']['custom_description'] == 'new words\nsecond line' update_video(video.file_group_id, description='') test_session.expire_all() video = Video.find_by_file_group_id(test_session, video.file_group_id) assert video.get_description() is None + assert json.loads(video.info_json_path.read_text())['description'] == 'old words' assert video.file_group.title == 'Kept' @@ -83,6 +92,44 @@ def test_update_video_title_empty(test_session, video_factory): assert Video.find_by_file_group_id(test_session, video.file_group_id).file_group.title == 'Old' +def test_extract_video_info_json_prefers_custom_title(test_session, video_factory): + """The modeler's reader prefers the wrolpi custom title over fulltitle/title.""" + video = video_factory(with_info_json={'title': 'Orig', 'fulltitle': 'Orig Full', + 'wrolpi': {'custom_title': 'Mine & yours'}}) + assert extract_video_info_json(video).title == 'Mine & yours', 'HTML entities are unescaped as for any title' + video2 = video_factory(with_info_json={'title': 'Orig', 'fulltitle': 'Orig Full', 'wrolpi': {}}) + assert extract_video_info_json(video2).title == 'Orig Full' + + +@pytest.mark.asyncio +async def test_custom_title_survives_refresh(test_session, test_directory, async_client, video_factory): + """A file refresh re-models the Video from its files; the custom title must win again.""" + from wrolpi.files.worker import file_worker + + video = video_factory(with_info_json={'title': 'Original'}) + test_session.commit() + update_video(video.file_group_id, title='Custom', description='custom words') + video_path = video.video_path + file_group_id = video.file_group_id + + # Force a full re-model, as a refresh of a modified file does. + test_session.expire_all() + video = Video.find_by_file_group_id(test_session, file_group_id) + video.file_group.indexed = False + video.file_group.title = None + video.file_group.c_text = None + test_session.commit() + + job_id = file_worker.queue_refresh([video_path], send_events=False) + await file_worker.wait_for_job(job_id) + + test_session.expire_all() + video = Video.find_by_file_group_id(test_session, file_group_id) + assert video.file_group.title == 'Custom' + assert video.get_description() == 'custom words' + assert json.loads(video.info_json_path.read_text())['title'] == 'Original' + + @pytest.mark.asyncio async def test_video_update_api(async_client, test_session, video_factory, wrol_mode_fixture): video = video_factory(with_info_json={'title': 'Old'}) @@ -133,3 +180,33 @@ async def test_update_video_wrol_mode(test_session, video_factory, wrol_mode_fix test_session.expire_all() assert Video.find_by_file_group_id(test_session, video.file_group_id).file_group.title == 'Old' + + +@pytest.mark.asyncio +async def test_edit_creates_info_json_that_survives_refresh(test_session, test_directory, async_client, + video_factory): + """A Video with no info json is edited (the edit creates one), then its file is refreshed: the + new sidecar must stay in the FileGroup and the custom title/description must still win.""" + from wrolpi.files.worker import file_worker + + video = video_factory() + test_session.commit() + video_path = video.video_path + file_group_id = video.file_group_id + + update_video(file_group_id, title='Custom', description='my words') + test_session.expire_all() + video = Video.find_by_file_group_id(test_session, file_group_id) + assert video.info_json_path and video.info_json_path.is_file() + assert video.get_description() == 'my words' + + job_id = file_worker.queue_refresh([video_path], send_events=False) + await file_worker.wait_for_job(job_id) + + test_session.expire_all() + video = Video.find_by_file_group_id(test_session, file_group_id) + assert video.info_json_path is not None, 'The refresh forgot the info json' + names = {i['path'] for i in video.file_group.files} + assert video_path.name in names and video.info_json_path.name in names, names + assert video.file_group.title == 'Custom' + assert video.get_description() == 'my words' diff --git a/modules/videos/video/lib.py b/modules/videos/video/lib.py index cd3ea779..dfad5d54 100644 --- a/modules/videos/video/lib.py +++ b/modules/videos/video/lib.py @@ -56,8 +56,9 @@ def update_video(file_group_id: int, title: str = None, description: str = None) Video's .info.json (created if missing) and the Video is validated again from it. Only the fields given (not None) are changed; an empty description clears it. - A later re-download of the Video's metadata will overwrite the info json, and this edit with - it. + Edits go into the info json's `wrolpi` section (`custom_title`, `custom_description`), never + over yt-dlp's own keys: the original title and description stay in the file, and a metadata + re-download carries the section over. @raise UnknownVideo: if the Video can not be found @raise ValidationError: when nothing is given, or the title is empty @@ -70,17 +71,19 @@ def update_video(file_group_id: int, title: str = None, description: str = None) if not title: raise ValidationError('Video title cannot be empty') + changes = dict() + if title is not None: + changes['custom_title'] = title + if description is not None: + changes['custom_description'] = description + with get_db_session(commit=True) as session: video = Video.find_by_file_group_id(session, file_group_id) - info_json = video.get_info_json() or dict() - if title is not None: - # `extract_video_info_json` prefers fulltitle over title; keep the two in agreement. - info_json['title'] = title - info_json['fulltitle'] = title - if description is not None: - info_json['description'] = description - # Keep the file as it was, other than the edited fields. - video.replace_info_json(info_json, clean=False) + if video.info_json_path is None or not video.info_json_path.is_file(): + # No info json yet: create one holding only the wrolpi section. + video.replace_info_json({'wrolpi': changes}, clean=False) + else: + video.file_group.update_wrolpi_json(changes) # Re-derive the title (and search text) from the file just written. video.validate(session) video.flush() diff --git a/wrolpi/files/models.py b/wrolpi/files/models.py index b5656f88..81b54984 100644 --- a/wrolpi/files/models.py +++ b/wrolpi/files/models.py @@ -432,6 +432,19 @@ class FileGroup(ModelHelper, Base): if json_path.name not in tracked_paths: self.append_files(json_path) + def get_wrolpi_json(self) -> dict: + """The `wrolpi` section of this FileGroup's metadata JSON: WROLPi-owned keys (a user's + custom title, the parent download) that yt-dlp/readability never write. Empty if none.""" + json_path = self.metadata_json_path + if not json_path or not json_path.is_file(): + return dict() + try: + contents = json.loads(json_path.read_text()) + except (OSError, ValueError): + return dict() + section = contents.get('wrolpi') if isinstance(contents, dict) else None + return dict(section) if isinstance(section, dict) else dict() + def update_wrolpi_json(self, data: dict): """Read this FileGroup's metadata JSON, merge `data` into its `wrolpi` section, and write back.