feat(theme): add Matrix films inspired theme and improve theme_picker…#2129
feat(theme): add Matrix films inspired theme and improve theme_picker…#2129malsony wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Matrix' theme to the TUI, adding the necessary color constants, theme definitions, and integration logic across the configuration and palette modules. Additionally, the theme picker's navigation logic was refactored for conciseness and its tests were updated to be more dynamic. Review feedback highlighted a readability issue with low-contrast reasoning text in the new theme and suggested adjusting background colors to better preserve visual separation between UI components.
| if theme == ThemeId::Matrix { | ||
| Color::Rgb(0x00, 0x55, 0x00) // #005500 | ||
| } else { | ||
| ui.mode_plan | ||
| } |
There was a problem hiding this comment.
The hardcoded color #005500 for reasoning text in the Matrix theme has very low contrast against the dark background (#000A00), making it difficult to read. Reasoning text is a primary content element and should be easily legible. Using a brighter green, such as the theme's mode_goal color, would significantly improve readability while maintaining the theme's aesthetic.
| if theme == ThemeId::Matrix { | |
| Color::Rgb(0x00, 0x55, 0x00) // #005500 | |
| } else { | |
| ui.mode_plan | |
| } | |
| if theme == ThemeId::Matrix { | |
| ui.mode_goal | |
| } else { | |
| ui.mode_plan | |
| } |
There was a problem hiding this comment.
The reasoning text is not fading to background, actually.
| surface_bg: Color::Rgb(MATRIX_SURFACE_RGB.0, MATRIX_SURFACE_RGB.1, MATRIX_SURFACE_RGB.2), | ||
| panel_bg: Color::Rgb(MATRIX_SURFACE_RGB.0, MATRIX_SURFACE_RGB.1, MATRIX_SURFACE_RGB.2), | ||
| elevated_bg: Color::Rgb(MATRIX_ELEVATED_RGB.0, MATRIX_ELEVATED_RGB.1, MATRIX_ELEVATED_RGB.2), | ||
| composer_bg: Color::Rgb(MATRIX_SURFACE_RGB.0, MATRIX_SURFACE_RGB.1, MATRIX_SURFACE_RGB.2), | ||
| selection_bg: Color::Rgb(MATRIX_SELECTION_RGB.0, MATRIX_SELECTION_RGB.1, MATRIX_SELECTION_RGB.2), | ||
| header_bg: Color::Rgb(MATRIX_SURFACE_RGB.0, MATRIX_SURFACE_RGB.1, MATRIX_SURFACE_RGB.2), | ||
| footer_bg: Color::Rgb(MATRIX_SURFACE_RGB.0, MATRIX_SURFACE_RGB.1, MATRIX_SURFACE_RGB.2), |
There was a problem hiding this comment.
The MATRIX_UI_THEME uses the same color (MATRIX_SURFACE_RGB) for surface_bg, panel_bg, composer_bg, header_bg, and footer_bg. This eliminates the visual separation between the sidebar, composer, and main transcript area, which is a core part of the TUI's layout design. Consider using a slightly different shade (e.g., MATRIX_ELEVATED_RGB) for the panel and composer backgrounds to restore visual hierarchy.
… logic - Add Matrix films inspired color scheme - Refactor theme_picker to use SELECTABLE_THEMES for last-theme lookup instead of hard-coding
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub const MATRIX_ELEVATED_RGB: (u8, u8, u8) = (0, 51, 0); // #003300 | ||
| pub const MATRIX_SELECTION_RGB: (u8, u8, u8) = (0, 51, 0); // #003300 |
There was a problem hiding this comment.
MATRIX_SELECTION_RGB and MATRIX_ELEVATED_RGB share the same hex value #003300. Because selection_bg and elevated_bg resolve to identical colours, a highlighted row in any list is visually indistinguishable from an elevated-panel background. The user receives no feedback when navigating the theme picker (or any other list) under the Matrix theme.
| pub const MATRIX_ELEVATED_RGB: (u8, u8, u8) = (0, 51, 0); // #003300 | |
| pub const MATRIX_SELECTION_RGB: (u8, u8, u8) = (0, 51, 0); // #003300 | |
| pub const MATRIX_ELEVATED_RGB: (u8, u8, u8) = (0, 51, 0); // #003300 | |
| pub const MATRIX_SELECTION_RGB: (u8, u8, u8) = (0, 85, 0); // #005500 |
| fn move_up(&mut self) { | ||
| let len = SELECTABLE_THEMES.len(); | ||
| if len == 0 { | ||
| self.selected = 0; | ||
| } else if self.selected == 0 { | ||
| self.selected = len - 1; | ||
| } else { | ||
| self.selected -= 1; | ||
| } | ||
| self.selected = (self.selected + SELECTABLE_THEMES.len() - 1) % SELECTABLE_THEMES.len(); | ||
| } |
There was a problem hiding this comment.
move_up uses unsigned arithmetic: if SELECTABLE_THEMES were ever empty (len == 0), 0 + 0 - 1 would underflow a usize and the subsequent % 0 would panic. The old code guarded this explicitly.
| fn move_up(&mut self) { | |
| let len = SELECTABLE_THEMES.len(); | |
| if len == 0 { | |
| self.selected = 0; | |
| } else if self.selected == 0 { | |
| self.selected = len - 1; | |
| } else { | |
| self.selected -= 1; | |
| } | |
| self.selected = (self.selected + SELECTABLE_THEMES.len() - 1) % SELECTABLE_THEMES.len(); | |
| } | |
| fn move_up(&mut self) { | |
| let len = SELECTABLE_THEMES.len(); | |
| if len > 0 { | |
| self.selected = (self.selected + len - 1) % len; | |
| } | |
| } |
| @@ -2060,4 +2125,4 @@ mod tests { | |||
| let _ = ColorDepth::detect(); | |||
There was a problem hiding this comment.
cargo fmt typically enforces a trailing newline; the absence here (also visible in config_ui.rs) will cause the cargo fmt --all -- --check step in the checklist to fail.
There was a problem hiding this comment.
Oops, I thought I added a newline once, maybe it was "washed away" when I was trying to build and update... for several times...
|
Independent review (Devin): Matrix theme successfully implements the classic green-on-black terminal aesthetic with faithful color choices (#88FF88 for primary text, #000A00 surface). The theme picker improvements are independently valuable - better navigation logic and dynamic test coverage that adapts to the growing theme list. Hard conflicts expected with sibling theme PRs (#2267 Claude, #2270 Solarized Light, #2276 terminal-transparent) as all append to SELECTABLE_THEMES. Will need coordinated merge order or conflict resolution when landing multiple themes together. Note: Will conflict with v0.8.48 PR #2256 if it touches palette.rs. |
|
Theme cluster coordination — this PR is one of 4 in flight that all add to
All 4 hard-conflict at the Cross-pinging the cluster: @AiurArtanis @HUQIANTAO @Hmbown — same coordination note on each PR. |
2 participants
… logic
Add Matrix films inspired color scheme
Refactor theme_picker to use SELECTABLE_THEMES for last-theme lookup instead of hard-coding
Summary
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR adds a Matrix-films-inspired green-on-black theme and refactors the
ThemePickerViewnavigation to derive the last-theme index fromSELECTABLE_THEMESrather than hard-codingThemeId::GruvboxDark.MATRIX_UI_THEMEconstant and all supportingMATRIX_*_RGBpalette entries are added topalette.rs;ThemeId::Matrixis registered in every relevant match arm, theSELECTABLE_THEMESslice, andnormalize_theme_name(with the alias\"hacker\").move_up/move_downare collapsed to single-expression modular arithmetic, the navigation test is updated to resolve the last entry dynamically, and a#[allow(clippy::too_many_arguments)]suppression is added tohandle_view_eventsinui.rs.Confidence Score: 4/5
Safe to merge as a feature addition, but the Matrix theme ships with palette values that make parts of the UI invisible or indistinguishable.
The theme registration, picker refactor, and enum/match-arm additions are mechanically correct. The concern is in the Matrix palette itself: selection_bg and elevated_bg share the same colour (#003300) so row highlighting in any list is invisible, and several text-role colours sit far below readable contrast thresholds on the #000A00 surface.
crates/tui/src/palette.rs — the MATRIX_UI_THEME colour assignments need a second pass for contrast and role distinctiveness before the theme is production-ready.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User opens /theme picker\nThemePickerView::new] --> B{original_name in\nSELECTABLE_THEMES?} B -- yes --> C[selected = matching index] B -- no --> D[selected = 0 / System] C & D --> E[Render modal\nwith live preview] E --> F{Keypress} F -- Up/k --> G["move_up()\n(selected + len - 1) % len"] F -- Down/j --> H["move_down()\n(selected + 1) % len"] F -- 1-9 --> I[Jump to index n-1\nif in bounds] F -- Home --> J[selected = 0] F -- End --> K[selected = len - 1] G & H & I & J & K --> L[Emit ConfigUpdated\npersist: false\nlive theme swap] L --> E F -- Enter --> M[EmitAndClose\npersist: true\nsave to settings.toml] F -- Esc --> N[EmitAndClose\npersist: false\nrestore original_name]Reviews (2): Last reviewed commit: "Merge branch 'Hmbown:main' into ui/theme" | Re-trigger Greptile