ommp.git

ommp

~ Oh My Music Player — a standalone terminal music player built with Rust and ratatui
git clone https://github.com/debuman/ommp.git
commit 3e8f73864576e9125d2be9f35286fbf7086d9ddd
parent 40ce1fb2337c46b7c1e7423070f40851ebf38e90
Author: sqzer-x <110329244+sqzer-x@users.noreply.github.com>
Date: Sat, 1 Aug 2026 15:58:34 +0900

Fix keybinding docs and modals that silently clipped content

The help modal never listed p (toggle the info panel), h/l, Esc or Ctrl+C,
and its 23 entries did not fit an 80x24 terminal — with no scrolling, the
tail including "q: Quit" was simply cut off. It is two columns now. The
README was missing c and Ctrl+C.

The playlist modal drew every playlist from the top and let the selection
walk past the bottom edge, so on a short terminal Enter toggled a playlist
that was not on screen. The search modal reported its full height as the
result viewport even though title_bottom reserves a row, which stopped
keyboard auto-scroll one result short and mapped clicks on the count footer
to a row that was never drawn.

The tab bar had six colours for seven tabs, so the modulo wrapped and
Playlists rendered in Queue's blue; hit-testing checked only the x range, so
clicking the bar's border switched tabs, and the divider columns were
attributed to the following tab.

lyrics_pane renders track metadata, not lyrics — the name outlived the
feature. Renamed, and its scroll offset, written by seven handlers and read
by none, is now applied: the field list grew to 13 rows and did not fit the
pane's default height.

Diffstat:
MREADME.md | 2++
Msrc/app/handler.rs | 10+++++-----
Msrc/ui/mod.rs | 8++++----
Dsrc/ui/panes/lyrics_pane.rs | 80-------------------------------------------------------------------------------
Msrc/ui/panes/mod.rs | 2+-
Asrc/ui/panes/track_info_pane.rs | 82+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Msrc/ui/widgets/help_modal.rs | 73++++++++++++++++++++++++++++++++++++++++++++-----------------------------
Msrc/ui/widgets/info_pane.rs | 10++++++++--
Msrc/ui/widgets/playlist_modal.rs | 9+++++++++
Msrc/ui/widgets/search_modal.rs | 14+++++++++++---
Msrc/ui/widgets/tab_bar.rs | 64++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
11 files changed, 224 insertion(+), 130 deletion(-)

diff --git a/README.md b/README.md
@@ -143,7 +143,9 @@ Press `Ctrl+E, h` inside the app to view the full keybinding list at any time.
 | `d` | Remove from queue |
 | `b` | Add to playlist |
 | `p` | Toggle info panel (Clock / Album Art) |
+| `c` | Clear queue |
 | `q` | Quit |
+| `Ctrl+C` | Quit |
 
 ### Chord commands (press `Ctrl+E`, then a key)
 
diff --git a/src/app/handler.rs b/src/app/handler.rs
@@ -401,7 +401,7 @@ pub fn handle_key_event(key: KeyEvent, app: &App, ui: &mut Ui) -> Vec<AppAction>
                 _ => ui.queue_pane.handle_key(key, app),
             }
         }
-        FocusedPane::Lyrics => ui.lyrics_pane.handle_key(key, app),
+        FocusedPane::Lyrics => ui.track_info_pane.handle_key(key, app),
     };
 
     // Auto-focus to Queue pane when adding tracks from Library
@@ -618,7 +618,7 @@ pub fn handle_mouse_event(
 
             // Tab bar click
             if y >= areas.tab_bar.y && y < areas.tab_bar.y + areas.tab_bar.height {
-                if let Some(tab_idx) = tab_bar::tab_hit_test(areas.tab_bar, x) {
+                if let Some(tab_idx) = tab_bar::tab_hit_test(areas.tab_bar, x, y) {
                     actions.push(AppAction::SwitchTab(Tab::from_index(tab_idx)));
                 }
                 return actions;
@@ -684,7 +684,7 @@ pub fn handle_mouse_event(
                     }
                 }
             } else if in_lyrics {
-                if let Some(a) = ui.lyrics_pane.handle_mouse(mouse, areas.lyrics, app) {
+                if let Some(a) = ui.track_info_pane.handle_mouse(mouse, areas.lyrics, app) {
                     actions.push(a);
                 }
             }
@@ -699,7 +699,7 @@ pub fn handle_mouse_event(
                     actions.push(a);
                 }
             } else if in_lyrics {
-                if let Some(a) = ui.lyrics_pane.handle_mouse(mouse, areas.lyrics, app) {
+                if let Some(a) = ui.track_info_pane.handle_mouse(mouse, areas.lyrics, app) {
                     actions.push(a);
                 }
             }
@@ -801,7 +801,7 @@ impl PaneHit {
 
 fn update_tab_hover(ui: &mut Ui, areas: &LayoutAreas, x: u16, y: u16) {
     ui.hovered_tab = if y >= areas.tab_bar.y && y < areas.tab_bar.y + areas.tab_bar.height {
-        tab_bar::tab_hit_test(areas.tab_bar, x)
+        tab_bar::tab_hit_test(areas.tab_bar, x, y)
     } else {
         None
     };
diff --git a/src/ui/mod.rs b/src/ui/mod.rs
@@ -18,7 +18,7 @@ use pane::Pane;
 use panes::dir_browser_pane::DirBrowserPane;
 use panes::library_pane::LibraryPane;
 use panes::list_pane::{self, ListPane};
-use panes::lyrics_pane::LyricsPane;
+use panes::track_info_pane::TrackInfoPane;
 use panes::queue_pane::QueuePane;
 
 /// Index into `Ui::list_panes` for each list tab. Queue and Directories have
@@ -45,7 +45,7 @@ pub struct Ui {
     pub queue_pane: QueuePane,
     /// One per list tab, keyed by Tab; each keeps its own selection and scroll.
     pub list_panes: [ListPane; 5],
-    pub lyrics_pane: LyricsPane,
+    pub track_info_pane: TrackInfoPane,
     pub last_click: Option<(std::time::Instant, u16, u16)>,
     /// Last known mouse position (column, row) for hover tracking
     pub mouse_pos: Option<(u16, u16)>,
@@ -107,7 +107,7 @@ impl Ui {
             dir_browser_pane: DirBrowserPane::new(music_dir),
             queue_pane: QueuePane::new(),
             list_panes: Default::default(),
-            lyrics_pane: LyricsPane::new(),
+            track_info_pane: TrackInfoPane::new(),
             last_click: None,
             mouse_pos: None,
             hovered_tab: None,
@@ -184,7 +184,7 @@ impl Ui {
 
         // Right pane bottom (Lyrics)
         let lyrics_focused = app.focus == FocusedPane::Lyrics;
-        self.lyrics_pane.render(frame, areas.lyrics, lyrics_focused, app, &self.theme);
+        self.track_info_pane.render(frame, areas.lyrics, lyrics_focused, app, &self.theme);
 
         // Progress bar
         progress_bar::render_progress_bar(frame, areas.progress_bar, app, &self.theme);
diff --git a/src/ui/panes/lyrics_pane.rs b/src/ui/panes/lyrics_pane.rs
@@ -1,80 +0,0 @@
-use crossterm::event::{KeyCode, KeyEvent, MouseEvent, MouseEventKind};
-use ratatui::layout::Rect;
-use ratatui::style::Style;
-use ratatui::widgets::{Block, Borders};
-use ratatui::Frame;
-
-use crate::app::{App, AppAction};
-use crate::ui::pane::Pane;
-use crate::ui::theme::Theme;
-use crate::ui::widgets::info_pane;
-
-pub struct LyricsPane {
-    pub scroll_offset: u16,
-}
-
-impl LyricsPane {
-    pub fn new() -> Self {
-        Self { scroll_offset: 0 }
-    }
-}
-
-impl Pane for LyricsPane {
-    fn render(&mut self, frame: &mut Frame, area: Rect, focused: bool, app: &App, theme: &Theme) {
-        let border_color = if focused {
-            theme.border_focused
-        } else {
-            theme.border_unfocused
-        };
-
-        let block = Block::default()
-            .borders(Borders::ALL)
-            .border_style(Style::default().fg(border_color));
-
-        let inner = block.inner(area);
-        frame.render_widget(block, area);
-
-        info_pane::render_track_info(frame, inner, app, theme);
-    }
-
-    fn handle_key(&mut self, key: KeyEvent, _app: &App) -> Option<AppAction> {
-        match key.code {
-            KeyCode::Char('j') | KeyCode::Down => {
-                self.scroll_offset = self.scroll_offset.saturating_add(1);
-                None
-            }
-            KeyCode::Char('k') | KeyCode::Up => {
-                self.scroll_offset = self.scroll_offset.saturating_sub(1);
-                None
-            }
-            KeyCode::Home | KeyCode::Char('g') => {
-                self.scroll_offset = 0;
-                None
-            }
-            _ => None,
-        }
-    }
-
-    fn handle_mouse(&mut self, event: MouseEvent, _area: Rect, _app: &App) -> Option<AppAction> {
-        match event.kind {
-            MouseEventKind::ScrollDown => {
-                self.scroll_offset = self.scroll_offset.saturating_add(3);
-                None
-            }
-            MouseEventKind::ScrollUp => {
-                self.scroll_offset = self.scroll_offset.saturating_sub(3);
-                None
-            }
-            _ => None,
-        }
-    }
-
-    fn handle_scroll(&mut self, up: bool, _app: &App) -> Option<AppAction> {
-        if up {
-            self.scroll_offset = self.scroll_offset.saturating_sub(3);
-        } else {
-            self.scroll_offset = self.scroll_offset.saturating_add(3);
-        }
-        None
-    }
-}
diff --git a/src/ui/panes/mod.rs b/src/ui/panes/mod.rs
@@ -2,4 +2,4 @@ pub mod library_pane;
 pub mod dir_browser_pane;
 pub mod queue_pane;
 pub mod list_pane;
-pub mod lyrics_pane;
+pub mod track_info_pane;
diff --git a/src/ui/panes/track_info_pane.rs b/src/ui/panes/track_info_pane.rs
@@ -0,0 +1,82 @@
+use crossterm::event::{KeyCode, KeyEvent, MouseEvent, MouseEventKind};
+use ratatui::layout::Rect;
+use ratatui::style::Style;
+use ratatui::widgets::{Block, Borders};
+use ratatui::Frame;
+
+use crate::app::{App, AppAction};
+use crate::ui::pane::Pane;
+use crate::ui::theme::Theme;
+use crate::ui::widgets::info_pane;
+
+/// The bottom-right pane. It renders track metadata, not lyrics — the name is
+/// left over from a feature that was removed.
+pub struct TrackInfoPane {
+    pub scroll_offset: u16,
+}
+
+impl TrackInfoPane {
+    pub fn new() -> Self {
+        Self { scroll_offset: 0 }
+    }
+}
+
+impl Pane for TrackInfoPane {
+    fn render(&mut self, frame: &mut Frame, area: Rect, focused: bool, app: &App, theme: &Theme) {
+        let border_color = if focused {
+            theme.border_focused
+        } else {
+            theme.border_unfocused
+        };
+
+        let block = Block::default()
+            .borders(Borders::ALL)
+            .border_style(Style::default().fg(border_color));
+
+        let inner = block.inner(area);
+        frame.render_widget(block, area);
+
+        info_pane::render_track_info(frame, inner, app, theme, self.scroll_offset);
+    }
+
+    fn handle_key(&mut self, key: KeyEvent, _app: &App) -> Option<AppAction> {
+        match key.code {
+            KeyCode::Char('j') | KeyCode::Down => {
+                self.scroll_offset = self.scroll_offset.saturating_add(1);
+                None
+            }
+            KeyCode::Char('k') | KeyCode::Up => {
+                self.scroll_offset = self.scroll_offset.saturating_sub(1);
+                None
+            }
+            KeyCode::Home | KeyCode::Char('g') => {
+                self.scroll_offset = 0;
+                None
+            }
+            _ => None,
+        }
+    }
+
+    fn handle_mouse(&mut self, event: MouseEvent, _area: Rect, _app: &App) -> Option<AppAction> {
+        match event.kind {
+            MouseEventKind::ScrollDown => {
+                self.scroll_offset = self.scroll_offset.saturating_add(3);
+                None
+            }
+            MouseEventKind::ScrollUp => {
+                self.scroll_offset = self.scroll_offset.saturating_sub(3);
+                None
+            }
+            _ => None,
+        }
+    }
+
+    fn handle_scroll(&mut self, up: bool, _app: &App) -> Option<AppAction> {
+        if up {
+            self.scroll_offset = self.scroll_offset.saturating_sub(3);
+        } else {
+            self.scroll_offset = self.scroll_offset.saturating_add(3);
+        }
+        None
+    }
+}
diff --git a/src/ui/widgets/help_modal.rs b/src/ui/widgets/help_modal.rs
@@ -1,39 +1,51 @@
-use ratatui::layout::Rect;
+use ratatui::layout::{Constraint, Direction, Layout, Rect};
 use ratatui::style::{Color, Modifier, Style};
 use ratatui::text::{Line, Span};
 use ratatui::widgets::{Block, Borders, Clear, Paragraph};
 use ratatui::Frame;
 
-use crate::ui::theme::Theme;
 use crate::ui::text::centered_rect;
+use crate::ui::theme::Theme;
 
-const KEYBINDINGS: &[(&str, &str)] = &[
-    ("Ctrl+E, s", "Search"),
-    ("Ctrl+E, h", "Help (this modal)"),
-    ("Ctrl+E, r", "Resize mode"),
-    ("Ctrl+E, i", "About OMMP"),
-    ("Ctrl+E, l", "Sync library"),
-    ("", ""),
+/// Left column: playback and global commands.
+const PLAYBACK: &[(&str, &str)] = &[
     ("Space", "Play / Pause"),
     ("n / N", "Next / Previous track"),
     ("+ / -", "Volume up / down"),
-    ("\u{2192} / \u{2190}", "Seek forward / backward"),
+    ("\u{2192} / \u{2190}", "Seek forward / back"),
     ("s", "Toggle shuffle"),
     ("r", "Cycle repeat mode"),
     ("b", "Add to playlist"),
+    ("p", "Info panel: clock / art"),
     ("", ""),
+    ("Ctrl+E, s", "Search"),
+    ("Ctrl+E, h", "Help (this modal)"),
+    ("Ctrl+E, r", "Resize mode"),
+    ("Ctrl+E, i", "About OMMP"),
+    ("Ctrl+E, l", "Sync library"),
+];
+
+/// Right column: navigation and the queue.
+const NAVIGATION: &[(&str, &str)] = &[
     ("1-7", "Switch tab"),
     ("Tab / Shift+Tab", "Cycle pane focus"),
+    ("h / l", "Focus prev / next pane"),
     ("j / k", "Navigate list"),
     ("g / G", "Jump to first / last"),
     ("Enter", "Select / Activate"),
+    ("", ""),
     ("d", "Remove from queue"),
     ("c", "Clear queue"),
+    ("", ""),
+    ("Esc", "Close modal"),
     ("q", "Quit"),
+    ("Ctrl+C", "Quit"),
 ];
 
 pub fn render_help_modal(frame: &mut Frame, area: Rect, theme: &Theme) {
-    let modal = centered_rect(50, 70, area);
+    // Two columns: a single list of 27 entries does not fit an 80x24 terminal,
+    // and there is no scrolling — the tail, including `q: Quit`, was simply cut.
+    let modal = centered_rect(72, 70, area);
 
     frame.render_widget(Clear, modal);
 
@@ -46,27 +58,30 @@ pub fn render_help_modal(frame: &mut Frame, area: Rect, theme: &Theme) {
     let inner = block.inner(modal);
     frame.render_widget(block, modal);
 
-    let lines: Vec<Line> = KEYBINDINGS
+    let columns = Layout::default()
+        .direction(Direction::Horizontal)
+        .constraints([Constraint::Percentage(50), Constraint::Percentage(50)])
+        .split(inner);
+
+    for (bindings, area) in [(PLAYBACK, columns[0]), (NAVIGATION, columns[1])] {
+        frame.render_widget(Paragraph::new(render_column(bindings, theme)), area);
+    }
+}
+
+fn render_column(bindings: &[(&str, &str)], theme: &Theme) -> Vec<Line<'static>> {
+    bindings
         .iter()
         .map(|(key, desc)| {
             if key.is_empty() {
-                Line::from("")
-            } else {
-                Line::from(vec![
-                    Span::styled(
-                        format!("  {:20}", key),
-                        Style::default().fg(Color::Yellow).add_modifier(Modifier::BOLD),
-                    ),
-                    Span::styled(
-                        desc.to_string(),
-                        Style::default().fg(theme.fg),
-                    ),
-                ])
+                return Line::from("");
             }
+            Line::from(vec![
+                Span::styled(
+                    format!("  {:16}", key),
+                    Style::default().fg(Color::Yellow).add_modifier(Modifier::BOLD),
+                ),
+                Span::styled(desc.to_string(), Style::default().fg(theme.fg)),
+            ])
         })
-        .collect();
-
-    let help_text = Paragraph::new(lines);
-    frame.render_widget(help_text, inner);
+        .collect()
 }
-
diff --git a/src/ui/widgets/info_pane.rs b/src/ui/widgets/info_pane.rs
@@ -264,7 +264,7 @@ fn render_album_art(frame: &mut Frame, area: Rect, app: &App, cache: &mut AlbumA
 
 // ── Track Info View ──────────────────────────────────────────────────────
 
-pub fn render_track_info(frame: &mut Frame, area: Rect, app: &App, theme: &Theme) {
+pub fn render_track_info(frame: &mut Frame, area: Rect, app: &App, theme: &Theme, scroll: u16) {
     let track = match app.current_track() {
         Some(t) => t,
         None => {
@@ -358,7 +358,13 @@ pub fn render_track_info(frame: &mut Frame, area: Rect, app: &App, theme: &Theme
         })
         .collect();
 
-    let para = Paragraph::new(lines).wrap(Wrap { trim: false });
+    // Honour the pane's scroll offset. It was maintained by seven handlers and
+    // never read, so j/k did nothing while the field list — now 13 rows — did
+    // not fit the pane's default height.
+    let max_scroll = (lines.len() as u16).saturating_sub(area.height);
+    let para = Paragraph::new(lines)
+        .wrap(Wrap { trim: false })
+        .scroll((scroll.min(max_scroll), 0));
     frame.render_widget(para, area);
 }
 
diff --git a/src/ui/widgets/playlist_modal.rs b/src/ui/widgets/playlist_modal.rs
@@ -96,10 +96,19 @@ pub fn render_playlist_modal(
             let current_track_idx = app.queue.current_index
                 .and_then(|qi| app.queue.tracks.get(qi).copied());
 
+            // Scroll the selection into view. Without this the list was drawn
+            // from the top and simply clipped, so on a short terminal `j` walked
+            // the highlight off the bottom and Enter toggled a playlist the user
+            // could not see.
+            let viewport = (inner.height as usize).saturating_sub(1).max(1);
+            let scroll = selected.saturating_sub(viewport.saturating_sub(1));
+
             let items: Vec<ListItem> = app
                 .playlists
                 .iter()
                 .enumerate()
+                .skip(scroll)
+                .take(viewport)
                 .map(|(i, pl)| {
                     let is_selected = i == selected;
                     let already_in = current_track_idx
diff --git a/src/ui/widgets/search_modal.rs b/src/ui/widgets/search_modal.rs
@@ -60,8 +60,16 @@ pub fn render_search_modal(
         chunks[1],
     );
 
-    // Results list
-    let result_height = chunks[2].height as usize;
+    // Results list. `title_bottom` on the list's block reserves a row even
+    // though the block is borderless, so one fewer result is drawn than
+    // chunks[2].height — reporting the full height back made keyboard
+    // auto-scroll stop one short and mapped clicks on the count footer to a row
+    // that isn't there.
+    let result_height = (chunks[2].height as usize).saturating_sub(1);
+    let result_area = Rect {
+        height: result_height as u16,
+        ..chunks[2]
+    };
     let result_width = chunks[2].width as usize;
 
     if results.is_empty() {
@@ -148,6 +156,6 @@ pub fn render_search_modal(
         }
     }
 
-    (result_height, chunks[2])
+    (result_height, result_area)
 }
 
diff --git a/src/ui/widgets/tab_bar.rs b/src/ui/widgets/tab_bar.rs
@@ -21,13 +21,16 @@ pub fn render_tab_bar(
     let inner = block.inner(area);
     frame.render_widget(block, area);
 
-    // Each tab has its own accent color
-    let tab_colors: [Color; 6] = [
+    // One accent colour per tab. This was `[Color; 6]` against seven tabs, so
+    // the modulo wrapped and Playlists rendered in Queue's sky blue — and every
+    // comment from index 5 named the wrong tab.
+    let tab_colors: [Color; Tab::ALL.len()] = [
         Color::Rgb(100, 220, 255), // Queue - sky blue
         Color::Rgb(120, 255, 180), // Directories - mint green
         Color::Rgb(255, 180, 100), // Artists - orange
         Color::Rgb(200, 130, 255), // Albums - purple
         Color::Rgb(255, 120, 150), // Genre - pink
+        Color::Rgb(120, 200, 255), // Format - steel blue
         Color::Rgb(255, 220, 100), // Playlists - gold
     ];
 
@@ -36,7 +39,7 @@ pub fn render_tab_bar(
         if i > 0 {
             spans.push(Span::styled(" \u{2502} ", theme.dim_style));
         }
-        let color = tab_colors[i % tab_colors.len()];
+        let color = tab_colors[i];
         let style = if i == current.index() {
             Style::default().fg(color).add_modifier(Modifier::BOLD)
         } else if hovered == Some(i) {
@@ -52,12 +55,19 @@ pub fn render_tab_bar(
     frame.render_widget(paragraph, inner);
 }
 
-/// Returns which tab index was clicked given mouse x position
-pub fn tab_hit_test(area: Rect, mouse_x: u16) -> Option<usize> {
+/// Which tab, if any, sits under the pointer.
+///
+/// `mouse_y` matters: only the x range was checked, so clicking the tab bar's
+/// top or bottom border switched tabs.
+pub fn tab_hit_test(area: Rect, mouse_x: u16, mouse_y: u16) -> Option<usize> {
     let block = Block::default().borders(Borders::ALL);
     let inner = block.inner(area);
 
-    if mouse_x < inner.x || mouse_x >= inner.x + inner.width {
+    if mouse_x < inner.x
+        || mouse_x >= inner.x + inner.width
+        || mouse_y < inner.y
+        || mouse_y >= inner.y + inner.height
+    {
         return None;
     }
 
@@ -87,6 +97,11 @@ pub fn tab_hit_test(area: Rect, mouse_x: u16) -> Option<usize> {
     let mut pos = 0;
     for (i, tab) in Tab::ALL.iter().enumerate() {
         if i > 0 {
+            // Skip the divider rather than folding it into the next tab: the
+            // three columns of " │ " used to select whichever tab followed.
+            if content_x < pos + divider_len {
+                return None;
+            }
             pos += divider_len;
         }
         let title_len = tab.title().len();
@@ -97,3 +112,40 @@ pub fn tab_hit_test(area: Rect, mouse_x: u16) -> Option<usize> {
     }
     None
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// Wide enough that the tab strip is not padded off-centre.
+    fn bar() -> Rect {
+        Rect::new(0, 0, 200, 3)
+    }
+
+    #[test]
+    fn borders_are_not_part_of_any_tab() {
+        // Row 0 and row 2 are the block's borders; row 1 is the content.
+        assert!(tab_hit_test(bar(), 100, 0).is_none());
+        assert!(tab_hit_test(bar(), 100, 2).is_none());
+        assert!(tab_hit_test(bar(), 100, 1).is_some());
+    }
+
+    #[test]
+    fn dividers_belong_to_neither_neighbour() {
+        let inner_x = 1;
+        let pad = (198 - total_content_width()) / 2;
+        let queue_len = Tab::ALL[0].title().len();
+        // The three divider columns sit right after "Queue".
+        for offset in 0..3 {
+            let x = inner_x + pad + queue_len + offset;
+            assert_eq!(tab_hit_test(bar(), x as u16, 1), None, "offset {offset}");
+        }
+        // The tab after the divider is still reachable.
+        let x = inner_x + pad + queue_len + 3;
+        assert_eq!(tab_hit_test(bar(), x as u16, 1), Some(1));
+    }
+
+    fn total_content_width() -> usize {
+        Tab::ALL.iter().map(|t| t.title().len()).sum::<usize>() + 3 * (Tab::ALL.len() - 1)
+    }
+}