|
| 1 | +# LineBuilder Migration Plan |
| 2 | + |
| 3 | +## Goal |
| 4 | + |
| 5 | +Make `letui-ffi/src/text_layout.rs` easier to read and maintain without changing behavior. |
| 6 | + |
| 7 | +The core idea: |
| 8 | + |
| 9 | +- keep one place that knows how to build visual lines |
| 10 | +- make wrap modes decide only where line breaks happen |
| 11 | +- stop repeating cursor logic, width handling, and line-reset code in multiple functions |
| 12 | + |
| 13 | +## Why this refactor makes sense |
| 14 | + |
| 15 | +Right now, `text_layout.rs` has three different paths that all do similar work: |
| 16 | + |
| 17 | +- no wrap |
| 18 | +- char wrap |
| 19 | +- word wrap |
| 20 | + |
| 21 | +Each path ends up doing the same kinds of things: |
| 22 | + |
| 23 | +- create a new `VisualLine` |
| 24 | +- track current display width |
| 25 | +- skip width-0 units |
| 26 | +- place cursor before and after units |
| 27 | +- push visible cells |
| 28 | +- decide whether a new line is needed |
| 29 | + |
| 30 | +That means the file is not large only because the problem is hard. |
| 31 | +It is also large because the same low-level work is repeated. |
| 32 | + |
| 33 | +The refactor should separate two concerns: |
| 34 | + |
| 35 | +- line building |
| 36 | +- wrap policy |
| 37 | + |
| 38 | +That split is the main architectural win. |
| 39 | + |
| 40 | +## Target design |
| 41 | + |
| 42 | +### 1. Keep source parsing separate |
| 43 | + |
| 44 | +`build_explicit_lines(...)` already has a clear job: |
| 45 | + |
| 46 | +- read source text |
| 47 | +- normalize break handling |
| 48 | +- attach span styling |
| 49 | +- produce styled source units |
| 50 | + |
| 51 | +That part can stay separate. |
| 52 | + |
| 53 | +### 2. Introduce `LineBuilder` |
| 54 | + |
| 55 | +Add a small internal helper that owns: |
| 56 | + |
| 57 | +- the current line being built |
| 58 | +- the list of completed lines |
| 59 | +- current row index |
| 60 | +- cursor placement state |
| 61 | + |
| 62 | +It should be the only place that knows how to: |
| 63 | + |
| 64 | +- create an empty line |
| 65 | +- push a visible unit |
| 66 | +- handle width-0 units |
| 67 | +- record cursor matches at byte boundaries |
| 68 | +- break the current line |
| 69 | +- finish layout and return the built lines |
| 70 | + |
| 71 | +In plain English: |
| 72 | + |
| 73 | +- wrap code should say "put this unit on the current line" |
| 74 | +- `LineBuilder` should know what that actually means |
| 75 | + |
| 76 | +### 3. Make wrap code only decide break behavior |
| 77 | + |
| 78 | +After `LineBuilder` exists: |
| 79 | + |
| 80 | +- no-wrap logic should decide when content stops fitting |
| 81 | +- char-wrap logic should decide when the next unit forces a line break |
| 82 | +- word-wrap logic should decide whether a whole segment fits, or whether it must fall back to unit-by-unit placement |
| 83 | + |
| 84 | +But none of those paths should manually manage `VisualLine` internals. |
| 85 | + |
| 86 | +## Proposed `LineBuilder` responsibilities |
| 87 | + |
| 88 | +Suggested responsibilities: |
| 89 | + |
| 90 | +- `mark_boundary(byte_index)` |
| 91 | +- `push_unit(unit)` |
| 92 | +- `can_fit(width)` |
| 93 | +- `break_line()` |
| 94 | +- `current_width()` |
| 95 | +- `finish()` |
| 96 | + |
| 97 | +Suggested behavior: |
| 98 | + |
| 99 | +- `mark_boundary(...)` |
| 100 | + - checks whether the requested cursor byte index matches this boundary |
| 101 | + - if it matches, records `(row, col)` |
| 102 | +- `push_unit(...)` |
| 103 | + - skips width-0 units cleanly |
| 104 | + - pushes a `VisualCell` for width-1 or width-2 units |
| 105 | + - updates current display width |
| 106 | +- `break_line()` |
| 107 | + - pushes current line into output |
| 108 | + - resets current line to empty |
| 109 | +- `finish()` |
| 110 | + - pushes the final line if needed |
| 111 | + - returns completed lines and cursor placement |
| 112 | + |
| 113 | +Important rule: |
| 114 | + |
| 115 | +- cursor logic should live here, not be repeated in every wrap function |
| 116 | + |
| 117 | +## Refactor strategy |
| 118 | + |
| 119 | +Do this in small steps. |
| 120 | +Do not rewrite the whole file in one shot. |
| 121 | + |
| 122 | +### Step 1. Lock behavior down with tests |
| 123 | + |
| 124 | +Before changing architecture, add focused Rust tests for current behavior. |
| 125 | + |
| 126 | +Cover: |
| 127 | + |
| 128 | +- empty text |
| 129 | +- explicit newline |
| 130 | +- consecutive newlines |
| 131 | +- trailing newline |
| 132 | +- width-0 characters |
| 133 | +- width-2 characters |
| 134 | +- no-wrap clip |
| 135 | +- no-wrap ellipsis |
| 136 | +- char wrap |
| 137 | +- word wrap |
| 138 | +- cursor at start |
| 139 | +- cursor in middle |
| 140 | +- cursor at end |
| 141 | +- cursor after newline |
| 142 | +- styled spans surviving layout |
| 143 | + |
| 144 | +Why first: |
| 145 | + |
| 146 | +- this refactor touches cursor and wrapping logic |
| 147 | +- without tests, behavior drift is too easy |
| 148 | + |
| 149 | +### Step 2. Extract tiny shared helpers |
| 150 | + |
| 151 | +Before adding `LineBuilder`, extract low-risk helpers such as: |
| 152 | + |
| 153 | +- `empty_line()` |
| 154 | +- `line_width_without_wrap(...)` |
| 155 | +- maybe a small default-color helper if it removes repeated noise |
| 156 | + |
| 157 | +This is a safe warm-up step. |
| 158 | + |
| 159 | +### Step 3. Add `LineBuilder` without changing wrap decisions |
| 160 | + |
| 161 | +Introduce the new helper, but keep the existing wrap functions in place for now. |
| 162 | + |
| 163 | +Use `LineBuilder` only to remove repeated low-level operations: |
| 164 | + |
| 165 | +- creating/resetting lines |
| 166 | +- pushing units |
| 167 | +- placing cursor before and after units |
| 168 | +- handling width-0 units |
| 169 | + |
| 170 | +This step should not change how wrapping decisions are made. |
| 171 | + |
| 172 | +### Step 4. Split no-wrap into two small paths |
| 173 | + |
| 174 | +Today, `finalize_line_for_no_wrap(...)` mixes: |
| 175 | + |
| 176 | +- simple clipping |
| 177 | +- ellipsis behavior |
| 178 | +- cursor handling |
| 179 | + |
| 180 | +That should become: |
| 181 | + |
| 182 | +- `layout_no_wrap_clip(...)` |
| 183 | +- `layout_no_wrap_ellipsis(...)` |
| 184 | + |
| 185 | +Both should use `LineBuilder`. |
| 186 | + |
| 187 | +Why: |
| 188 | + |
| 189 | +- no-wrap becomes easier to understand |
| 190 | +- ellipsis rules stop being buried inside one large function |
| 191 | + |
| 192 | +### Step 5. Rewrite char-wrap on top of `LineBuilder` |
| 193 | + |
| 194 | +Char wrap should become straightforward: |
| 195 | + |
| 196 | +- for each unit |
| 197 | +- mark its start boundary |
| 198 | +- if it does not fit, break the line first |
| 199 | +- push it |
| 200 | +- mark its end boundary |
| 201 | + |
| 202 | +That path should become much shorter after low-level work moves into the builder. |
| 203 | + |
| 204 | +### Step 6. Rewrite word-wrap on top of `LineBuilder` |
| 205 | + |
| 206 | +Word wrap should focus only on segment strategy: |
| 207 | + |
| 208 | +- if a non-space segment fits, place it whole |
| 209 | +- if it does not fit, break first |
| 210 | +- if the segment itself is too wide, fall back to unit-by-unit placement |
| 211 | + |
| 212 | +Again: |
| 213 | + |
| 214 | +- word-wrap decides break policy |
| 215 | +- `LineBuilder` performs actual line mutation |
| 216 | + |
| 217 | +### Step 7. Simplify `layout_text(...)` |
| 218 | + |
| 219 | +After the wrap functions are simplified, `layout_text(...)` should read like a coordinator: |
| 220 | + |
| 221 | +- parse explicit lines |
| 222 | +- choose wrap strategy |
| 223 | +- feed units into the chosen policy |
| 224 | +- finalize lines |
| 225 | +- compute width and height |
| 226 | +- apply final cursor fallback if needed |
| 227 | + |
| 228 | +If `layout_text(...)` still feels busy after this, the refactor is not complete. |
| 229 | + |
| 230 | +### Step 8. Review `measure_min_content(...)` |
| 231 | + |
| 232 | +This function may stay separate, but it should reuse shared helpers where possible. |
| 233 | + |
| 234 | +The goal is not to force it into the builder. |
| 235 | +The goal is to avoid a second hidden wrap engine. |
| 236 | + |
| 237 | +Good candidates for reuse: |
| 238 | + |
| 239 | +- segment width helpers |
| 240 | +- line width helpers |
| 241 | +- source parsing output |
| 242 | + |
| 243 | +## What should not change in this refactor |
| 244 | + |
| 245 | +Keep these stable: |
| 246 | + |
| 247 | +- public behavior of `layout_text(...)` |
| 248 | +- `TextLayoutRequest` |
| 249 | +- `TextLayoutResult` |
| 250 | +- current clipping and ellipsis semantics |
| 251 | +- current byte-index cursor model |
| 252 | + |
| 253 | +This is a structure refactor first, not a feature rewrite. |
| 254 | + |
| 255 | +## Risks |
| 256 | + |
| 257 | +Main risks: |
| 258 | + |
| 259 | +- cursor placement regressions |
| 260 | +- off-by-one ellipsis bugs |
| 261 | +- trailing newline behavior changing |
| 262 | +- width-2 glyph regressions |
| 263 | +- subtle word-wrap changes around spaces |
| 264 | + |
| 265 | +That is why the tests need to come first. |
| 266 | + |
| 267 | +## Verification plan |
| 268 | + |
| 269 | +After each step: |
| 270 | + |
| 271 | +- run Rust tests for the text layout module |
| 272 | +- run `cargo check --manifest-path letui-ffi/Cargo.toml` |
| 273 | + |
| 274 | +After the full refactor: |
| 275 | + |
| 276 | +- run manual smoke checks in the TUI |
| 277 | +- verify multiline input, wrapping, wide glyphs, and clipping still behave correctly |
| 278 | + |
| 279 | +## Recommended implementation order |
| 280 | + |
| 281 | +1. add tests that describe current behavior |
| 282 | +2. extract tiny helpers |
| 283 | +3. introduce `LineBuilder` |
| 284 | +4. migrate no-wrap clip |
| 285 | +5. migrate no-wrap ellipsis |
| 286 | +6. migrate char wrap |
| 287 | +7. migrate word wrap |
| 288 | +8. simplify `layout_text(...)` |
| 289 | +9. review `measure_min_content(...)` |
| 290 | + |
| 291 | +## Expected outcome |
| 292 | + |
| 293 | +If this goes well: |
| 294 | + |
| 295 | +- `text_layout.rs` gets shorter |
| 296 | +- more importantly, it gets less repetitive |
| 297 | +- cursor logic exists in one place |
| 298 | +- line construction exists in one place |
| 299 | +- wrap modes read like policy instead of mini renderers |
| 300 | + |
| 301 | +That is the right tradeoff here. |
| 302 | +The problem is still complex, but the code should stop making the same decision in three different ways. |
0 commit comments