From f38033c782d76d7188d8ccf466ec85694f9c1e11 Mon Sep 17 00:00:00 2001 From: Zacharias-Brohn Date: Tue, 3 Feb 2026 22:10:31 +0100 Subject: [PATCH] file cleanup --- ATLAS_REFACTOR.md | 359 ---------------------- CONFIG_REORG.md | 248 ---------------- MAIN_REORG.md | 346 ---------------------- MISC_REORG.md | 220 -------------- REDUNDANCY_AUDIT.md | 705 -------------------------------------------- RENDERER_REORG.md | 314 -------------------- TERMINAL_REORG.md | 356 ---------------------- VT_PARSER_REORG.md | 295 ------------------ 8 files changed, 2843 deletions(-) delete mode 100644 ATLAS_REFACTOR.md delete mode 100644 CONFIG_REORG.md delete mode 100644 MAIN_REORG.md delete mode 100644 MISC_REORG.md delete mode 100644 REDUNDANCY_AUDIT.md delete mode 100644 RENDERER_REORG.md delete mode 100644 TERMINAL_REORG.md delete mode 100644 VT_PARSER_REORG.md diff --git a/ATLAS_REFACTOR.md b/ATLAS_REFACTOR.md deleted file mode 100644 index ad86ef1..0000000 --- a/ATLAS_REFACTOR.md +++ /dev/null @@ -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; -``` - -**Sampling:** -```wgsl -let sample = textureSample(atlas_texture, atlas_sampler, uv, layer_index); -``` - -## New Implementation - -### Rust (renderer.rs) - -**Struct fields:** -```rust -atlas_textures: Vec, // Vector of separate textures -atlas_views: Vec, // 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>; -``` - -**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, -atlas_views: Vec, -``` - -### 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; - -// New -@group(0) @binding(0) -var atlas_textures: binding_array>; -``` - -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`) diff --git a/CONFIG_REORG.md b/CONFIG_REORG.md deleted file mode 100644 index c0027a6..0000000 --- a/CONFIG_REORG.md +++ /dev/null @@ -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 diff --git a/MAIN_REORG.md b/MAIN_REORG.md deleted file mode 100644 index 4d4c890..0000000 --- a/MAIN_REORG.md +++ /dev/null @@ -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 -fn build_git_section(cwd: &str) -> Option -``` - -### 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> -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. diff --git a/MISC_REORG.md b/MISC_REORG.md deleted file mode 100644 index 91526e8..0000000 --- a/MISC_REORG.md +++ /dev/null @@ -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. diff --git a/REDUNDANCY_AUDIT.md b/REDUNDANCY_AUDIT.md deleted file mode 100644 index 878b177..0000000 --- a/REDUNDANCY_AUDIT.md +++ /dev/null @@ -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::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 { ... } -``` - ---- - -### 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 { - 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( - &mut self, - bytes: &[u8], - pos: usize, - buffer: &mut Vec, - 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 -``` - ---- - -### 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 -``` - ---- - -### 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 { - 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) { - 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, 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 { - 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, 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 { - 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 { - let pgid = self.foreground_pgid()?; - Some(std::path::PathBuf::from(format!("/proc/{}/{}", pgid, file))) -} -``` diff --git a/RENDERER_REORG.md b/RENDERER_REORG.md deleted file mode 100644 index d906894..0000000 --- a/RENDERER_REORG.md +++ /dev/null @@ -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, -} -``` - ---- - -### 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. diff --git a/TERMINAL_REORG.md b/TERMINAL_REORG.md deleted file mode 100644 index 47d229a..0000000 --- a/TERMINAL_REORG.md +++ /dev/null @@ -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 -``` - -**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 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")]` diff --git a/VT_PARSER_REORG.md b/VT_PARSER_REORG.md deleted file mode 100644 index a1b6479..0000000 --- a/VT_PARSER_REORG.md +++ /dev/null @@ -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) -> (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)