WIP: remove session from playlistmodel

app/playlistmodel.py

@@ -44,11 +44,8 @@ from helpers import (
     get_embedded_time,
     get_relative_date,
     ms_to_mmss,
-    remove_substring_case_insensitive,
-    set_track_metadata,
 )
 from log import log, log_call
-from models import db, NoteColours, Playdates, PlaylistRows, Tracks
 from playlistrow import PlaylistRow, TrackSequence
 import repository
 
@@ -61,6 +58,9 @@ class PlaylistModel(QAbstractTableModel):
     """
     The Playlist Model
 
+    Cache the database info in self.playlist_rows, a dictionary of
+    PlaylistRow objects indexed by row_number.
+
     Update strategy: update the database and then refresh the
     row-indexed cached copy (self.playlist_rows). Do not edit
     self.playlist_rows directly because keeping it and the
@@ -943,8 +943,6 @@ class PlaylistModel(QAbstractTableModel):
                 playlist_row.note = note
             self.refresh_row(existing_plr.row_number)
 
-        # Carry out the move outside of the session context to ensure
-        # database updated with any note change
         self.move_rows([existing_plr.row_number], new_row_number)
         self.signals.resize_rows_signal.emit(self.playlist_id)
 
@@ -1067,21 +1065,16 @@ class PlaylistModel(QAbstractTableModel):
         Rescan track at passed row number
         """
 
-        track_id = self.playlist_rows[row_number].track_id
+        track = self.playlist_rows[row_number]
-        if track_id:
+        _ = repository.update_track(track.path, track.track_id)
-            with db.Session() as session:
+
-                track = session.get(Tracks, track_id)
+        roles = [
-                set_track_metadata(track)
+            Qt.ItemDataRole.BackgroundRole,
-                self.refresh_row(row_number)
+            Qt.ItemDataRole.DisplayRole,
-                self.update_track_times()
+        ]
-                roles = [
+        # only invalidate required roles
-                    Qt.ItemDataRole.BackgroundRole,
+        self.invalidate_row(row_number, roles)
-                    Qt.ItemDataRole.DisplayRole,
+        self.signals.resize_rows_signal.emit(self.playlist_id)
-                ]
-                # only invalidate required roles
-                self.invalidate_row(row_number, roles)
-                self.signals.resize_rows_signal.emit(self.playlist_id)
-                session.commit()
 
     @log_call
     def reset_track_sequence_row_numbers(self) -> None:
@@ -1113,21 +1106,8 @@ class PlaylistModel(QAbstractTableModel):
         ):
             return
 
-        with db.Session() as session:
+        repository.remove_comments(self.playlist_id, row_numbers)
-            for row_number in row_numbers:
+
-                playlist_row = session.get(
-                    PlaylistRows, self.playlist_rows[row_number].playlistrow_id
-                )
-                if playlist_row.track_id:
-                    playlist_row.note = ""
-                    # We can't use refresh_data() because its
-                    # optimisations mean it won't update comments in
-                    # self.playlist_rows
-                    # The "correct" approach would be to re-read from the
-                    # database but we optimise here by simply updating
-                    # self.playlist_rows directly.
-                    self.playlist_rows[row_number].note = ""
-            session.commit()
         # only invalidate required roles
         roles = [
             Qt.ItemDataRole.BackgroundRole,
@@ -1185,31 +1165,7 @@ class PlaylistModel(QAbstractTableModel):
             header_text = header_text[0:-1]
 
         # Parse passed header text and remove the first colour match string
-        with db.Session() as session:
+        return repository.remove_colour_substring(header_text)
-            for rec in NoteColours.get_all(session):
-                if not rec.strip_substring:
-                    continue
-                if rec.is_regex:
-                    flags = re.UNICODE
-                    if not rec.is_casesensitive:
-                        flags |= re.IGNORECASE
-                    p = re.compile(rec.substring, flags)
-                    if p.match(header_text):
-                        header_text = re.sub(p, "", header_text)
-                        break
-                else:
-                    if rec.is_casesensitive:
-                        if rec.substring.lower() in header_text.lower():
-                            header_text = remove_substring_case_insensitive(
-                                header_text, rec.substring
-                            )
-                            break
-                    else:
-                        if rec.substring in header_text:
-                            header_text = header_text.replace(rec.substring, "")
-                        break
-
-        return header_text
 
     def rowCount(self, index: QModelIndex = QModelIndex()) -> int:
         """Standard function for view"""
@@ -1357,6 +1313,7 @@ class PlaylistModel(QAbstractTableModel):
         self.signals.next_track_changed_signal.emit()
         self.update_track_times()
 
+    @log_call
     def setData(
         self,
         index: QModelIndex,
@@ -1372,46 +1329,23 @@ class PlaylistModel(QAbstractTableModel):
 
         row_number = index.row()
         column = index.column()
+        plr = self.playlist_rows[row_number]
 
-        with db.Session() as session:
+        if column == Col.TITLE.value:
-            playlist_row = session.get(
+            plr.title = str(value)
-                PlaylistRows, self.playlist_rows[row_number].playlistrow_id
+        elif column == Col.ARTIST.value:
-            )
+            plr.artist = str(value)
-            if not playlist_row:
+        elif column == Col.INTRO.value:
-                log.error(
+            plr.intro = int(round(float(value), 1) * 1000)
-                    f"{self}: Error saving data: {row_number=}, {column=}, {value=}"
+        elif column == Col.NOTE.value:
-                )
+            plr.note = str(value)
-                return False
+        else:
+            raise ApplicationError(f"setData called with unexpected column ({column=})")
 
-            if playlist_row.track_id:
+        self.refresh_row(row_number)
-                if column in [Col.TITLE.value, Col.ARTIST.value, Col.INTRO.value]:
+        self.dataChanged.emit(index, index, [Qt.ItemDataRole.DisplayRole, role])
-                    track = session.get(Tracks, playlist_row.track_id)
-                    if not track:
-                        log.error(f"{self}: Error retreiving track: {playlist_row=}")
-                        return False
-                    if column == Col.TITLE.value:
-                        track.title = str(value)
-                    elif column == Col.ARTIST.value:
-                        track.artist = str(value)
-                    elif column == Col.INTRO.value:
-                        track.intro = int(round(float(value), 1) * 1000)
-                    else:
-                        log.error(f"{self}: Error updating track: {column=}, {value=}")
-                        return False
-                elif column == Col.NOTE.value:
-                    playlist_row.note = str(value)
 
-            else:
+        return True
-                # This is a header row
-                if column == HEADER_NOTES_COLUMN:
-                    playlist_row.note = str(value)
-
-            # commit changes before refreshing data
-            session.commit()
-            self.refresh_row(row_number)
-            self.dataChanged.emit(index, index, [Qt.ItemDataRole.DisplayRole, role])
-
-            return True
 
     def sort_by_artist(self, row_numbers: list[int]) -> None:
         """
@@ -1509,17 +1443,12 @@ class PlaylistModel(QAbstractTableModel):
 
         if column != Col.LAST_PLAYED.value:
             return ""
-        with db.Session() as session:
+
-            track_id = self.playlist_rows[row].track_id
+        track_id = self.playlist_rows[row].track_id
-            if not track_id:
+        if not track_id:
-                return ""
+            return ""
-            playdates = Playdates.last_playdates(session, track_id)
+
-            return "<br>".join(
+        return repository.get_last_played_dates(track_id)
-                [
-                    a.lastplayed.strftime(Config.LAST_PLAYED_TOOLTIP_DATE_FORMAT)
-                    for a in playdates
-                ]
-            )
 
     @log_call
     def update_or_insert(self, track_id: int, row_number: int) -> None:

app/playlistrow.py

@@ -64,6 +64,10 @@ class PlaylistRow:
     def artist(self):
         return self.dto.artist
 
+    @artist.setter
+    def artist(self, value: str) -> None:
+        print(f"set artist attribute for {self=}, {value=}")
+
     @property
     def bitrate(self):
         return self.dto.bitrate
@@ -80,6 +84,10 @@ class PlaylistRow:
     def intro(self):
         return self.dto.intro
 
+    @intro.setter
+    def intro(self, value: int) -> None:
+        print(f"set intro attribute for {self=}, {value=}")
+
     @property
     def lastplayed(self):
         return self.dto.lastplayed
@@ -100,6 +108,10 @@ class PlaylistRow:
     def title(self):
         return self.dto.title
 
+    @title.setter
+    def title(self, value: str) -> None:
+        print(f"set title attribute for {self=}, {value=}")
+
     @property
     def track_id(self):
         return self.dto.track_id

app/playlists.py

@@ -51,7 +51,6 @@ from helpers import (
     show_warning,
 )
 from log import log, log_call
-from models import db, Settings
 from playlistrow import TrackSequence
 from playlistmodel import PlaylistModel, PlaylistProxyModel
 import repository

app/repository.py

@@ -19,7 +19,7 @@ from classes import ApplicationError, PlaylistRowDTO
 from classes import PlaylistDTO, TrackDTO
 from config import Config
 import helpers
-from log import log
+from log import log, log_call
 from models import (
     db,
     NoteColours,
@@ -32,17 +32,11 @@ from models import (
 
 
 # Notecolour functions
-def get_colour(text: str, foreground: bool = False) -> str:
+def _get_colour_record(text: str) -> tuple[NoteColours | None, str]:
     """
-    Parse text and return background (foreground if foreground==True)
+    Parse text and return first matching colour record or None
-    colour string if matched, else None
     """
 
-    if not text:
-        return ""
-
-    match = False
-
     with db.Session() as session:
         for rec in NoteColours.get_all(session):
             if rec.is_regex:
@@ -51,21 +45,49 @@ def get_colour(text: str, foreground: bool = False) -> str:
                     flags |= re.IGNORECASE
                 p = re.compile(rec.substring, flags)
                 if p.match(text):
-                    match = True
+                    if rec.strip_substring:
+                        return_text = re.sub(p, "", text)
+                    else:
+                        return_text = text
+                    return (rec, return_text)
             else:
                 if rec.is_casesensitive:
                     if rec.substring in text:
-                        match = True
+                        return_text = text.replace(rec.substring, "")
+                        return (rec, return_text)
                 else:
                     if rec.substring.lower() in text.lower():
-                        match = True
+                        return_text = helpers.remove_substring_case_insensitive(
+                            text, rec.substring
+                        )
+                        return (rec, return_text)
 
-            if match:
+        return (None, text)
-                if foreground:
+
-                    return rec.foreground or ""
+
-                else:
+def get_colour(text: str, foreground: bool = False) -> str:
-                    return rec.colour
+    """
+    Parse text and return background (foreground if foreground==True)
+    colour string if matched, else None
+    """
+
+    (rec, _) = _get_colour_record(text)
+    if rec is None:
         return ""
+    elif foreground:
+        return rec.foreground or ""
+    else:
+        return rec.colour
+
+
+def remove_colour_substring(text: str) -> str:
+    """
+    Remove text that identifies the colour to be used if strip_substring is True
+    """
+
+    (rec, stripped_text) = _get_colour_record(text)
+
+    return stripped_text
 
 
 # Track functions
@@ -115,6 +137,34 @@ def create_track(path: str) -> TrackDTO:
     return new_track
 
 
+def update_track(path: str, track_id: int) -> TrackDTO:
+    """
+    Update an existing track db entry return the DTO
+    """
+
+    metadata = helpers.get_all_track_metadata(path)
+    with db.Session() as session:
+        track = session.get(Tracks, track_id)
+        if not track:
+            raise ApplicationError(f"Can't retrieve Track ({track_id=})")
+        track.path = (str(metadata["path"]),)
+        track.title = (str(metadata["title"]),)
+        track.artist = (str(metadata["artist"]),)
+        track.duration = (int(metadata["duration"]),)
+        track.start_gap = (int(metadata["start_gap"]),)
+        track.fade_at = (int(metadata["fade_at"]),)
+        track.silence_at = (int(metadata["silence_at"]),)
+        track.bitrate = (int(metadata["bitrate"]),)
+
+        session.commit()
+
+    updated_track = track_by_id(track_id)
+    if not updated_track:
+        raise ApplicationError("Unable to retrieve updated track")
+
+    return updated_track
+
+
 def get_all_tracks() -> list[TrackDTO]:
     """Return a list of all tracks"""
 
@@ -245,10 +295,7 @@ def track_with_path(path: str) -> bool:
 
     with db.Session() as session:
         track = (
-            session.execute(
+            session.execute(select(Tracks).where(Tracks.path == path))
-                select(Tracks)
-                .where(Tracks.path == path)
-            )
             .scalars()
             .one_or_none()
         )
@@ -272,6 +319,28 @@ def tracks_like_title(filter_str: str) -> list[TrackDTO]:
     return _tracks_where(Tracks.title.ilike(f"%{filter_str}%"))
 
 
+def get_last_played_dates(track_id: int, limit: int = 5) -> str:
+    """
+    Return the most recent 'limit' dates that this track has been played
+    as a text list
+    """
+
+    with db.Session() as session:
+        playdates = session.scalars(
+            Playdates.select()
+            .where(Playdates.track_id == track_id)
+            .order_by(Playdates.lastplayed.desc())
+            .limit(limit)
+        ).all()
+
+        return "<br>".join(
+            [
+                a.lastplayed.strftime(Config.LAST_PLAYED_TOOLTIP_DATE_FORMAT)
+                for a in playdates
+            ]
+        )
+
+
 # Playlist functions
 def _check_playlist_integrity(
     session: Session, playlist_id: int, fix: bool = False
@@ -305,6 +374,7 @@ def _check_playlist_integrity(
             raise ApplicationError(msg)
 
 
+@log_call
 def _shift_rows(
     session: Session, playlist_id: int, starting_row: int, shift_by: int
 ) -> None:
@@ -313,8 +383,6 @@ def _shift_rows(
     down; if -ve, shift them up.
     """
 
-    log.debug(f"(_shift_rows_down({playlist_id=}, {starting_row=}, {shift_by=}")
-
     session.execute(
         update(PlaylistRows)
         .where(
@@ -325,8 +393,12 @@ def _shift_rows(
     )
 
 
+@log_call
 def move_rows(
-    from_rows: list[int], from_playlist_id: int, to_row: int, to_playlist_id: int | None = None
+    from_rows: list[int],
+    from_playlist_id: int,
+    to_row: int,
+    to_playlist_id: int | None = None,
 ) -> None:
     """
     Move rows with or between playlists.
@@ -341,10 +413,6 @@ def move_rows(
      - Sanity check row numbers
     """
 
-    log.debug(
-        f"move_rows_to_playlist({from_rows=}, {from_playlist_id=}, {to_row=}, {to_playlist_id=})"
-    )
-
     # If to_playlist_id isn't specified, we're moving within the one
     # playlist.
     if to_playlist_id is None:
@@ -690,6 +758,25 @@ def insert_row(
     return new_playlist_row
 
 
+@log_call
+def remove_comments(playlist_id: int, row_numbers: list[int]) -> None:
+    """
+    Remove comments from rows in playlist
+    """
+
+    with db.Session() as session:
+        session.execute(
+            update(PlaylistRows)
+            .where(
+                PlaylistRows.playlist_id == playlist_id,
+                PlaylistRows.row_number.in_(row_numbers),
+            )
+            .values(note="")
+        )
+        session.commit()
+
+
+@log_call
 def remove_rows(playlist_id: int, row_numbers: list[int]) -> None:
     """
     Remove rows from playlist
@@ -697,8 +784,6 @@ def remove_rows(playlist_id: int, row_numbers: list[int]) -> None:
     Delete from highest row back so that not yet deleted row numbers don't change.
     """
 
-    log.debug(f"remove_rows({playlist_id=}, {row_numbers=}")
-
     with db.Session() as session:
         for row_number in sorted(row_numbers, reverse=True):
             session.execute(
@@ -749,9 +834,11 @@ def get_setting(name: str) -> int | None:
     """
 
     with db.Session() as session:
-        record = session.execute(
+        record = (
-            select(Settings).where(Settings.name == name)
+            session.execute(select(Settings).where(Settings.name == name))
-        ).scalars().one_or_none()
+            .scalars()
+            .one_or_none()
+        )
         if not record:
             return None
 
@@ -764,9 +851,11 @@ def set_setting(name: str, value: int) -> None:
     """
 
     with db.Session() as session:
-        record = session.execute(
+        record = (
-            select(Settings).where(Settings.name == name)
+            session.execute(select(Settings).where(Settings.name == name))
-        ).scalars().one_or_none()
+            .scalars()
+            .one_or_none()
+        )
         if not record:
             record = Settings(session=session, name=name)
             if not record: