From 4cc48ae98fa446ab8502911d7de13ff48067dc89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Sat, 25 Jul 2026 01:35:23 +0200 Subject: [PATCH] Fix medium-severity defects across download, formats and tooling - Format table: a missing acodec was treated as "has audio", skipping the +bestaudio merge and producing silent videos for extractors that omit the field. - Progress bar: separate video/audio stream downloads each reported 0-100%, making the bar jump backwards; per-phase scaling now maps the two streams onto 0-50/50-100. - Custom commands: parse with shlex (quoted arguments with spaces were shredded by str.split), keep POSIX mode off on Windows so backslash paths survive, hide the console window like every other call site, close the stdout pipe, and support cancellation of a running command. - Settings dialog: _("settings", "error_saving", ...) passed two positional args to the i18n helper, raising TypeError inside the except handler instead of showing the intended error dialog. - ffmpeg on Windows: Path(os.getenv("LOCALAPPDATA")) crashed with TypeError when the variable is unset; fall back to the standard AppData/Local location. - Version cache: cached path (str) was compared against a Path, so the cache never hit and every version query spawned a subprocess. Co-Authored-By: Claude Fable 5 --- ytsage/core/ytsage_downloader.py | 10 ++++++ ytsage/core/ytsage_ffmpeg.py | 4 +-- ytsage/core/ytsage_utils.py | 7 ++-- .../ytsage_dialogs_custom.py | 35 ++++++++++++++----- .../ytsage_dialogs_settings.py | 2 +- ytsage/gui/ytsage_gui_format_table.py | 5 ++- 6 files changed, 49 insertions(+), 14 deletions(-) diff --git a/ytsage/core/ytsage_downloader.py b/ytsage/core/ytsage_downloader.py index 0d9ee54..add24c9 100644 --- a/ytsage/core/ytsage_downloader.py +++ b/ytsage/core/ytsage_downloader.py @@ -113,6 +113,8 @@ class DownloadThread(QThread): self.subtitle_files: List[str] = [] # Track subtitle files that are created self.initial_subtitle_files: Set[Path] = set() # Track initial subtitle files before download self.download_files: Set[Path] = set() # Every destination path this download wrote to + self.expected_phases: int = 1 # 2 when video and audio download separately before merge + self._media_phase: int = 0 # Index of the media stream currently downloading def cleanup_partial_files(self) -> None: """Delete partial files (.part/.ytdl and unmerged .fNNN. streams), but only @@ -293,6 +295,7 @@ class DownloadThread(QThread): logger.debug(f"Using progressive format with bundled audio: {clean_format_id}") else: cmd.extend(["-f", f"{clean_format_id}+bestaudio/best"]) + self.expected_phases = 2 # separate video and audio downloads logger.debug(f"Using video-only format merged with best audio: {clean_format_id}+bestaudio/best") else: # If no specific format ID, use resolution-based sorting (-S) @@ -635,6 +638,8 @@ class DownloadThread(QThread): filepath = dest_match.group(1).strip() self.current_filename = Path(filepath).name self.last_file_path = filepath # Store the full path for later cleanup + if Path(filepath) not in self.download_files and Path(filepath).suffix.lower() not in SUBTITLE_EXTENSIONS: + self._media_phase += 1 self.download_files.add(Path(filepath)) logger.debug(f"Extracted filename: {self.current_filename}") # DEBUG @@ -755,6 +760,11 @@ class DownloadThread(QThread): if percent_match: try: percent = float(percent_match.group(1)) + # When video and audio download as separate streams, scale each + # phase into its share of the bar instead of jumping 0-100 twice + if self.expected_phases > 1 and not self.is_playlist: + completed = max(0, min(self._media_phase - 1, self.expected_phases - 1)) + percent = (completed * 100.0 + percent) / self.expected_phases self.progress_signal.emit(percent) except (ValueError, IndexError): pass diff --git a/ytsage/core/ytsage_ffmpeg.py b/ytsage/core/ytsage_ffmpeg.py index 51842cb..a604cd2 100644 --- a/ytsage/core/ytsage_ffmpeg.py +++ b/ytsage/core/ytsage_ffmpeg.py @@ -91,7 +91,7 @@ def get_ffmpeg_install_path() -> Path: For Windows, tries to find the latest essentials build dynamically. """ if OS_NAME == "Windows": - ffmpeg_base = Path(os.getenv("LOCALAPPDATA")) / "ffmpeg" # type: ignore + ffmpeg_base = Path(os.getenv("LOCALAPPDATA", str(Path.home() / "AppData" / "Local"))) / "ffmpeg" # If the directory exists, look for any ffmpeg-*-essentials_build folder if ffmpeg_base.exists(): @@ -205,7 +205,7 @@ def install_ffmpeg_windows(progress_callback=None) -> bool: try: # Define variables for essentials build - extract_dir = Path(os.getenv("LOCALAPPDATA")) / "ffmpeg" # type: ignore + extract_dir = Path(os.getenv("LOCALAPPDATA", str(Path.home() / "AppData" / "Local"))) / "ffmpeg" # Create extraction directory if it doesn't exist extract_dir.mkdir(exist_ok=True) diff --git a/ytsage/core/ytsage_utils.py b/ytsage/core/ytsage_utils.py index b5164f1..e7ca0bf 100644 --- a/ytsage/core/ytsage_utils.py +++ b/ytsage/core/ytsage_utils.py @@ -72,8 +72,11 @@ def should_refresh_cache(tool_name: str, current_path: Optional[str]) -> bool: if not cache.get("version"): return True - # Refresh if path changed - if cache.get("path") != current_path: + # Refresh if path changed (cache stores str; callers may pass Path - + # compare normalized strings or the cache would never hit) + cached_path = cache.get("path") + normalized_current = str(current_path) if current_path is not None else None + if cached_path != normalized_current: return True # Refresh if file was modified diff --git a/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_custom.py b/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_custom.py index 8b1cf55..cf09777 100644 --- a/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_custom.py +++ b/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_custom.py @@ -3,6 +3,8 @@ Custom functionality dialogs for YTSage application. Contains dialogs for custom commands, cookies, time ranges, and other special features. """ +import os +import shlex import subprocess import threading from pathlib import Path @@ -32,7 +34,7 @@ from PySide6.QtWidgets import ( from ..ytsage_smooth_tab_widget import SmoothTabWidget from ...core.ytsage_yt_dlp import get_yt_dlp_path from ...core.ytsage_utils import update_auto_update_settings -from ...utils.ytsage_constants import YTDLP_DOCS_URL +from ...utils.ytsage_constants import SUBPROCESS_CREATIONFLAGS, YTDLP_DOCS_URL from ...utils.ytsage_config_manager import ConfigManager from ...utils.ytsage_localization import LocalizationManager, _ from ...utils.ytsage_logger import logger @@ -55,16 +57,28 @@ class CommandWorker(QObject): self.command = command self.url = url self.path = path + self._proc = None + self._cancelled = False + + def cancel(self): + """Terminate a running command.""" + self._cancelled = True + if self._proc and self._proc.poll() is None: + try: + self._proc.terminate() + except Exception: + pass def run_command(self): """Run the yt-dlp command and emit signals for output""" try: - # Split command into arguments - args = self.command.split() + # Split command into arguments; posix=False on Windows so quoted + # backslash paths like "C:\Users\..." survive intact + args = shlex.split(self.command, posix=(os.name != "nt")) # Build the full command yt_dlp_path = get_yt_dlp_path() - base_cmd = [yt_dlp_path] + args + base_cmd = [str(yt_dlp_path)] + args # Add download path if specified if self.path: @@ -78,19 +92,24 @@ class CommandWorker(QObject): self.output_received.emit("=" * 50) # Run the command - proc = subprocess.Popen( + self._proc = subprocess.Popen( base_cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, encoding="utf-8", errors="replace", + creationflags=SUBPROCESS_CREATIONFLAGS, ) + proc = self._proc # Stream output - for line in proc.stdout: # type: ignore[reportOptionalIterable] - if line.strip(): # Only show non-empty lines - self.output_received.emit(line.rstrip()) + with proc.stdout: # type: ignore[union-attr] + for line in proc.stdout: # type: ignore[reportOptionalIterable] + if self._cancelled: + break + if line.strip(): # Only show non-empty lines + self.output_received.emit(line.rstrip()) ret = proc.wait() self.output_received.emit("=" * 50) diff --git a/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_settings.py b/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_settings.py index e24822f..f27bc22 100644 --- a/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_settings.py +++ b/ytsage/gui/ytsage_gui_dialogs/ytsage_dialogs_settings.py @@ -985,5 +985,5 @@ class AutoUpdateSettingsDialog(QDialog): msg_box.exec() except Exception as e: logger.exception(f"Error saving auto-update settings: {e}") - msg_box = self._create_styled_message_box(QMessageBox.Icon.Critical, _("settings.error_title"), _("settings", "error_saving", error=str(e))) + msg_box = self._create_styled_message_box(QMessageBox.Icon.Critical, _("settings.error_title"), _("settings.error_saving", error=str(e))) msg_box.exec() diff --git a/ytsage/gui/ytsage_gui_format_table.py b/ytsage/gui/ytsage_gui_format_table.py index 651ff1f..97e5a15 100644 --- a/ytsage/gui/ytsage_gui_format_table.py +++ b/ytsage/gui/ytsage_gui_format_table.py @@ -346,7 +346,10 @@ class FormatTableMixin: checkbox.setStyleSheet("QCheckBox { margin-left: 8px; }") checkbox.format_id = f["format_id"] checkbox.is_audio_only = f.get("vcodec") == "none" - checkbox.has_audio = f.get("acodec") != "none" + # A missing acodec means unknown, not progressive - treating it as + # has-audio skips the +bestaudio merge and yields silent videos + acodec = f.get("acodec") + checkbox.has_audio = acodec is not None and acodec != "none" checkbox.clicked.connect(lambda checked, cb=checkbox: self.handle_checkbox_click(cb)) self.format_checkboxes.append(checkbox)