Fix theme config, command line, search, and sorting regressions#42
Merged
Conversation
…h ASCII fallback - Move theme ColorPalette from static RwLock into AppState.theme_colors - Thread &ColorPalette through all render functions (panels, dialogs, viewer, etc.) - Add IconTheme enum (Emoji/Ascii/NerdFont) with serde config support - Add ASCII fallback icons for each file category - Update all 50+ Theme::*() accessors to take &ColorPalette parameter
- Remove dead needs_watcher_sync field (set but never read) - Reset unfiltered_dirty=false after refresh_panel/full_refresh_panel - Preserve non-UTF-8 config paths via to_string_lossy() - Parse $EDITOR with shlex::split for quoted-arg support - Switch watcher upsert from O(n) .find() to O(1) path_index.get() - Replace BufRead::split with read_until + reusable buffer in content search - Cache natsort_key result in FileEntry to avoid per-sort recomputation - Add FileEntry::size() docstring - Wire Alt+X binding + menu entry to open empty command line
Reviewer's GuideRefactors theme handling to remove global state and introduce palette-aware rendering APIs, adds configurable icon themes (Emoji/ASCII/Nerd Font) with robust config parsing, fixes several regressions around path persistence, search content line numbering, natural sort stability, watcher sync, and command-line/menu behavior, and wires theme colors and icon theme through the rendering pipeline. Sequence diagram for corrected content search line numberingsequenceDiagram
participant FS as FileSearch
participant BR as BufReader
participant OUT as SearchOutcome
FS->>BR: new(file)
FS->>FS: line_no = 0
FS->>FS: line_buf.clear()
loop read_until EOF
FS->>BR: read_until('\n', line_buf)
alt bytes_read == 0
BR-->>FS: Ok(0)
FS->>FS: break
else bytes_read > 0
BR-->>FS: Ok(bytes_read)
FS->>FS: line_no += 1
FS->>FS: slice line (trim trailing '\n')
alt [matches.len() >= MAX_CONTENT_RESULTS]
FS->>OUT: set truncated = ContentResultLimit
FS->>FS: return
else [line contains NUL]
FS->>OUT: set truncated = BinaryFile
FS->>FS: return
else [line.len() > MAX_CONTENT_LINE_BYTES]
FS->>OUT: set truncated = LineTooLong (if None)
FS->>FS: continue (skip long line, keep line_no)
else [UTF-8 ok]
FS->>FS: line_text = from_utf8(line)
alt [pattern matches]
FS->>OUT: matches.push((path, line_no, line_text))
end
end
else read error
BR-->>FS: Err(err)
FS->>OUT: errors.push(format!("Failed to read"))
FS->>FS: return
end
FS->>FS: line_buf.clear()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logic for entering command-line mode is duplicated between
handle_alt_keys(Alt+X) andMenuAction::CommandLine; consider extracting a small helper (e.g.,AppState::enter_command_line_mode()) to centralize the state resets forcommand_line,command_cursor,history_index, andprev_mode. - Now that
ColorPaletteis carried inAppState::theme_colors, the staticTheme::icon_theme()and other default-based helpers still read fromDEFAULT_COLORS; if any new code starts using these static getters it will silently ignore the active theme, so it may be safer either to route them throughAppState::theme_colorsor clearly limit their visibility/usage to avoid future confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for entering command-line mode is duplicated between `handle_alt_keys` (Alt+X) and `MenuAction::CommandLine`; consider extracting a small helper (e.g., `AppState::enter_command_line_mode()`) to centralize the state resets for `command_line`, `command_cursor`, `history_index`, and `prev_mode`.
- Now that `ColorPalette` is carried in `AppState::theme_colors`, the static `Theme::icon_theme()` and other default-based helpers still read from `DEFAULT_COLORS`; if any new code starts using these static getters it will silently ignore the active theme, so it may be safer either to route them through `AppState::theme_colors` or clearly limit their visibility/usage to avoid future confusion.
## Individual Comments
### Comment 1
<location path="src/ui/theme.rs" line_range="405-406" />
<code_context>
+ Ok(())
+ }
+
+ pub fn icon_theme() -> IconTheme {
+ DEFAULT_COLORS.icon_theme
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `Theme::icon_theme()` ignores the runtime palette and always returns the compile-time default.
Other new APIs take a `&ColorPalette` so they can use `state.theme_colors`. This one reads `DEFAULT_COLORS` instead, so it will never reflect user configuration. If this is meant to expose the active icon theme, it should probably take a `&ColorPalette` (like the other `*_with_colors` helpers) or be removed to avoid misuse.
Suggested implementation:
```rust
Ok(())
}
pub fn icon_theme(colors: &ColorPalette) -> IconTheme {
colors.icon_theme
}
```
1. Update all call sites of `Theme::icon_theme()` to pass a `&ColorPalette`, e.g. `Theme::icon_theme(&state.theme_colors)` or whatever palette is appropriate in that context.
2. If `ColorPalette` does not currently expose `icon_theme` as a public field or method, you will need to:
- Either make the `icon_theme` field public (`pub icon_theme: IconTheme`), or
- Add a method `impl ColorPalette { pub fn icon_theme(&self) -> IconTheme { self.icon_theme } }` and then change the body above to `colors.icon_theme()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 refactors the theme system to use instance-based ColorPalette management instead of global state, introduces IconTheme support (Emoji, ASCII, NerdFont), and adds a command-line mode triggered by Alt+X. It also integrates the shlex crate for safer command splitting and optimizes directory watcher updates using a path index. Review feedback identifies a potential panic in launch_editor when slicing empty command vectors and points out that apply_from_value currently discards its results. Further suggestions include hardening IconTheme deserialization against non-string types and addressing misleading static theme accessors that now only return default values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix multiple regressions around theming, search, sorting, configuration persistence, and command-line activation while introducing a more explicit theme palette API and icon theme support.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: