file cleanup
This commit is contained in:
Zacharias-Brohn
@@ -1,359 +0,0 @@
|
||||
# Atlas Texture Refactor: Array of Textures
|
||||
|
||||
## Problem
|
||||
|
||||
When adding a new layer to the glyph atlas, the current implementation creates a new texture array with N+1 layers and copies all N existing layers to it. This causes performance issues that scale with the number of layers:
|
||||
|
||||
- Layer 1→2: Copy 256MB (8192×8192×4 bytes)
|
||||
- Layer 2→3: Copy 512MB
|
||||
- Layer 3→4: Copy 768MB
|
||||
- etc.
|
||||
|
||||
Observed frame times when adding layers:
|
||||
- Layer 1 added: 14.4ms
|
||||
- Layer 2 added: 21.9ms
|
||||
- Layer 3 added: 34.2ms
|
||||
|
||||
## Solution
|
||||
|
||||
Instead of using a single `texture_2d_array` that must be reallocated and copied when growing, use a **`Vec` of separate 2D textures**. When a new layer is needed, simply create a new texture and add it to the vector. No copying of existing texture data is required.
|
||||
|
||||
The bind group must be recreated to include the new texture, but this is a cheap CPU-side operation (just creating metadata/pointers).
|
||||
|
||||
## Current Implementation
|
||||
|
||||
### Rust (renderer.rs)
|
||||
|
||||
**Struct fields:**
|
||||
```rust
|
||||
atlas_texture: wgpu::Texture, // Single texture array
|
||||
atlas_view: wgpu::TextureView, // Single view
|
||||
atlas_num_layers: u32, // Number of layers in the array
|
||||
atlas_current_layer: u32, // Current layer being written to
|
||||
```
|
||||
|
||||
**Bind group layout (binding 0):**
|
||||
```rust
|
||||
ty: wgpu::BindingType::Texture {
|
||||
view_dimension: wgpu::TextureViewDimension::D2Array,
|
||||
// ...
|
||||
},
|
||||
count: None, // Single texture
|
||||
```
|
||||
|
||||
**Bind group entry:**
|
||||
```rust
|
||||
wgpu::BindGroupEntry {
|
||||
binding: 0,
|
||||
resource: wgpu::BindingResource::TextureView(&atlas_view),
|
||||
}
|
||||
```
|
||||
|
||||
### WGSL (glyph_shader.wgsl)
|
||||
|
||||
**Texture declaration:**
|
||||
```wgsl
|
||||
@group(0) @binding(0)
|
||||
var atlas_texture: texture_2d_array<f32>;
|
||||
```
|
||||
|
||||
**Sampling:**
|
||||
```wgsl
|
||||
let sample = textureSample(atlas_texture, atlas_sampler, uv, layer_index);
|
||||
```
|
||||
|
||||
## New Implementation
|
||||
|
||||
### Rust (renderer.rs)
|
||||
|
||||
**Struct fields:**
|
||||
```rust
|
||||
atlas_textures: Vec<wgpu::Texture>, // Vector of separate textures
|
||||
atlas_views: Vec<wgpu::TextureView>, // Vector of views (one per texture)
|
||||
atlas_current_layer: u32, // Current layer being written to
|
||||
// atlas_num_layers removed - use atlas_textures.len() instead
|
||||
```
|
||||
|
||||
**Bind group layout (binding 0):**
|
||||
```rust
|
||||
ty: wgpu::BindingType::Texture {
|
||||
view_dimension: wgpu::TextureViewDimension::D2, // Changed from D2Array
|
||||
// ...
|
||||
},
|
||||
count: Some(NonZeroU32::new(MAX_ATLAS_LAYERS).unwrap()), // Array of textures
|
||||
```
|
||||
|
||||
**Bind group entry:**
|
||||
```rust
|
||||
wgpu::BindGroupEntry {
|
||||
binding: 0,
|
||||
resource: wgpu::BindingResource::TextureViewArray(&atlas_view_refs),
|
||||
}
|
||||
```
|
||||
|
||||
Where `atlas_view_refs` is a `Vec<&wgpu::TextureView>` containing references to all views.
|
||||
|
||||
**Note:** wgpu requires the bind group to have exactly `count` textures. We'll need to either:
|
||||
1. Pre-create dummy textures to fill unused slots, OR
|
||||
2. Recreate the bind group layout when adding textures (more complex)
|
||||
|
||||
Option 1 is simpler: create small 1x1 dummy textures for unused slots up to MAX_ATLAS_LAYERS.
|
||||
|
||||
### WGSL (glyph_shader.wgsl)
|
||||
|
||||
**Texture declaration:**
|
||||
```wgsl
|
||||
@group(0) @binding(0)
|
||||
var atlas_textures: binding_array<texture_2d<f32>>;
|
||||
```
|
||||
|
||||
**Sampling:**
|
||||
```wgsl
|
||||
let sample = textureSample(atlas_textures[layer_index], atlas_sampler, uv);
|
||||
```
|
||||
|
||||
**Note:** `binding_array` requires the `binding_array` feature in wgpu, which should be enabled by default on most backends.
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
### Step 1: Update Struct Fields
|
||||
|
||||
In `renderer.rs`, change:
|
||||
```rust
|
||||
// Old
|
||||
atlas_texture: wgpu::Texture,
|
||||
atlas_view: wgpu::TextureView,
|
||||
atlas_num_layers: u32,
|
||||
|
||||
// New
|
||||
atlas_textures: Vec<wgpu::Texture>,
|
||||
atlas_views: Vec<wgpu::TextureView>,
|
||||
```
|
||||
|
||||
### Step 2: Create Helper for New Atlas Layer
|
||||
|
||||
```rust
|
||||
fn create_atlas_layer(device: &wgpu::Device) -> (wgpu::Texture, wgpu::TextureView) {
|
||||
let texture = device.create_texture(&wgpu::TextureDescriptor {
|
||||
label: Some("Glyph Atlas Layer"),
|
||||
size: wgpu::Extent3d {
|
||||
width: ATLAS_SIZE,
|
||||
height: ATLAS_SIZE,
|
||||
depth_or_array_layers: 1,
|
||||
},
|
||||
mip_level_count: 1,
|
||||
sample_count: 1,
|
||||
dimension: wgpu::TextureDimension::D2,
|
||||
format: wgpu::TextureFormat::Rgba8UnormSrgb,
|
||||
usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST,
|
||||
view_formats: &[],
|
||||
});
|
||||
let view = texture.create_view(&wgpu::TextureViewDescriptor::default());
|
||||
(texture, view)
|
||||
}
|
||||
```
|
||||
|
||||
### Step 3: Update Bind Group Layout
|
||||
|
||||
```rust
|
||||
use std::num::NonZeroU32;
|
||||
|
||||
let glyph_bind_group_layout = device.create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor {
|
||||
label: Some("Glyph Bind Group Layout"),
|
||||
entries: &[
|
||||
wgpu::BindGroupLayoutEntry {
|
||||
binding: 0,
|
||||
visibility: wgpu::ShaderStages::FRAGMENT,
|
||||
ty: wgpu::BindingType::Texture {
|
||||
sample_type: wgpu::TextureSampleType::Float { filterable: false },
|
||||
view_dimension: wgpu::TextureViewDimension::D2,
|
||||
multisampled: false,
|
||||
},
|
||||
count: Some(NonZeroU32::new(MAX_ATLAS_LAYERS).unwrap()),
|
||||
},
|
||||
// ... sampler entry unchanged
|
||||
],
|
||||
});
|
||||
```
|
||||
|
||||
### Step 4: Initialize with Dummy Textures
|
||||
|
||||
At initialization, create one real texture and fill the rest with 1x1 dummy textures:
|
||||
|
||||
```rust
|
||||
fn create_dummy_texture(device: &wgpu::Device) -> (wgpu::Texture, wgpu::TextureView) {
|
||||
let texture = device.create_texture(&wgpu::TextureDescriptor {
|
||||
label: Some("Dummy Atlas Texture"),
|
||||
size: wgpu::Extent3d {
|
||||
width: 1,
|
||||
height: 1,
|
||||
depth_or_array_layers: 1,
|
||||
},
|
||||
mip_level_count: 1,
|
||||
sample_count: 1,
|
||||
dimension: wgpu::TextureDimension::D2,
|
||||
format: wgpu::TextureFormat::Rgba8UnormSrgb,
|
||||
usage: wgpu::TextureUsages::TEXTURE_BINDING,
|
||||
view_formats: &[],
|
||||
});
|
||||
let view = texture.create_view(&wgpu::TextureViewDescriptor::default());
|
||||
(texture, view)
|
||||
}
|
||||
|
||||
// In new():
|
||||
let mut atlas_textures = Vec::with_capacity(MAX_ATLAS_LAYERS as usize);
|
||||
let mut atlas_views = Vec::with_capacity(MAX_ATLAS_LAYERS as usize);
|
||||
|
||||
// First texture is real
|
||||
let (tex, view) = create_atlas_layer(&device);
|
||||
atlas_textures.push(tex);
|
||||
atlas_views.push(view);
|
||||
|
||||
// Fill rest with dummies
|
||||
for _ in 1..MAX_ATLAS_LAYERS {
|
||||
let (tex, view) = create_dummy_texture(&device);
|
||||
atlas_textures.push(tex);
|
||||
atlas_views.push(view);
|
||||
}
|
||||
```
|
||||
|
||||
### Step 5: Update Bind Group Creation
|
||||
|
||||
```rust
|
||||
fn create_atlas_bind_group(&self) -> wgpu::BindGroup {
|
||||
let view_refs: Vec<&wgpu::TextureView> = self.atlas_views.iter().collect();
|
||||
|
||||
self.device.create_bind_group(&wgpu::BindGroupDescriptor {
|
||||
label: Some("Glyph Bind Group"),
|
||||
layout: &self.glyph_bind_group_layout,
|
||||
entries: &[
|
||||
wgpu::BindGroupEntry {
|
||||
binding: 0,
|
||||
resource: wgpu::BindingResource::TextureViewArray(&view_refs),
|
||||
},
|
||||
wgpu::BindGroupEntry {
|
||||
binding: 1,
|
||||
resource: wgpu::BindingResource::Sampler(&self.atlas_sampler),
|
||||
},
|
||||
],
|
||||
})
|
||||
}
|
||||
```
|
||||
|
||||
### Step 6: Update add_atlas_layer / ensure_atlas_layer_capacity
|
||||
|
||||
```rust
|
||||
fn ensure_atlas_layer_capacity(&mut self, target_layer: u32) {
|
||||
// Count real layers (non-dummy)
|
||||
let current_real_layers = self.atlas_current_layer + 1;
|
||||
|
||||
if target_layer < current_real_layers {
|
||||
return; // Already have this layer
|
||||
}
|
||||
|
||||
if target_layer >= MAX_ATLAS_LAYERS {
|
||||
log::error!("Atlas layer limit reached");
|
||||
return;
|
||||
}
|
||||
|
||||
log::info!("Adding atlas layer {}", target_layer);
|
||||
|
||||
// Create new real texture
|
||||
let (texture, view) = Self::create_atlas_layer(&self.device);
|
||||
|
||||
// Replace dummy at this index with real texture
|
||||
self.atlas_textures[target_layer as usize] = texture;
|
||||
self.atlas_views[target_layer as usize] = view;
|
||||
|
||||
// Recreate bind group with updated view
|
||||
self.glyph_bind_group = self.create_atlas_bind_group();
|
||||
}
|
||||
```
|
||||
|
||||
### Step 7: Update upload_cell_canvas_to_atlas
|
||||
|
||||
Change texture reference from `&self.atlas_texture` to `&self.atlas_textures[layer as usize]`:
|
||||
|
||||
```rust
|
||||
self.queue.write_texture(
|
||||
wgpu::TexelCopyTextureInfo {
|
||||
texture: &self.atlas_textures[layer as usize], // Changed
|
||||
mip_level: 0,
|
||||
origin: wgpu::Origin3d {
|
||||
x: self.atlas_cursor_x,
|
||||
y: self.atlas_cursor_y,
|
||||
z: 0, // Always 0 now, layer is selected by texture index
|
||||
},
|
||||
aspect: wgpu::TextureAspect::All,
|
||||
},
|
||||
// ... rest unchanged
|
||||
);
|
||||
```
|
||||
|
||||
### Step 8: Update Shader
|
||||
|
||||
In `glyph_shader.wgsl`:
|
||||
|
||||
```wgsl
|
||||
// Old
|
||||
@group(0) @binding(0)
|
||||
var atlas_texture: texture_2d_array<f32>;
|
||||
|
||||
// New
|
||||
@group(0) @binding(0)
|
||||
var atlas_textures: binding_array<texture_2d<f32>>;
|
||||
```
|
||||
|
||||
Update all `textureSample` calls:
|
||||
|
||||
```wgsl
|
||||
// Old
|
||||
let sample = textureSample(atlas_texture, atlas_sampler, uv, layer_index);
|
||||
|
||||
// New
|
||||
let sample = textureSample(atlas_textures[layer_index], atlas_sampler, uv);
|
||||
```
|
||||
|
||||
**Locations to update in glyph_shader.wgsl:**
|
||||
- Line 91: Declaration
|
||||
- Line 106: `fs_main` sampling
|
||||
- Line 700: Cursor sprite sampling in `fs_cell`
|
||||
- Line 723: Glyph sampling in `fs_cell`
|
||||
- Line 747: Underline sampling in `fs_cell`
|
||||
- Line 761: Strikethrough sampling in `fs_cell`
|
||||
|
||||
### Step 9: Update statusline_shader.wgsl
|
||||
|
||||
The statusline shader also uses the atlas. Check and update similarly.
|
||||
|
||||
### Step 10: Update Other References
|
||||
|
||||
Search for all uses of:
|
||||
- `atlas_texture`
|
||||
- `atlas_view`
|
||||
- `atlas_num_layers`
|
||||
- `D2Array`
|
||||
|
||||
And update accordingly.
|
||||
|
||||
## Testing
|
||||
|
||||
1. Build and run: `cargo build && cargo run`
|
||||
2. Verify glyphs render correctly
|
||||
3. Use terminal heavily to trigger layer additions
|
||||
4. Check logs for "Adding atlas layer" messages
|
||||
5. Verify no slow frame warnings during layer addition
|
||||
6. Test with emoji (color glyphs) to ensure they still work
|
||||
|
||||
## Rollback Plan
|
||||
|
||||
If issues arise, the changes can be reverted by:
|
||||
1. Restoring `texture_2d_array` in shaders
|
||||
2. Restoring single `atlas_texture`/`atlas_view` in Rust
|
||||
3. Restoring the copy-based layer addition
|
||||
|
||||
## References
|
||||
|
||||
- wgpu binding arrays: https://docs.rs/wgpu/latest/wgpu/enum.BindingResource.html#variant.TextureViewArray
|
||||
- WGSL binding_array: https://www.w3.org/TR/WGSL/#binding-array
|
||||
- Kitty's approach: `/tmp/kitty/kitty/shaders.c` (uses `GL_TEXTURE_2D_ARRAY` with `glCopyImageSubData`)
|
||||
-248
@@ -1,248 +0,0 @@
|
||||
# Config.rs Reorganization Analysis
|
||||
|
||||
This document analyzes `/src/config.rs` (438 lines) and identifies sections that could be extracted into separate files to improve code organization.
|
||||
|
||||
## Current Structure Overview
|
||||
|
||||
The file contains:
|
||||
- `TabBarPosition` enum (lines 10-21)
|
||||
- `Keybind` struct + parsing logic (lines 23-166)
|
||||
- `Action` enum (lines 168-206)
|
||||
- `Keybindings` struct + default impl (lines 208-321)
|
||||
- `Config` struct + load/save logic (lines 323-437)
|
||||
|
||||
---
|
||||
|
||||
## Recommended Extractions
|
||||
|
||||
### 1. Keybind Parsing Module
|
||||
|
||||
**Proposed file:** `src/keybind.rs`
|
||||
|
||||
**Lines:** 23-166 (144 lines)
|
||||
|
||||
**Types involved:**
|
||||
- `struct Keybind`
|
||||
- `impl Keybind` (including `parse()` and `normalize_key_name()`)
|
||||
|
||||
**Current code:**
|
||||
```rust
|
||||
pub struct Keybind(pub String);
|
||||
|
||||
impl Keybind {
|
||||
pub fn parse(&self) -> Option<(bool, bool, bool, bool, String)> { ... }
|
||||
fn normalize_key_name(name: &str) -> Option<&'static str> { ... }
|
||||
}
|
||||
```
|
||||
|
||||
**Dependencies needed:**
|
||||
- `serde::{Deserialize, Serialize}` (for derive macros)
|
||||
|
||||
**Why extract:**
|
||||
- Self-contained parsing logic with no dependencies on other config types
|
||||
- The `normalize_key_name` function is a substantial lookup table (70+ lines)
|
||||
- Could be tested independently with unit tests for key parsing edge cases
|
||||
- Reusable if keybinding logic is needed elsewhere (e.g., a config editor UI)
|
||||
|
||||
**Challenges:**
|
||||
- None significant. This is a clean extraction.
|
||||
|
||||
**After extraction, config.rs would:**
|
||||
```rust
|
||||
pub use keybind::Keybind;
|
||||
// or
|
||||
mod keybind;
|
||||
pub use keybind::Keybind;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. Actions Module
|
||||
|
||||
**Proposed file:** `src/action.rs`
|
||||
|
||||
**Lines:** 168-206 (39 lines)
|
||||
|
||||
**Types involved:**
|
||||
- `enum Action`
|
||||
|
||||
**Current code:**
|
||||
```rust
|
||||
pub enum Action {
|
||||
NewTab,
|
||||
NextTab,
|
||||
PrevTab,
|
||||
Tab1, Tab2, ... Tab9,
|
||||
SplitHorizontal,
|
||||
SplitVertical,
|
||||
ClosePane,
|
||||
FocusPaneUp, FocusPaneDown, FocusPaneLeft, FocusPaneRight,
|
||||
Copy,
|
||||
Paste,
|
||||
}
|
||||
```
|
||||
|
||||
**Dependencies needed:**
|
||||
- `serde::{Deserialize, Serialize}`
|
||||
|
||||
**Why extract:**
|
||||
- `Action` is a distinct concept used throughout the codebase (keyboard.rs likely references it)
|
||||
- Decouples the "what can be done" from "how it's configured"
|
||||
- Makes it easy to add new actions without touching config logic
|
||||
- Could be extended with action metadata (description, default keybind, etc.)
|
||||
|
||||
**Challenges:**
|
||||
- Small file on its own (39 lines). Could be bundled with `keybind.rs` as a combined `src/keybindings.rs` module.
|
||||
|
||||
**Alternative:** Combine with Keybindings into a single module (see option 3).
|
||||
|
||||
---
|
||||
|
||||
### 3. Combined Keybindings Module (Recommended)
|
||||
|
||||
**Proposed file:** `src/keybindings.rs`
|
||||
|
||||
**Lines:** 23-321 (299 lines)
|
||||
|
||||
**Types involved:**
|
||||
- `struct Keybind` + impl
|
||||
- `enum Action`
|
||||
- `struct Keybindings` + impl (including `Default` and `build_action_map`)
|
||||
|
||||
**Dependencies needed:**
|
||||
- `serde::{Deserialize, Serialize}`
|
||||
- `std::collections::HashMap`
|
||||
|
||||
**Why extract:**
|
||||
- These three types form a cohesive "keybindings subsystem"
|
||||
- `Keybindings::build_action_map()` ties together `Keybind`, `Action`, and `Keybindings`
|
||||
- Reduces config.rs from 438 lines to ~140 lines
|
||||
- Clear separation: config.rs handles general settings, keybindings.rs handles input mapping
|
||||
|
||||
**What stays in config.rs:**
|
||||
- `TabBarPosition` enum
|
||||
- `Config` struct with `keybindings: Keybindings` field
|
||||
- `Config::load()`, `Config::save()`, `Config::config_path()`
|
||||
|
||||
**After extraction, config.rs would:**
|
||||
```rust
|
||||
mod keybindings;
|
||||
pub use keybindings::{Action, Keybind, Keybindings};
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- Need to ensure `Keybindings` is re-exported for external use
|
||||
- The `Keybindings` struct is embedded in `Config`, so it must remain `pub`
|
||||
|
||||
---
|
||||
|
||||
### 4. TabBarPosition Enum
|
||||
|
||||
**Proposed file:** Could stay in `config.rs` or move to `src/ui.rs` / `src/types.rs`
|
||||
|
||||
**Lines:** 10-21 (12 lines)
|
||||
|
||||
**Types involved:**
|
||||
- `enum TabBarPosition`
|
||||
|
||||
**Why extract:**
|
||||
- Very small (12 lines) - extraction may be overkill
|
||||
- Could be grouped with other UI-related enums if more are added in the future
|
||||
|
||||
**Recommendation:** Keep in `config.rs` for now. Only extract if you add more UI-related configuration enums (e.g., `CursorStyle`, `ScrollbarPosition`, etc.).
|
||||
|
||||
---
|
||||
|
||||
## Recommended Module Structure
|
||||
|
||||
```
|
||||
src/
|
||||
config.rs # Config struct, load/save, TabBarPosition (~140 lines)
|
||||
keybindings.rs # Keybind, Action, Keybindings (~300 lines)
|
||||
# or alternatively:
|
||||
keybindings/
|
||||
mod.rs # Re-exports
|
||||
keybind.rs # Keybind struct + parsing
|
||||
action.rs # Action enum
|
||||
bindings.rs # Keybindings struct
|
||||
```
|
||||
|
||||
For a codebase of this size, the single `keybindings.rs` file is recommended over a subdirectory.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
### Step 1: Create `src/keybindings.rs`
|
||||
|
||||
1. Create new file `src/keybindings.rs`
|
||||
2. Move lines 23-321 from config.rs
|
||||
3. Add module header:
|
||||
```rust
|
||||
//! Keybinding types and parsing for ZTerm.
|
||||
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::collections::HashMap;
|
||||
```
|
||||
4. Ensure all types are `pub`
|
||||
|
||||
### Step 2: Update `src/config.rs`
|
||||
|
||||
1. Remove lines 23-321
|
||||
2. Add at the top (after the module doc comment):
|
||||
```rust
|
||||
mod keybindings;
|
||||
pub use keybindings::{Action, Keybind, Keybindings};
|
||||
```
|
||||
3. Keep existing imports that `Config` needs
|
||||
|
||||
### Step 3: Update `src/lib.rs` or `src/main.rs`
|
||||
|
||||
If `keybindings` types are used directly elsewhere, update the module declarations:
|
||||
```rust
|
||||
// In lib.rs, if keybindings needs to be public:
|
||||
pub mod keybindings;
|
||||
// Or re-export from config:
|
||||
pub use config::{Action, Keybind, Keybindings};
|
||||
```
|
||||
|
||||
### Step 4: Verify compilation
|
||||
|
||||
```bash
|
||||
cargo check
|
||||
cargo test
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary Table
|
||||
|
||||
| Section | Lines | New File | Priority | Effort |
|
||||
|---------|-------|----------|----------|--------|
|
||||
| Keybind + Action + Keybindings | 23-321 (299 lines) | `keybindings.rs` | **High** | Low |
|
||||
| TabBarPosition | 10-21 (12 lines) | Keep in config.rs | Low | N/A |
|
||||
| Config struct + impls | 323-437 (115 lines) | Keep in config.rs | N/A | N/A |
|
||||
|
||||
**Recommended action:** Extract the keybindings module as a single file. This provides the best balance of organization improvement vs. complexity.
|
||||
|
||||
---
|
||||
|
||||
## Benefits After Reorganization
|
||||
|
||||
1. **config.rs** becomes focused on:
|
||||
- Core configuration values (font, opacity, scrollback, etc.)
|
||||
- File I/O (load/save)
|
||||
- ~140 lines instead of 438
|
||||
|
||||
2. **keybindings.rs** becomes a focused module for:
|
||||
- Input parsing and normalization
|
||||
- Action definitions
|
||||
- Keybinding-to-action mapping
|
||||
- ~300 lines, highly cohesive
|
||||
|
||||
3. **Testing:** Keybinding parsing can be unit tested in isolation
|
||||
|
||||
4. **Future extensibility:**
|
||||
- Adding new actions: edit `keybindings.rs`
|
||||
- Adding new config options: edit `config.rs`
|
||||
- Clear separation of concerns
|
||||
-346
@@ -1,346 +0,0 @@
|
||||
# Main.rs Reorganization Plan
|
||||
|
||||
This document identifies sections of `src/main.rs` (2793 lines) that could be extracted into separate modules to improve code organization, maintainability, and testability.
|
||||
|
||||
## Summary of Proposed Extractions
|
||||
|
||||
| Module Name | Lines | Primary Contents |
|
||||
|-------------|-------|------------------|
|
||||
| `pty_buffer.rs` | ~180 | `SharedPtyBuffer`, `BufferState` |
|
||||
| `pane.rs` | ~400 | `PaneId`, `Pane`, `PaneGeometry`, `SplitNode` |
|
||||
| `tab.rs` | ~150 | `TabId`, `Tab` |
|
||||
| `selection.rs` | ~45 | `CellPosition`, `Selection` |
|
||||
| `statusline.rs` | ~220 | `GitStatus`, `build_cwd_section()`, `build_git_section()`, `get_git_status()` |
|
||||
| `instance.rs` | ~45 | PID file management, `signal_existing_instance()` |
|
||||
|
||||
---
|
||||
|
||||
## 1. PTY Buffer Module (`pty_buffer.rs`)
|
||||
|
||||
**Lines: 30-227 (~197 lines)**
|
||||
|
||||
### Contents
|
||||
- `PTY_BUF_SIZE` constant
|
||||
- `SharedPtyBuffer` struct and impl
|
||||
- `BufferState` struct
|
||||
- `unsafe impl Sync/Send for SharedPtyBuffer`
|
||||
|
||||
### Description
|
||||
This is a self-contained, zero-copy PTY I/O buffer implementation inspired by Kitty. It has no dependencies on other application-specific types and is purely concerned with efficient I/O buffering between an I/O thread and the main thread.
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
const PTY_BUF_SIZE: usize
|
||||
struct BufferState
|
||||
struct SharedPtyBuffer
|
||||
impl SharedPtyBuffer
|
||||
impl Drop for SharedPtyBuffer
|
||||
unsafe impl Sync for SharedPtyBuffer
|
||||
unsafe impl Send for SharedPtyBuffer
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- `std::cell::UnsafeCell`
|
||||
- `std::sync::Mutex`
|
||||
- `libc` (for `eventfd`, `read`, `write`, `close`)
|
||||
|
||||
### Challenges
|
||||
- **Unsafe code**: Contains `UnsafeCell` and raw pointer manipulation. Must carefully preserve safety invariants.
|
||||
- **libc dependency**: Uses Linux-specific `eventfd` syscalls.
|
||||
|
||||
### Recommendation
|
||||
**High priority extraction.** This is a well-documented, self-contained component with clear boundaries. Would benefit from its own unit tests for buffer operations.
|
||||
|
||||
---
|
||||
|
||||
## 2. Pane Module (`pane.rs`)
|
||||
|
||||
**Lines: 229-691 (~462 lines)**
|
||||
|
||||
### Contents
|
||||
- `PaneId` - unique identifier with atomic generation
|
||||
- `Pane` - terminal + PTY + selection state
|
||||
- `PaneGeometry` - pixel layout information
|
||||
- `SplitNode` - tree structure for split pane layouts
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
struct PaneId
|
||||
impl PaneId
|
||||
|
||||
struct Pane
|
||||
impl Pane
|
||||
- new()
|
||||
- resize()
|
||||
- write_to_pty()
|
||||
- child_exited()
|
||||
- foreground_matches()
|
||||
- calculate_dim_factor()
|
||||
|
||||
struct PaneGeometry
|
||||
|
||||
enum SplitNode
|
||||
impl SplitNode
|
||||
- leaf()
|
||||
- split()
|
||||
- layout()
|
||||
- find_geometry()
|
||||
- collect_geometries()
|
||||
- find_neighbor()
|
||||
- overlaps_horizontally()
|
||||
- overlaps_vertically()
|
||||
- remove_pane()
|
||||
- contains_pane()
|
||||
- split_pane()
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- `zterm::terminal::{Direction, Terminal, TerminalCommand, MouseTrackingMode}`
|
||||
- `zterm::pty::Pty`
|
||||
- `std::sync::Arc`
|
||||
- `std::os::fd::AsRawFd`
|
||||
- `SharedPtyBuffer` (from pty_buffer module)
|
||||
- `Selection` (from selection module)
|
||||
|
||||
### Challenges
|
||||
- **Cross-module dependencies**: `Pane` references `SharedPtyBuffer` and `Selection`, so those would need to be extracted first or extracted together.
|
||||
- **`SplitNode` complexity**: The recursive tree structure has complex layout logic. Consider keeping `SplitNode` with `Pane` since they're tightly coupled.
|
||||
|
||||
### Recommendation
|
||||
**High priority extraction.** The pane management is a distinct concern from the main application loop. This would make the split tree logic easier to test in isolation.
|
||||
|
||||
---
|
||||
|
||||
## 3. Tab Module (`tab.rs`)
|
||||
|
||||
**Lines: 693-872 (~180 lines)**
|
||||
|
||||
### Contents
|
||||
- `TabId` - unique identifier with atomic generation
|
||||
- `Tab` - collection of panes with a split tree
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
struct TabId
|
||||
impl TabId
|
||||
|
||||
struct Tab
|
||||
impl Tab
|
||||
- new()
|
||||
- active_pane() / active_pane_mut()
|
||||
- resize()
|
||||
- write_to_pty()
|
||||
- check_exited_panes()
|
||||
- split()
|
||||
- remove_pane()
|
||||
- close_active_pane()
|
||||
- focus_neighbor()
|
||||
- get_pane() / get_pane_mut()
|
||||
- collect_pane_geometries()
|
||||
- child_exited()
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- `std::collections::HashMap`
|
||||
- `PaneId`, `Pane`, `PaneGeometry`, `SplitNode` (from pane module)
|
||||
- `zterm::terminal::Direction`
|
||||
|
||||
### Challenges
|
||||
- **Tight coupling with Pane module**: Tab is essentially a container for panes. Consider combining into a single `pane.rs` module or using `pane/mod.rs` with submodules.
|
||||
|
||||
### Recommendation
|
||||
**Medium priority.** Could be combined with the pane module under `pane/mod.rs` with `pane/tab.rs` as a submodule, or kept as a separate `tab.rs`.
|
||||
|
||||
---
|
||||
|
||||
## 4. Selection Module (`selection.rs`)
|
||||
|
||||
**Lines: 1218-1259 (~42 lines)**
|
||||
|
||||
### Contents
|
||||
- `CellPosition` - column/row position
|
||||
- `Selection` - start/end positions for text selection
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
struct CellPosition
|
||||
struct Selection
|
||||
impl Selection
|
||||
- normalized()
|
||||
- to_screen_coords()
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- None (completely self-contained)
|
||||
|
||||
### Challenges
|
||||
- **Very small**: Only 42 lines. May be too small to justify its own file.
|
||||
|
||||
### Recommendation
|
||||
**Low priority as standalone.** Consider bundling with the pane module since selection is per-pane state, or creating a `types.rs` for small shared types.
|
||||
|
||||
---
|
||||
|
||||
## 5. Statusline Module (`statusline.rs`)
|
||||
|
||||
**Lines: 917-1216 (~300 lines)**
|
||||
|
||||
### Contents
|
||||
- `build_cwd_section()` - creates CWD statusline section
|
||||
- `GitStatus` struct - git repository state
|
||||
- `get_git_status()` - queries git for repo status
|
||||
- `build_git_section()` - creates git statusline section
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
fn build_cwd_section(cwd: &str) -> StatuslineSection
|
||||
struct GitStatus
|
||||
fn get_git_status(cwd: &str) -> Option<GitStatus>
|
||||
fn build_git_section(cwd: &str) -> Option<StatuslineSection>
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- `zterm::renderer::{StatuslineComponent, StatuslineSection}`
|
||||
- `std::process::Command` (for git commands)
|
||||
- `std::env` (for HOME variable)
|
||||
|
||||
### Challenges
|
||||
- **External process calls**: Uses `git` commands. Consider whether this should be async or cached.
|
||||
- **Powerline icons**: Uses hardcoded Unicode codepoints (Nerd Font icons).
|
||||
|
||||
### Recommendation
|
||||
**High priority extraction.** This is completely independent of the main application state. Would benefit from:
|
||||
- Caching git status (it's currently queried every frame)
|
||||
- Unit tests for path transformation logic
|
||||
- Potential async git queries
|
||||
|
||||
---
|
||||
|
||||
## 6. Instance Management Module (`instance.rs`)
|
||||
|
||||
**Lines: 874-915, 2780-2792 (~55 lines total)**
|
||||
|
||||
### Contents
|
||||
- PID file path management
|
||||
- Single-instance detection and signaling
|
||||
- Signal handler for SIGUSR1
|
||||
|
||||
### Types/Functions to Extract
|
||||
```rust
|
||||
fn pid_file_path() -> PathBuf
|
||||
fn signal_existing_instance() -> bool
|
||||
fn write_pid_file() -> std::io::Result<()>
|
||||
fn remove_pid_file()
|
||||
|
||||
// From end of file:
|
||||
static mut EVENT_PROXY: Option<EventLoopProxy<UserEvent>>
|
||||
extern "C" fn handle_sigusr1(_: i32)
|
||||
```
|
||||
|
||||
### Dependencies
|
||||
- `std::fs`
|
||||
- `std::path::PathBuf`
|
||||
- `libc` (for `kill` syscall)
|
||||
- `winit::event_loop::EventLoopProxy`
|
||||
- `UserEvent` enum
|
||||
|
||||
### Challenges
|
||||
- **Global static**: The `EVENT_PROXY` static is `unsafe` and tightly coupled to the signal handler.
|
||||
- **Split location**: The signal handler is at the end of the file, separate from PID functions.
|
||||
|
||||
### Recommendation
|
||||
**Medium priority.** Small but distinct concern. The global static handling could be cleaner in a dedicated module.
|
||||
|
||||
---
|
||||
|
||||
## 7. Additional Observations
|
||||
|
||||
### UserEvent Enum (Line 1262-1270)
|
||||
```rust
|
||||
enum UserEvent {
|
||||
ShowWindow,
|
||||
PtyReadable(PaneId),
|
||||
ConfigReloaded,
|
||||
}
|
||||
```
|
||||
This is a small enum but is referenced throughout. Consider placing in a `types.rs` or `events.rs` module.
|
||||
|
||||
### Config Watcher (Lines 2674-2735)
|
||||
The `setup_config_watcher()` function is self-contained and could go in the instance module or a dedicated `config_watcher.rs`.
|
||||
|
||||
### App Struct (Lines 1272-1334)
|
||||
The `App` struct and its impl are the core of the application and should remain in `main.rs`. However, some of its methods could potentially be split:
|
||||
- I/O thread management (lines 1418-1553)
|
||||
- Keyboard/keybinding handling (lines 1773-2221)
|
||||
- Mouse handling (scattered through `window_event`)
|
||||
|
||||
### Keyboard Handling
|
||||
Lines 1773-2221 contain significant keyboard handling logic. This could potentially be extracted, but it's tightly integrated with the `App` state.
|
||||
|
||||
---
|
||||
|
||||
## Suggested Module Structure
|
||||
|
||||
```
|
||||
src/
|
||||
main.rs (~1200 lines - App, ApplicationHandler, main())
|
||||
lib.rs (existing)
|
||||
pty_buffer.rs (new - ~200 lines)
|
||||
pane.rs (new - ~500 lines, includes SplitNode)
|
||||
tab.rs (new - ~180 lines)
|
||||
selection.rs (new - ~45 lines, or merge with pane.rs)
|
||||
statusline.rs (new - ~300 lines)
|
||||
instance.rs (new - ~60 lines)
|
||||
config.rs (existing)
|
||||
...
|
||||
```
|
||||
|
||||
Alternative with submodules:
|
||||
```
|
||||
src/
|
||||
main.rs
|
||||
lib.rs
|
||||
pane/
|
||||
mod.rs (re-exports)
|
||||
pane.rs (Pane, PaneId)
|
||||
split.rs (SplitNode, PaneGeometry)
|
||||
selection.rs (Selection, CellPosition)
|
||||
tab.rs
|
||||
pty_buffer.rs
|
||||
statusline.rs
|
||||
instance.rs
|
||||
...
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Implementation Order
|
||||
|
||||
1. **`pty_buffer.rs`** - No internal dependencies, completely self-contained
|
||||
2. **`selection.rs`** - No dependencies, simple extraction
|
||||
3. **`statusline.rs`** - No internal dependencies, high value for testability
|
||||
4. **`instance.rs`** - Small, isolated concern
|
||||
5. **`pane.rs`** - Depends on pty_buffer and selection
|
||||
6. **`tab.rs`** - Depends on pane
|
||||
|
||||
---
|
||||
|
||||
## Testing Opportunities
|
||||
|
||||
After extraction, these modules would benefit from unit tests:
|
||||
|
||||
| Module | Testable Functionality |
|
||||
|--------|----------------------|
|
||||
| `pty_buffer` | Buffer overflow handling, space checking, wakeup signaling |
|
||||
| `selection` | `normalized()` ordering, `to_screen_coords()` boundary conditions |
|
||||
| `statusline` | Path normalization (~/ replacement), git status parsing |
|
||||
| `pane` / `SplitNode` | Layout calculations, neighbor finding, tree operations |
|
||||
| `instance` | PID file creation/cleanup (integration test) |
|
||||
|
||||
---
|
||||
|
||||
## Notes on Maintaining Backward Compatibility
|
||||
|
||||
All extracted types should be re-exported from `lib.rs` or a prelude if they're used externally. The current architecture appears to be internal to the binary, so this is likely not a concern.
|
||||
|
||||
The `Pane` and `Tab` types are not part of the public API (defined in `main.rs`), so extraction won't affect external consumers.
|
||||
-220
@@ -1,220 +0,0 @@
|
||||
# Code Reorganization Analysis
|
||||
|
||||
This document identifies sections of code that could be moved into separate files to improve code organization.
|
||||
|
||||
## Summary
|
||||
|
||||
| File | Lines | Assessment | Recommended Extractions |
|
||||
|------|-------|------------|------------------------|
|
||||
| `pty.rs` | 260 | Well-organized | None needed |
|
||||
| `keyboard.rs` | 558 | Could benefit from split | 1 extraction recommended |
|
||||
| `graphics.rs` | 1846 | Needs refactoring | 3-4 extractions recommended |
|
||||
|
||||
---
|
||||
|
||||
## pty.rs (260 lines)
|
||||
|
||||
### Assessment: Well-Organized - No Changes Needed
|
||||
|
||||
The file is focused and cohesive:
|
||||
- `PtyError` enum (lines 10-28) - Error types for PTY operations
|
||||
- `Pty` struct and impl (lines 31-250) - Core PTY functionality
|
||||
- All code directly relates to PTY management
|
||||
|
||||
**Rationale for keeping as-is:**
|
||||
- Single responsibility: pseudo-terminal handling
|
||||
- Manageable size (260 lines)
|
||||
- All components are tightly coupled
|
||||
- No reusable utilities that other modules would need
|
||||
|
||||
---
|
||||
|
||||
## keyboard.rs (558 lines)
|
||||
|
||||
### Assessment: Could Benefit from Minor Refactoring
|
||||
|
||||
The file contains the Kitty keyboard protocol implementation with several logical sections.
|
||||
|
||||
### Recommended Extraction: None (Optional Minor Refactoring)
|
||||
|
||||
While the file has distinct sections, they are all tightly coupled around the keyboard protocol:
|
||||
|
||||
1. **Flags/Types** (lines 8-95): `KeyboardFlags`, `KeyEventType`, `Modifiers`
|
||||
2. **FunctionalKey enum** (lines 96-195): Key code definitions
|
||||
3. **KeyboardState** (lines 197-292): Protocol state management
|
||||
4. **KeyEncoder** (lines 294-548): Key encoding logic
|
||||
|
||||
**Rationale for keeping as-is:**
|
||||
- All components implement a single protocol specification
|
||||
- `KeyEncoder` depends on `KeyboardState`, `Modifiers`, `FunctionalKey`
|
||||
- The file is under 600 lines, which is manageable
|
||||
- Splitting would require importing everything back together in practice
|
||||
|
||||
**Optional Consideration:**
|
||||
If the codebase grows to support multiple keyboard protocols, consider:
|
||||
- `keyboard/mod.rs` - Public API
|
||||
- `keyboard/kitty.rs` - Kitty protocol implementation
|
||||
- `keyboard/legacy.rs` - Legacy encoding (currently in `encode_legacy_*` methods)
|
||||
|
||||
---
|
||||
|
||||
## graphics.rs (1846 lines)
|
||||
|
||||
### Assessment: Needs Refactoring - Multiple Extractions Recommended
|
||||
|
||||
This file is too large and contains several distinct logical modules that could be separated.
|
||||
|
||||
### Extraction 1: Animation Module
|
||||
|
||||
**Location:** Lines 391-746 (animation-related code)
|
||||
|
||||
**Components to extract:**
|
||||
- `AnimationState` enum (lines 707-717)
|
||||
- `AnimationData` struct (lines 719-736)
|
||||
- `AnimationFrame` struct (lines 738-745)
|
||||
- `decode_gif()` function (lines 393-459)
|
||||
- `decode_webm()` function (lines 461-646, when feature enabled)
|
||||
|
||||
**Suggested file:** `src/graphics/animation.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use std::io::{Cursor, Read};
|
||||
use std::time::Instant;
|
||||
use image::{codecs::gif::GifDecoder, AnimationDecoder};
|
||||
use super::GraphicsError;
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- `decode_webm` is feature-gated (`#[cfg(feature = "webm")]`)
|
||||
- Need to re-export types from `graphics/mod.rs`
|
||||
|
||||
---
|
||||
|
||||
### Extraction 2: Graphics Protocol Types
|
||||
|
||||
**Location:** Lines 16-162, 648-789
|
||||
|
||||
**Components to extract:**
|
||||
- `Action` enum (lines 17-34)
|
||||
- `Format` enum (lines 36-48)
|
||||
- `Transmission` enum (lines 50-62)
|
||||
- `Compression` enum (lines 64-69)
|
||||
- `DeleteTarget` enum (lines 71-92)
|
||||
- `GraphicsCommand` struct (lines 94-162)
|
||||
- `GraphicsError` enum (lines 648-673)
|
||||
- `ImageData` struct (lines 675-705)
|
||||
- `PlacementResult` struct (lines 747-758)
|
||||
- `ImagePlacement` struct (lines 760-789)
|
||||
|
||||
**Suggested file:** `src/graphics/types.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use std::time::Instant;
|
||||
use super::animation::{AnimationData, AnimationState};
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- `GraphicsCommand` has methods that depend on decoding logic
|
||||
- Consider keeping `GraphicsCommand::parse()` and `decode_*` methods in types.rs or a separate `parsing.rs`
|
||||
|
||||
---
|
||||
|
||||
### Extraction 3: Image Storage
|
||||
|
||||
**Location:** Lines 791-1807
|
||||
|
||||
**Components to extract:**
|
||||
- `ImageStorage` struct and impl (lines 791-1807)
|
||||
- `ChunkBuffer` struct (lines 808-813)
|
||||
|
||||
**Suggested file:** `src/graphics/storage.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use std::collections::HashMap;
|
||||
use std::time::Instant;
|
||||
use super::types::*;
|
||||
use super::animation::*;
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- This is the largest section (~1000 lines)
|
||||
- Contains many handler methods (`handle_transmit`, `handle_put`, etc.)
|
||||
- Could be further split into:
|
||||
- `storage.rs` - Core storage and simple operations
|
||||
- `handlers.rs` - Command handlers
|
||||
- `animation_handlers.rs` - Animation-specific handlers (lines 1026-1399)
|
||||
|
||||
---
|
||||
|
||||
### Extraction 4: Base64 Utility
|
||||
|
||||
**Location:** Lines 1809-1817
|
||||
|
||||
**Components to extract:**
|
||||
- `base64_decode()` function
|
||||
|
||||
**Suggested file:** Could go in a general `src/utils.rs` or stay in graphics
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use base64::Engine;
|
||||
use super::GraphicsError;
|
||||
```
|
||||
|
||||
**Challenges:** Minimal - this is a simple utility function
|
||||
|
||||
---
|
||||
|
||||
### Recommended Graphics Module Structure
|
||||
|
||||
```
|
||||
src/
|
||||
graphics/
|
||||
mod.rs # Re-exports, module declarations
|
||||
types.rs # Enums, structs, GraphicsCommand parsing
|
||||
animation.rs # AnimationData, AnimationFrame, GIF/WebM decoding
|
||||
storage.rs # ImageStorage, placement logic
|
||||
handlers.rs # Command handlers (optional further split)
|
||||
```
|
||||
|
||||
**mod.rs example:**
|
||||
```rust
|
||||
mod animation;
|
||||
mod handlers;
|
||||
mod storage;
|
||||
mod types;
|
||||
|
||||
pub use animation::{AnimationData, AnimationFrame, AnimationState, decode_gif};
|
||||
pub use storage::ImageStorage;
|
||||
pub use types::*;
|
||||
|
||||
#[cfg(feature = "webm")]
|
||||
pub use animation::decode_webm;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Implementation Priority
|
||||
|
||||
1. **High Priority:** Split `graphics.rs` - it's nearly 1900 lines and hard to navigate
|
||||
2. **Low Priority:** `keyboard.rs` is fine as-is but could be modularized if protocol support expands
|
||||
3. **No Action:** `pty.rs` is well-organized
|
||||
|
||||
---
|
||||
|
||||
## Migration Notes
|
||||
|
||||
When splitting `graphics.rs`:
|
||||
|
||||
1. Start by creating `src/graphics/` directory
|
||||
2. Move `graphics.rs` to `src/graphics/mod.rs` temporarily
|
||||
3. Extract types first (fewest dependencies)
|
||||
4. Extract animation module
|
||||
5. Extract storage module
|
||||
6. Update imports in `renderer.rs`, `terminal.rs`, and any other consumers
|
||||
7. Run tests after each extraction to catch breakages
|
||||
|
||||
The tests at the bottom of `graphics.rs` (lines 1819-1845) should remain in `mod.rs` or be split into module-specific test files.
|
||||
@@ -1,705 +0,0 @@
|
||||
# ZTerm Redundancy Audit
|
||||
|
||||
Generated: 2025-12-20
|
||||
|
||||
## Summary
|
||||
|
||||
| File | Current Lines | Est. Savings | % Reduction |
|
||||
|------|---------------|--------------|-------------|
|
||||
| renderer.rs | 8426 | ~1000-1400 | 12-17% |
|
||||
| terminal.rs | 2366 | ~150-180 | ~7% |
|
||||
| main.rs | 2787 | ~150-180 | ~6% |
|
||||
| vt_parser.rs | 1044 | ~60-90 | 6-9% |
|
||||
| Other files | ~3000 | ~100 | ~3% |
|
||||
| **Total** | **~17600** | **~1500-2000** | **~10%** |
|
||||
|
||||
---
|
||||
|
||||
## High Priority
|
||||
|
||||
### 1. renderer.rs: Pipeline Creation Boilerplate
|
||||
**Lines:** 2200-2960 (~760 lines)
|
||||
**Estimated Savings:** 400-500 lines
|
||||
|
||||
**Problem:** Nearly identical pipeline creation code with slight variations in entry points, blend modes, and bind group layouts.
|
||||
|
||||
**Suggested Fix:** Create a pipeline builder:
|
||||
```rust
|
||||
struct PipelineBuilder<'a> {
|
||||
device: &'a wgpu::Device,
|
||||
shader: &'a wgpu::ShaderModule,
|
||||
layout: &'a wgpu::PipelineLayout,
|
||||
format: wgpu::TextureFormat,
|
||||
}
|
||||
|
||||
impl PipelineBuilder<'_> {
|
||||
fn build(self, vs_entry: &str, fs_entry: &str, blend: Option<wgpu::BlendState>) -> wgpu::RenderPipeline
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. renderer.rs: Box Drawing Character Rendering
|
||||
**Lines:** 4800-5943 (~1150 lines)
|
||||
**Estimated Savings:** 400-600 lines
|
||||
|
||||
**Problem:** Massive match statement with repeated `hline()`/`vline()`/`fill_rect()` patterns. Many similar light/heavy/double line variants.
|
||||
|
||||
**Suggested Fix:**
|
||||
- Use lookup tables for line positions/thicknesses
|
||||
- Create a `BoxDrawingSpec` struct with line positions
|
||||
- Consolidate repeated drawing patterns into parameterized helpers
|
||||
|
||||
---
|
||||
|
||||
### 3. main.rs: Duplicate NamedKey-to-String Matching
|
||||
**Lines:** 1778-1812, 2146-2181
|
||||
**Estimated Savings:** ~70 lines
|
||||
|
||||
**Problem:** `check_keybinding()` and `handle_keyboard_input()` both have nearly identical `NamedKey` to string/FunctionalKey matching logic for F1-F12, arrow keys, Home/End/PageUp/PageDown, etc.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn named_key_to_str(named: &NamedKey) -> Option<&'static str> { ... }
|
||||
fn named_key_to_functional(named: &NamedKey) -> Option<FunctionalKey> { ... }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 4. terminal.rs: Duplicated SGR Extended Color Parsing
|
||||
**Lines:** 2046-2106
|
||||
**Estimated Savings:** ~30 lines
|
||||
|
||||
**Problem:** SGR 38 (foreground) and SGR 48 (background) extended color parsing logic is nearly identical - duplicated twice each (once for sub-params, once for regular params).
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn parse_extended_color(&self, params: &CsiParams, i: &mut usize) -> Option<Color> {
|
||||
let mode = params.get(*i + 1, 0);
|
||||
if mode == 5 && *i + 2 < params.num_params {
|
||||
*i += 2;
|
||||
Some(Color::Indexed(params.params[*i] as u8))
|
||||
} else if mode == 2 && *i + 4 < params.num_params {
|
||||
let color = Color::Rgb(
|
||||
params.params[*i + 2] as u8,
|
||||
params.params[*i + 3] as u8,
|
||||
params.params[*i + 4] as u8,
|
||||
);
|
||||
*i += 4;
|
||||
Some(color)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 5. terminal.rs: Cursor-Row-With-Scroll Pattern
|
||||
**Lines:** 1239-1246, 1262-1270, 1319-1324, 1836-1842, 1844-1851, 1916-1922, 1934-1940
|
||||
**Estimated Savings:** ~25 lines
|
||||
|
||||
**Problem:** The pattern "increment cursor_row, check against scroll_bottom, scroll_up(1) if needed" is repeated 7 times.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
#[inline]
|
||||
fn advance_row(&mut self) {
|
||||
self.cursor_row += 1;
|
||||
if self.cursor_row > self.scroll_bottom {
|
||||
self.scroll_up(1);
|
||||
self.cursor_row = self.scroll_bottom;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 6. terminal.rs: Cell Construction with Current Attributes
|
||||
**Lines:** 1278-1287, 1963-1972, 1985-1994
|
||||
**Estimated Savings:** ~20 lines
|
||||
|
||||
**Problem:** `Cell` construction using current attributes is repeated 3 times with nearly identical code.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
#[inline]
|
||||
fn current_cell(&self, character: char, wide_continuation: bool) -> Cell {
|
||||
Cell {
|
||||
character,
|
||||
fg_color: self.current_fg,
|
||||
bg_color: self.current_bg,
|
||||
bold: self.current_bold,
|
||||
italic: self.current_italic,
|
||||
underline_style: self.current_underline_style,
|
||||
strikethrough: self.current_strikethrough,
|
||||
wide_continuation,
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 7. config.rs: normalize_key_name Allocates String for Static Values
|
||||
**Lines:** 89-160
|
||||
**Estimated Savings:** Eliminates 50+ string allocations
|
||||
|
||||
**Problem:** Every match arm allocates a new String even though most are static:
|
||||
```rust
|
||||
"left" | "arrowleft" | "arrow_left" => "left".to_string(),
|
||||
```
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn normalize_key_name(name: &str) -> Cow<'static, str> {
|
||||
match name {
|
||||
"left" | "arrowleft" | "arrow_left" => Cow::Borrowed("left"),
|
||||
// ...
|
||||
_ => Cow::Owned(name.to_string()),
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 8. config.rs: Repeated Parse-and-Insert Blocks in build_action_map
|
||||
**Lines:** 281-349
|
||||
**Estimated Savings:** ~40 lines
|
||||
|
||||
**Problem:** 20+ repeated blocks:
|
||||
```rust
|
||||
if let Some(parsed) = self.new_tab.parse() {
|
||||
map.insert(parsed, Action::NewTab);
|
||||
}
|
||||
```
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
let bindings: &[(&Keybind, Action)] = &[
|
||||
(&self.new_tab, Action::NewTab),
|
||||
(&self.next_tab, Action::NextTab),
|
||||
// ...
|
||||
];
|
||||
for (keybind, action) in bindings {
|
||||
if let Some(parsed) = keybind.parse() {
|
||||
map.insert(parsed, *action);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 9. vt_parser.rs: Duplicate OSC/String Command Terminator Handling
|
||||
**Lines:** 683-755, 773-843
|
||||
**Estimated Savings:** ~30 lines
|
||||
|
||||
**Problem:** `consume_osc` and `consume_string_command` have nearly identical structure for finding and handling terminators (ESC, BEL, C1 ST).
|
||||
|
||||
**Suggested Fix:** Extract a common helper:
|
||||
```rust
|
||||
fn consume_st_terminated<F>(
|
||||
&mut self,
|
||||
bytes: &[u8],
|
||||
pos: usize,
|
||||
buffer: &mut Vec<u8>,
|
||||
include_bel: bool,
|
||||
on_complete: F,
|
||||
) -> usize
|
||||
where
|
||||
F: FnOnce(&mut Self, &[u8])
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 10. vt_parser.rs: Duplicate Control Char Handling in CSI States
|
||||
**Lines:** 547-551, 593-596, 650-653
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** Identical control character handling appears in all three `CsiState` match arms:
|
||||
```rust
|
||||
0x00..=0x1F => {
|
||||
if ch != 0x1B {
|
||||
handler.control(ch);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Suggested Fix:** Move control char handling before the `match self.csi.state` block:
|
||||
```rust
|
||||
if ch <= 0x1F && ch != 0x1B {
|
||||
handler.control(ch);
|
||||
consumed += 1;
|
||||
continue;
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 11. main.rs: Repeated active_tab().and_then(active_pane()) Pattern
|
||||
**Lines:** Various (10+ occurrences)
|
||||
**Estimated Savings:** ~30 lines
|
||||
|
||||
**Problem:** This nested Option chain appears throughout the code.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn active_pane(&self) -> Option<&Pane> {
|
||||
self.active_tab().and_then(|t| t.active_pane())
|
||||
}
|
||||
|
||||
fn active_pane_mut(&mut self) -> Option<&mut Pane> {
|
||||
self.active_tab_mut().and_then(|t| t.active_pane_mut())
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority
|
||||
|
||||
### 12. renderer.rs: set_scale_factor and set_font_size Duplicate Cell Metric Recalc
|
||||
**Lines:** 3306-3413
|
||||
**Estimated Savings:** ~50 lines
|
||||
|
||||
**Problem:** ~100 lines of nearly identical cell metric recalculation logic.
|
||||
|
||||
**Suggested Fix:** Extract to a shared `recalculate_cell_metrics(&mut self)` method.
|
||||
|
||||
---
|
||||
|
||||
### 13. renderer.rs: find_font_for_char and find_color_font_for_char Similar
|
||||
**Lines:** 939-1081
|
||||
**Estimated Savings:** ~60-80 lines
|
||||
|
||||
**Problem:** ~140 lines of similar fontconfig query patterns.
|
||||
|
||||
**Suggested Fix:** Extract common fontconfig query helper, parameterize the charset/color requirements.
|
||||
|
||||
---
|
||||
|
||||
### 14. renderer.rs: place_glyph_in_cell_canvas vs Color Variant
|
||||
**Lines:** 6468-6554
|
||||
**Estimated Savings:** ~40-50 lines
|
||||
|
||||
**Problem:** ~90 lines of nearly identical logic, differing only in bytes-per-pixel (1 vs 4).
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn place_glyph_in_cell_canvas_impl(&self, bitmap: &[u8], ..., bytes_per_pixel: usize) -> Vec<u8>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 15. renderer.rs: render_rect vs render_overlay_rect Near-Identical
|
||||
**Lines:** 7192-7215
|
||||
**Estimated Savings:** ~10 lines
|
||||
|
||||
**Problem:** Near-identical functions pushing to different Vec.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn render_quad(&mut self, x: f32, y: f32, w: f32, h: f32, color: [f32; 4], overlay: bool)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 16. renderer.rs: Pane Border Adjacency Checks Repeated
|
||||
**Lines:** 7471-7587
|
||||
**Estimated Savings:** ~60-80 lines
|
||||
|
||||
**Problem:** ~120 lines of repetitive adjacency detection for 4 directions.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn check_pane_adjacency(&self, a: &PaneInfo, b: &PaneInfo) -> Vec<Border>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 17. terminal.rs: to_rgba and to_rgba_bg Nearly Identical
|
||||
**Lines:** 212-239
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** These two methods differ only in the `Color::Default` case.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
pub fn to_rgba(&self, color: &Color, is_bg: bool) -> [f32; 4] {
|
||||
let [r, g, b] = match color {
|
||||
Color::Default => if is_bg { self.default_bg } else { self.default_fg },
|
||||
Color::Rgb(r, g, b) => [*r, *g, *b],
|
||||
Color::Indexed(idx) => self.colors[*idx as usize],
|
||||
};
|
||||
[r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, 1.0]
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 18. terminal.rs: insert_lines/delete_lines Share Dirty Marking
|
||||
**Lines:** 1080-1134
|
||||
**Estimated Savings:** ~6 lines
|
||||
|
||||
**Problem:** Both use identical dirty marking loop that could use existing `mark_region_dirty`.
|
||||
|
||||
**Suggested Fix:** Replace:
|
||||
```rust
|
||||
for line in self.cursor_row..=self.scroll_bottom {
|
||||
self.mark_line_dirty(line);
|
||||
}
|
||||
```
|
||||
with:
|
||||
```rust
|
||||
self.mark_region_dirty(self.cursor_row, self.scroll_bottom);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 19. terminal.rs: handle_dec_private_mode_set/reset Are Mirror Images
|
||||
**Lines:** 2148-2295
|
||||
**Estimated Savings:** ~50 lines
|
||||
|
||||
**Problem:** ~70 lines each, almost mirror images with `true` vs `false`.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn handle_dec_private_mode(&mut self, params: &CsiParams, set: bool) {
|
||||
for i in 0..params.num_params {
|
||||
match params.params[i] {
|
||||
1 => self.application_cursor_keys = set,
|
||||
7 => self.auto_wrap = set,
|
||||
25 => self.cursor_visible = set,
|
||||
// ...
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 20. main.rs: Duplicate Git Status String Building
|
||||
**Lines:** 1095-1118
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** Identical logic for building `working_string` and `staging_string` with `~N`, `+N`, `-N` format.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn format_git_changes(modified: usize, added: usize, deleted: usize) -> String {
|
||||
let mut parts = Vec::new();
|
||||
if modified > 0 { parts.push(format!("~{}", modified)); }
|
||||
if added > 0 { parts.push(format!("+{}", added)); }
|
||||
if deleted > 0 { parts.push(format!("-{}", deleted)); }
|
||||
parts.join(" ")
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 21. main.rs: Repeated StatuslineComponent RGB Color Application
|
||||
**Lines:** 1169-1212
|
||||
**Estimated Savings:** ~10 lines
|
||||
|
||||
**Problem:** Multiple `StatuslineComponent::new(...).rgb_fg(fg_color.0, fg_color.1, fg_color.2)` calls with exact same color.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
let with_fg = |text: &str| StatuslineComponent::new(text).rgb_fg(fg_color.0, fg_color.1, fg_color.2);
|
||||
components.push(with_fg(" "));
|
||||
components.push(with_fg(&head_text));
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 22. main.rs: Tab1-Tab9 as Separate Match Arms
|
||||
**Lines:** 1862-1870
|
||||
**Estimated Savings:** ~8 lines
|
||||
|
||||
**Problem:** Nine separate match arms each calling `self.switch_to_tab(N)`.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
impl Action {
|
||||
fn tab_index(&self) -> Option<usize> {
|
||||
match self {
|
||||
Action::Tab1 => Some(0),
|
||||
Action::Tab2 => Some(1),
|
||||
// ...
|
||||
}
|
||||
}
|
||||
}
|
||||
// Then in match:
|
||||
action if action.tab_index().is_some() => {
|
||||
self.switch_to_tab(action.tab_index().unwrap());
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 23. keyboard.rs: encode_arrow and encode_f1_f4 Are Identical
|
||||
**Lines:** 462-485
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** These two methods have identical implementations.
|
||||
|
||||
**Suggested Fix:** Remove `encode_f1_f4` and use `encode_arrow` for both, or rename to `encode_ss3_key`.
|
||||
|
||||
---
|
||||
|
||||
### 24. keyboard.rs: Repeated to_string().as_bytes() Allocations
|
||||
**Lines:** 356, 365, 372, 378, 466, 479, 490, 493, 554
|
||||
**Estimated Savings:** Reduced allocations
|
||||
|
||||
**Problem:** Multiple places call `.to_string().as_bytes()` on integers.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn write_u32_to_vec(n: u32, buf: &mut Vec<u8>) {
|
||||
use std::io::Write;
|
||||
write!(buf, "{}", n).unwrap();
|
||||
}
|
||||
```
|
||||
Or use `itoa` crate for zero-allocation integer formatting.
|
||||
|
||||
---
|
||||
|
||||
### 25. graphics.rs: Duplicate AnimationData Construction
|
||||
**Lines:** 444-456, 623-635
|
||||
**Estimated Savings:** ~10 lines
|
||||
|
||||
**Problem:** Both `decode_gif` and `decode_webm` create identical `AnimationData` structs.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
impl AnimationData {
|
||||
pub fn new(frames: Vec<AnimationFrame>, total_duration_ms: u64) -> Self {
|
||||
Self {
|
||||
frames,
|
||||
current_frame: 0,
|
||||
frame_start: None,
|
||||
looping: true,
|
||||
total_duration_ms,
|
||||
state: AnimationState::Running,
|
||||
loops_remaining: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 26. graphics.rs: Duplicate RGBA Stride Handling
|
||||
**Lines:** 554-566, 596-607
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** Code for handling RGBA stride appears twice (nearly identical).
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn copy_with_stride(data: &[u8], width: u32, height: u32, stride: usize) -> Vec<u8> {
|
||||
let row_bytes = (width * 4) as usize;
|
||||
if stride == row_bytes {
|
||||
data[..(width * height * 4) as usize].to_vec()
|
||||
} else {
|
||||
let mut result = Vec::with_capacity((width * height * 4) as usize);
|
||||
for row in 0..height as usize {
|
||||
let start = row * stride;
|
||||
result.extend_from_slice(&data[start..start + row_bytes]);
|
||||
}
|
||||
result
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 27. graphics.rs: Duplicate File/TempFile/SharedMemory Reading Logic
|
||||
**Lines:** 1051-1098, 1410-1489
|
||||
**Estimated Savings:** ~30 lines
|
||||
|
||||
**Problem:** Both `handle_animation_frame` and `store_image` have similar file reading logic.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn load_transmission_data(&mut self, cmd: &mut GraphicsCommand) -> Result<Vec<u8>, GraphicsError>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 28. pty.rs: Duplicate Winsize/ioctl Pattern
|
||||
**Lines:** 57-67, 170-185
|
||||
**Estimated Savings:** ~8 lines
|
||||
|
||||
**Problem:** Same `libc::winsize` struct creation and `TIOCSWINSZ` ioctl pattern duplicated.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn set_winsize(fd: RawFd, cols: u16, rows: u16, xpixel: u16, ypixel: u16) -> Result<(), PtyError> {
|
||||
let winsize = libc::winsize {
|
||||
ws_row: rows,
|
||||
ws_col: cols,
|
||||
ws_xpixel: xpixel,
|
||||
ws_ypixel: ypixel,
|
||||
};
|
||||
let result = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, &winsize) };
|
||||
if result == -1 {
|
||||
Err(PtyError::Io(std::io::Error::last_os_error()))
|
||||
} else {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 29. vt_parser.rs: Repeated Max Length Check Pattern
|
||||
**Lines:** 537-541, 693-697, 745-749, 785-789, 831-835
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** This pattern appears 5 times:
|
||||
```rust
|
||||
if self.escape_len + X > MAX_ESCAPE_LEN {
|
||||
log::debug!("... sequence too long, aborting");
|
||||
self.state = State::Normal;
|
||||
return consumed;
|
||||
}
|
||||
```
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
#[inline]
|
||||
fn check_max_len(&mut self, additional: usize) -> bool {
|
||||
if self.escape_len + additional > MAX_ESCAPE_LEN {
|
||||
log::debug!("Escape sequence too long, aborting");
|
||||
self.state = State::Normal;
|
||||
true
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Low Priority
|
||||
|
||||
### 30. main.rs: PaneId/TabId Have Identical Implementations
|
||||
**Lines:** 230-239, 694-703
|
||||
**Estimated Savings:** ~15 lines
|
||||
|
||||
**Problem:** Both use identical `new()` implementations with static atomics.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
macro_rules! define_id {
|
||||
($name:ident) => {
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
|
||||
pub struct $name(u64);
|
||||
|
||||
impl $name {
|
||||
pub fn new() -> Self {
|
||||
use std::sync::atomic::{AtomicU64, Ordering};
|
||||
static NEXT_ID: AtomicU64 = AtomicU64::new(1);
|
||||
Self(NEXT_ID.fetch_add(1, Ordering::Relaxed))
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
define_id!(PaneId);
|
||||
define_id!(TabId);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 31. main.rs: Duplicate "All Tabs Closed" Exit Checks
|
||||
**Lines:** 2622-2653
|
||||
**Estimated Savings:** ~5 lines
|
||||
|
||||
**Problem:** Two separate checks with same log message in `about_to_wait()`.
|
||||
|
||||
**Suggested Fix:** Restructure to have a single exit point after cleanup logic.
|
||||
|
||||
---
|
||||
|
||||
### 32. terminal.rs: scroll_viewport_up/down Similarity
|
||||
**Lines:** 885-915
|
||||
**Estimated Savings:** ~8 lines
|
||||
|
||||
**Problem:** Similar structure - both check for alternate screen, calculate offset, set dirty.
|
||||
|
||||
**Suggested Fix:** Merge into single method with signed delta parameter.
|
||||
|
||||
---
|
||||
|
||||
### 33. terminal.rs: screen_alignment Cell Construction Verbose
|
||||
**Lines:** 1881-1890
|
||||
**Estimated Savings:** ~6 lines
|
||||
|
||||
**Problem:** Constructs Cell with all default values explicitly.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
*cell = Cell { character: 'E', ..Cell::default() };
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 34. keyboard.rs: Repeated UTF-8 Char Encoding Pattern
|
||||
**Lines:** 503-505, 529-532, 539-541
|
||||
**Estimated Savings:** ~8 lines
|
||||
|
||||
**Problem:** Pattern appears 3 times:
|
||||
```rust
|
||||
let mut buf = [0u8; 4];
|
||||
let s = c.encode_utf8(&mut buf);
|
||||
return s.as_bytes().to_vec();
|
||||
```
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn char_to_vec(c: char) -> Vec<u8> {
|
||||
let mut buf = [0u8; 4];
|
||||
c.encode_utf8(&mut buf).as_bytes().to_vec()
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 35. config.rs: Tab1-Tab9 as Separate Enum Variants/Fields
|
||||
**Lines:** 174-182, 214-230
|
||||
**Estimated Savings:** Structural improvement
|
||||
|
||||
**Problem:** Having separate `Tab1`, `Tab2`, ... `Tab9` variants and corresponding struct fields is verbose.
|
||||
|
||||
**Suggested Fix:** Use `Tab(u8)` variant and `tab_keys: [Keybind; 9]` array. Note: This changes the JSON config format.
|
||||
|
||||
---
|
||||
|
||||
### 36. pty.rs: Inconsistent AsRawFd Usage
|
||||
**Lines:** 63, 177
|
||||
**Estimated Savings:** Cleanup only
|
||||
|
||||
**Problem:** Uses fully-qualified `std::os::fd::AsRawFd::as_raw_fd` despite importing the trait.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
// Change from:
|
||||
let fd = std::os::fd::AsRawFd::as_raw_fd(&master);
|
||||
// To:
|
||||
let fd = master.as_raw_fd();
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 37. pty.rs: Repeated /proc Path Pattern
|
||||
**Lines:** 222-243
|
||||
**Estimated Savings:** ~4 lines
|
||||
|
||||
**Problem:** Both `foreground_process_name` and `foreground_cwd` build `/proc/{pgid}/...` paths similarly.
|
||||
|
||||
**Suggested Fix:**
|
||||
```rust
|
||||
fn proc_path(&self, file: &str) -> Option<std::path::PathBuf> {
|
||||
let pgid = self.foreground_pgid()?;
|
||||
Some(std::path::PathBuf::from(format!("/proc/{}/{}", pgid, file)))
|
||||
}
|
||||
```
|
||||
@@ -1,314 +0,0 @@
|
||||
# Renderer.rs Reorganization Analysis
|
||||
|
||||
This document analyzes `/src/renderer.rs` (8189 lines) and identifies sections that could be extracted into separate modules for better code organization and maintainability.
|
||||
|
||||
## Overview
|
||||
|
||||
The `renderer.rs` file is the largest file in the codebase and handles:
|
||||
- GPU pipeline setup and rendering
|
||||
- Font loading and glyph caching
|
||||
- Color/emoji font rendering via FreeType/Cairo
|
||||
- Box drawing character generation
|
||||
- Statusline rendering
|
||||
- Image/Kitty graphics protocol support
|
||||
- Edge glow effects
|
||||
|
||||
---
|
||||
|
||||
## Extraction Opportunities
|
||||
|
||||
### 1. Color Utilities / Linear Palette
|
||||
**Lines:** 23-77
|
||||
**Suggested file:** `src/color.rs` or `src/palette.rs`
|
||||
|
||||
**Contents:**
|
||||
- `LinearPalette` struct
|
||||
- `srgb_to_linear()` function
|
||||
- `make_linear_palette()` function
|
||||
|
||||
**Dependencies:**
|
||||
- `crate::config::ColorScheme`
|
||||
|
||||
**Complexity:** Low - self-contained utility code with minimal dependencies.
|
||||
|
||||
```rust
|
||||
pub struct LinearPalette {
|
||||
pub foreground: [f32; 4],
|
||||
pub background: [f32; 4],
|
||||
pub cursor: [f32; 4],
|
||||
pub colors: [[f32; 4]; 256],
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. Statusline Types
|
||||
**Lines:** 123-258
|
||||
**Suggested file:** `src/statusline.rs`
|
||||
|
||||
**Contents:**
|
||||
- `StatuslineColor` enum
|
||||
- `StatuslineComponent` struct
|
||||
- `StatuslineSection` struct
|
||||
- `StatuslineContent` struct and its `impl` block
|
||||
|
||||
**Dependencies:**
|
||||
- Standard library only
|
||||
|
||||
**Complexity:** Low - pure data structures with simple logic.
|
||||
|
||||
**Note:** The `parse_ansi_statusline` method (lines 4029-4144) should also move here.
|
||||
|
||||
---
|
||||
|
||||
### 3. Edge Glow Animation
|
||||
**Lines:** 260-307
|
||||
**Suggested file:** `src/effects.rs` or `src/edge_glow.rs`
|
||||
|
||||
**Contents:**
|
||||
- `EdgeGlowSide` enum
|
||||
- `EdgeGlow` struct
|
||||
- Animation logic (`update`, `is_active`, `intensity` methods)
|
||||
|
||||
**Dependencies:**
|
||||
- `std::time::Instant`
|
||||
|
||||
**Complexity:** Low - isolated animation state machine.
|
||||
|
||||
---
|
||||
|
||||
### 4. GPU Data Structures
|
||||
**Lines:** 399-710
|
||||
**Suggested file:** `src/gpu_types.rs`
|
||||
|
||||
**Contents:**
|
||||
- `GlyphVertex` struct
|
||||
- `GlowInstance` struct
|
||||
- `EdgeGlowUniforms` struct
|
||||
- `ImageUniforms` struct
|
||||
- `GPUCell` struct
|
||||
- `SpriteInfo` struct
|
||||
- `GridParams` struct
|
||||
- `Quad` struct
|
||||
- `QuadParams` struct
|
||||
- `StatuslineParams` struct
|
||||
- Various constants (`GLYPH_ATLAS_SIZE`, `CELL_INSTANCE_SIZE`, etc.)
|
||||
|
||||
**Dependencies:**
|
||||
- `bytemuck::{Pod, Zeroable}`
|
||||
|
||||
**Complexity:** Low - pure data structures with `#[repr(C)]` layouts for GPU compatibility.
|
||||
|
||||
---
|
||||
|
||||
### 5. Font Loading Helpers
|
||||
**Lines:** 929-996, 1002-1081, 1382-1565
|
||||
**Suggested file:** `src/font_loader.rs`
|
||||
|
||||
**Contents:**
|
||||
- `find_font_for_char()` - finds a font file that can render a given character
|
||||
- `find_color_font_for_char()` - finds color emoji fonts
|
||||
- `load_font_variant()` - loads a specific font variant (bold, italic, etc.)
|
||||
- `find_font_family_variants()` - discovers all variants of a font family
|
||||
- `load_font_family()` - loads an entire font family
|
||||
|
||||
**Dependencies:**
|
||||
- `fontconfig` crate
|
||||
- `fontdue` crate
|
||||
- `std::fs`
|
||||
- `std::collections::HashMap`
|
||||
|
||||
**Complexity:** Medium - these are standalone functions but have some interdependencies. Would need to pass fontconfig patterns and settings as parameters.
|
||||
|
||||
---
|
||||
|
||||
### 6. Color Font Renderer (Emoji)
|
||||
**Lines:** 1083-1380
|
||||
**Suggested file:** `src/color_font.rs` or `src/emoji_renderer.rs`
|
||||
|
||||
**Contents:**
|
||||
- `ColorFontRenderer` struct
|
||||
- FreeType library/face management
|
||||
- Cairo surface rendering
|
||||
- Emoji glyph rasterization
|
||||
|
||||
**Dependencies:**
|
||||
- `freetype` crate
|
||||
- `cairo` crate
|
||||
- `std::collections::HashMap`
|
||||
- `std::ptr`
|
||||
|
||||
**Complexity:** Medium - self-contained but uses unsafe code and external C libraries. The struct is instantiated inside `Renderer::new()`.
|
||||
|
||||
```rust
|
||||
struct ColorFontRenderer {
|
||||
library: freetype::Library,
|
||||
faces: HashMap<String, FontFaceEntry>,
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 7. Box Drawing / Supersampled Canvas
|
||||
**Lines:** 1567-1943, 4500-5706
|
||||
**Suggested file:** `src/box_drawing.rs`
|
||||
|
||||
**Contents:**
|
||||
- `Corner` enum
|
||||
- `SupersampledCanvas` struct
|
||||
- Canvas rendering methods (lines, arcs, shading, etc.)
|
||||
- `render_box_char()` method (1200+ lines of box drawing logic)
|
||||
|
||||
**Dependencies:**
|
||||
- Standard library only
|
||||
|
||||
**Complexity:** High - the `render_box_char` method is massive (1200+ lines) and handles all Unicode box drawing, block elements, and legacy graphics characters. However, it's functionally isolated.
|
||||
|
||||
**Recommendation:** This is the highest-value extraction target. The box drawing code is:
|
||||
1. Completely self-contained
|
||||
2. Very large (adds ~2400 lines)
|
||||
3. Rarely needs modification
|
||||
4. Easy to test in isolation
|
||||
|
||||
---
|
||||
|
||||
### 8. Pipeline Builder
|
||||
**Lines:** 1947-2019
|
||||
**Suggested file:** `src/pipeline.rs`
|
||||
|
||||
**Contents:**
|
||||
- `PipelineBuilder` struct
|
||||
- Builder pattern for wgpu render pipelines
|
||||
|
||||
**Dependencies:**
|
||||
- `wgpu` crate
|
||||
|
||||
**Complexity:** Low - clean builder pattern, easily extractable.
|
||||
|
||||
```rust
|
||||
struct PipelineBuilder<'a> {
|
||||
device: &'a wgpu::Device,
|
||||
label: &'a str,
|
||||
// ... other fields
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 9. Pane GPU Resources
|
||||
**Lines:** 105-121, 3710-3785
|
||||
**Suggested file:** `src/pane_resources.rs`
|
||||
|
||||
**Contents:**
|
||||
- `PaneGpuResources` struct
|
||||
- Buffer management for per-pane GPU state
|
||||
- Methods: `new()`, `ensure_grid_capacity()`, `ensure_glyph_capacity()`
|
||||
|
||||
**Dependencies:**
|
||||
- `wgpu` crate
|
||||
|
||||
**Complexity:** Medium - the struct is simple but tightly coupled to the `Renderer` for buffer creation. Would need to pass `device` as parameter.
|
||||
|
||||
---
|
||||
|
||||
### 10. Image Rendering / Kitty Graphics
|
||||
**Lines:** 7940-8186 (+ `GpuImage` at 483-491)
|
||||
**Suggested file:** `src/image_renderer.rs`
|
||||
|
||||
**Contents:**
|
||||
- `GpuImage` struct
|
||||
- `upload_image()` method
|
||||
- `remove_image()` method
|
||||
- `sync_images()` method
|
||||
- `prepare_image_renders()` method
|
||||
|
||||
**Dependencies:**
|
||||
- `wgpu` crate
|
||||
- `crate::terminal::ImageData`
|
||||
|
||||
**Complexity:** Medium - these methods operate on `Renderer` state but could be extracted into a helper struct that holds image-specific GPU resources.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Extraction Order
|
||||
|
||||
Based on complexity and value, here's a suggested order:
|
||||
|
||||
| Priority | Module | Lines Saved | Complexity | Value |
|
||||
|----------|--------|-------------|------------|-------|
|
||||
| 1 | `box_drawing.rs` | ~2400 | High | Very High |
|
||||
| 2 | `gpu_types.rs` | ~310 | Low | High |
|
||||
| 3 | `color_font.rs` | ~300 | Medium | High |
|
||||
| 4 | `font_loader.rs` | ~330 | Medium | Medium |
|
||||
| 5 | `statusline.rs` | ~250 | Low | Medium |
|
||||
| 6 | `pipeline.rs` | ~75 | Low | Medium |
|
||||
| 7 | `color.rs` | ~55 | Low | Low |
|
||||
| 8 | `edge_glow.rs` | ~50 | Low | Low |
|
||||
| 9 | `pane_resources.rs` | ~80 | Medium | Medium |
|
||||
| 10 | `image_renderer.rs` | ~250 | Medium | Medium |
|
||||
|
||||
---
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
### The Renderer Struct (Lines 712-927)
|
||||
|
||||
The main `Renderer` struct ties everything together and would remain in `renderer.rs`. After extraction, it would:
|
||||
|
||||
1. Import types from the new modules
|
||||
2. Potentially hold instances of extracted helper structs (e.g., `ColorFontRenderer`, `ImageRenderer`)
|
||||
3. Still contain the core rendering logic (`render()`, `prepare_pane_data()`, etc.)
|
||||
|
||||
### Module Structure
|
||||
|
||||
After refactoring, the structure might look like:
|
||||
|
||||
```
|
||||
src/
|
||||
├── renderer/
|
||||
│ ├── mod.rs # Main Renderer struct and render logic
|
||||
│ ├── box_drawing.rs # SupersampledCanvas + render_box_char
|
||||
│ ├── color_font.rs # ColorFontRenderer for emoji
|
||||
│ ├── font_loader.rs # Font discovery and loading
|
||||
│ ├── gpu_types.rs # GPU data structures
|
||||
│ ├── image.rs # Kitty graphics support
|
||||
│ ├── pipeline.rs # PipelineBuilder
|
||||
│ ├── statusline.rs # Statusline types and parsing
|
||||
│ └── effects.rs # EdgeGlow and other effects
|
||||
├── color.rs # LinearPalette (or keep in renderer/)
|
||||
└── ...
|
||||
```
|
||||
|
||||
### Challenges
|
||||
|
||||
1. **Circular dependencies:** The `Renderer` struct is used throughout. Extracted modules should receive what they need via parameters, not by importing `Renderer`.
|
||||
|
||||
2. **GPU resources:** Many extracted components need `&wgpu::Device` and `&wgpu::Queue`. These should be passed as parameters rather than stored.
|
||||
|
||||
3. **Method extraction:** Some methods like `render_box_char` are currently `impl Renderer` methods. They'd need to become standalone functions or methods on the extracted structs.
|
||||
|
||||
4. **Testing:** Extracted modules will be easier to unit test, which is a significant benefit.
|
||||
|
||||
---
|
||||
|
||||
## Quick Wins
|
||||
|
||||
These can be extracted with minimal refactoring:
|
||||
|
||||
1. **`gpu_types.rs`** - Just move the structs and constants, add `pub use` in renderer
|
||||
2. **`color.rs`** - Move `LinearPalette` and helper functions
|
||||
3. **`pipeline.rs`** - Move `PipelineBuilder` as-is
|
||||
4. **`edge_glow.rs`** - Move `EdgeGlow` and related types
|
||||
|
||||
---
|
||||
|
||||
## Conclusion
|
||||
|
||||
The `renderer.rs` file is doing too much. Extracting the identified modules would:
|
||||
|
||||
- Reduce `renderer.rs` from ~8200 lines to ~4000-4500 lines
|
||||
- Improve code organization and discoverability
|
||||
- Enable better unit testing of isolated components
|
||||
- Make the codebase more approachable for new contributors
|
||||
|
||||
The highest-impact extraction is `box_drawing.rs`, which alone would remove ~2400 lines of self-contained code.
|
||||
@@ -1,356 +0,0 @@
|
||||
# Terminal.rs Reorganization Plan
|
||||
|
||||
This document identifies sections of `src/terminal.rs` (2336 lines) that could be extracted into separate files to improve code organization, maintainability, and testability.
|
||||
|
||||
---
|
||||
|
||||
## Summary of Proposed Extractions
|
||||
|
||||
| New File | Lines | Primary Contents |
|
||||
|----------|-------|------------------|
|
||||
| `src/cell.rs` | ~90 | `Cell`, `Color`, `CursorShape` |
|
||||
| `src/color.rs` | ~115 | `ColorPalette` and color parsing |
|
||||
| `src/mouse.rs` | ~110 | `MouseTrackingMode`, `MouseEncoding`, `encode_mouse()` |
|
||||
| `src/scrollback.rs` | ~100 | `ScrollbackBuffer` ring buffer |
|
||||
| `src/terminal_commands.rs` | ~30 | `TerminalCommand`, `Direction` |
|
||||
| `src/stats.rs` | ~45 | `ProcessingStats` |
|
||||
|
||||
**Total extractable: ~490 lines** (reducing terminal.rs by ~21%)
|
||||
|
||||
---
|
||||
|
||||
## 1. Cell Types and Color Enum
|
||||
|
||||
**Lines:** 30-87 (Cell, Color, CursorShape)
|
||||
|
||||
**Contents:**
|
||||
- `Cell` struct (lines 31-45) - terminal grid cell with character and attributes
|
||||
- `impl Default for Cell` (lines 47-60)
|
||||
- `Color` enum (lines 63-69) - `Default`, `Rgb(u8, u8, u8)`, `Indexed(u8)`
|
||||
- `CursorShape` enum (lines 72-87) - cursor style variants
|
||||
|
||||
**Proposed file:** `src/cell.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
// No external dependencies - these are self-contained types
|
||||
```
|
||||
|
||||
**Exports needed by terminal.rs:**
|
||||
```rust
|
||||
pub use cell::{Cell, Color, CursorShape};
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- None. These are simple data types with no logic dependencies.
|
||||
|
||||
**Benefits:**
|
||||
- `Cell` and `Color` are referenced by renderer and could be imported directly
|
||||
- Makes the core data structures discoverable
|
||||
- Easy to test color conversion independently
|
||||
|
||||
---
|
||||
|
||||
## 2. Color Palette
|
||||
|
||||
**Lines:** 119-240
|
||||
|
||||
**Contents:**
|
||||
- `ColorPalette` struct (lines 120-128) - 256-color palette storage
|
||||
- `impl Default for ColorPalette` (lines 130-177) - ANSI + 216 color cube + grayscale
|
||||
- `ColorPalette::parse_color_spec()` (lines 181-209) - parse `#RRGGBB` and `rgb:RR/GG/BB`
|
||||
- `ColorPalette::to_rgba()` (lines 212-224) - foreground color conversion
|
||||
- `ColorPalette::to_rgba_bg()` (lines 227-239) - background color conversion
|
||||
|
||||
**Proposed file:** `src/color.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use crate::cell::Color; // For Color enum
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- Depends on `Color` enum (extract cell.rs first)
|
||||
- `to_rgba()` and `to_rgba_bg()` are called by the renderer
|
||||
|
||||
**Benefits:**
|
||||
- Color parsing logic is self-contained and testable
|
||||
- Palette initialization is complex (color cube math) and benefits from isolation
|
||||
- Could add color scheme loading from config files in the future
|
||||
|
||||
---
|
||||
|
||||
## 3. Mouse Tracking and Encoding
|
||||
|
||||
**Lines:** 89-117, 962-1059
|
||||
|
||||
**Contents:**
|
||||
- `MouseTrackingMode` enum (lines 90-103)
|
||||
- `MouseEncoding` enum (lines 106-117)
|
||||
- `Terminal::encode_mouse()` method (lines 964-1059) - encode mouse events for PTY
|
||||
|
||||
**Proposed file:** `src/mouse.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
// MouseTrackingMode and MouseEncoding are self-contained enums
|
||||
// encode_mouse() would need to be a standalone function or trait
|
||||
```
|
||||
|
||||
**Refactoring approach:**
|
||||
```rust
|
||||
// In mouse.rs
|
||||
pub fn encode_mouse_event(
|
||||
tracking: MouseTrackingMode,
|
||||
encoding: MouseEncoding,
|
||||
button: u8,
|
||||
col: u16,
|
||||
row: u16,
|
||||
pressed: bool,
|
||||
is_motion: bool,
|
||||
modifiers: u8,
|
||||
) -> Vec<u8>
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- `encode_mouse()` is currently a method on `Terminal` but only reads `mouse_tracking` and `mouse_encoding`
|
||||
- Need to change call sites to pass mode/encoding explicitly, OR keep as Terminal method but move enums
|
||||
|
||||
**Benefits:**
|
||||
- Mouse protocol logic is self-contained and well-documented
|
||||
- Could add unit tests for X10, SGR, URXVT encoding without instantiating Terminal
|
||||
|
||||
---
|
||||
|
||||
## 4. Scrollback Buffer
|
||||
|
||||
**Lines:** 314-425
|
||||
|
||||
**Contents:**
|
||||
- `ScrollbackBuffer` struct (lines 326-335) - Kitty-style ring buffer
|
||||
- `ScrollbackBuffer::new()` (lines 340-351) - lazy allocation
|
||||
- `ScrollbackBuffer::len()`, `is_empty()`, `is_full()` (lines 354-369)
|
||||
- `ScrollbackBuffer::push()` (lines 377-403) - O(1) ring buffer insertion
|
||||
- `ScrollbackBuffer::get()` (lines 407-415) - logical index access
|
||||
- `ScrollbackBuffer::clear()` (lines 419-424)
|
||||
|
||||
**Proposed file:** `src/scrollback.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
use crate::cell::Cell; // For Vec<Cell> line storage
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- Depends on `Cell` type (extract cell.rs first)
|
||||
- Otherwise completely self-contained with great documentation
|
||||
|
||||
**Benefits:**
|
||||
- Ring buffer is a reusable data structure
|
||||
- Excellent candidate for unit testing (push, wrap-around, get by index)
|
||||
- Performance-critical code that benefits from isolation for profiling
|
||||
|
||||
---
|
||||
|
||||
## 5. Terminal Commands
|
||||
|
||||
**Lines:** 8-28
|
||||
|
||||
**Contents:**
|
||||
- `TerminalCommand` enum (lines 10-19) - commands sent from terminal to application
|
||||
- `Direction` enum (lines 22-28) - navigation direction
|
||||
|
||||
**Proposed file:** `src/terminal_commands.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
// No dependencies - self-contained enums
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- Very small extraction, but cleanly separates protocol from implementation
|
||||
|
||||
**Benefits:**
|
||||
- Defines the terminal-to-application interface
|
||||
- Could grow as more custom OSC commands are added
|
||||
- Clear documentation of what commands exist
|
||||
|
||||
---
|
||||
|
||||
## 6. Processing Stats
|
||||
|
||||
**Lines:** 268-312
|
||||
|
||||
**Contents:**
|
||||
- `ProcessingStats` struct (lines 269-288) - timing/debugging statistics
|
||||
- `ProcessingStats::reset()` (lines 291-293)
|
||||
- `ProcessingStats::log_if_slow()` (lines 295-311) - conditional performance logging
|
||||
|
||||
**Proposed file:** `src/stats.rs`
|
||||
|
||||
**Dependencies:**
|
||||
```rust
|
||||
// No dependencies - uses only log crate
|
||||
```
|
||||
|
||||
**Challenges:**
|
||||
- Only used for debugging/profiling
|
||||
- Could be feature-gated behind a `profiling` feature flag
|
||||
|
||||
**Benefits:**
|
||||
- Separates debug instrumentation from core logic
|
||||
- Could be conditionally compiled out for release builds
|
||||
|
||||
---
|
||||
|
||||
## 7. Saved Cursor and Alternate Screen (Keep in terminal.rs)
|
||||
|
||||
**Lines:** 243-265
|
||||
|
||||
**Contents:**
|
||||
- `SavedCursor` struct (lines 243-253)
|
||||
- `AlternateScreen` struct (lines 256-265)
|
||||
|
||||
**Recommendation:** Keep in terminal.rs
|
||||
|
||||
**Rationale:**
|
||||
- These are private implementation details of cursor save/restore and alternate screen
|
||||
- Tightly coupled to Terminal's internal state
|
||||
- No benefit from extraction
|
||||
|
||||
---
|
||||
|
||||
## 8. Handler Implementation (Keep in terminal.rs)
|
||||
|
||||
**Lines:** 1236-1904, 1906-2335
|
||||
|
||||
**Contents:**
|
||||
- `impl Handler for Terminal` - VT parser callback implementations
|
||||
- CSI handling, SGR parsing, DEC private modes
|
||||
- Keyboard protocol, OSC handling, graphics protocol
|
||||
|
||||
**Recommendation:** Keep in terminal.rs (or consider splitting if file grows further)
|
||||
|
||||
**Rationale:**
|
||||
- These are the core terminal emulation callbacks
|
||||
- Heavily intertwined with Terminal's internal state
|
||||
- Could potentially split into `terminal_csi.rs`, `terminal_sgr.rs`, etc. but adds complexity
|
||||
|
||||
---
|
||||
|
||||
## Recommended Extraction Order
|
||||
|
||||
1. **`src/cell.rs`** - No dependencies, foundational types
|
||||
2. **`src/terminal_commands.rs`** - No dependencies, simple enums
|
||||
3. **`src/stats.rs`** - No dependencies, debugging utility
|
||||
4. **`src/color.rs`** - Depends on `cell.rs`, self-contained logic
|
||||
5. **`src/scrollback.rs`** - Depends on `cell.rs`, self-contained data structure
|
||||
6. **`src/mouse.rs`** - Self-contained enums, may need refactoring for encode function
|
||||
|
||||
---
|
||||
|
||||
## Example: cell.rs Implementation
|
||||
|
||||
```rust
|
||||
//! Terminal cell and color types.
|
||||
|
||||
/// A single cell in the terminal grid.
|
||||
#[derive(Clone, Copy, Debug)]
|
||||
pub struct Cell {
|
||||
pub character: char,
|
||||
pub fg_color: Color,
|
||||
pub bg_color: Color,
|
||||
pub bold: bool,
|
||||
pub italic: bool,
|
||||
/// Underline style: 0=none, 1=single, 2=double, 3=curly, 4=dotted, 5=dashed
|
||||
pub underline_style: u8,
|
||||
/// Strikethrough decoration
|
||||
pub strikethrough: bool,
|
||||
/// If true, this cell is the continuation of a wide (double-width) character.
|
||||
pub wide_continuation: bool,
|
||||
}
|
||||
|
||||
impl Default for Cell {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
character: ' ',
|
||||
fg_color: Color::Default,
|
||||
bg_color: Color::Default,
|
||||
bold: false,
|
||||
italic: false,
|
||||
underline_style: 0,
|
||||
strikethrough: false,
|
||||
wide_continuation: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Terminal colors.
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Default)]
|
||||
pub enum Color {
|
||||
#[default]
|
||||
Default,
|
||||
Rgb(u8, u8, u8),
|
||||
Indexed(u8),
|
||||
}
|
||||
|
||||
/// Cursor shape styles (DECSCUSR).
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Default)]
|
||||
pub enum CursorShape {
|
||||
#[default]
|
||||
BlinkingBlock,
|
||||
SteadyBlock,
|
||||
BlinkingUnderline,
|
||||
SteadyUnderline,
|
||||
BlinkingBar,
|
||||
SteadyBar,
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Impact on lib.rs
|
||||
|
||||
After extraction, `lib.rs` would need:
|
||||
|
||||
```rust
|
||||
pub mod cell;
|
||||
pub mod color;
|
||||
pub mod mouse;
|
||||
pub mod scrollback;
|
||||
pub mod stats;
|
||||
pub mod terminal;
|
||||
pub mod terminal_commands;
|
||||
// ... existing modules ...
|
||||
```
|
||||
|
||||
And `terminal.rs` would add:
|
||||
|
||||
```rust
|
||||
use crate::cell::{Cell, Color, CursorShape};
|
||||
use crate::color::ColorPalette;
|
||||
use crate::mouse::{MouseEncoding, MouseTrackingMode};
|
||||
use crate::scrollback::ScrollbackBuffer;
|
||||
use crate::stats::ProcessingStats;
|
||||
use crate::terminal_commands::{Direction, TerminalCommand};
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Testing Opportunities
|
||||
|
||||
Extracting these modules enables focused unit tests:
|
||||
|
||||
- **cell.rs**: Default cell values, wide_continuation handling
|
||||
- **color.rs**: `parse_color_spec()` for various formats, palette indexing, RGBA conversion
|
||||
- **mouse.rs**: Encoding tests for X10, SGR, URXVT formats, tracking mode filtering
|
||||
- **scrollback.rs**: Ring buffer push/get/wrap, capacity limits, clear behavior
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
- The `Terminal` struct itself (lines 428-512) should remain in `terminal.rs` as the central state container
|
||||
- Private helper structs like `SavedCursor` and `AlternateScreen` should stay in `terminal.rs`
|
||||
- The `Handler` trait implementation spans ~600 lines but is core terminal logic
|
||||
- Consider feature-gating `ProcessingStats` behind `#[cfg(feature = "profiling")]`
|
||||
@@ -1,295 +0,0 @@
|
||||
# VT Parser Reorganization Recommendations
|
||||
|
||||
This document analyzes `src/vt_parser.rs` (1033 lines) and identifies sections that could be extracted into separate files to improve code organization, testability, and maintainability.
|
||||
|
||||
## Current File Structure Overview
|
||||
|
||||
| Lines | Section | Description |
|
||||
|-------|---------|-------------|
|
||||
| 1-49 | Constants & UTF-8 Tables | Parser limits, UTF-8 DFA decode table |
|
||||
| 51-133 | UTF-8 Decoder | `Utf8Decoder` struct and implementation |
|
||||
| 135-265 | State & CSI Types | `State` enum, `CsiState` enum, `CsiParams` struct |
|
||||
| 267-832 | Parser Core | Main `Parser` struct with all parsing logic |
|
||||
| 835-906 | Handler Trait | `Handler` trait definition |
|
||||
| 908-1032 | Tests | Unit tests |
|
||||
|
||||
---
|
||||
|
||||
## Recommended Extractions
|
||||
|
||||
### 1. UTF-8 Decoder Module
|
||||
|
||||
**File:** `src/utf8_decoder.rs`
|
||||
|
||||
**Lines:** 27-133
|
||||
|
||||
**Components:**
|
||||
- `UTF8_ACCEPT`, `UTF8_REJECT` constants (lines 28-29)
|
||||
- `UTF8_DECODE_TABLE` static (lines 33-49)
|
||||
- `decode_utf8()` function (lines 52-62)
|
||||
- `Utf8Decoder` struct and impl (lines 66-133)
|
||||
- `REPLACEMENT_CHAR` constant (line 25)
|
||||
|
||||
**Dependencies:**
|
||||
- None (completely self-contained)
|
||||
|
||||
**Rationale:**
|
||||
- This is a completely standalone UTF-8 DFA decoder based on Bjoern Hoehrmann's design
|
||||
- Zero dependencies on the rest of the parser
|
||||
- Could be reused in other parts of the codebase (keyboard input, file parsing)
|
||||
- Independently testable
|
||||
- ~100 lines, a good size for a focused module
|
||||
|
||||
**Extraction Difficulty:** Easy
|
||||
|
||||
**Example structure:**
|
||||
```rust
|
||||
// src/utf8_decoder.rs
|
||||
pub const REPLACEMENT_CHAR: char = '\u{FFFD}';
|
||||
|
||||
const UTF8_ACCEPT: u8 = 0;
|
||||
const UTF8_REJECT: u8 = 12;
|
||||
|
||||
static UTF8_DECODE_TABLE: [u8; 364] = [ /* ... */ ];
|
||||
|
||||
#[inline]
|
||||
fn decode_utf8(state: &mut u8, codep: &mut u32, byte: u8) -> u8 { /* ... */ }
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub struct Utf8Decoder { /* ... */ }
|
||||
|
||||
impl Utf8Decoder {
|
||||
pub fn new() -> Self { /* ... */ }
|
||||
pub fn reset(&mut self) { /* ... */ }
|
||||
pub fn decode_to_esc(&mut self, src: &[u8], output: &mut Vec<char>) -> (usize, bool) { /* ... */ }
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. CSI Parameters Module
|
||||
|
||||
**File:** `src/csi_params.rs`
|
||||
|
||||
**Lines:** 14-265 (constants and CSI-related types)
|
||||
|
||||
**Components:**
|
||||
- `MAX_CSI_PARAMS` constant (line 15)
|
||||
- `CsiState` enum (lines 165-171)
|
||||
- `CsiParams` struct and impl (lines 174-265)
|
||||
|
||||
**Dependencies:**
|
||||
- None (self-contained data structure)
|
||||
|
||||
**Rationale:**
|
||||
- `CsiParams` is a self-contained data structure for CSI parameter parsing
|
||||
- Has its own sub-state machine (`CsiState`)
|
||||
- The struct is 2KB+ in size due to the arrays - isolating it makes the size impact clearer
|
||||
- Could be tested independently for parameter parsing edge cases
|
||||
- The `get()`, `add_digit()`, `commit_param()` methods form a cohesive unit
|
||||
|
||||
**Extraction Difficulty:** Easy
|
||||
|
||||
**Note:** `CsiState` is currently private and only used within CSI parsing. It should remain private to the module.
|
||||
|
||||
---
|
||||
|
||||
### 3. Handler Trait Module
|
||||
|
||||
**File:** `src/vt_handler.rs`
|
||||
|
||||
**Lines:** 835-906
|
||||
|
||||
**Components:**
|
||||
- `Handler` trait (lines 840-906)
|
||||
- `CsiParams` would need to be re-exported or the trait would depend on `csi_params` module
|
||||
|
||||
**Dependencies:**
|
||||
- `CsiParams` type (for `csi()` method signature)
|
||||
|
||||
**Rationale:**
|
||||
- Clear separation between the parser implementation and the callback interface
|
||||
- Makes it easier for consumers to implement handlers without pulling in parser internals
|
||||
- Trait documentation is substantial and benefits from its own file
|
||||
- Allows different modules to implement handlers without circular dependencies
|
||||
|
||||
**Extraction Difficulty:** Easy (after `CsiParams` is extracted)
|
||||
|
||||
---
|
||||
|
||||
### 4. Parser Constants Module
|
||||
|
||||
**File:** `src/vt_constants.rs` (or inline in a `mod.rs` approach)
|
||||
|
||||
**Lines:** 14-25
|
||||
|
||||
**Components:**
|
||||
- `MAX_CSI_PARAMS` (already mentioned above)
|
||||
- `MAX_OSC_LEN` (line 19)
|
||||
- `MAX_ESCAPE_LEN` (line 22)
|
||||
- `REPLACEMENT_CHAR` (line 25, if not moved to utf8_decoder)
|
||||
|
||||
**Dependencies:**
|
||||
- None
|
||||
|
||||
**Rationale:**
|
||||
- Centralizes magic numbers
|
||||
- Easy to find and adjust limits
|
||||
- However, these are only 4 constants, so this extraction is optional
|
||||
|
||||
**Extraction Difficulty:** Trivial
|
||||
|
||||
**Recommendation:** Keep these in the main parser file or move to a `mod.rs` if using a directory structure.
|
||||
|
||||
---
|
||||
|
||||
### 5. Parser State Enum
|
||||
|
||||
**File:** Could remain in `vt_parser.rs` or move to `vt_handler.rs`
|
||||
|
||||
**Lines:** 136-162
|
||||
|
||||
**Components:**
|
||||
- `State` enum (lines 136-156)
|
||||
- `Default` impl (lines 158-162)
|
||||
|
||||
**Dependencies:**
|
||||
- None
|
||||
|
||||
**Rationale:**
|
||||
- The `State` enum is public and part of the `Parser` struct
|
||||
- It's tightly coupled with the parser's operation
|
||||
- Small enough (~25 lines) to not warrant its own file
|
||||
|
||||
**Recommendation:** Keep in main parser file or combine with handler trait.
|
||||
|
||||
---
|
||||
|
||||
## Proposed Directory Structure
|
||||
|
||||
### Option A: Flat Module Structure (Recommended)
|
||||
|
||||
```
|
||||
src/
|
||||
vt_parser.rs # Main Parser struct, State enum, parsing logic (~700 lines)
|
||||
utf8_decoder.rs # UTF-8 DFA decoder (~110 lines)
|
||||
csi_params.rs # CsiParams struct and CsiState (~100 lines)
|
||||
vt_handler.rs # Handler trait (~75 lines)
|
||||
```
|
||||
|
||||
**lib.rs changes:**
|
||||
```rust
|
||||
mod utf8_decoder;
|
||||
mod csi_params;
|
||||
mod vt_handler;
|
||||
mod vt_parser;
|
||||
|
||||
pub use vt_parser::{Parser, State};
|
||||
pub use csi_params::{CsiParams, MAX_CSI_PARAMS};
|
||||
pub use vt_handler::Handler;
|
||||
```
|
||||
|
||||
### Option B: Directory Module Structure
|
||||
|
||||
```
|
||||
src/
|
||||
vt_parser/
|
||||
mod.rs # Re-exports and constants
|
||||
parser.rs # Main Parser struct
|
||||
utf8.rs # UTF-8 decoder
|
||||
csi.rs # CSI params
|
||||
handler.rs # Handler trait
|
||||
tests.rs # Tests (optional, can stay inline)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Extraction Priority
|
||||
|
||||
| Priority | Module | Lines Saved | Benefit |
|
||||
|----------|--------|-------------|---------|
|
||||
| 1 | `utf8_decoder.rs` | ~110 | Completely independent, reusable |
|
||||
| 2 | `csi_params.rs` | ~100 | Clear data structure boundary |
|
||||
| 3 | `vt_handler.rs` | ~75 | Cleaner API surface |
|
||||
| 4 | Constants | ~10 | Optional, low impact |
|
||||
|
||||
---
|
||||
|
||||
## Challenges and Considerations
|
||||
|
||||
### 1. Test Organization
|
||||
- Lines 908-1032 contain tests that use private test helpers (`TestHandler`)
|
||||
- If the `Handler` trait is extracted, `TestHandler` could move to a test module
|
||||
- Consider using `#[cfg(test)]` modules in each file
|
||||
|
||||
### 2. Circular Dependencies
|
||||
- `Handler` trait references `CsiParams` - extract `CsiParams` first
|
||||
- `Parser` uses both `Utf8Decoder` and `CsiParams` - both should be extracted before any handler extraction
|
||||
|
||||
### 3. Public API Surface
|
||||
- Currently public: `MAX_CSI_PARAMS`, `State`, `CsiParams`, `Parser`, `Handler`, `Utf8Decoder`
|
||||
- After extraction, ensure re-exports maintain the same public API
|
||||
|
||||
### 4. Performance Considerations
|
||||
- The UTF-8 decoder uses `#[inline]` extensively - ensure this is preserved
|
||||
- `CsiParams::reset()` is hot and optimized to avoid memset - document this
|
||||
|
||||
---
|
||||
|
||||
## Migration Steps
|
||||
|
||||
1. **Extract `utf8_decoder.rs`**
|
||||
- Move lines 25-133 to new file
|
||||
- Add `mod utf8_decoder;` to lib.rs
|
||||
- Update `vt_parser.rs` to `use crate::utf8_decoder::Utf8Decoder;`
|
||||
|
||||
2. **Extract `csi_params.rs`**
|
||||
- Move lines 14-15 (MAX_CSI_PARAMS) and 164-265 to new file
|
||||
- Make `CsiState` private to the module (`pub(crate)` at most)
|
||||
- Add `mod csi_params;` to lib.rs
|
||||
|
||||
3. **Extract `vt_handler.rs`**
|
||||
- Move lines 835-906 to new file
|
||||
- Add `use crate::csi_params::CsiParams;`
|
||||
- Add `mod vt_handler;` to lib.rs
|
||||
|
||||
4. **Update imports in `vt_parser.rs`**
|
||||
```rust
|
||||
use crate::utf8_decoder::Utf8Decoder;
|
||||
use crate::csi_params::{CsiParams, CsiState, MAX_CSI_PARAMS};
|
||||
use crate::vt_handler::Handler;
|
||||
```
|
||||
|
||||
5. **Verify public API unchanged**
|
||||
- Ensure lib.rs re-exports all previously public items
|
||||
- Run tests to verify nothing broke
|
||||
|
||||
---
|
||||
|
||||
## Code That Should Stay in `vt_parser.rs`
|
||||
|
||||
The following should remain in the main parser file:
|
||||
|
||||
- `State` enum (lines 136-162) - tightly coupled to parser
|
||||
- `Parser` struct (lines 268-299) - core type
|
||||
- All `Parser` methods (lines 301-832) - core parsing logic
|
||||
- Constants `MAX_OSC_LEN`, `MAX_ESCAPE_LEN` (lines 19, 22) - parser-specific limits
|
||||
|
||||
After extraction, `vt_parser.rs` would be ~700 lines focused purely on the state machine and escape sequence parsing logic.
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
The `vt_parser.rs` file has clear natural boundaries:
|
||||
|
||||
1. **UTF-8 decoding** - completely standalone, based on external algorithm
|
||||
2. **CSI parameter handling** - self-contained data structure with its own state
|
||||
3. **Handler trait** - defines the callback interface
|
||||
4. **Core parser** - the state machine and escape sequence processing
|
||||
|
||||
Extracting the first three would reduce `vt_parser.rs` from 1033 lines to ~700 lines while improving:
|
||||
- Code navigation
|
||||
- Testability of individual components
|
||||
- Reusability of the UTF-8 decoder
|
||||
- API clarity (handler trait in its own file)
|
||||
Reference in New Issue
Block a user