WIP V3 model development

app/models.py

@@ -123,7 +123,7 @@ class NoteColours(Base):
         Return all records
         """
 
-        return session.execute(select(cls)).scalars().all()
+        return session.scalars(select(cls)).all()
 
     @staticmethod
     def get_colour(session: scoped_session, text: str) -> Optional[str]:
@@ -135,12 +135,11 @@ class NoteColours(Base):
             return None
 
         for rec in (
-            session.execute(
+            session.scalars(
                 select(NoteColours)
                 .filter(NoteColours.enabled.is_(True))
                 .order_by(NoteColours.order)
             )
-            .scalars()
             .all()
         ):
             if rec.is_regex:
@@ -204,12 +203,11 @@ class Playdates(Base):
         """Return a list of Playdates objects since passed time"""
 
         return (
-            session.execute(
+            session.scalars(
                 select(Playdates)
                 .where(Playdates.lastplayed >= since)
                 .order_by(Playdates.lastplayed)
             )
-            .scalars()
             .all()
         )
 
@@ -288,12 +286,11 @@ class Playlists(Base):
         """Returns a list of all playlists ordered by last use"""
 
         return (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .filter(cls.is_template.is_(False))
                 .order_by(cls.tab.desc(), cls.last_used.desc())
             )
-            .scalars()
             .all()
         )
 
@@ -302,10 +299,9 @@ class Playlists(Base):
         """Returns a list of all templates ordered by name"""
 
         return (
-            session.execute(
+            session.scalars(
                 select(cls).filter(cls.is_template.is_(True)).order_by(cls.name)
             )
-            .scalars()
             .all()
         )
 
@@ -314,7 +310,7 @@ class Playlists(Base):
         """Returns a list of all closed playlists ordered by last use"""
 
         return (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .filter(
                     cls.tab.is_(None),
@@ -323,7 +319,6 @@ class Playlists(Base):
                 )
                 .order_by(cls.last_used.desc())
             )
-            .scalars()
             .all()
         )
 
@@ -334,8 +329,7 @@ class Playlists(Base):
         """
 
         return (
-            session.execute(select(cls).where(cls.tab.is_not(None)).order_by(cls.tab))
-            .scalars()
+            session.scalars(select(cls).where(cls.tab.is_not(None)).order_by(cls.tab))
             .all()
         )
 
@@ -440,10 +434,9 @@ class PlaylistRows(Base):
         """Copy playlist entries"""
 
         src_rows = (
-            session.execute(
+            session.scalars(
                 select(PlaylistRows).filter(PlaylistRows.playlist_id == src_id)
             )
-            .scalars()
             .all()
         )
 
@@ -520,12 +513,11 @@ class PlaylistRows(Base):
         """
 
         plrs = (
-            session.execute(
+            session.scalars(
                 select(PlaylistRows)
                 .where(PlaylistRows.playlist_id == playlist_id)
                 .order_by(PlaylistRows.plr_rownum)
             )
-            .scalars()
             .all()
         )
 
@@ -545,12 +537,11 @@ class PlaylistRows(Base):
         """
 
         plrs = (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .where(cls.playlist_id == playlist_id, cls.id.in_(plr_ids))
                 .order_by(cls.plr_rownum)
             )
-            .scalars()
             .all()
         )
 
@@ -591,12 +582,11 @@ class PlaylistRows(Base):
         """
 
         plrs = (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .where(cls.playlist_id == playlist_id, cls.played.is_(True))
                 .order_by(cls.plr_rownum)
             )
-            .scalars()
             .all()
         )
 
@@ -623,7 +613,7 @@ class PlaylistRows(Base):
         if to_row is not None:
             query = query.where(cls.plr_rownum <= to_row)
 
-        plrs = session.execute((query).order_by(cls.plr_rownum)).scalars().all()
+        plrs = session.scalars((query).order_by(cls.plr_rownum)).all()
 
         return plrs
 
@@ -637,7 +627,7 @@ class PlaylistRows(Base):
         """
 
         plrs = (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .where(
                     cls.playlist_id == playlist_id,
@@ -646,7 +636,6 @@ class PlaylistRows(Base):
                 )
                 .order_by(cls.plr_rownum)
             )
-            .scalars()
             .all()
         )
 
@@ -702,7 +691,7 @@ class Settings(Base):
 
         result = {}
 
-        settings = session.execute(select(cls)).scalars().all()
+        settings = session.scalars(select(cls)).all()
         for setting in settings:
             result[setting.name] = setting
 
@@ -789,7 +778,7 @@ class Tracks(Base):
     def get_all(cls, session) -> List["Tracks"]:
         """Return a list of all tracks"""
 
-        return session.execute(select(cls)).scalars().unique().all()
+        return session.scalars(select(cls)).unique().all()
 
     @classmethod
     def get_by_path(cls, session: scoped_session, path: str) -> Optional["Tracks"]:
@@ -817,13 +806,12 @@ class Tracks(Base):
         """
 
         return (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .options(joinedload(Tracks.playdates))
                 .where(cls.artist.ilike(f"%{text}%"))
                 .order_by(cls.title)
             )
-            .scalars()
             .unique()
             .all()
         )
@@ -838,13 +826,12 @@ class Tracks(Base):
         https://docs.sqlalchemy.org/en/20/orm/queryguide/relationships.html#joined-eager-loading
         """
         return (
-            session.execute(
+            session.scalars(
                 select(cls)
                 .options(joinedload(Tracks.playdates))
                 .where(cls.title.like(f"{text}%"))
                 .order_by(cls.title)
             )
-            .scalars()
             .unique()
             .all()
         )

app/musicmuster.py

@@ -22,6 +22,8 @@ from typing import (
     Sequence,
 )
 
+from playlistmodel import PlaylistModel
+
 from sqlalchemy import text
 
 from PyQt6.QtCore import (
@@ -1038,9 +1040,11 @@ class Window(QMainWindow, Ui_MainWindow):
         """Show dialog box to enter header text and add to playlist"""
 
         try:
-            playlist_tab = self.visible_playlist_tab()
+            model = cast(PlaylistModel, self.visible_playlist_tab().model())
+            if model is None:
+                return
         except AttributeError:
-            # Just return if there's no visible playlist tab
+            # Just return if there's no visible playlist tab model
             return
 
         # Get header text
@@ -1050,9 +1054,10 @@ class Window(QMainWindow, Ui_MainWindow):
         dlg.resize(500, 100)
         ok = dlg.exec()
         if ok:
-            with Session() as session:
-                playlist_tab.insert_header(session, dlg.textValue())
-                playlist_tab.save_playlist(session)
+            model.insert_header_row(
+                self.visible_playlist_tab().get_selected_row_number(),
+                dlg.textValue()
+            )
 
     def insert_track(self) -> None:
         """Show dialog box to select and add track from database"""
@@ -1513,20 +1518,24 @@ class Window(QMainWindow, Ui_MainWindow):
 
         visible_playlist_id = self.visible_playlist_tab().playlist_id
         # Get row number of duplicate rows
-        sql = text(f"""
+        sql = text(
+            f"""
             SELECT max(plr_rownum)
             FROM playlist_rows
             WHERE playlist_id = {visible_playlist_id}
                 AND track_id != 0
             GROUP BY track_id
             HAVING count(id) > 1
-            """)
+            """
+        )
 
         with Session() as session:
             row_numbers = [int(a) for a in session.execute(sql).scalars().all()]
             if row_numbers:
                 self.visible_playlist_tab().select_rows(row_numbers)
-            self.statusbar.showMessage(f"{len(row_numbers)} duplicate rows selected", 10000)
+            self.statusbar.showMessage(
+                f"{len(row_numbers)} duplicate rows selected", 10000
+            )
 
     def select_next_row(self) -> None:
         """Select next or first row in playlist"""

app/playlistmodel.py

@@ -81,6 +81,18 @@ class PlaylistRowData:
 
 
 class PlaylistModel(QAbstractTableModel):
+    """
+    The Playlist Model
+
+    Update strategy: update the database and then refresh the cached copy (self.playlist_rows).
+    We do not try to edit playlist_rows directly. It would be too easy for a bug to get us
+    out of sync with the database, and if that wasn't immediately apparent then debugging it
+    would be hard.
+
+    refresh_row() will populate one row of playlist_rows from the database
+    refresh_data() will repopulate all of playlist_rows from the database
+    """
+
     def __init__(
         self, playlist_id: int, signals: "MusicMusterSignals", *args, **kwargs
     ):
@@ -93,7 +105,7 @@ class PlaylistModel(QAbstractTableModel):
         self.refresh_data()
 
     def __repr__(self) -> str:
-        return f"<PlaylistModel: playlist_id={self.playlist_id}>"
+        return f"<PlaylistModel: playlist_id={self.playlist_id}, {self.rowCount() rows>"
 
     def background_role(self, row: int, column: int, prd: PlaylistRowData) -> QBrush:
         """Return background setting"""
@@ -106,6 +118,7 @@ class PlaylistModel(QAbstractTableModel):
         if file_is_unreadable(prd.path):
             return QBrush(QColor(Config.COLOUR_UNREADABLE))
 
+        # Individual cell colouring
         if column == Col.START_GAP.value:
             if prd.start_gap and prd.start_gap >= Config.START_GAP_WARNING_THRESHOLD:
                 return QBrush(QColor(Config.COLOUR_LONG_START))
@@ -171,13 +184,8 @@ class PlaylistModel(QAbstractTableModel):
         Return text for display
         """
 
-        # Detect whether this is a header row
         if not prd.path:
-            header = prd.note
-        else:
-            header = ""
-
-        if header:
+            # No track so this is a header row
             if column == HEADER_NOTES_COLUMN:
                 self.signals.span_cells_signal.emit(
                     row, HEADER_NOTES_COLUMN, 1, self.columnCount() - 1
@@ -282,53 +290,89 @@ class PlaylistModel(QAbstractTableModel):
 
         return QVariant()
 
-    def insert_row(self, session: scoped_session, row_number: int) -> int:
+    def insert_header_row(self, row_number: Optional[int], text: str) -> Optional[int]:
+        """
+        Insert a header row. Return row number or None if insertion failed.
+        """
+
+        with Session() as session:
+            prd = self._insert_row(session, row_number)
+            # Update playlist_rows
+            prd.note = text
+            # Get row from db and update
+            plr = session.get(PlaylistRows, prd.plrid)
+            if plr:
+                plr.note = text
+                self.refresh_row(session, plr.plr_rownum)
+                self.invalidate_row(plr.plr_rownum)
+                return plr.plr_rownum
+
+        return None
+
+    def _insert_row(
+        self, session: scoped_session, row_number: Optional[int]
+    ) -> PlaylistRowData:
         """
         Make space for a row at row_number. If row_number is greater
         than length of list plus 1, or if row number is -1, put row at
         end of list.
 
-        Return new row number that has an empty, valid entry in self.playlist_rows.
+        Return the new PlaylistData structure
         """
 
-        if row_number > len(self.playlist_rows) or row_number == -1:
-            new_row_number = len(self.playlist_rows) + 1
+        modified_rows: List[int] = []
+
+        if row_number is None or row_number > len(self.playlist_rows):
+            new_row_number = len(self.playlist_rows)
         elif row_number < 0:
             raise ValueError(
                 f"playlistmodel.insert_row, invalid row number ({row_number})"
             )
         else:
             new_row_number = row_number
+        modified_rows.append(new_row_number)
 
         # Move rows below new row down
-        modified_rows: List[int] = []
         for i in reversed(range(new_row_number, len(self.playlist_rows))):
             self.playlist_rows[i + 1] = self.playlist_rows[i]
             self.playlist_rows[i + 1].plr_rownum += 1
             modified_rows.append(i + 1)
+        # If we are not adding to the end of the list, we need to clear
+        # out the existing recored at new_row_number (which we have
+        # already copied to its new location)
+        if new_row_number in self.playlist_rows:
+            del self.playlist_rows[new_row_number]
 
-        # Replace old row
-        self.playlist_rows[new_row_number] = PlaylistRowData(
-            PlaylistRows(
+        *** Problem here is that we haven't yet updated the database so when we insert a new row
+        with the PlaylistRows.__init__ call below, we'll get a duplicate. How best to keep
+        playlist_rows in step with database?
+
+        # Insert new row, possibly replace old row
+        plr = PlaylistRows(
             session=session, playlist_id=self.playlist_id, row_number=new_row_number
         )
+        prd = PlaylistRowData(plr)
+        # Add row to playlist_rows
+        self.playlist_rows[new_row_number] = prd
+
+        return prd
+
+    def invalidate_row(self, modified_row: int) -> None:
+        """
+        Signal to view to refresh invlidated row
+        """
+
+        self.dataChanged.emit(
+            self.index(modified_row, 0), self.index(modified_row, self.columnCount())
         )
 
-        # Refresh rows
-        self.invalidate_rows(modified_rows)
-
-        return new_row_number
-
     def invalidate_rows(self, modified_rows: List[int]) -> None:
         """
         Signal to view to refresh invlidated rows
         """
 
         for modified_row in modified_rows:
-            self.dataChanged.emit(
-                self.index(modified_row, 0),
-                self.index(modified_row, self.columnCount()),
-            )
+            self.invalidate_row(modified_row)
 
     def move_rows(self, from_rows: List[int], to_row: int) -> None:
         """
@@ -379,12 +423,13 @@ class PlaylistModel(QAbstractTableModel):
                 # Fix in self.playlist_rows
                 self.playlist_rows[idx].plr_rownum = idx
                 # Update display
-                self.invalidate_rows([idx])
+                self.invalidate_row(idx)
 
     def refresh_data(self):
         """Populate dicts for data calls"""
 
         # Populate self.playlist_rows with playlist data
+        self.playlist_rows.clear()
         with Session() as session:
             for p in PlaylistRows.deep_rows(session, self.playlist_id):
                 self.playlist_rows[p.plr_rownum] = PlaylistRowData(p)
@@ -393,7 +438,6 @@ class PlaylistModel(QAbstractTableModel):
         """Populate dict for one row for data calls"""
 
         p = PlaylistRows.deep_row(session, self.playlist_id, row_number)
-        self.playlist_rows.clear()
         self.playlist_rows[p.plr_rownum] = PlaylistRowData(p)
 
     def rowCount(self, index: QModelIndex = QModelIndex()) -> int:

app/playlists.py

@@ -522,6 +522,18 @@ class PlaylistTab(QTableView):
         self.clearSelection()
         # self.setDragEnabled(False)
 
+    def get_selected_row_number(self) -> Optional[int]:
+        """
+        Return the selected row number or None if none selected.
+        """
+
+        sm = self.selectionModel()
+        if sm and sm.hasSelection():
+            index = sm.currentIndex()
+            if index.isValid():
+                return index.row()
+        return None
+
     # def get_new_row_number(self) -> int:
     #     """
     #     Return the selected row or the row count if no row selected

conftest.py

@@ -20,7 +20,7 @@ def db_engine():
 
 
 @pytest.fixture(scope="function")
-def Session(db_engine):
+def session(db_engine):
     connection = db_engine.connect()
     transaction = connection.begin()
     sm = sessionmaker(bind=connection)

test_playlistmodel.py

@@ -12,41 +12,39 @@ def create_model_with_playlist_rows(
     # Create a model
     model = playlistmodel.PlaylistModel(playlist.id, None)
     for row in range(rows):
-        newrow = model.insert_row(session, row)
+        plr = model._insert_row(session, row)
+        newrow = plr.plr_rownum
         model.playlist_rows[newrow].note = str(newrow)
 
     session.commit()
     return model
 
 
-def test_insert_row(monkeypatch, Session):
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+def test_insert_row(monkeypatch, session):
+    monkeypatch.setattr(playlistmodel, "Session", session)
     # Create a playlist
-    with Session() as session:
-        playlist = Playlists(Session, "test playlist")
+    playlist = Playlists(session, "test playlist")
     # Create a model
     model = playlistmodel.PlaylistModel(playlist.id, None)
     assert model.rowCount() == 0
-        model.insert_row(session, 0)
+    model._insert_row(session, 0)
     assert model.rowCount() == 1
 
 
-def test_insert_high_row(monkeypatch, Session):
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+def test_insert_high_row(monkeypatch, session):
+    monkeypatch.setattr(playlistmodel, "Session", session)
     # Create a playlist
-    with Session() as session:
-        playlist = Playlists(Session, "test playlist")
+    playlist = Playlists(session, "test playlist")
     # Create a model
     model = playlistmodel.PlaylistModel(playlist.id, None)
     assert model.rowCount() == 0
-        model.insert_row(session, 5)
+    model._insert_row(session, 5)
     assert model.rowCount() == 1
 
 
-def test_11_row_playlist(monkeypatch, Session):
+def test_11_row_playlist(monkeypatch, session):
     # Create multirow playlist
-    monkeypatch.setattr(playlistmodel, "Session", Session)
-    with Session() as session:
+    monkeypatch.setattr(playlistmodel, "Session", session)
     model = create_model_with_playlist_rows(session, 11)
     assert model.rowCount() == 11
     assert max(model.playlist_rows.keys()) == 10
@@ -55,10 +53,9 @@ def test_11_row_playlist(monkeypatch, Session):
         assert model.playlist_rows[row].plr_rownum == row
 
 
-def test_move_rows_test2(monkeypatch, Session):
+def test_move_rows_test2(monkeypatch, session):
     # move row 3 to row 5
-    monkeypatch.setattr(playlistmodel, "Session", Session)
-    with Session() as session:
+    monkeypatch.setattr(playlistmodel, "Session", session)
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([3], 5)
     # Check we have all rows and plr_rownums are correct
@@ -75,12 +72,11 @@ def test_move_rows_test2(monkeypatch, Session):
             assert model.playlist_rows[row].note == str(3)
 
 
-def test_move_rows_test3(monkeypatch, Session):
+def test_move_rows_test3(monkeypatch, session):
     # move row 4 to row 3
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([4], 3)
 
@@ -96,12 +92,11 @@ def test_move_rows_test3(monkeypatch, Session):
             assert model.playlist_rows[row].note == str(3)
 
 
-def test_move_rows_test4(monkeypatch, Session):
+def test_move_rows_test4(monkeypatch, session):
     # move row 4 to row 2
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([4], 2)
 
@@ -119,12 +114,11 @@ def test_move_rows_test4(monkeypatch, Session):
             assert model.playlist_rows[row].note == str(3)
 
 
-def test_move_rows_test5(monkeypatch, Session):
+def test_move_rows_test5(monkeypatch, session):
     # move rows [1, 4, 5, 10] → 8
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([1, 4, 5, 10], 8)
 
@@ -137,12 +131,11 @@ def test_move_rows_test5(monkeypatch, Session):
     assert new_order == [0, 2, 3, 6, 7, 8, 9, 1, 4, 5, 10]
 
 
-def test_move_rows_test6(monkeypatch, Session):
+def test_move_rows_test6(monkeypatch, session):
     # move rows [3, 6] → 5
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([3, 6], 5)
 
@@ -155,12 +148,11 @@ def test_move_rows_test6(monkeypatch, Session):
     assert new_order == [0, 1, 2, 4, 5, 3, 6, 7, 8, 9, 10]
 
 
-def test_move_rows_test7(monkeypatch, Session):
+def test_move_rows_test7(monkeypatch, session):
     # move rows [3, 5, 6] → 8
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([3, 5, 6], 8)
 
@@ -173,12 +165,11 @@ def test_move_rows_test7(monkeypatch, Session):
     assert new_order == [0, 1, 2, 4, 7, 8, 9, 10, 3, 5, 6]
 
 
-def test_move_rows_test8(monkeypatch, Session):
+def test_move_rows_test8(monkeypatch, session):
     # move rows [7, 8, 10] → 5
 
-    monkeypatch.setattr(playlistmodel, "Session", Session)
+    monkeypatch.setattr(playlistmodel, "Session", session)
 
-    with Session() as session:
     model = create_model_with_playlist_rows(session, 11)
     model.move_rows([7, 8, 10], 5)
 
@@ -189,3 +180,19 @@ def test_move_rows_test8(monkeypatch, Session):
         assert model.playlist_rows[row].plr_rownum == row
         new_order.append(int(model.playlist_rows[row].note))
     assert new_order == [0, 1, 2, 3, 4, 7, 8, 10, 5, 6, 9]
+
+
+def test_insert_header_row_end(monkeypatch, session):
+    # insert header row at end of playlist
+
+    monkeypatch.setattr(playlistmodel, "Session", session)
+    note_text = "test text"
+
+    model = create_model_with_playlist_rows(session, 11)
+    row_number = model.insert_header_row(None, note_text)
+    session.flush()
+    assert model.rowCount() == row_number + 1
+    prd = model.playlist_rows[row_number]
+    # Test against edit_role because display_role for headers is
+    # handled differently (sets up row span)
+    assert model.edit_role(row_number, playlistmodel.Col.NOTE.value, prd) == note_text