diff --git a/lintunes/gui/main_window.py b/lintunes/gui/main_window.py index 6662bd6..67466c3 100644 --- a/lintunes/gui/main_window.py +++ b/lintunes/gui/main_window.py @@ -110,6 +110,9 @@ class MainWindow(QMainWindow): manager.tag_write_failed.connect( lambda name, err: self.statusBar().showMessage( f"Couldn't write tags to “{name}”: {err}", 8000)) + manager.file_move_failed.connect( + lambda name, err: self.statusBar().showMessage( + f"Couldn't move the file for “{name}”: {err}", 8000)) manager.library_reloaded.connect(self._on_library_reloaded) manager.conflict_resolved.connect(self._on_conflict_resolved) prefs.changed.connect(self._on_prefs_changed) @@ -305,7 +308,9 @@ class MainWindow(QMainWindow): # ---- file import ---- def _music_import_dir(self) -> Path: - return Path(self._manager.library.music_folder) / "Music" + # Fall back to ./Music when no music folder is set (fresh library), + # matching the pre-organize_root behavior. + return self._manager.organize_root() or Path("Music") def _open_files_dialog(self): paths, _filter = QFileDialog.getOpenFileNames( diff --git a/lintunes/library_manager.py b/lintunes/library_manager.py index 4448296..af33367 100644 --- a/lintunes/library_manager.py +++ b/lintunes/library_manager.py @@ -8,6 +8,7 @@ from PyQt6.QtCore import QObject, QTimer, pyqtSignal log = logging.getLogger(__name__) from lintunes import smart, tagging +from lintunes.importers.file_importer import organized_destination, unique_path from lintunes.models import Library, Playlist, PlaylistType from lintunes.storage import json_storage from lintunes.undo import Command, UndoStack @@ -36,6 +37,7 @@ class LibraryManager(QObject): library_reloaded = pyqtSignal() # library re-read from disk (sync) conflict_resolved = pyqtSignal(list) # list[ConflictSummary] just merged tag_write_failed = pyqtSignal(str, str) # track name, error text + file_move_failed = pyqtSignal(str, str) # track name, error text def __init__(self, library: Library, data_dir: Path, parent=None): super().__init__(parent) @@ -374,6 +376,10 @@ class LibraryManager(QObject): # File write failed: leave the library untouched so memory and file # don't diverge, and don't record an un-undoable phantom edit. return + new_loc = self._maybe_move_file(track, changed) + if new_loc is not None: + old = {**old, "location": track.location} + changed = {**changed, "location": new_loc} self._apply_track_fields(track_id, changed) self.undo_stack.push(Command( "Edit Info", @@ -398,6 +404,10 @@ class LibraryManager(QObject): if not changed or not self._write_track_tags(track, changed): continue old = {k: getattr(track, k) for k in changed} + new_loc = self._maybe_move_file(track, changed) + if new_loc is not None: + old = {**old, "location": track.location} + changed = {**changed, "location": new_loc} self._apply_track_fields(track_id, changed) changes.append((track_id, changed, old)) if not changes: @@ -441,8 +451,93 @@ class LibraryManager(QObject): track = self.library.tracks.get(track_id) if track: self._write_track_tags(track, field_map) # best-effort + field_map = self._revert_file_move(track, field_map) self._apply_track_fields(track_id, field_map) + def _revert_file_move(self, track, field_map: dict) -> dict: + """Undo/redo of an edit that moved the file: move it to the map's + location, best-effort. If the physical move fails, drop ``location`` + from the map so memory keeps pointing at where the file really is.""" + target = field_map.get("location") + if target is None or target == track.location: + return field_map + src = Path(track.location) + try: + if not src.is_file(): + raise OSError(f"missing source file {src}") + actual = self._move_file(src, Path(target)) + return {**field_map, "location": str(actual)} + except OSError as e: + log.warning("Could not move %s back to %s: %s", src, target, e) + self.file_move_failed.emit(track.name or str(src), str(e)) + return {k: v for k, v in field_map.items() if k != "location"} + + # ---- keeping the music folder organized ---- + + _ORGANIZE_FIELDS = frozenset({"artist", "album_artist", "album"}) + + def organize_root(self) -> Path | None: + """Root of the organized music tree (/Music) — the only + place imports copy into and rename-moves manage.""" + if not self.library.music_folder: + return None + return Path(self.library.music_folder) / "Music" + + def _maybe_move_file(self, track, changed: dict) -> str | None: + """If an artist/album_artist/album edit changes where the file belongs + in the organized Artist/Album tree, move it there (creating folders as + needed) and return the new absolute path, else None. + + Only files already inside the organize root are managed — an external + file the user never imported must not get pulled into the synced tree + by a tag edit. A failed move is reported via ``file_move_failed`` and + the edit proceeds with the old location (the tags are already written, + so the library stays consistent, just unorganized).""" + if not (self._ORGANIZE_FIELDS & changed.keys()) or not track.location: + return None + root = self.organize_root() + src = Path(track.location) + if root is None or root not in src.parents or not src.is_file(): + return None + merged = {key: changed.get(key, getattr(track, key)) + for key in self._ORGANIZE_FIELDS} + # Keep the exact on-disk basename: it's already valid here, and + # re-sanitizing could gratuitously rename files that came from macOS. + dest = organized_destination(root, merged, src.name).with_name(src.name) + if dest == src: + return None + try: + return str(self._move_file(src, dest)) + except OSError as e: + log.warning("Could not move %s -> %s: %s", src, dest, e) + self.file_move_failed.emit(track.name or str(src), str(e)) + return None + + def _move_file(self, src: Path, dest: Path) -> Path: + """Physically relocate a music file and prune the directories it left + empty. Same-filesystem rename, so this is safe even while the player + has the file open (the fd stays valid; the path is re-read at next + play). Returns the actual destination (collision-suffixed if taken).""" + dest = unique_path(dest) + dest.parent.mkdir(parents=True, exist_ok=True) + src.rename(dest) + root = self.organize_root() + if root is not None: + self._prune_empty_dirs(src.parent, root) + return dest + + @staticmethod + def _prune_empty_dirs(start: Path, root: Path): + """Remove now-empty directories from ``start`` upward, strictly below + ``root`` (the organize root itself is never touched).""" + current = start + while root in current.parents: + try: + current.rmdir() # raises OSError if not empty + except OSError: + return + current = current.parent + def record_play(self, track_id: int): track = self.library.tracks.get(track_id) if not track: diff --git a/lintunes/storage/conflict_resolver.py b/lintunes/storage/conflict_resolver.py index 63e639d..8584184 100644 --- a/lintunes/storage/conflict_resolver.py +++ b/lintunes/storage/conflict_resolver.py @@ -178,10 +178,15 @@ def _merge_track_fields(orig: dict, conflict: dict) -> list[str]: if conflict.get("rating") != orig.get("rating") and "rating" in conflict: notes.append(f"rating {_stars(orig.get('rating'))} → " f"{_stars(conflict.get('rating'))} (newer)") + # location rides along: renaming artist/album moves the file (and bumps + # date_modified), so the machine with the newest edit also knows where + # the file now lives. Both sides are relative here (raw JSON) and both + # absolute in the reload_from_disk reconcile — always like-with-like. for field_name in ("rating", "name", "artist", "album_artist", "album", "genre", "composer", "grouping", "comments", "sort_name", "sort_artist", "sort_album_artist", - "sort_album", "sort_composer", "date_modified"): + "sort_album", "sort_composer", "location", + "date_modified"): if field_name in conflict: orig[field_name] = conflict[field_name] return notes diff --git a/tests/test_round18.py b/tests/test_round18.py index e388340..b67c943 100644 --- a/tests/test_round18.py +++ b/tests/test_round18.py @@ -6,10 +6,17 @@ folder organized (Artist/Album, Unknown Artist fallback). """ +import shutil +from pathlib import Path + +import pytest + from PyQt6.QtCore import QObject, pyqtSignal from PyQt6.QtGui import QColor from lintunes import theme +from lintunes.library_manager import LibraryManager +from lintunes.models import Library, Track from lintunes.preferences import Preferences from lintunes.gui.visualizer import ( VisualizerWidget, MODE_ON, MODE_DIM, MODE_OFF) @@ -106,8 +113,6 @@ class TestVisualizerGrayColor: # -------------------------------------------------------------------------- def _seeded_manager(tmp_path, track_ids=(1, 2, 3)): - from lintunes.models import Library, Track - from lintunes.library_manager import LibraryManager tracks = {tid: Track(track_id=tid, name=f"T{tid}") for tid in track_ids} return LibraryManager(Library(tracks=tracks), tmp_path / "data") @@ -171,3 +176,191 @@ class TestFileDropPosition: assert received == [(["/x.mp3"], playlist.persistent_id, 2), (["/y.mp3"], playlist.persistent_id, None)] + + +# -------------------------------------------------------------------------- +# 3. Renaming artist/album moves the file (keep the music folder organized) +# -------------------------------------------------------------------------- + +@pytest.fixture +def organized(qapp, tmp_path, mp3_file): + """A library whose music folder holds one organized track: + /Music/Old Artist/Old Album/test.mp3.""" + share = tmp_path / "share" + src = share / "Music" / "Old Artist" / "Old Album" / "test.mp3" + src.parent.mkdir(parents=True) + shutil.copy2(mp3_file, src) + track = Track(track_id=1, name="Song", artist="Old Artist", + album="Old Album", location=str(src)) + library = Library(tracks={1: track}, music_folder=str(share)) + manager = LibraryManager(library, share / "lintunes") + return manager, track, share + + +class TestRenameMovesFile: + def test_artist_edit_moves_file(self, organized): + manager, track, share = organized + old_path = Path(track.location) + + manager.edit_track_fields(1, {"artist": "New Artist"}) + + expected = share / "Music" / "New Artist" / "Old Album" / "test.mp3" + assert Path(track.location) == expected + assert expected.is_file() + assert not old_path.exists() + assert track.artist == "New Artist" + + def test_album_edit_moves_file(self, organized): + manager, track, share = organized + + manager.edit_track_fields(1, {"album": "New Album"}) + + expected = share / "Music" / "Old Artist" / "New Album" / "test.mp3" + assert Path(track.location) == expected + assert expected.is_file() + + def test_album_artist_shields_artist_edit(self, organized): + manager, track, share = organized + track.album_artist = "Main Act" + dest = share / "Music" / "Main Act" / "Old Album" / "test.mp3" + dest.parent.mkdir(parents=True) + Path(track.location).rename(dest) + track.location = str(dest) + + # Folder is keyed on album_artist, so an artist edit changes nothing. + manager.edit_track_fields(1, {"artist": "Feat Guy"}) + + assert Path(track.location) == dest + assert dest.is_file() + assert track.artist == "Feat Guy" + + def test_empty_dirs_pruned_but_not_root(self, organized): + manager, track, share = organized + + manager.edit_track_fields(1, {"artist": "New Artist", + "album": "New Album"}) + + assert not (share / "Music" / "Old Artist").exists() # pruned + assert (share / "Music").is_dir() # root survives + + def test_outside_root_file_untouched(self, organized, mp3_file): + manager, _track, share = organized + outside = Path(mp3_file) + external = Track(track_id=2, name="Ext", artist="Someone", + location=str(outside)) + manager.library.tracks[2] = external + + manager.edit_track_fields(2, {"artist": "Someone Else"}) + + assert Path(external.location) == outside + assert outside.is_file() + assert external.artist == "Someone Else" + + def test_undo_moves_back_redo_removes(self, organized): + manager, track, share = organized + old_path = Path(track.location) + + manager.edit_track_fields(1, {"artist": "New Artist"}) + new_path = Path(track.location) + + manager.undo_stack.undo() + assert Path(track.location) == old_path + assert old_path.is_file() and not new_path.exists() + assert track.artist == "Old Artist" + + manager.undo_stack.redo() + assert Path(track.location) == new_path + assert new_path.is_file() and not old_path.exists() + assert track.artist == "New Artist" + + def test_collision_gets_suffix(self, organized): + manager, track, share = organized + taken = share / "Music" / "New Artist" / "Old Album" / "test.mp3" + taken.parent.mkdir(parents=True) + taken.write_bytes(b"other file already here") + + manager.edit_track_fields(1, {"artist": "New Artist"}) + + assert Path(track.location) == taken.with_stem("test 1") + assert Path(track.location).is_file() + assert taken.read_bytes() == b"other file already here" + + def test_move_failure_keeps_edit_and_old_location(self, organized, + monkeypatch): + manager, track, share = organized + old_path = Path(track.location) + failures = [] + manager.file_move_failed.connect( + lambda name, err: failures.append((name, err))) + monkeypatch.setattr( + LibraryManager, "_move_file", + lambda self, src, dest: (_ for _ in ()).throw(OSError("nope"))) + + manager.edit_track_fields(1, {"artist": "New Artist"}) + + assert track.artist == "New Artist" # edit applied + assert Path(track.location) == old_path # location kept + assert old_path.is_file() + assert failures and failures[0][0] == "Song" + + def test_bulk_album_rename_moves_all_and_one_undo(self, organized, + mp3_file): + manager, track, share = organized + second_src = share / "Music" / "Old Artist" / "Old Album" / "two.mp3" + shutil.copy2(mp3_file, second_src) + second = Track(track_id=2, name="Two", artist="Old Artist", + album="Old Album", location=str(second_src)) + manager.library.tracks[2] = second + + manager.edit_tracks_fields([1, 2], {"album": "Remaster"}) + + new_dir = share / "Music" / "Old Artist" / "Remaster" + assert Path(track.location).parent == new_dir + assert Path(second.location).parent == new_dir + assert Path(track.location).is_file() + assert Path(second.location).is_file() + + manager.undo_stack.undo() # ONE undo restores both + old_dir = share / "Music" / "Old Artist" / "Old Album" + assert Path(track.location).parent == old_dir + assert Path(second.location).parent == old_dir + assert Path(track.location).is_file() + assert Path(second.location).is_file() + assert not new_dir.exists() # pruned again + + +class TestLocationSync: + def test_merge_track_fields_newest_wins_location(self): + from lintunes.storage.conflict_resolver import _merge_track_fields + orig = {"location": "Music/A/B/x.mp3", "date_modified": "2026-01-01"} + newer = {"location": "Music/C/B/x.mp3", "date_modified": "2026-06-01"} + _merge_track_fields(orig, newer) + assert orig["location"] == "Music/C/B/x.mp3" + + older = {"location": "Music/Z/B/x.mp3", "date_modified": "2025-01-01"} + _merge_track_fields(orig, older) + assert orig["location"] == "Music/C/B/x.mp3" # older copy loses + + def test_reload_from_disk_adopts_external_move(self, organized): + from lintunes.storage import json_storage + manager, track, share = organized + data_dir = manager.data_dir + json_storage.save_library(manager.library, data_dir) + + # Another machine renamed the artist: file moved + library.json + # rewritten with a newer date_modified and the new relative path. + new_loc = share / "Music" / "Elsewhere" / "Old Album" / "test.mp3" + new_loc.parent.mkdir(parents=True) + Path(track.location).rename(new_loc) + from lintunes.storage.json_storage import read_json, write_json + disk = read_json(data_dir / "library.json") + entry = disk["1"] + entry["artist"] = "Elsewhere" + entry["location"] = "../Music/Elsewhere/Old Album/test.mp3" + entry["date_modified"] = "2099-01-01T00:00:00" + write_json(data_dir / "library.json", disk) + + manager.reload_from_disk() + + assert Path(track.location) == new_loc + assert track.artist == "Elsewhere"