[PR #910] [MERGED] Fix confirmation dialog handling on playlist delete. #1086

New issue

Closed

opened 2026-02-28 14:54:24 +03:00 by kerem · 0 comments

kerem commented

2026-02-28 14:54:24 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Rigellute/spotify-tui/pull/910
Author: @majabojarska
Created: 10/24/2021
Status: ✅ Merged
Merged: 11/17/2021
Merged by: @Rigellute

Base: master ← Head: fix_playlist_delete

📝 Commits (2)

000c11d Fix playlist deletion handling.
9dda556 Refactor get_current_user_saved_artists_next.

📊 Changes

5 files changed (+14 additions, -12 deletions)

View changed files

📝 src/app.rs (+3 -7)
📝 src/handlers/common_key_events.rs (+1 -0)
📝 src/handlers/playlist.rs (+5 -3)
📝 src/handlers/search_results.rs (+4 -2)
📝 src/ui/mod.rs (+1 -0)

📄 Description

Resolves #884.

Observations

I might not know the architecture of this application very well, but I believe I've narrowed down the root cause and drafted a quick solution. As of commit ecddb5e, the push_navigation_stack method of the App class does not actually push any element to the navigation stack if the next route's id is the same as last route's id:

// app.rs
pub fn push_navigation_stack(&mut self, next_route_id: RouteId, next_active_block: ActiveBlock) {
    if !self
      .navigation_stack
      .last()
      .map(|last_route| last_route.id == next_route_id)
      .unwrap_or(false)
    { // Enter branch only if the last route's id is different from the next route's id.
      // Push to stack...
    }
  }

That's the case when the uppercase D character is entered, when a playlist is selected - the last route's id is the same as the next route's id (RouteId::Home):

// playlist.rs
Key::Char('D') => {
      if let (Some(playlists), Some(selected_index)) = (&app.playlists, app.selected_playlist_index)
      {
        let selected_playlist = &playlists.items[selected_index].name;
        app.dialog = Some(selected_playlist.clone());
        app.confirm = false;

        let route = app.get_current_route().id.clone(); // Pass current route's id as the next route's id
        app.push_navigation_stack(route, ActiveBlock::Dialog(DialogContext::PlaylistWindow));
      }
    }

As a result, no new element is pushed to the navigation stack, hence the confirmation dialog window does not appear, and the playlist cannot be deleted.

Solution

My solution is to add a RouteId::Dialog, which is specific to confirmation dialog windows. When a dialog is to be opened (e.g. when deleting a playlist), a new route (different than the current one) is pushed onto the navigation stack. Then, the logic responsible for drawing and handling user input in the confirmation dialog gets executed and works just fine.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Rigellute/spotify-tui/pull/910 **Author:** [@majabojarska](https://github.com/majabojarska) **Created:** 10/24/2021 **Status:** ✅ Merged **Merged:** 11/17/2021 **Merged by:** [@Rigellute](https://github.com/Rigellute) **Base:** `master` ← **Head:** `fix_playlist_delete` --- ### 📝 Commits (2) - [`000c11d`](https://github.com/Rigellute/spotify-tui/commit/000c11d8a041185368e7ebe9fb03ba5c939648fd) Fix playlist deletion handling. - [`9dda556`](https://github.com/Rigellute/spotify-tui/commit/9dda55657f42a3ec95596fc65522c44266767582) Refactor get_current_user_saved_artists_next. ### 📊 Changes **5 files changed** (+14 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `src/app.rs` (+3 -7) 📝 `src/handlers/common_key_events.rs` (+1 -0) 📝 `src/handlers/playlist.rs` (+5 -3) 📝 `src/handlers/search_results.rs` (+4 -2) 📝 `src/ui/mod.rs` (+1 -0) </details> ### 📄 Description Resolves #884. ### Observations I might not know the architecture of this application very well, but I believe I've narrowed down the root cause and drafted a quick solution. As of commit [ecddb5e](https://github.com/Rigellute/spotify-tui/commit/ecddb5eb4fb441bf46075d44cf12cd2294a251c7), the `push_navigation_stack` method of the `App` class does not actually push any element to the navigation stack if the next route's id is the same as last route's id: ```rust // app.rs pub fn push_navigation_stack(&mut self, next_route_id: RouteId, next_active_block: ActiveBlock) { if !self .navigation_stack .last() .map(|last_route| last_route.id == next_route_id) .unwrap_or(false) { // Enter branch only if the last route's id is different from the next route's id. // Push to stack... } } ``` That's the case when the uppercase `D` character is entered, when a playlist is selected - the last route's id is the same as the next route's id (`RouteId::Home`): ```rust // playlist.rs Key::Char('D') => { if let (Some(playlists), Some(selected_index)) = (&app.playlists, app.selected_playlist_index) { let selected_playlist = &playlists.items[selected_index].name; app.dialog = Some(selected_playlist.clone()); app.confirm = false; let route = app.get_current_route().id.clone(); // Pass current route's id as the next route's id app.push_navigation_stack(route, ActiveBlock::Dialog(DialogContext::PlaylistWindow)); } } ``` As a result, no new element is pushed to the navigation stack, hence the confirmation dialog window does not appear, and the playlist cannot be deleted. ### Solution My solution is to add a `RouteId::Dialog`, which is specific to confirmation dialog windows. When a dialog is to be opened (e.g. when deleting a playlist), a new route (different than the current one) is pushed onto the navigation stack. Then, the logic responsible for drawing and handling user input in the confirmation dialog gets executed and works just fine. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>