diff --git a/TASKS.md b/TASKS.md index 02d00ab..15ce161 100644 --- a/TASKS.md +++ b/TASKS.md @@ -3,7 +3,33 @@ Legend: `[ ]` todo · `[~]` in progress · `[x]` done. When a round closes, move its finished items to `tasks-done.md`. -## Round 19 (current) — version display, git self-update, BPM fix +## Round 20 (current) — Ctrl+I save hang on the Debian machine (v0.1.1) + +Plan reference: `~/.claude/plans/i-m-having-some-trouble-eager-gosling.md`. +Tests in `tests/test_round20.py`. Fix-only round → patch bump **0.1.1**. + +Background: on the Debian machine, OK in Get Info froze the GUI thread past +mutter's ~5 s check-alive → "Force Quit / Wait" dialog. Happens on +single-track edits. Whole save path is synchronous on the GUI thread. + +- [x] Perf instrumentation: new `lintunes/perf.py` (`timed()` context + manager, INFO on the `lintunes.perf` logger → stderr/journal). Times + tag saves, artwork saves, file moves, edit_track(s)_fields, browser + rebuild, smart recompute, and the debounced JSON flush. +- [x] `tagging.write_tags` now parses + saves the file ONCE per edit: + grouping/compilation/bpm ride the same save via registered Easy keys + (GRP1/TCMP/TBPM on EasyID3, cpil on EasyMP4) instead of + `_write_extra_tags` doing a second full parse+save. +- [x] `LibraryView` coalesces browser rebuilds through a 0 ms single-shot + timer: an N-track Get Info edit rebuilds the genre/artist/album + cascade once, not N times (was O(edited × library)). +- [ ] **Diagnose on the Debian machine**: self-update, reproduce a Ctrl+I + edit, read `lintunes.perf` timings (terminal run or + `journalctl --user`) to see which stage eats the ~5 s — likely the + audio-file rewrite or the 15 MB library.json flush. Then decide on + moving that stage off the GUI thread (deliberately deferred). + +## Round 19 — version display, git self-update, BPM fix Plan reference: `~/.claude/plans/some-fixes-features-for-lintues-merry-journal.md`. Tests land in `tests/test_round19.py`. First versioned release: **0.1.0**. diff --git a/lintunes/__init__.py b/lintunes/__init__.py index a8c01dc..6d8e3af 100644 --- a/lintunes/__init__.py +++ b/lintunes/__init__.py @@ -1,3 +1,3 @@ """LinTunes — iTunes-style music library manager and player for Linux.""" -__version__ = "0.1.0" +__version__ = "0.1.1" diff --git a/lintunes/gui/library_view.py b/lintunes/gui/library_view.py index 7c0a59d..41fe410 100644 --- a/lintunes/gui/library_view.py +++ b/lintunes/gui/library_view.py @@ -8,6 +8,7 @@ from PyQt6.QtCore import Qt, QTimer, pyqtSignal from lintunes.gui.table_settings import TableSettingsMixin from lintunes.gui.track_table import TrackTableView +from lintunes.perf import timed _ARTICLE_RE = re.compile(r"^the\s+", re.IGNORECASE) @@ -162,6 +163,13 @@ class LibraryView(TableSettingsMixin, QWidget): manager.track_updated.connect(self.table.model_.refresh_track) manager.track_fields_edited.connect(self._on_track_fields_edited) + # Coalesces browser rebuilds: a multi-track edit emits + # track_fields_edited once per track, and rebuilding the whole + # genre/artist/album cascade per signal is O(edited × library). + self._browser_rebuild_timer = QTimer(self) + self._browser_rebuild_timer.setSingleShot(True) + self._browser_rebuild_timer.setInterval(0) + self._browser_rebuild_timer.timeout.connect(self._rebuild_browser) self.reload() @@ -295,8 +303,17 @@ class LibraryView(TableSettingsMixin, QWidget): rebuild the lists and re-filter so the renamed value sorts to its new spot and clicking it still finds its tracks (the plain row repaint from track_updated leaves the browser stale). Non-browser edits (name, bpm…) - are handled by the proxy's live re-sort, so they need nothing here.""" + are handled by the proxy's live re-sort, so they need nothing here. + + The rebuild is deferred to a 0 ms single-shot timer so an N-track edit + (which fires this once per track) rebuilds the browser once, not N + times.""" if _BROWSER_FIELDS.intersection(changed_fields): + self._browser_rebuild_timer.start() + + def _rebuild_browser(self): + with timed("library browser rebuild (%d tracks)", + len(self._all_tracks)): self._populate_genres() self._populate_artists() self._populate_albums() diff --git a/lintunes/library_manager.py b/lintunes/library_manager.py index af33367..521c763 100644 --- a/lintunes/library_manager.py +++ b/lintunes/library_manager.py @@ -10,6 +10,7 @@ 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.perf import timed from lintunes.storage import json_storage from lintunes.undo import Command, UndoStack @@ -371,16 +372,18 @@ class LibraryManager(QObject): if hasattr(track, k) and getattr(track, k) != v} if not changed: return - old = {k: getattr(track, k) for k in changed} - if not self._write_track_tags(track, changed): - # 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) + with timed("edit_track_fields %r", track.name): + old = {k: getattr(track, k) for k in changed} + if not self._write_track_tags(track, changed): + # 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", undo=lambda: self._revert_track_fields(track_id, old), @@ -395,21 +398,22 @@ class LibraryManager(QObject): skipped entirely (no file write, no undo entry). """ changes = [] # (track_id, new_fields, old_fields) - for track_id in track_ids: - track = self.library.tracks.get(track_id) - if not track: - continue - changed = {k: v for k, v in fields.items() - if hasattr(track, k) and getattr(track, k) != v} - 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)) + with timed("edit_tracks_fields (%d tracks)", len(track_ids)): + for track_id in track_ids: + track = self.library.tracks.get(track_id) + if not track: + continue + changed = {k: v for k, v in fields.items() + if hasattr(track, k) and getattr(track, k) != v} + 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: return self.undo_stack.push(Command( @@ -518,12 +522,13 @@ class LibraryManager(QObject): 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) + with timed("move file %s", src.name): + 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 @@ -616,18 +621,22 @@ class LibraryManager(QObject): self._pending_recompute_fields = set() self._recomputing = True try: - for pid, playlist in list(self.library.playlists.items()): - if not playlist.is_smart: - continue - criteria = playlist.smart_criteria - if criteria is None or criteria.unsupported or not criteria.live_update: - continue - if fields is not None and not self._criteria_touched(criteria, fields): - continue - self.recompute_smart_playlist(pid) + with timed("smart playlist recompute"): + self._recompute_touched(fields) finally: self._recomputing = False + def _recompute_touched(self, fields: set[str] | None): + for pid, playlist in list(self.library.playlists.items()): + if not playlist.is_smart: + continue + criteria = playlist.smart_criteria + if criteria is None or criteria.unsupported or not criteria.live_update: + continue + if fields is not None and not self._criteria_touched(criteria, fields): + continue + self.recompute_smart_playlist(pid) + @staticmethod def _criteria_touched(criteria, fields: set[str]) -> bool: if criteria.referenced_attrs() & fields: @@ -660,13 +669,16 @@ class LibraryManager(QObject): def flush(self): self._save_timer.stop() if self._dirty_tracks: - json_storage.save_tracks(self.library, self.data_dir) + with timed("flush library.json (%d tracks)", + len(self.library.tracks)): + json_storage.save_tracks(self.library, self.data_dir) self._dirty_tracks = False self._record_sig(self.data_dir / "library.json") for pid in list(self._dirty_playlists): playlist = self.library.playlists.get(pid) if playlist: - json_storage.save_playlist(playlist, self.data_dir) + with timed("flush playlist %s", pid): + json_storage.save_playlist(playlist, self.data_dir) self._record_sig(self.data_dir / "playlists" / f"{pid}.json") self._dirty_playlists.clear() for pid in list(self._deleted_playlists): diff --git a/lintunes/perf.py b/lintunes/perf.py new file mode 100644 index 0000000..599cf27 --- /dev/null +++ b/lintunes/perf.py @@ -0,0 +1,22 @@ +"""Wall-clock timing for the save/edit hot paths. + +Logs at INFO on the ``lintunes.perf`` logger, so a normal desktop launch +records where the time went (visible via ``journalctl --user`` or a +terminal run). Used to diagnose GUI-thread stalls on slow machines. +""" +import logging +import time +from contextlib import contextmanager + +log = logging.getLogger("lintunes.perf") + + +@contextmanager +def timed(label: str, *args): + """``with timed("write_tags %s", path):`` — logs the block's duration.""" + start = time.perf_counter() + try: + yield + finally: + ms = (time.perf_counter() - start) * 1000 + log.info("%s: %.0f ms", label % args if args else label, ms) diff --git a/lintunes/tagging.py b/lintunes/tagging.py index 5bd0015..e4e85cd 100644 --- a/lintunes/tagging.py +++ b/lintunes/tagging.py @@ -9,11 +9,13 @@ from pathlib import Path import mutagen from mutagen.easyid3 import EasyID3 -from mutagen.id3 import ID3, COMM +from mutagen.id3 import ID3, COMM, GRP1, TCMP, TBPM from mutagen.mp3 import MP3 from mutagen.mp4 import MP4 from mutagen.flac import FLAC +from lintunes.perf import timed + # EasyID3 has no 'comment' key by default; COMM frames need explicit handling. def _comm_get(id3, _key): @@ -32,10 +34,55 @@ def _comm_delete(id3, _key): EasyID3.RegisterKey("comment", _comm_get, _comm_set, _comm_delete) + +# Register grouping/compilation/bpm as Easy keys (iTunes-style frames) so +# write_tags can set every field on one object and save the file ONCE — +# a second parse+save per edit is real I/O on big files / slow disks. +def _register_id3_frame_key(key: str, frame_cls): + frame_id = frame_cls.__name__ + + def getter(id3, _key): + frame = id3.get(frame_id) + return list(frame.text) if frame else [] + + def setter(id3, _key, value): + id3.delall(frame_id) + id3.add(frame_cls(encoding=3, text=value)) + + def deleter(id3, _key): + id3.delall(frame_id) + + EasyID3.RegisterKey(key, getter, setter, deleter) + + +_register_id3_frame_key("grouping", GRP1) # iTunes GRP1, not TIT1 +_register_id3_frame_key("compilation", TCMP) +_register_id3_frame_key("bpm", TBPM) + # EasyMP4 is missing 'composer' (©wrt) out of the box from mutagen.easymp4 import EasyMP4Tags # noqa: E402 if "composer" not in EasyMP4Tags.Set: EasyMP4Tags.RegisterTextKey("composer", "\xa9wrt") +if "grouping" not in EasyMP4Tags.Set: + EasyMP4Tags.RegisterTextKey("grouping", "\xa9grp") +if "bpm" not in EasyMP4Tags.Set: + EasyMP4Tags.RegisterIntKey("bpm", "tmpo") + + +def _cpil_get(tags, _key): + return ["1"] if tags.get("cpil") else [] + + +def _cpil_set(tags, _key, value): + tags["cpil"] = bool(value and str(value[0]) not in ("", "0")) + + +def _cpil_delete(tags, _key): + tags.pop("cpil", None) + + +if "compilation" not in EasyMP4Tags.Set: + EasyMP4Tags.RegisterKey("compilation", _cpil_get, _cpil_set, _cpil_delete) # Easy-interface tag names shared by EasyID3 / EasyMP4 / FLAC (Vorbis) _TEXT_FIELDS = { @@ -96,7 +143,10 @@ def read_tags(path: str | Path) -> dict: def write_tags(path: str | Path, fields: dict): - """Write the given Track-style fields to the file's tags.""" + """Write the given Track-style fields to the file's tags. + + One parse and ONE save per call: grouping/compilation/bpm are covered by + the Easy keys registered above, so no second raw-tag pass is needed.""" path = str(path) audio = mutagen.File(path, easy=True) if audio is None: @@ -130,61 +180,28 @@ def write_tags(path: str | Path, fields: dict): else: audio.tags.pop(tag_key, None) - audio.save() - _write_extra_tags(path, fields) + if "grouping" in fields: + _set_or_pop(audio.tags, "grouping", + str(fields["grouping"]) if fields["grouping"] else None) + if "bpm" in fields: + _set_or_pop(audio.tags, "bpm", + str(fields["bpm"]) if fields["bpm"] else None) + if "compilation" in fields: + _set_or_pop(audio.tags, "compilation", + "1" if fields["compilation"] else None) + + with timed("tag save %s", path): + audio.save() -def _write_extra_tags(path: str, fields: dict): - """Fields the easy interfaces don't cover uniformly: grouping, compilation, bpm.""" - wants = {k for k in ("grouping", "compilation", "bpm") if k in fields} - if not wants: +def _set_or_pop(tags, key, value): + if value: + tags[key] = [value] return - audio = mutagen.File(path) - if isinstance(audio, MP3): - from mutagen.id3 import GRP1, TCMP, TBPM - if "grouping" in fields: - audio.tags.delall("GRP1") - if fields["grouping"]: - audio.tags.add(GRP1(encoding=3, text=[fields["grouping"]])) - if "compilation" in fields: - audio.tags.delall("TCMP") - if fields["compilation"]: - audio.tags.add(TCMP(encoding=3, text=["1"])) - if "bpm" in fields: - audio.tags.delall("TBPM") - if fields["bpm"]: - audio.tags.add(TBPM(encoding=3, text=[str(fields["bpm"])])) - audio.save() - elif isinstance(audio, MP4): - if "grouping" in fields: - _mp4_set(audio, "\xa9grp", fields["grouping"]) - if "compilation" in fields: - audio.tags["cpil"] = bool(fields["compilation"]) - if "bpm" in fields: - _mp4_set(audio, "tmpo", int(fields["bpm"]) if fields["bpm"] else None) - audio.save() - elif isinstance(audio, FLAC): - if "grouping" in fields: - _vorbis_set(audio, "grouping", fields["grouping"]) - if "compilation" in fields: - _vorbis_set(audio, "compilation", "1" if fields["compilation"] else "") - if "bpm" in fields: - _vorbis_set(audio, "bpm", str(fields["bpm"]) if fields["bpm"] else "") - audio.save() - - -def _mp4_set(audio, key, value): - if value: - audio.tags[key] = [value] - else: - audio.tags.pop(key, None) - - -def _vorbis_set(audio, key, value): - if value: - audio.tags[key] = [value] - else: - audio.tags.pop(key, None) + try: + del tags[key] # FLAC's VCommentDict has no pop-with-default + except KeyError: + pass def _kind_for(audio) -> str: @@ -230,7 +247,8 @@ def write_artwork(path: str | Path, image_bytes: bytes, mime: str = "image/jpeg" data=image_bytes)) else: raise ValueError(f"Don't know how to embed artwork in {path}") - audio.save() + with timed("artwork save %s", path): + audio.save() def read_embedded_artwork(path: str | Path) -> bytes | None: diff --git a/tests/test_round20.py b/tests/test_round20.py new file mode 100644 index 0000000..ea191e6 --- /dev/null +++ b/tests/test_round20.py @@ -0,0 +1,130 @@ +"""Round 20: Ctrl+I save-path performance. + +- ``tagging.write_tags`` now does ONE mutagen parse + ONE save per call: + grouping/compilation/bpm ride the same save (via registered Easy keys) + instead of a second full parse+save of the audio file. +- ``LibraryView`` coalesces browser rebuilds through a 0 ms single-shot + timer, so an N-track edit rebuilds the genre/artist/album cascade once. +- ``perf.timed`` logs wall-clock durations on the ``lintunes.perf`` logger. +""" +import logging + +import mutagen +import pytest + +from lintunes import tagging +from lintunes.library_manager import LibraryManager +from lintunes.models import Track +from lintunes.models.library import Library +from lintunes.perf import timed + + +def _manager(tmp_path, tracks=()): + library = Library() + for track in tracks: + library.tracks[track.track_id] = track + return LibraryManager(library, tmp_path) + + +EXTRAS = {"grouping": "Test Group", "compilation": True, "bpm": 128} + + +# ---- single parse + save in write_tags ---- + +class TestSingleSave: + @pytest.mark.parametrize("fixture", ["mp3_file", "m4a_file", "flac_file"]) + def test_one_parse_per_write(self, fixture, request, monkeypatch): + path = request.getfixturevalue(fixture) + opens = [] + real_file = mutagen.File + + def counting_file(*args, **kwargs): + opens.append(args) + return real_file(*args, **kwargs) + + monkeypatch.setattr(tagging.mutagen, "File", counting_file) + tagging.write_tags(path, {"name": "Title", **EXTRAS}) + assert len(opens) == 1 + + def test_mp3_extras_use_itunes_frames(self, mp3_file): + tagging.write_tags(mp3_file, EXTRAS) + raw = mutagen.File(mp3_file) + assert [str(t) for t in raw.tags["GRP1"].text] == ["Test Group"] + assert str(raw.tags["TCMP"].text[0]) == "1" + assert str(raw.tags["TBPM"].text[0]) == "128" + + def test_m4a_extras_use_itunes_atoms(self, m4a_file): + tagging.write_tags(m4a_file, EXTRAS) + raw = mutagen.File(m4a_file) + assert raw.tags["\xa9grp"] == ["Test Group"] + assert bool(raw.tags["cpil"]) + assert list(raw.tags["tmpo"]) == [128] + + def test_flac_extras_use_vorbis_comments(self, flac_file): + tagging.write_tags(flac_file, EXTRAS) + raw = mutagen.File(flac_file) + assert raw["grouping"] == ["Test Group"] + assert raw["compilation"] == ["1"] + assert raw["bpm"] == ["128"] + + @pytest.mark.parametrize("fixture", ["mp3_file", "m4a_file", "flac_file"]) + def test_clearing_extras_removes_them(self, fixture, request): + path = request.getfixturevalue(fixture) + tagging.write_tags(path, EXTRAS) + tagging.write_tags(path, {"grouping": "", "compilation": False, + "bpm": 0}) + easy = mutagen.File(path, easy=True) + for key in ("grouping", "compilation", "bpm"): + assert not easy.tags.get(key), key + + +# ---- coalesced browser rebuild ---- + +class TestCoalescedBrowserRebuild: + def _view_with_counter(self, qapp, tmp_path, monkeypatch, tracks): + from lintunes.gui.library_view import LibraryView + manager = _manager(tmp_path, tracks) + view = LibraryView(manager) + rebuilds = [] + # _rebuild_browser looks methods up on self, so patching one of them + # counts rebuilds without disturbing the timer wiring. + monkeypatch.setattr(view, "_populate_genres", + lambda: rebuilds.append(1)) + return manager, view, rebuilds + + def test_multi_edit_rebuilds_once(self, qapp, tmp_path, monkeypatch): + tracks = [Track(track_id=i, name=f"T{i}", artist="Old") + for i in (1, 2, 3)] + manager, view, rebuilds = self._view_with_counter( + qapp, tmp_path, monkeypatch, tracks) + manager.edit_tracks_fields([1, 2, 3], {"artist": "New"}) + assert rebuilds == [] # deferred to the event loop + qapp.processEvents() + assert len(rebuilds) == 1 + assert all(t.artist == "New" for t in tracks) + + def test_single_edit_still_rebuilds(self, qapp, tmp_path, monkeypatch): + track = Track(track_id=1, name="T", genre="Pop") + manager, view, rebuilds = self._view_with_counter( + qapp, tmp_path, monkeypatch, [track]) + manager.edit_track_fields(1, {"genre": "Rock"}) + qapp.processEvents() + assert len(rebuilds) == 1 + + def test_non_browser_edit_skips_rebuild(self, qapp, tmp_path, monkeypatch): + track = Track(track_id=1, name="T") + manager, view, rebuilds = self._view_with_counter( + qapp, tmp_path, monkeypatch, [track]) + manager.edit_track_fields(1, {"name": "Renamed"}) + qapp.processEvents() + assert rebuilds == [] + + +# ---- perf logging ---- + +def test_timed_logs_duration(caplog): + with caplog.at_level(logging.INFO, logger="lintunes.perf"): + with timed("unit test %s", "label"): + pass + messages = [r.getMessage() for r in caplog.records] + assert any("unit test label" in m and "ms" in m for m in messages)