WIP: moving rows within playlist works

app/config.py

@@ -112,6 +112,8 @@ class Config(object):
     PLAYLIST_ICON_CURRENT = ":/icons/green-circle.png"
     PLAYLIST_ICON_NEXT = ":/icons/yellow-circle.png"
     PLAYLIST_ICON_TEMPLATE = ":/icons/redstar.png"
+    PLAYLIST_PENDING_MOVE = -1
+    PLAYLIST_FAILED_MOVE = -2
     PREVIEW_ADVANCE_MS = 5000
     PREVIEW_BACK_MS = 5000
     PREVIEW_END_BUFFER_MS = 1000

app/musicmuster.py

@@ -2665,8 +2665,6 @@ class Window(QMainWindow):
         Update track clocks.
         """
 
-        self.timer1000.stop()
-        raise ApplicationError("test")
         # If track is playing, update track clocks time and colours
         if self.track_sequence.current and self.track_sequence.current.is_playing():
             # Elapsed time

app/playlistmodel.py

@@ -25,7 +25,6 @@ from PyQt6.QtGui import (
 
 # Third party imports
 # import line_profiler
-from sqlalchemy.orm.session import Session
 import obswebsocket  # type: ignore
 
 # import snoop  # type: ignore
@@ -833,12 +832,17 @@ class PlaylistModel(QAbstractTableModel):
         ]
         self.invalidate_rows(row_numbers, roles)
 
-    def move_rows(self, from_rows: list[PlaylistRow], to_row_number: int) -> None:
+    def move_rows(self, from_rows: list[int], to_row_number: int) -> bool:
         """
-        Move the playlist rows given to to_row and below.
+        Move the playlist rows in from_rows to to_row. Return True if successful
+        else False.
         """
 
-        log.debug(f"{self}: move_rows({from_rows=}, {to_row_number=}")
+        log.debug(f"move_rows({from_rows=}, {to_row_number=})")
+
+        if not from_rows:
+            log.debug("move_rows called with no from_rows")
+            return False
 
         # Don't move current row
         if self.track_sequence.current:
@@ -937,10 +941,6 @@ class PlaylistModel(QAbstractTableModel):
 
         # Handle the moves in row_group chunks
 
-        # TODO: use bool QAbstractItemModel::beginMoveRows(const
-        # QModelIndex &sourceParent, int sourceFirst, int sourceLast,
-        # const QModelIndex &destinationParent, int destinationChild)
-
         for row_group in row_groups:
             # Prepare source model
             super().beginRemoveRows(QModelIndex(), min(row_group), max(row_group))
@@ -1073,6 +1073,7 @@ class PlaylistModel(QAbstractTableModel):
             else:
                 new_playlist_row = self.playlist_rows[plrid_to_row[dto.playlistrow_id]]
                 new_playlist_row.row_number = dto.row_number
+                new_playlist_rows[dto.row_number] = new_playlist_row
 
         # Copy to self.playlist_rows
         self.playlist_rows = new_playlist_rows

app/repository.py

@@ -17,6 +17,7 @@ from classes import ApplicationError, PlaylistRowDTO
 
 # App imports
 from classes import PlaylistDTO, TrackDTO
+from config import Config
 import helpers
 from log import log
 from models import (
@@ -254,6 +255,39 @@ def tracks_like_title(filter_str: str) -> list[TrackDTO]:
 
 
 # Playlist functions
+def _check_playlist_integrity(
+    session: Session, playlist_id: int, fix: bool = False
+) -> None:
+    """
+    Ensure the row numbers are contiguous. Fix and log if fix==True,
+    else raise ApplicationError.
+    """
+
+    playlist_rows = (
+        session.execute(
+            select(PlaylistRows)
+            .where(PlaylistRows.playlist_id == playlist_id)
+            .order_by(PlaylistRows.row_number)
+        )
+        .scalars()
+        .all()
+    )
+    for idx, plr in enumerate(playlist_rows):
+        if plr.row_number == idx:
+            continue
+
+        msg = (
+            "_check_playlist_integrity: incorrect row number "
+            f"({plr.id=}, {plr.row_number=}, {idx=})"
+        )
+        if fix:
+            log.debug(msg)
+            plr.row_number = idx
+            session.commit()
+        else:
+            raise ApplicationError(msg)
+
+
 def _move_rows(
     session: Session, playlist_id: int, starting_row: int, move_by: int
 ) -> None:
@@ -301,83 +335,112 @@ def move_rows_to_playlist(
             update(PlaylistRows)
             .where(
                 PlaylistRows.playlist_id == from_playlist_id,
-                PlaylistRows.row_number.in_(from_rows)
+                PlaylistRows.row_number.in_(from_rows),
             )
             .values(
                 playlist_id=to_playlist_id,
-                row_number=PlaylistRows.row_number + row_offset
+                row_number=PlaylistRows.row_number + row_offset,
             )
         )
         session.execute(stmt)
         # Remove gaps in source
-        _move_rows(session=session,
+        _move_rows(
+            session=session,
             playlist_id=from_playlist_id,
             starting_row=max(from_rows) + 1,
-                   move_by=(len(from_rows) * -1)
+            move_by=(len(from_rows) * -1),
         )
         # Commit changes
         session.commit()
         # Sanity check
-    _check_playlist_integrity(session, get_playlist_rows(from_playlist_id), fix=False)
+    _check_playlist_integrity(session, from_playlist_id, fix=False)
-    _check_playlist_integrity(session, get_playlist_rows(to_playlist_id), fix=False)
+    _check_playlist_integrity(session, to_playlist_id, fix=False)
 
 
-def move_rows_within_playlist(playlist_id: int, from_rows: list[int], to_row: int) -> None:
+def move_rows_within_playlist(
+    playlist_id: int, from_rows: list[int], to_row: int
+) -> None:
     """
     Move rows within a playlist.
+
+    Algorithm:
+     - Sanity check row numbers
+     - Check there are no playlist rows with playlist_id == PENDING_MOVE
+     - Put rows to be moved into PENDING_MOVE playlist
+     - Resequence remaining row numbers
+     - Make space for moved rows
+     - Move the PENDING_MOVE rows back and fixup row numbers
+     - Sanity check row numbers
     """
 
     log.debug(f"move_rows_within_playlist({playlist_id=}, {from_rows=}, {to_row=})")
 
-    playlistrows_dto = get_playlist_rows(playlist_id)
-    new_order: dict[int, int | None] = dict.fromkeys(range(len(playlistrows_dto)))
-
-    # The destination row number will need to be reduced by the
-    # number of rows being move from above the destination row
-    # otherwise rows below the destination row will end up above the
-    # moved rows.
-    # next_row = to_row - len([a for a in from_rows if a < to_row])
-    # Need to ensure the moved rows won't overrun the total number of
-    # rows
-    next_row = to_row
-    if next_row + len(from_rows) > len(playlistrows_dto):
-        next_row = len(playlistrows_dto) - len(from_rows)
-
-    # Populate new_order with moved rows
-    # # We need to keep, where possible, the rows after to_row unmoved
-    # if to_row + len(from_rows) > len(playlistrows_dto):
-    #     next_row = max(to_row - len(from_rows) - len([a for a in from_rows if a < to_row]) + 1, 0)
-    for from_row in from_rows:
-        new_order[next_row] = from_row
-        next_row += 1
-
-    # Move remaining rows
-    remaining_rows = set(new_order.keys()) - set(from_rows)
-    next_row = 0
-    for row in remaining_rows:
-        while new_order[next_row] is not None:
-            next_row += 1
-        new_order[next_row] = row
-        next_row += 1
-
-    # Sanity check
-    if None in new_order:
-        raise ApplicationError(f"None remains after move: {new_order=}")
-
-    # Update database
-    # Build a list of dicts of (id: value, row_number: value}
-    update_list = []
-    for new_row_number, old_row_number in new_order.items():
-        plrid = [a.playlistrow_id for a in playlistrows_dto if a.row_number == old_row_number][0]
-        update_list.append(dict(id=plrid, row_number=new_row_number))
-
-    # Update rows
     with db.Session() as session:
+        # Sanity check row numbers
+        _check_playlist_integrity(session, playlist_id, fix=False)
+
+        # Check there are no playlist rows with playlist_id == PENDING_MOVE
+        pending_move_rows = get_playlist_rows(Config.PLAYLIST_PENDING_MOVE)
+        if pending_move_rows:
+            raise ApplicationError(f"move_rows_within_playlist: {pending_move_rows=}")
+
+        # Get length of playlist
+        playlist_length = len(get_playlist_rows(playlist_id))
+
+        # Put rows to be moved into PENDING_MOVE playlist
+        session.execute(
+            update(PlaylistRows)
+            .where(
+                PlaylistRows.playlist_id == playlist_id,
+                PlaylistRows.row_number.in_(from_rows),
+            )
+            .values(playlist_id=Config.PLAYLIST_PENDING_MOVE)
+        )
+        session.commit()
+
+        # Resequence remaining row numbers
+        _check_playlist_integrity(session, playlist_id, fix=True)
+
+        # Make space for moved rows. Determning where to make the space
+        # is non-trivial. For example, if the playlist has ten entries
+        # and we're moving four of them to row 8, after we've moved the
+        # rows to the PLAYLIST_PENDING_MOVE there will only be six
+        # entries left. Clearly we can't make space at row 8...
+        overflow = max(to_row + len(from_rows) - playlist_length, 0)
+        if overflow == 0:
+            space_row = to_row
+        else:
+            space_row = to_row - overflow - len([a for a in from_rows if a > to_row])
+        _move_rows(session, playlist_id, space_row, len(from_rows))
+
+        # Move the PENDING_MOVE rows back and fixup row numbers
+        update_list: list[dict[str, int]] = []
+        next_row = space_row
+        for row_to_move in get_playlist_rows(Config.PLAYLIST_PENDING_MOVE):
+            update_list.append({"id": row_to_move.playlistrow_id, "row_number": next_row})
+            update_list.append({"id": row_to_move.playlistrow_id, "playlist_id": playlist_id})
+            next_row += 1
         session.execute(update(PlaylistRows), update_list)
         session.commit()
 
+        # Sanity check row numbers
+        _check_playlist_integrity(session, playlist_id, fix=False)
+
+
+def update_row_numbers(
+    playlist_id: int, id_to_row_number: list[dict[int, int]]
+) -> None:
+    """
+    Update playlistrows rownumbers for pass playlistrow_ids
+    playlist_id is only needed for sanity checking
+    """
+
+    with db.Session() as session:
+        session.execute(update(PlaylistRows), id_to_row_number)
+        session.commit()
+
         # Sanity check
-        _check_playlist_integrity(session, get_playlist_rows(playlist_id), fix=False)
+        _check_playlist_integrity(session, playlist_id, fix=False)
 
 
 def create_playlist(name: str, template_id: int) -> PlaylistDTO:
@@ -491,27 +554,6 @@ def get_playlist_row(playlistrow_id: int) -> PlaylistRowDTO | None:
     return dto
 
 
-def _check_playlist_integrity(
-    session: Session, playlist_rows: list[PlaylistRowDTO], fix: bool = False
-) -> None:
-    """
-    Ensure the row numbers are contiguous. Fix and log if fix==True,
-    else raise ApplicationError.
-    """
-
-    for idx, plr in enumerate(playlist_rows):
-        if plr.row_number == idx:
-            continue
-
-        msg = f"_check_playlist_integrity: incorrect row number ({plr.playlistrow_id=}, {idx=})"
-        if fix:
-            log.debug(msg)
-            plr.row_number = idx
-            session.commit()
-        else:
-            raise ApplicationError(msg)
-
-
 def get_playlist_rows(playlist_id: int) -> list[PlaylistRowDTO]:
     # Alias PlaydatesTable for subquery
     LatestPlaydate = aliased(Playdates)
@@ -614,7 +656,7 @@ def insert_row(
 
     with db.Session() as session:
         # Sanity check
-        _check_playlist_integrity(session, get_playlist_rows(playlist_id), fix=False)
+        _check_playlist_integrity(session, playlist_id, fix=False)
 
         # Make space for new row
         _move_rows(
@@ -632,7 +674,7 @@ def insert_row(
         playlist_row_id = playlist_row.id
 
         # Sanity check
-        _check_playlist_integrity(session, get_playlist_rows(playlist_id), fix=False)
+        _check_playlist_integrity(session, playlist_id, fix=False)
 
     new_playlist_row = get_playlist_row(playlistrow_id=playlist_row_id)
     if not new_playlist_row:

tests/test_repository.py

@@ -36,7 +36,9 @@ class MyTestCase(unittest.TestCase):
 
         db.create_all()
 
-    def create_playlist_and_model(self, playlist_name: str) -> (PlaylistDTO, PlaylistModel):
+    def create_playlist_and_model(
+        self, playlist_name: str
+    ) -> (PlaylistDTO, PlaylistModel):
         # Create a playlist and model
         playlist = repository.create_playlist(name=playlist_name, template_id=0)
         assert playlist
@@ -72,7 +74,9 @@ class MyTestCase(unittest.TestCase):
             note="track 2",
         )
 
-    def create_rows(self, playlist_name: str, number_of_rows: int) -> (PlaylistDTO, PlaylistModel):
+    def create_rows(
+        self, playlist_name: str, number_of_rows: int
+    ) -> (PlaylistDTO, PlaylistModel):
         (playlist, model) = self.create_playlist_and_model(playlist_name)
         for row_number in range(number_of_rows):
             repository.insert_row(
@@ -179,7 +183,7 @@ class MyTestCase(unittest.TestCase):
         new_order = []
         for row in repository.get_playlist_rows(playlist.playlist_id):
             new_order.append(int(row.note))
-        assert new_order == [0, 2, 3, 6, 7, 8, 9, 1, 4, 5, 10]
+        assert new_order == [0, 2, 3, 6, 7, 8, 1, 4, 5, 10, 9]
 
     def test_move_rows_test5(self):
         # move rows [3, 6] → 5
@@ -213,7 +217,7 @@ class MyTestCase(unittest.TestCase):
         # move rows [7, 8, 10] → 5
 
         number_of_rows = 11
-        (playlist, model) = self.create_rows("test_move_rows_test6", number_of_rows)
+        (playlist, model) = self.create_rows("test_move_rows_test7", number_of_rows)
 
         repository.move_rows_within_playlist(playlist.playlist_id, [7, 8, 10], 5)
 
@@ -228,7 +232,7 @@ class MyTestCase(unittest.TestCase):
         # Replicate issue 244
 
         number_of_rows = 11
-        (playlist, model) = self.create_rows("test_move_rows_test6", number_of_rows)
+        (playlist, model) = self.create_rows("test_move_rows_test8", number_of_rows)
 
         repository.move_rows_within_playlist(playlist.playlist_id, [0, 1, 2, 3], 0)