From 3cd764c89318432e38185c7af59c1e5a21e658dc Mon Sep 17 00:00:00 2001 From: Keith Edmunds Date: Thu, 27 Mar 2025 11:24:13 +0000 Subject: [PATCH] WIP: moving rows within playlist works --- app/config.py | 2 + app/musicmuster.py | 2 - app/playlistmodel.py | 17 ++-- app/repository.py | 196 ++++++++++++++++++++++++--------------- tests/test_repository.py | 14 ++- 5 files changed, 139 insertions(+), 92 deletions(-) diff --git a/app/config.py b/app/config.py index 431dce0..58db885 100644 --- a/app/config.py +++ b/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 diff --git a/app/musicmuster.py b/app/musicmuster.py index bb23a98..90145d7 100755 --- a/app/musicmuster.py +++ b/app/musicmuster.py @@ -2667,8 +2667,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 diff --git a/app/playlistmodel.py b/app/playlistmodel.py index 5bdd56e..a6e1205 100644 --- a/app/playlistmodel.py +++ b/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 diff --git a/app/repository.py b/app/repository.py index c81acb1..aae57a4 100644 --- a/app/repository.py +++ b/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, - playlist_id=from_playlist_id, - starting_row=max(from_rows) + 1, - move_by=(len(from_rows) * -1) - ) + _move_rows( + session=session, + playlist_id=from_playlist_id, + starting_row=max(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, get_playlist_rows(to_playlist_id), fix=False) + _check_playlist_integrity(session, from_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: diff --git a/tests/test_repository.py b/tests/test_repository.py index f2a8ce6..89afd1e 100644 --- a/tests/test_repository.py +++ b/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)