|
| 1 | +# M7 — `$ctx` threading plan (de-globalisation finish) |
| 2 | + |
| 3 | +The pure-parser/geometry extractions (REFACTOR.md §3.1–3.2, first half) are |
| 4 | +done: `CCFE::MenuFile`, `FormFile`, `Config`, `Action`, `Layout` are split out, |
| 5 | +unit-tested, and released in 2.1.1. What remains of M7 is the harder half: |
| 6 | +**replace the package globals and `local` dynamic scope in `ccfe.pl` with an |
| 7 | +explicit `$ctx` state object** (REFACTOR.md §3.2, bullet 3 — "the spookiest |
| 8 | +action-at-a-distance in the code"). |
| 9 | + |
| 10 | +This file is the execution plan: what to thread, in what order, and where to |
| 11 | +stop. Every step keeps all **302 tests green** and the `ccfe-plugin-sysmon` |
| 12 | +conformance fixture installing and running unchanged. |
| 13 | + |
| 14 | +--- |
| 15 | + |
| 16 | +## 1. Inventory — not all globals are equal |
| 17 | + |
| 18 | +`ccfe.pl` (~5,200 lines, 46 subs, no `use strict`) declares its globals as |
| 19 | +barewords at file scope (`:76–276`). They fall into four groups with very |
| 20 | +different threading value: |
| 21 | + |
| 22 | +| Group | Examples | Count | Mutated after startup? | Thread it? | |
| 23 | +|-------|----------|-------|------------------------|------------| |
| 24 | +| **A. Constants** | `$NO/$YES`, `$ES_*`, type consts (`$STRING`…), row dims (`$MS_*`,`$FS_*`,`$RS_*`), `%bool_vals`/`%type_vals`/`%sep_type_vals`, margins | ~120 | No | **No** — move to `CCFE::Const`, leave read-only | |
| 25 | +| **B. Config settings** | `$LAYOUT`, `$HIDE_CURSOR`, `$ENABLE_MOUSE`, `$SHOW_DOTS`, `$FIELD_VALUE_POS`, the 17 colour-attr vars (`$labelFg`…`$MENU_*_ATTR`), `%keys`, `$RESTRICTED`/`@RESTRICTED_ALLOW` | ~30 | Once, by `load_config` | **Yes** — one read-only `$ctx->{cfg}` sub-object | |
| 26 | +| **C. Runtime mutable state** | `%form` (~160 sites), `%menu` (~38), `%field_vals` (8), `@fp`, `$cform`, `$SCREEN_DIR`, `$last_item_id`, `$pad_lines`, `$exec_args` | ~250 sites | Continuously, per screen | **Yes** — this is the real target | |
| 27 | +| **D. Curses / process** | `$LINES`, `$COLS` (Curses-owned, 81 reads), `$cpid`/`$tmpfh` (SIGINT handler) | — | By the library / signals | **No** — not ours; leave | |
| 28 | + |
| 29 | +**Key decision:** scope `$ctx` to groups **B + C**. Threading the ~120 group-A |
| 30 | +constants through every signature is pure noise for zero correctness gain; they |
| 31 | +become an imported constant namespace instead. This roughly halves the churn |
| 32 | +and the risk versus a literal "thread all `:76–276`" reading of REFACTOR.md. |
| 33 | + |
| 34 | +## 2. The hard part — the nested-sub trap |
| 35 | + |
| 36 | +`do_form`, `do_menu` and `run_browse` use `local %form / @fp / $cform / |
| 37 | +%field_vals / %menu` and define **named** inner subs that read them: |
| 38 | + |
| 39 | +- in `do_form`: `sync_fields_val`, `set_field_attr`, `set_field_active_attr`, |
| 40 | + `check_val_changes`, `prepare_action`, `save_persistent`, `load_persistent` |
| 41 | +- in `run_browse`: `get_search_buff`, `search_next`, `search_all`, `load_pad` |
| 42 | +- in `init_footer`: `sort_fnkeys` |
| 43 | + |
| 44 | +A Perl **named** sub does *not* close over a per-call lexical — it binds the |
| 45 | +first call's copy. These inner subs work today *only* because `local` makes the |
| 46 | +globals dynamically scoped, so each `do_form` call's `local %form` is the one |
| 47 | +the inner subs see for the duration of that call. Removing `local %form` breaks |
| 48 | +that unless each inner sub is converted to **either**: |
| 49 | + |
| 50 | +- **(a) an anonymous closure** `my $sync = sub { ... $ctx ... };` created inside |
| 51 | + the call (genuinely closes over that call's `my $ctx`), **or** |
| 52 | +- **(b) a top-level sub taking `$ctx` explicitly** `sync_fields_val($ctx, …)`. |
| 53 | + |
| 54 | +Recommendation: **(b)** for subs that are conceptually standalone |
| 55 | +(`save_persistent`, `prepare_action`, `load_persistent` — they belong at file |
| 56 | +scope and are independently testable once they take `$ctx`); **(a)** only for |
| 57 | +the few that are truly part of the loop's inner machinery. Solving this on the |
| 58 | +*smallest* structure first (Phase 1) de-risks it before `%form`. |
| 59 | + |
| 60 | +## 3. Phased, individually-shippable steps |
| 61 | + |
| 62 | +Each phase is one (or a few) commits, each keeping the full suite + sysmon |
| 63 | +fixture green. The pty tests (`t/03` smoke, `t/08` multipage, `t/10` resize) are |
| 64 | +the integration guard — they drive `do_menu`/`do_form` with real state, so a |
| 65 | +threading mistake shows up as a crash or a mis-drawn screen, not a silent pass. |
| 66 | + |
| 67 | +### Phase 0 — container + constants namespace *(low risk, mechanical)* |
| 68 | +- Add `CCFE::Const` (or a `ctx_new()` constructor) and a `$ctx` hashref built |
| 69 | + once in the main body. Seed `$ctx->{cfg}` empty. |
| 70 | +- No behaviour change; nothing reads `$ctx` yet. Lands the scaffolding + the |
| 71 | + group-A constants move so later diffs are pure threading, not mixed. |
| 72 | + |
| 73 | +### Phase 1 — `%field_vals` → `$ctx->{field_vals}` *(8 sites — the pattern-setter)* |
| 74 | +- Smallest structure, but it is read by the **nested** `sync_fields_val` / |
| 75 | + `save_persistent` / `load_persistent`, so this phase *is* where the §2 |
| 76 | + nested-sub conversion gets proven on a small surface. |
| 77 | +- Convert those three inner subs to take `$ctx` (option b). Replace `local |
| 78 | + %field_vals` with `$ctx->{field_vals} = {}` per `do_form` call. |
| 79 | +- **Gate:** `t/08` (field defaults/values) + persistent-save tests must pass. |
| 80 | + |
| 81 | +### Phase 2 — `%menu` → `$ctx->{menu}` *(38 sites)* |
| 82 | +- `load_menu` fills `$ctx->{menu}` (it already returns a structure from |
| 83 | + `CCFE::MenuFile`; just stop copying into the global). `do_menu`'s `local |
| 84 | + %menu` → `my $menu` passed into the draw closures. |
| 85 | +- **Gate:** `t/03` smoke, 1-item-menu regression (`t/02`). |
| 86 | + |
| 87 | +### Phase 3 — `%form` + `@fp` + `$cform` → `$ctx->{form}` *(~160 sites — the big one)* |
| 88 | +- The bulk of the work and of the value: this is what kills the `local %form` |
| 89 | + dynamic scope. `do_form`'s `local %form/@fp/$cform` become a per-call form |
| 90 | + sub-object; `load_form`, `redraw_form_page`, `resize_form` and the remaining |
| 91 | + inner subs take it explicitly. |
| 92 | +- Split if needed: **3a** convert the 7 inner subs to `$ctx`-taking form |
| 93 | + (behaviour-preserving, globals still present); **3b** flip `%form` itself to |
| 94 | + `$ctx->{form}` and delete the `local`. |
| 95 | +- **Gate:** `t/08` multipage, `t/10` resize, `t/11` layout — the screens that |
| 96 | + have historically broken. |
| 97 | + |
| 98 | +### Phase 4 — config settings → `$ctx->{cfg}` *(~30 vars, incl. 17 colour attrs)* |
| 99 | +- `load_config`'s per-section dispatch writes `$ctx->{cfg}{...}`; the |
| 100 | + `eval "$VAR = ..."` colour assignments become `eval "\$ctx->{cfg}{labelFg} = |
| 101 | + ..."` (still evaluated in `ccfe.pl`'s package for the colour helpers). |
| 102 | +- Read-only after startup, so readers are a mechanical `$X` → `$ctx->{cfg}{X}`. |
| 103 | +- **Gate:** `t/06` colour/theme integration. |
| 104 | + |
| 105 | +### Phase 5 — residual scalar runtime state |
| 106 | +- `$SCREEN_DIR`, `$last_item_id`, `$pad_lines`, `$exec_args`, `$child_es`. |
| 107 | +- `$cpid`/`$tmpfh` stay global (the SIGINT handler closure owns them) — document |
| 108 | + why rather than forcing them in. |
| 109 | + |
| 110 | +### Phase 6 — capstone: modern pragma on `ccfe.pl` *(the correctness payoff)* |
| 111 | +- Only reachable once barewords are gone (group A imported, B/C on `$ctx`, |
| 112 | + remaining locals `my`-declared). Turn on strict+warnings, fix the fallout |
| 113 | + (undeclared vars, indirect-object calls, undef comparisons the no-strict code |
| 114 | + hid). Then tighten `.perlcriticrc` to lint `ccfe.pl` too, not just `src/lib`. |
| 115 | +- **Feature level:** use **`use v5.36`** to match the existing `lib/CCFE/*` |
| 116 | + modules (it already gives strict + warnings + signatures + `say` — everything |
| 117 | + the capstone needs). A newer level (`v5.38`/`v5.40`) is fine if a specific |
| 118 | + feature ever justifies it; the version is not sacred. **Whatever level is |
| 119 | + chosen, `debian/control` must declare the matching `perl (>= X)`** in both |
| 120 | + `Depends` and `Build-Depends`. |
| 121 | +- **Latent packaging gap to close here (or sooner):** the shipped 2.1.1 deb |
| 122 | + already requires Perl 5.36 (the `lib/CCFE/*` modules `use v5.36`) but |
| 123 | + `debian/control` still only says `perl` with no version bound. Add the |
| 124 | + `perl (>= 5.36.0)` floor as part of this phase — or pull it forward as a tiny |
| 125 | + standalone fix, since it is already technically under-specified today. |
| 126 | + |
| 127 | +## 4. Where the value is — and where to stop |
| 128 | + |
| 129 | +``` |
| 130 | +value ████████████████ Phase 3 (%form / local removal) ← ~70% of the win |
| 131 | + █████████ Phases 1–2 (field_vals, menu) |
| 132 | + ████ Phase 4 (cfg object) |
| 133 | + ███ Phase 6 (strict capstone, unlocks tighter gates) |
| 134 | + █ Phases 0,5 (scaffolding, residue) |
| 135 | +``` |
| 136 | + |
| 137 | +**Scope decision (agreed):** all phases **0–6 are in scope for M7** — the full |
| 138 | +de-globalisation, finishing with the strict capstone. The value bars above set |
| 139 | +the *order*, not the cut line: lead with Phase 3 (highest value), and treat 4–5 |
| 140 | +as required-but-lower-priority rather than optional. |
| 141 | + |
| 142 | +**Stop-and-reassess gate:** still observe a checkpoint after Phase 3. If the |
| 143 | +inner-sub conversion proves more invasive than the test coverage can safely |
| 144 | +guard, pause and widen the tests before pressing on — the `local`-based code is |
| 145 | +*correct and shipping*, so a half-threaded `%form` hybrid is worse than a clean |
| 146 | +stop. De-globalisation is a quality investment, not a user-facing fix; it should |
| 147 | +never trade away a working screen. (Per the agreed scope we resume to the end |
| 148 | +once the checkpoint is clear; the gate is a safety valve, not the finish line.) |
| 149 | + |
| 150 | +## 5. Effort / risk summary |
| 151 | + |
| 152 | +| Phase | Surface | Risk | Notes | |
| 153 | +|-------|---------|------|-------| |
| 154 | +| 0 | scaffolding + ~120 consts | low | mechanical move | |
| 155 | +| 1 | 8 sites + 3 inner subs | **med** | proves the nested-sub conversion | |
| 156 | +| 2 | 38 sites | low–med | `load_menu` already returns a structure | |
| 157 | +| 3 | ~160 sites + 7 inner subs | **high** | the core; split 3a/3b | |
| 158 | +| 4 | ~30 vars | low–med | eval targets change shape | |
| 159 | +| 5 | ~5 scalars | low | leave signal state global | |
| 160 | +| 6 | whole file | med | strict fallout, one-time | |
| 161 | + |
| 162 | +No new runtime dependencies; `$ctx` is a plain hashref. Each phase is its own |
| 163 | +reviewable commit with the standard trailer, gated on the four CI checks. |
0 commit comments