From 0ffbc05e91f487f708bc787d16af9561d8031515 Mon Sep 17 00:00:00 2001 From: Roland Date: Thu, 5 Mar 2026 16:18:03 -0700 Subject: [PATCH] Unifying video download settings so that global/channel/rss/video download settings have inheritance. Allowing more granular control of potentially failed downloads. --- app/src/App.css | 5 + app/src/api.js | 5 + app/src/components/Download.js | 276 +++++++++++++++++++++---- app/src/components/Theme.tsx | 33 ++- app/src/components/Videos.js | 36 +++- modules/videos/api.py | 8 + modules/videos/downloader.py | 45 ++-- modules/videos/lib.py | 52 ++++- modules/videos/test/test_downloader.py | 126 +++++++++++ modules/videos/test/test_lib.py | 62 ++++++ wrolpi/downloader.py | 27 ++- wrolpi/schema.py | 10 + wrolpi/test/test_downloader.py | 69 +++++++ 13 files changed, 683 insertions(+), 71 deletions(-) diff --git a/app/src/App.css b/app/src/App.css index 02b3e20e..3f46f9d3 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -197,6 +197,11 @@ textarea.otp { color: #DDDDDD !important; } +.ui.inverted.accordion .title, +.ui.inverted.accordion .content { + background-color: #171616 !important; +} + /* Mobile-responsive Confirm modal */ .ui.modal.themed-confirm { max-width: 95vw; diff --git a/app/src/api.js b/app/src/api.js index ec367a73..bcd2262b 100644 --- a/app/src/api.js +++ b/app/src/api.js @@ -421,6 +421,11 @@ export async function fetchVideoDownloaderConfig() { return await getConfig('videos_downloader.yaml'); } +export async function fetchVideoDownloadDefaults() { + const response = await apiGet(`${VIDEOS_API}/download-defaults`); + return await response.json(); +} + export async function updateConfig(fileName, config) { const body = {config}; const response = await apiPost(`${API_URI}/config?file_name=${fileName}`, body); diff --git a/app/src/components/Download.js b/app/src/components/Download.js index 070f0bf4..53ff272d 100644 --- a/app/src/components/Download.js +++ b/app/src/components/Download.js @@ -1,5 +1,5 @@ import React, {useState} from "react"; -import {fetchVideoDownloaderConfig, getDownloaders, postDownload, putDownload} from "../api"; +import {fetchVideoDownloadDefaults, getDownloaders, postDownload, putDownload} from "../api"; import { APIButton, DirectorySearch, @@ -11,7 +11,7 @@ import { } from "./Common"; import Icon from "semantic-ui-react/dist/commonjs/elements/Icon"; import Message from "semantic-ui-react/dist/commonjs/collections/Message"; -import {Button, Form, Header} from "./Theme"; +import {Accordion, Button, Form, Header} from "./Theme"; import {Form as SForm, FormDropdown} from "semantic-ui-react"; import {Link} from "react-router"; import {TagsSelector} from "../Tags"; @@ -298,7 +298,7 @@ export function VideoResolutionSelectorForm({ } -function VideoFormatSelectorForm({form, name = 'video_format', path = 'settings.video_format'}) { +export function VideoFormatSelectorForm({form, name = 'video_format', path = 'settings.video_format'}) { const [inputProps, inputAttrs] = form.getSelectionProps({ name, path, @@ -359,6 +359,116 @@ export function ChannelTagNameForm({form}) { } +function AdvancedVideoSettings({form, isVideoLevel = false, isConfigLoaded = true}) { + const [active, setActive] = React.useState(false); + + if (!isConfigLoaded) return null; + + return + setActive(!active)}> + + Advanced Settings + + + + + + + + + + + + + + + + + + + + {isVideoLevel && + + + + + + + } + + + + + + + + + + + + + + + + +} + export function VideosDownloadForm({ singleDownload = true, onCancel, @@ -370,6 +480,7 @@ export function VideosDownloadForm({ const [showMessage, setShowMessage] = React.useState(false); const [userChangedResolutions, setUserChangedResolutions] = React.useState(false); const [config, setConfig] = React.useState(null); + const [isConfigLoaded, setIsConfigLoaded] = React.useState(false); // Keep video format in session to help user start downloads consistently. const [defaultVideoFormat, setDefaultVideoFormat] = useLocalStorage('video_format', defaultVideoFormatOption); @@ -385,6 +496,15 @@ export function VideosDownloadForm({ settings: { video_format: defaultVideoFormat, video_resolutions: configResolutions, + writesubtitles: false, + writeautomaticsub: false, + writethumbnail: false, + writeinfojson: false, + yt_dlp_extra_args: '', + sleep_requests: '', + user_agent: '', + continue_dl: false, + nooverwrites: false, } } @@ -414,25 +534,43 @@ export function VideosDownloadForm({ onSuccess, }); - // Fetch video downloader config when the modal opens + // Fetch global defaults to pre-fill the form React.useEffect(() => { - const fetchConfig = async () => { + const fetchDefaults = async () => { try { - const result = await fetchVideoDownloaderConfig(); - setConfig(result); + const defaults = await fetchVideoDownloadDefaults(); + setConfig(defaults); - // Update form with video resolutions from config only if user hasn't changed them - // and if the download doesn't already have video_resolutions set - const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; - if (result && result.video_resolutions && !userChangedResolutions && !downloadHasResolutions) { - form.setValue('settings.video_resolutions', result.video_resolutions); + if (defaults && download) { + // Edit mode: only fill settings not already in the download + for (const [key, value] of Object.entries(defaults)) { + if (download.settings?.[key] === undefined || download.settings?.[key] === null) { + form.setValue(`settings.${key}`, value); + } + } + } else if (defaults) { + // New download: pre-fill all settings from global defaults + const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; + for (const [key, value] of Object.entries(defaults)) { + if (key === 'video_resolutions' && (userChangedResolutions || downloadHasResolutions)) { + continue; + } + if (key === 'video_format') { + continue; // Handled by localStorage default + } + form.setValue(`settings.${key}`, value); + } + if (!userChangedResolutions && !downloadHasResolutions && defaults.video_resolutions) { + form.setValue('settings.video_resolutions', defaults.video_resolutions); + } } } catch (error) { - console.error('Failed to fetch video downloader config:', error); + console.error('Failed to fetch video download defaults:', error); } + setIsConfigLoaded(true); }; - fetchConfig(); + fetchDefaults(); }, []); React.useEffect(() => { @@ -496,6 +634,11 @@ export function VideosDownloadForm({ + + + + + {showMessage && } @@ -625,6 +768,13 @@ export function ChannelDownloadForm({ video_count_limit: null, video_format: defaultVideoFormat, video_resolutions: configResolutions, + writesubtitles: false, + writeautomaticsub: false, + writethumbnail: false, + writeinfojson: false, + yt_dlp_extra_args: '', + sleep_requests: '', + user_agent: '', }, sub_downloader: Downloaders.Video, tag_names: [], @@ -638,27 +788,42 @@ export function ChannelDownloadForm({ clearOnSuccess, }); - // Fetch video downloader config when the modal opens + // Fetch global defaults to pre-fill the form React.useEffect(() => { - const fetchConfig = async () => { + const fetchDefaults = async () => { try { - const result = await fetchVideoDownloaderConfig(); - setConfig(result); + const defaults = await fetchVideoDownloadDefaults(); + setConfig(defaults); setIsConfigLoaded(true); - // Update form with video resolutions from config only if user hasn't changed them - // and if the download doesn't already have video_resolutions set - const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; - if (result && result.video_resolutions && !userChangedResolutions && !downloadHasResolutions) { - form.setValue('settings.video_resolutions', result.video_resolutions); + if (defaults && download) { + // Edit mode: only fill settings not already in the download + for (const [key, value] of Object.entries(defaults)) { + if (key === 'continue_dl' || key === 'nooverwrites') continue; + if (download.settings?.[key] === undefined || download.settings?.[key] === null) { + form.setValue(`settings.${key}`, value); + } + } + } else if (defaults) { + // New download: pre-fill all inheritable settings from global defaults + const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; + for (const [key, value] of Object.entries(defaults)) { + if (key === 'continue_dl' || key === 'nooverwrites') continue; + if (key === 'video_resolutions' && (userChangedResolutions || downloadHasResolutions)) continue; + if (key === 'video_format') continue; + form.setValue(`settings.${key}`, value); + } + if (!userChangedResolutions && !downloadHasResolutions && defaults.video_resolutions) { + form.setValue('settings.video_resolutions', defaults.video_resolutions); + } } } catch (error) { - console.error('Failed to fetch video downloader config:', error); + console.error('Failed to fetch video download defaults:', error); setIsConfigLoaded(true); } }; - fetchConfig(); + fetchDefaults(); }, []); React.useEffect(() => { @@ -745,6 +910,11 @@ export function ChannelDownloadForm({ + + + + + {showMessage && } @@ -828,7 +998,14 @@ export function EditArchiveDownloadForm({ /> } -export function ArchiveDownloadForm({singleDownload = false, download, onCancel, onSuccess: propOnSuccess, submitter: propSubmitter, actions}) { +export function ArchiveDownloadForm({ + singleDownload = false, + download, + onCancel, + onSuccess: propOnSuccess, + submitter: propSubmitter, + actions + }) { const [showMessage, setShowMessage] = React.useState(false); const submitter = propSubmitter || (async (formData) => { @@ -933,6 +1110,13 @@ export function RSSDownloadForm({download, submitter, onDelete, onCancel, action title_include: null, video_resolutions: configResolutions, video_format: defaultVideoFormat, + writesubtitles: false, + writeautomaticsub: false, + writethumbnail: false, + writeinfojson: false, + yt_dlp_extra_args: '', + sleep_requests: '', + user_agent: '', }, tag_names: [], url: '', @@ -946,27 +1130,40 @@ export function RSSDownloadForm({download, submitter, onDelete, onCancel, action onSuccess: async () => setShowMessage(true), }); - // Fetch video downloader config when the modal opens + // Fetch global defaults to pre-fill the form React.useEffect(() => { - const fetchConfig = async () => { + const fetchDefaults = async () => { try { - const result = await fetchVideoDownloaderConfig(); - setConfig(result); + const defaults = await fetchVideoDownloadDefaults(); + setConfig(defaults); setIsConfigLoaded(true); - // Update form with video resolutions from config only if user hasn't changed them - // and if the download doesn't already have video_resolutions set - const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; - if (result && result.video_resolutions && !userChangedResolutions && !downloadHasResolutions) { - form.setValue('settings.video_resolutions', result.video_resolutions); + if (defaults && download) { + for (const [key, value] of Object.entries(defaults)) { + if (key === 'continue_dl' || key === 'nooverwrites') continue; + if (download.settings?.[key] === undefined || download.settings?.[key] === null) { + form.setValue(`settings.${key}`, value); + } + } + } else if (defaults) { + const downloadHasResolutions = download?.settings?.video_resolutions?.length > 0; + for (const [key, value] of Object.entries(defaults)) { + if (key === 'continue_dl' || key === 'nooverwrites') continue; + if (key === 'video_resolutions' && (userChangedResolutions || downloadHasResolutions)) continue; + if (key === 'video_format') continue; + form.setValue(`settings.${key}`, value); + } + if (!userChangedResolutions && !downloadHasResolutions && defaults.video_resolutions) { + form.setValue('settings.video_resolutions', defaults.video_resolutions); + } } } catch (error) { - console.error('Failed to fetch video downloader config:', error); + console.error('Failed to fetch video download defaults:', error); setIsConfigLoaded(true); } }; - fetchConfig(); + fetchDefaults(); }, []); React.useEffect(() => { @@ -1018,6 +1215,11 @@ export function RSSDownloadForm({download, submitter, onDelete, onCancel, action + + + + + ; } else if (form.formData.sub_downloader === Downloaders.Archive) { downloaderRows = diff --git a/app/src/components/Theme.tsx b/app/src/components/Theme.tsx index 8fac67cd..a4e30536 100644 --- a/app/src/components/Theme.tsx +++ b/app/src/components/Theme.tsx @@ -2,6 +2,8 @@ import React, {useContext, useEffect, useState, forwardRef, HTMLAttributes, CSSP import {ThemeContext} from "../contexts/contexts"; import { Accordion as SAccordion, + AccordionContent as SAccordionContent, + AccordionTitle as SAccordionTitle, Breadcrumb as SBreadcrumb, Button as SButton, ButtonGroup as SButtonGroup, @@ -36,7 +38,9 @@ import { TableRow as STableRow, TabPane as STabPane, TextArea as STextArea, + AccordionContentProps, AccordionProps, + AccordionTitleProps, BreadcrumbProps, ButtonProps, ButtonGroupProps, @@ -218,11 +222,36 @@ const defaultGrey = (props: T, inverted: boolean): T => { return props; } -export function Accordion(props: AccordionProps) { +// ---------------------------------------------------------------------------- +// Accordion Compound Component +// ---------------------------------------------------------------------------- + +type AccordionTitleComponent = React.FC; +type AccordionContentComponent = React.FC; + +interface AccordionComponent extends React.FC { + Title: AccordionTitleComponent; + Content: AccordionContentComponent; +} + +const AccordionTitle: AccordionTitleComponent = (props) => { + return +}; + +const AccordionContent: AccordionContentComponent = (props) => { + return +}; + +const AccordionBase: React.FC = (props) => { const {i, inverted} = useContext(ThemeContext); const mergedProps = defaultGrey({...i, ...props}, !!inverted); return -} +}; + +export const Accordion: AccordionComponent = Object.assign(AccordionBase, { + Title: AccordionTitle, + Content: AccordionContent, +}); // ---------------------------------------------------------------------------- // Button Compound Component diff --git a/app/src/components/Videos.js b/app/src/components/Videos.js index e0cb2316..7f28c475 100644 --- a/app/src/components/Videos.js +++ b/app/src/components/Videos.js @@ -67,7 +67,7 @@ import {Media, QueryContext, ThemeContext} from "../contexts/contexts"; import _ from "lodash"; import {defaultFileOrder, defaultSearchOrder, HELP_VIEWER_URI} from "./Vars"; import {InputForm, ToggleForm, useForm} from "../hooks/useForm"; -import {VideoResolutionSelectorForm} from "./Download"; +import {VideoFormatSelectorForm, VideoResolutionSelectorForm} from "./Download"; import {BatchReorganizeModal} from "./collections/BatchReorganizeModal"; export function VideoWrapper() { @@ -332,9 +332,12 @@ function VideosSettingsPage() { const emptyFormData = { video_resolutions: ['1080p', '720p', '480p', 'maximum'], yt_dlp_options: { + continue_dl: true, file_name_format: '%(uploader)s_%(upload_date)s_%(id)s_%(title)s.%(ext)s', + merge_output_format: 'mp4', nooverwrites: true, writeautomaticsub: true, + writeinfojson: true, writesubtitles: true, writethumbnail: true, }, @@ -384,7 +387,14 @@ function VideosSettingsPage() { path='video_resolutions' /> - + + + + @@ -408,7 +418,7 @@ function VideosSettingsPage() { /> - + + + + + + + + + 0: - cmd = (*cmd, '--sleep-requests', str(config.sleep_requests)) + if effective['yt_dlp_extra_args']: + cmd = (*cmd, *effective['yt_dlp_extra_args'].split(' ')) + if effective['sleep_requests'] > 0: + cmd = (*cmd, '--sleep-requests', str(effective['sleep_requests'])) if cookies_unlocked(): # Use encrypted cookies file logger.info(f'Using encrypted cookies for video download: {url}') with cookies_for_download() as cookies_path: cmd = (*cmd, '--cookies', str(cookies_path)) - if config.user_agent: - cmd = (*cmd, '--add-headers', f'User-Agent:{config.user_agent}') + if effective['user_agent']: + cmd = (*cmd, '--add-headers', f'User-Agent:{effective["user_agent"]}') cmd = (*cmd, '-o', video_path, url) result = await self.process_runner(download, cmd, out_dir, debug=True) else: diff --git a/modules/videos/lib.py b/modules/videos/lib.py index 5ed0be21..3f28b5af 100644 --- a/modules/videos/lib.py +++ b/modules/videos/lib.py @@ -562,9 +562,14 @@ class VideoDownloaderConfig(ConfigFile): return self._config['yt_dlp_options']['nooverwrites'] @property - def merge_output_format(self) -> bool: + def merge_output_format(self) -> str: return self._config['yt_dlp_options']['merge_output_format'] + @property + def video_format(self) -> str: + """Alias for merge_output_format, matching the name used in Download.settings.""" + return self.merge_output_format + @property def quiet(self) -> bool: return self._config['yt_dlp_options']['quiet'] @@ -652,6 +657,51 @@ def set_test_downloader_config(enabled: bool): TEST_VIDEOS_DOWNLOADER_CONFIG = VideoDownloaderConfig() if enabled else None +# Names of settings that can be overridden at channel/RSS or video level. +# Actual default values come from VideoDownloaderConfig. +INHERITABLE_VIDEO_SETTINGS = { + 'video_resolutions', 'video_format', 'writesubtitles', 'writeautomaticsub', + 'writethumbnail', 'writeinfojson', 'yt_dlp_extra_args', 'sleep_requests', + 'user_agent', 'continue_dl', 'nooverwrites', +} + +# Subset of inheritable settings allowed at channel/RSS level. +# continue_dl and nooverwrites are global+video only. +CHANNEL_INHERITABLE_SETTINGS = INHERITABLE_VIDEO_SETTINGS - {'continue_dl', 'nooverwrites'} + + +def _extract_global_inheritable(config: VideoDownloaderConfig) -> dict: + """Read current global config values for all inheritable settings, flattened.""" + return { + 'video_resolutions': config.video_resolutions, + 'video_format': config.merge_output_format, + 'writesubtitles': config.writesubtitles, + 'writeautomaticsub': config.writeautomaticsub, + 'writethumbnail': config.writethumbnail, + 'writeinfojson': config.writeinfojson, + 'yt_dlp_extra_args': config.yt_dlp_extra_args, + 'sleep_requests': config.sleep_requests, + 'user_agent': config.user_agent, + 'continue_dl': config.continue_dl, + 'nooverwrites': config.nooverwrites, + } + + +def get_effective_video_settings(download_settings: dict = None) -> dict: + """Merge global defaults with download-level overrides. + + At download time: effective = {**global, **download_settings} + Only inheritable keys present in download_settings override the global value. + """ + config = get_videos_downloader_config() + effective = _extract_global_inheritable(config) + if download_settings: + for key in INHERITABLE_VIDEO_SETTINGS: + if key in download_settings: + effective[key] = download_settings[key] + return effective + + def get_yt_dlp_http_headers() -> dict: """Get HTTP headers for yt-dlp, including user-agent if configured. diff --git a/modules/videos/test/test_downloader.py b/modules/videos/test/test_downloader.py index d77c8a8c..7a44209f 100644 --- a/modules/videos/test/test_downloader.py +++ b/modules/videos/test/test_downloader.py @@ -695,3 +695,129 @@ async def test_channel_downloader_sets_collection_id(test_session, channel_facto test_session.refresh(download) assert download.collection_id == channel.collection_id, \ 'ChannelDownloader should link download to channel collection' + + +@pytest.mark.asyncio +async def test_channel_passes_all_inheritable_settings_to_video(test_session, channel_factory, + mock_video_extract_info, + video_download_manager, await_switches): + """ChannelDownloader passes all CHANNEL_INHERITABLE_SETTINGS to child video downloads.""" + from modules.videos.lib import CHANNEL_INHERITABLE_SETTINGS + + channel = channel_factory(url='https://example.com/channel', source_id='UCtest456') + + channel_settings = { + 'video_resolutions': ['720p'], + 'video_format': 'mkv', + 'writesubtitles': False, + 'sleep_requests': 3.0, + 'user_agent': 'TestBot/1.0', + } + + download = Download( + url='https://example.com/channel', + downloader=channel_downloader.name, + frequency=99, + settings=channel_settings, + ) + test_session.add(download) + test_session.commit() + + mock_info = { + 'id': 'UCtest456', + 'channel_id': 'UCtest456', + 'uploader': channel.name, + 'entries': [{'id': 'vid1', 'url': 'https://youtube.com/watch?v=vid1', 'title': 'Video 1'}], + 'webpage_url_basename': 'videos', + } + mock_video_extract_info.return_value = mock_info + + with mock.patch('modules.videos.downloader.VideoDownloader.do_download') as mock_video_do_download, \ + mock.patch.object(Channel, 'refresh_files', return_value='test-job-id'), \ + mock.patch('modules.videos.downloader.file_worker.wait_for_job', new_callable=mock.AsyncMock): + mock_video_do_download.return_value = DownloadResult(success=True) + result = await channel_downloader.do_download(download) + + # Verify all channel-level inheritable settings are passed through + for key in ('video_resolutions', 'video_format', 'writesubtitles', 'sleep_requests', 'user_agent'): + assert key in result.settings, f'{key} missing from DownloadResult.settings' + assert result.settings[key] == channel_settings[key], \ + f'{key}: expected {channel_settings[key]}, got {result.settings[key]}' + + +@pytest.mark.asyncio +async def test_channel_does_not_pass_continue_dl_nooverwrites(test_session, channel_factory, + mock_video_extract_info, + video_download_manager, await_switches): + """continue_dl and nooverwrites should NOT be passed from channel to video.""" + channel = channel_factory(url='https://example.com/channel', source_id='UCtest789') + + channel_settings = { + 'continue_dl': False, + 'nooverwrites': False, + 'video_format': 'mkv', + } + + download = Download( + url='https://example.com/channel', + downloader=channel_downloader.name, + frequency=99, + settings=channel_settings, + ) + test_session.add(download) + test_session.commit() + + mock_info = { + 'id': 'UCtest789', + 'channel_id': 'UCtest789', + 'uploader': channel.name, + 'entries': [{'id': 'vid2', 'url': 'https://youtube.com/watch?v=vid2', 'title': 'Video 2'}], + 'webpage_url_basename': 'videos', + } + mock_video_extract_info.return_value = mock_info + + with mock.patch('modules.videos.downloader.VideoDownloader.do_download') as mock_video_do_download, \ + mock.patch.object(Channel, 'refresh_files', return_value='test-job-id'), \ + mock.patch('modules.videos.downloader.file_worker.wait_for_job', new_callable=mock.AsyncMock): + mock_video_do_download.return_value = DownloadResult(success=True) + result = await channel_downloader.do_download(download) + + assert 'continue_dl' not in result.settings, 'continue_dl should not be passed to child video downloads' + assert 'nooverwrites' not in result.settings, 'nooverwrites should not be passed to child video downloads' + assert result.settings['video_format'] == 'mkv', 'video_format should be passed through' + + +@pytest.mark.asyncio +async def test_video_download_uses_effective_settings(test_session, test_directory, mock_video_extract_info, + video_download_manager, mock_video_process_runner, + image_file, simple_channel, test_videos_downloader_config): + """VideoDownloader uses merged effective settings from global + download overrides.""" + simple_channel.source_id = example_video_json['channel_id'] + simple_channel.directory = test_directory / 'videos/channel name' + simple_channel.directory.mkdir(parents=True) + + video_path = simple_channel.directory / 'a video.mp4' + shutil.copy(PROJECT_DIR / 'test/big_buck_bunny_720p_1mb.mp4', video_path) + image_file.rename(video_path.with_suffix('.jpg')) + + url = 'https://www.youtube.com/watch?v=31jPEBiAC3c' + + # Download with writesubtitles=False override (global default is True) + settings = {'writesubtitles': False, 'sleep_requests': 5.0} + + with mock.patch('modules.videos.downloader.VideoDownloader.prepare_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, settings=settings) + await video_download_manager.wait_for_all_downloads() + + mock_video_process_runner.assert_called_once() + download, cmd, out_dir = mock_video_process_runner.call_args[0] + + # writesubtitles=False means no --write-subs in command + assert '--write-subs' not in cmd, 'writesubtitles=False should prevent --write-subs' + # sleep_requests=5.0 should appear + assert '--sleep-requests' in cmd, 'sleep_requests override should appear in command' + sleep_idx = cmd.index('--sleep-requests') + assert cmd[sleep_idx + 1] == '5.0', f'Expected sleep_requests=5.0, got {cmd[sleep_idx + 1]}' diff --git a/modules/videos/test/test_lib.py b/modules/videos/test/test_lib.py index 6c8ba151..48959875 100644 --- a/modules/videos/test/test_lib.py +++ b/modules/videos/test/test_lib.py @@ -439,3 +439,65 @@ def test_format_video_filename_uses_channel_name_over_info_json(test_session, vi # Should use WROLPi channel name, NOT the YouTube uploader assert 'My Custom Channel' in result assert 'Learning Self Reliance' not in result + + +def test_extract_global_inheritable(test_directory, test_videos_downloader_config): + """Verify _extract_global_inheritable returns all expected keys with correct mapping.""" + config = lib.get_videos_downloader_config() + result = lib._extract_global_inheritable(config) + assert set(result.keys()) == lib.INHERITABLE_VIDEO_SETTINGS + # merge_output_format maps to video_format + assert result['video_format'] == config.merge_output_format + assert result['video_resolutions'] == config.video_resolutions + assert result['writesubtitles'] == config.writesubtitles + assert result['continue_dl'] == config.continue_dl + + +def test_get_effective_video_settings_no_overrides(test_directory, test_videos_downloader_config): + """With no download settings, effective settings equal global defaults.""" + config = lib.get_videos_downloader_config() + effective = lib.get_effective_video_settings() + assert effective['video_resolutions'] == config.video_resolutions + assert effective['video_format'] == config.merge_output_format + assert effective['writesubtitles'] == config.writesubtitles + + +def test_get_effective_video_settings_partial_override(test_directory, test_videos_downloader_config): + """Download settings override only the keys they specify.""" + config = lib.get_videos_downloader_config() + overrides = {'writesubtitles': False, 'sleep_requests': 2.0} + effective = lib.get_effective_video_settings(overrides) + assert effective['writesubtitles'] is False + assert effective['sleep_requests'] == 2.0 + # Everything else stays at global default + assert effective['video_resolutions'] == config.video_resolutions + assert effective['writethumbnail'] == config.writethumbnail + + +def test_get_effective_video_settings_global_change_propagates(test_directory, test_videos_downloader_config): + """Changing global config propagates to downloads without overrides.""" + config = lib.get_videos_downloader_config() + # Change global + config.update({**config._config, 'sleep_requests': 5.0}) + # Download with no override should pick up new global + effective = lib.get_effective_video_settings({}) + assert effective['sleep_requests'] == 5.0 + # Download with override should keep its value + effective = lib.get_effective_video_settings({'sleep_requests': 1.0}) + assert effective['sleep_requests'] == 1.0 + + +@pytest.mark.asyncio +async def test_get_download_defaults_endpoint(async_client, test_directory, test_videos_downloader_config): + """GET /api/videos/download-defaults returns flattened global settings.""" + request, response = await async_client.get('/api/videos/download-defaults') + assert response.status == HTTPStatus.OK + data = response.json + assert 'video_resolutions' in data + assert 'video_format' in data + assert 'writesubtitles' in data + assert 'continue_dl' in data + assert 'nooverwrites' in data + # Should NOT contain global-only settings + assert 'file_name_format' not in data + assert 'quiet' not in data diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index dc38d99c..22405a54 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -1372,6 +1372,22 @@ class DownloadManagerConfig(ConfigFile): def downloads(self, value: List[dict]): self.update({'downloads': value}) + @staticmethod + def _filter_settings_for_yaml(settings: dict) -> dict: + """Remove inheritable video settings that match current global defaults. + Keeps YAML minimal — only overrides are persisted.""" + if not settings: + return settings + from modules.videos.lib import INHERITABLE_VIDEO_SETTINGS, _extract_global_inheritable, \ + get_videos_downloader_config + global_defaults = _extract_global_inheritable(get_videos_downloader_config()) + filtered = {} + for k, v in settings.items(): + if k in INHERITABLE_VIDEO_SETTINGS and v == global_defaults.get(k): + continue + filtered[k] = v + return filtered or None + def dump_config(self, file: pathlib.Path = None, send_events=False, overwrite=False): try: with get_db_session() as session: @@ -1388,7 +1404,7 @@ class DownloadManagerConfig(ConfigFile): frequency=download.frequency, last_successful_download=download.last_successful_download, next_download=download.next_download, - settings=download.settings, + settings=self._filter_settings_for_yaml(download.settings), status=download.status, sub_downloader=download.sub_downloader, tag_names=download.tag_names, @@ -1622,12 +1638,13 @@ class RSSDownloader(Downloader): logger.info(f'Successfully got {len(urls)} new URLs from RSS {download.url}') # Pass settings onto the next Downloader. + from modules.videos.lib import CHANNEL_INHERITABLE_SETTINGS next_download_settings = dict() settings = download.settings or dict() - if i := settings.get('video_resolutions'): - next_download_settings['video_resolutions'] = i - if i := settings.get('video_format'): - next_download_settings['video_format'] = i + # Pass all channel-level inheritable settings to child video downloads. + for key in CHANNEL_INHERITABLE_SETTINGS: + if key in settings: + next_download_settings[key] = settings[key] # Use download.destination column (settings['destination'] is legacy) if download.destination: next_download_settings['destination'] = str(download.destination) diff --git a/wrolpi/schema.py b/wrolpi/schema.py index 0dc3a636..a08dd42d 100644 --- a/wrolpi/schema.py +++ b/wrolpi/schema.py @@ -236,6 +236,16 @@ class DownloadSettings: video_format: Optional[str] = None video_resolutions: List[str] = field(default_factory=list) parent_download_url: Optional[str] = None + # Inheritable video settings (Optional so they're only set when user overrides) + writesubtitles: Optional[bool] = None + writeautomaticsub: Optional[bool] = None + writethumbnail: Optional[bool] = None + writeinfojson: Optional[bool] = None + yt_dlp_extra_args: Optional[str] = None + sleep_requests: Optional[float] = None + user_agent: Optional[str] = None + continue_dl: Optional[bool] = None + nooverwrites: Optional[bool] = None def __post_init__(self): if self.excluded_urls and self.excluded_urls.endswith(','): diff --git a/wrolpi/test/test_downloader.py b/wrolpi/test/test_downloader.py index b7aa9c4c..85c9c1b0 100644 --- a/wrolpi/test/test_downloader.py +++ b/wrolpi/test/test_downloader.py @@ -853,3 +853,72 @@ async def test_child_downloads_have_parent_download_url(test_session, test_downl assert 'parent_download_url' in child.settings, f'Child download {child.url} missing parent_download_url' assert child.settings['parent_download_url'] == parent_url, \ f'Child download {child.url} has wrong parent_download_url: {child.settings.get("parent_download_url")}' + + +def test_yaml_dump_filters_default_settings(test_session, test_download_manager, test_download_manager_config, + test_downloader): + """Settings matching global defaults should be stripped from YAML.""" + from modules.videos.lib import set_test_downloader_config, get_videos_downloader_config + + set_test_downloader_config(True) + try: + config = get_videos_downloader_config() + + # Create a download with settings that match global defaults + download = test_download_manager.create_download( + test_session, 'https://example.com/channel', test_downloader.name, + settings={ + 'video_resolutions': config.video_resolutions, + 'video_format': config.merge_output_format, + 'writesubtitles': config.writesubtitles, + 'title_include': 'important', # non-inheritable, should be kept + }, + ) + download.frequency = 99 + test_session.commit() + + get_download_manager_config().dump_config() + + # Read the YAML and check + yaml_data = get_download_manager_config()._config + download_entry = next(d for d in yaml_data['downloads'] if d['url'] == 'https://example.com/channel') + + # Inheritable defaults should be filtered out + settings = download_entry['settings'] + assert 'video_resolutions' not in settings, 'Default video_resolutions should be filtered from YAML' + assert 'video_format' not in settings, 'Default video_format should be filtered from YAML' + assert 'writesubtitles' not in settings, 'Default writesubtitles should be filtered from YAML' + # Non-inheritable settings should be preserved + assert settings['title_include'] == 'important', 'Non-inheritable settings should be kept in YAML' + finally: + set_test_downloader_config(False) + + +def test_yaml_dump_keeps_overrides(test_session, test_download_manager, test_download_manager_config, + test_downloader): + """Settings that differ from global defaults should be kept in YAML.""" + from modules.videos.lib import set_test_downloader_config + + set_test_downloader_config(True) + try: + # Create a download with settings that differ from global defaults + download = test_download_manager.create_download( + test_session, 'https://example.com/channel2', test_downloader.name, + settings={ + 'video_resolutions': ['720p'], # differs from default + 'writesubtitles': False, # differs from default True + }, + ) + download.frequency = 99 + test_session.commit() + + get_download_manager_config().dump_config() + + yaml_data = get_download_manager_config()._config + download_entry = next(d for d in yaml_data['downloads'] if d['url'] == 'https://example.com/channel2') + settings = download_entry['settings'] + + assert settings['video_resolutions'] == ['720p'], 'Non-default video_resolutions should be kept' + assert settings['writesubtitles'] is False, 'Non-default writesubtitles should be kept' + finally: + set_test_downloader_config(False)