diff --git a/app/playlistmodel.py b/app/playlistmodel.py index 152d2c4..ed409c8 100644 --- a/app/playlistmodel.py +++ b/app/playlistmodel.py @@ -1,5 +1,6 @@ from datetime import datetime from enum import auto, Enum +from sqlalchemy import update from typing import List, Optional, TYPE_CHECKING from dbconfig import scoped_session, Session @@ -84,13 +85,16 @@ 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. + 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 + database in sync is uncessarily challenging. - refresh_row() will populate one row of playlist_rows from the database - refresh_data() will repopulate all of playlist_rows from the database + 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__( @@ -102,10 +106,13 @@ class PlaylistModel(QAbstractTableModel): self.playlist_rows: dict[int, PlaylistRowData] = {} - self.refresh_data() + with Session() as session: + self.refresh_data(session) def __repr__(self) -> str: - return f"" + return ( + f"" + ) def background_role(self, row: int, column: int, prd: PlaylistRowData) -> QBrush: """Return background setting""" @@ -290,72 +297,55 @@ class PlaylistModel(QAbstractTableModel): return QVariant() - def insert_header_row(self, row_number: Optional[int], text: str) -> Optional[int]: + def insert_header_row(self, row_number: Optional[int], text: str) -> None: """ 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 + plr = self._insert_row(session, row_number) + # Update the PlaylistRows object + plr.note = text + # Repopulate self.playlist_rows + self.refresh_data(session) + # Update the display from the new row onwards + self.invalidate_rows(list(range(plr.plr_rownum, len(self.playlist_rows)))) def _insert_row( self, session: scoped_session, row_number: Optional[int] - ) -> PlaylistRowData: + ) -> PlaylistRows: """ - 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. + Insert a row in the database. - Return the new PlaylistData structure + If row_number is greater than length of list plus 1, or if row + number is None, put row at end of list. + + Move existing rows to make space if ncessary. + + Return the new PlaylistRows object. """ - modified_rows: List[int] = [] - if row_number is None or row_number > len(self.playlist_rows): + # We are adding to the end of the list so we can optimise new_row_number = len(self.playlist_rows) + return PlaylistRows(session, self.playlist_id, new_row_number) elif row_number < 0: raise ValueError( - f"playlistmodel.insert_row, invalid row number ({row_number})" + 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 - 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] - - *** 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 + stmt = ( + update(PlaylistRows) + .where(PlaylistRows.plr_rownum >= new_row_number) + .values({PlaylistRows.plr_rownum: PlaylistRows.plr_rownum + 1}) ) - prd = PlaylistRowData(plr) - # Add row to playlist_rows - self.playlist_rows[new_row_number] = prd + session.execute(stmt) - return prd + # Insert the new row and return it + return PlaylistRows(session, self.playlist_id, new_row_number) def invalidate_row(self, modified_row: int) -> None: """ @@ -425,14 +415,13 @@ class PlaylistModel(QAbstractTableModel): # Update display self.invalidate_row(idx) - def refresh_data(self): + def refresh_data(self, session: scoped_session): """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) + for p in PlaylistRows.deep_rows(session, self.playlist_id): + self.playlist_rows[p.plr_rownum] = PlaylistRowData(p) def refresh_row(self, session, row_number): """Populate dict for one row for data calls""" diff --git a/test_playlistmodel.py b/test_playlistmodel.py index 81a13d1..ac8a993 100644 --- a/test_playlistmodel.py +++ b/test_playlistmodel.py @@ -14,34 +14,13 @@ def create_model_with_playlist_rows( for row in range(rows): plr = model._insert_row(session, row) newrow = plr.plr_rownum + model.playlist_rows[newrow] = playlistmodel.PlaylistRowData(plr) model.playlist_rows[newrow].note = str(newrow) session.commit() return model -def test_insert_row(monkeypatch, session): - monkeypatch.setattr(playlistmodel, "Session", session) - # Create a playlist - playlist = Playlists(session, "test playlist") - # Create a model - model = playlistmodel.PlaylistModel(playlist.id, None) - assert model.rowCount() == 0 - model._insert_row(session, 0) - assert model.rowCount() == 1 - - -def test_insert_high_row(monkeypatch, session): - monkeypatch.setattr(playlistmodel, "Session", session) - # Create a playlist - playlist = Playlists(session, "test playlist") - # Create a model - model = playlistmodel.PlaylistModel(playlist.id, None) - assert model.rowCount() == 0 - model._insert_row(session, 5) - assert model.rowCount() == 1 - - def test_11_row_playlist(monkeypatch, session): # Create multirow playlist monkeypatch.setattr(playlistmodel, "Session", session) @@ -187,12 +166,29 @@ def test_insert_header_row_end(monkeypatch, session): monkeypatch.setattr(playlistmodel, "Session", session) note_text = "test text" + initial_row_count = 11 - 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] + model = create_model_with_playlist_rows(session, initial_row_count) + model.insert_header_row(None, note_text) + assert model.rowCount() == initial_row_count + 1 + prd = model.playlist_rows[model.rowCount() - 1] # 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 + assert model.edit_role(model.rowCount(), playlistmodel.Col.NOTE.value, prd) == note_text + + +def test_insert_header_row_middle(monkeypatch, session): + # insert header row in middle of playlist + + monkeypatch.setattr(playlistmodel, "Session", session) + note_text = "test text" + initial_row_count = 11 + insert_row = 6 + + model = create_model_with_playlist_rows(session, initial_row_count) + model.insert_header_row(insert_row, note_text) + assert model.rowCount() == initial_row_count + 1 + prd = model.playlist_rows[insert_row] + # Test against edit_role because display_role for headers is + # handled differently (sets up row span) + assert model.edit_role(model.rowCount(), playlistmodel.Col.NOTE.value, prd) == note_text