fix: the emergency 'Default' alphabet was selectable and dead (#88) - #88
Merged
Merged
Conversation
Adaptive appends land in <user_dir>/training_<alphabet>.txt, but the startup scan walked the data dir only — every split-dir frontend (Android, GTK, Apple) silently discarded all learning between sessions; Windows survived only because it passes one dir for both. - FileUtils::ScanDirectory(parser, pattern, dir): the walk extracted from ScanFiles, pointed at an explicit directory instead of the process-global data dir; ScanFiles unchanged in behaviour - FileUtils::IsSameDirectory: equivalent()-based with weakly_canonical fallback for not-yet-created dirs - CDasherInterfaceBase: per-context data dir beside the existing per-context user dir (last-create-wins globals owned the training scan before; NCManager now reads interface-owned dirs) - CNodeCreationManager: train from this context's data dir, then its user dir when distinct (single-dir setups scan once — never double) - dasher_capi_version() = 1 so frontends can gate stopgap post-create re-imports and avoid double-counting on newer cores Tests: user-dir training shifts the model; per-context isolation (a context created after another with training must match the clean baseline despite the globals pointing elsewhere); single-dir smoke. 43/43 green. Signed-off-by: will wade <willwade@gmail.com>
Reported live: 'as I drive it I dont see any text go into the output text writing area'. Root cause chain, isolated by a GTK-side engine repro: the user's settings held AlphabetID='Default' — the bare a-z 'last ditch' fallback CAlphIO registered unconditionally — and that alphabet is structurally dead: the engine renders, reports no error, and commits no output however long you drive. - CAlphIO no longer eagerly registers 'Default' (it vanished from pickers and permitted values). GetInfo falls back to the preferred real alphabet; the emergency alphabet is fabricated only when nothing at all loaded — the case it existed for - Realize heals SP_ALPHABET_ID=='Default' to the loaded default AFTER the lazy alphabet load (pickers and the next settings save hold the healed id; the parameter event rides the normal alphabet-switch path). Old settings self-heal on first launch - HandleParameterChange(BP_CONTROL_MODE): null guard — setting control mode pre-realize was a latent null-deref, exposed by the create-flow shift (NCManager ctor reads the flag, so pre-realize sets still apply at realize) Regression test: settings naming Default heal to 'English with limited punctuation' and the canonical drive produces output. 43/43. Signed-off-by: will wade <willwade@gmail.com>
With a custom data dir that has alphabets but NOT the preferred 'English with limited punctuation', GetDefault() returns the literal 'Default' and the heal assignment no-oped back into the dead id. The heal now: preferred if loaded; else bulk-load the data dir's alphabet files (ScanFiles basename-glob — the lazy name index is empty at that point, so listing candidates cannot work) and heal to the first real id that parsed; else leave SP alone (no loadable alphabets = original last-ditch behaviour). New test: custom dir with ONLY alphabet.english.without.punctuation + settings Default -> heals to 'English without punctuation' and the canonical drive produces output. 43/43. Signed-off-by: will wade <willwade@gmail.com>
…#88) The multi-context test exposed more than the flagged bulk-load: the lazy load block's LoadAlphabetFile and ScanNameIndex ALSO route through FileUtils' process-global data directory (last-create-wins), so realizing context A after creating context B loaded B's bundle into A — A could heal to an alphabet its own data dir does not ship. - CAlphIO::ScanNameIndex(dir): scans an explicit directory per-context, global fallback preserved for non-CAPI hosts - Realize: index + loadById + the heal's bulk scan all use this context's m_dataDir via FileUtils::ScanDirectory (#86); loadById basename-normalises index entries (dir-relative paths never match ScanFiles' basename patterns) - New test pins the interleaving: create A(custom)+B(full), realize A after B — A heals to its OWN 'English without punctuation', not B's preferred ChangeAlphabet's post-realize on-demand load still routes globally — single-context-sequential only; same pre-existing class, follow-up. 43/43. Signed-off-by: will wade <willwade@gmail.com>
…low-up on #88) Same class as the realize-path fix: LoadAlphabetFile scans FileUtils' process-global data directory (last-create-wins) — under multiple live contexts an on-demand switch could load the OTHER context's file under this id. Basename-normalised + ScanDirectory(m_dataDir) with the global fallback for hosts that never set a data dir. 43/43. Signed-off-by: will wade <willwade@gmail.com>
#88) The basename normalisation discarded the index's tier-resolved choice: with the same filename in multiple directories (user-dir override vs bundle, v6 vs legacy copy) the basename glob re-parsed every match in traversal order and the filesystem decided which definition won. LoadAlphabetById now: indexed path (weakly-canonicalised when relative) parsed EXACTLY via ScanDirectory's absolute-pattern branch; basename scan only for the synthesized fallback name; both per-context. Shared by the realize lazy loader and ChangeAlphabet's on-demand block. 43/43. Signed-off-by: will wade <willwade@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Live-reported bug
Root cause, isolated by an engine-level repro on the reporter's actual profile: their settings held
AlphabetID="Default"— the bare a–z "last ditch" fallback alphabet thatCAlphIO's constructor registered unconditionally. That alphabet is structurally dead: with it selected, the engine renders, reports no error, and commits no output however long you drive (no space, no newline, no groups). Because it was registered like any real alphabet, it appeared in pickers' permitted values and round-tripped through settings.The editor-contract work was exonerated by the repro: with a fresh profile the same binary drives text into the pane normally.
Fix
CAlphIOno longer eagerly registers"Default"— it disappears from pickers/permitted values.GetInfofalls back to the preferred real alphabet; the emergency alphabet is fabricated (once, lazily) only when nothing at all loaded — the case it was written for.Realize()healsSP_ALPHABET_ID == "Default"to the loaded default, after the lazy alphabet-load block — pickers and the next settings save hold the healed id. Old settings self-heal on first launch after upgrade.HandleParameterChange(BP_CONTROL_MODE): null guard. Setting control mode pre-realize was a latent null-deref (no NCManager yet); exposed by the create-flow shift, fixed properly — the NCManager ctor reads the flag, so pre-realize sets still apply at realize.Two collateral traps dodged (documented in the investigation)
ChangeAlphabetbroke the on-demand alphabet loader andset_string_parameter's stored-value contract (the "English, lower case" comma-normalisation wrinkle: name index vs parsed-name mismatch — worth its own issue).Tests
New regression case in
dasher_alphabet_xml_tests: settings namingDefaultheal to'English with limited punctuation'and the canonical drive produces output. Full matrix 43/43; clang-format clean.Frontend follow-ups
DCO signed.
The PR appears safe to merge; no actionable new defect or outstanding blocking finding remains.
Summary
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Realize context] --> B[Scan this context's alphabet index] B --> C[Load configured alphabet by exact indexed path] C --> D{Stored ID is Default?} D -- No --> G[Create model and node manager] D -- Yes --> E{Preferred alphabet loaded?} E -- Yes --> F[Persist preferred real ID] E -- No --> H[Load real alphabets from this context's data directory] H --> I{Any real alphabet loaded?} I -- Yes --> J[Persist first loaded real ID] I -- No --> K[Retain last-ditch emergency behavior] F --> G J --> G K --> G G --> L[Load bundled training corpus] L --> M{User and data directories differ?} M -- Yes --> N[Load accumulated user training] M -- No --> O[Skip duplicate scan]Reviews (5) · Last reviewed commit: "fix: on-demand load honours the indexed ..."