Skip to content

Improve cover editing and playlist windows - #38

Merged
RetricSu merged 2 commits into
developfrom
fix/player-window-playlist-reorder
Jul 8, 2026
Merged

RetricSu merged 2 commits into
developfrom
fix/player-window-playlist-reorder

Conversation

@RetricSu

@RetricSu RetricSu commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • make the player cover clickable so users can pick a new image and update the audio file metadata
  • move YouTube discover/download flows into standalone egui viewports
  • support dragging playlist tabs to reorder them and persist playlist tab order

Verification

  • cargo fmt --check
  • cargo check

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several UI and backend enhancements, including drag-and-drop reordering for playlist tabs (backed by a new sort_order database column), refactoring YouTube discover and download dialogs to use immediate viewports, and adding support for changing track cover art directly from the UI. Feedback on these changes highlights a bug where clearing the dragging state inside the loop breaks rightward tab dragging, as well as performance concerns regarding marking all playlists as dirty during reordering and executing multiple database updates outside of a single transaction.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/app/services/playlist_service.rs Outdated
Comment on lines +90 to +92
for playlist in playlists.iter_mut() {
playlist.is_dirty = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Setting playlist.is_dirty = true for all playlists on every drag-and-drop reorder operation is a significant performance bottleneck.\n\nWhen a playlist is marked as dirty, the subsequent save_state() call will completely rewrite the playlist's metadata and all of its tracks (deleting and re-inserting them) in the database. If a user has multiple playlists or large playlists, this will cause a noticeable UI freeze during dragging.\n\nSince save_playlists already updates the sort_order column for all playlists regardless of their dirty status, we can safely remove this loop entirely.

Comment thread src/app/components/playlist_tabs.rs Outdated
Comment on lines +114 to +116
if tab_response.drag_stopped() {
ctx.ui_state.playlist_tab_dragging = None;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Clearing the playlist_tab_dragging state immediately inside the loop when tab_response.drag_stopped() is detected introduces a bug where dragging a tab to the right (to a higher index) fails or is extremely unreliable.\n\nWhen dragging tab A (lower index) to the right over tab B (higher index) and releasing:\n1. Tab A is processed first in the loop. Since the drag has stopped, tab_response.drag_stopped() is true, which immediately clears playlist_tab_dragging to None.\n2. When tab B is processed later in the same loop, playlist_tab_dragging is already None, so the drop/reorder logic is never triggered.\n\nSince you already have a global cleanup check at the end of show_tabs (lines 139-141) that clears the dragging state when the pointer is released, you can safely remove this drag_stopped() check from inside the loop. This ensures the dragging state remains available for all tabs during the drop frame.

Comment thread src/app/services/db_persistence.rs Outdated
Comment on lines 22 to 31
for (sort_order, playlist) in playlists.iter_mut().enumerate() {
playlist.save_to_db_and_update_id(db_conn)?;
if let Some(id) = playlist.id {
let conn = db_conn.lock().unwrap();
conn.execute(
"UPDATE playlists SET sort_order = ?1 WHERE id = ?2",
rusqlite::params![sort_order as i64, id],
)?;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Updating the sort_order of each playlist by locking the database connection and executing a separate query inside the loop is highly inefficient. In SQLite, executing multiple independent update statements outside of a transaction results in a separate transaction (and disk sync) for each statement, which can be extremely slow.\n\nWe can optimize this by first saving any dirty playlists, and then locking the connection once to update all sort_order values within a single database transaction.

        for playlist in playlists.iter_mut() {
            playlist.save_to_db_and_update_id(db_conn)?;
        }

        let mut conn = db_conn.lock().unwrap();
        let tx = conn.transaction()?;
        {
            let mut stmt = tx.prepare("UPDATE playlists SET sort_order = ?1 WHERE id = ?2")?;
            for (sort_order, playlist) in playlists.iter().enumerate() {
                if let Some(id) = playlist.id {
                    stmt.execute(rusqlite::params![sort_order as i64, id])?;
                }
            }
        }
        tx.commit()?;

@RetricSu
RetricSu merged commit 7efb12d into develop Jul 8, 2026
2 checks passed
@RetricSu
RetricSu deleted the fix/player-window-playlist-reorder branch July 8, 2026 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant