fix: wire the interactive demo's advertised keys to its handler [patch] - #121
Merged
Merged
Conversation
The instructions panel advertised five controls, and every one of them did nothing. UIApplication.ProcessInputAsync offers each key to the root element only, and no shipped element type overrides UIElementBase.OnHandleInput — it returns false — so InteractiveDemo.HandleInput was never reached from anywhere in the assembly. The demo now supplies a root that does override it. No library change was needed: UIContainerBase.HandleInput already walks the children before offering the key to the container, so an application that wants keys only has to provide an element that handles them. HandleInput takes an InputResult and switches on ConsoleKey rather than a key name, which is what the console provider actually produces, and returns whether it acted so an unrecognised key is still reported as unhandled. Two supporting changes fell out of it: - Toggling the theme never worked, because TextStyle.Foreground round-trips through System.Drawing.Color and reads back canonicalised, so the `== "cyan"` comparison never matched and the counter stayed one colour. The comparison is now case-insensitive. - The instructions panel is rendered from the same control list the handler is tested against, so a control cannot be advertised without a key behind it. Demo state moves from static fields to instance state so the key handling can be exercised without one test inheriting another's counter. Fixes #115 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018q8Pwj87WCcqL2HSesWSsg
SonarCloud reported nine MSTEST0046/MSTEST0037 findings on the new test file: StringAssert.Contains and Assert.AreNotEqual(0, …Count) in place of Assert.Contains and Assert.IsNotEmpty. The rest of the suite already uses the newer forms, so this is the file catching up rather than a new convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018q8Pwj87WCcqL2HSesWSsg
|
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.



Fixes #115
The problem
InteractiveDemo's instructions panel advertised ↑/↓, SPACE, R and T, and every one of them did nothing.UIApplication.ProcessInputAsyncoffers each key to the root element and nothing else, and no shipped element type overridesUIElementBase.OnHandleInput— it returnsfalse— soInteractiveDemo.HandleInputhad exactly one reference in the assembly, its own definition.Which of the two options this takes
The issue and its triage left a choice open: remove the false claims, or wire input through properly — with the second flagged as exposing "a genuine gap … no way for application code to receive a key at all".
That gap turns out not to exist.
UIContainerBase.HandleInputalready walks the children and then offers the key to the container itself, andOnHandleInputisprotected virtualon a public base class, so an application that wants keys only has to supply an element that handles them. This PR does exactly that and changes nothing inTUI.Core— the real fix, at no library-design cost, so the honest-but-lesser stopgap was not needed.The demo's root is now an
InputRoutingPanel : StackPanelwhoseOnHandleInputforwards to the demo.The rest of the change
HandleInputtakes anInputResult. It switched on a key name string ("UP","SPACE") that nothing produced;SpectreConsoleProvider.ReadInputAsyncemitsInputResult.FromKey, so it now switches onConsoleKey. It returns whether it acted, so an unrecognised key is reported on screen but still counted unhandled.TextStyle.Foregroundround-trips throughSystem.Drawing.Colorand its getter returns the canonical name, so a style built from"cyan"reads back as"Cyan"andcurrentStyle.Foreground == "cyan"never matched — T left the counter on one colour however many times it was pressed. This is inside the acceptance criterion ("visibly updates the counter/status/theme"), so it is fixed here rather than deferred; the comparison is now case-insensitive.Controls), so a control cannot be advertised on screen without naming a key something has to act on. That is the recurrence guard for this exact bug.RunAsyncis unchanged fromProgram.cs's point of view.Tests
TUI.Testnow referencesTUI.App.TUI.App/AssemblyInfo.csalready carriedInternalsVisibleTo("ktsu.TUI.Test"); the project reference is what makes that reachable.TUI.Test/InteractiveDemoInputTests.cspresses keys throughdemo.RootElement.HandleInput(...)rather than calling the handler directly — the bug was never in the handler, it was that nothing in the element tree reached it, so a test that calledHandleInputitself would have passed before and after.Verified the tests fail without the fix. With the routing reverted (
OnHandleInputback tofalse), 9 of the 11 new tests fail:The two that still pass are correct to:
NonKeyboardInputIsIgnoredassertsfalseeither way, andTKeepsTheCounterBoldholds trivially when nothing changes.The theme half was reverted separately (routing restored, comparison back to
== CounterColour) to confirm it is covered on its own:total: 165 failed: 1—TTogglesTheCounterColourBackAndForth.Full suite: 165/165 pass (154 before this PR).
dotnet build TUI.slnis clean — 0 warnings, 0 errors — acrossnet10.0;net9.0;net8.0.Not verified here
Driving
TUI.Appunder a pseudo-terminal, for the same reason recorded in #117:SetCursorVisibilityissues a cursor-position query (ESC[6n) that nothing in a headless container answers, so the app never gets past hiding the cursor. The acceptance criterion is covered by the tests above, which exercise the sameIUIElement.HandleInputpathProcessInputAsynccalls.🤖 Generated with Claude Code
https://claude.ai/code/session_018q8Pwj87WCcqL2HSesWSsg
Generated by Claude Code