Converged naming, import and demo conventions, and fixed four defects it surfaced. - #138
Conversation
The dominant convention was already case-insensitive alphabetical ordering in 190 of 235 files with two or more imports. The 45 outliers hoisted a Model import above an alphabetically earlier namespace, most often placing Model above Input.
…ses. Every other data provider in the suite yields string-keyed rows so a failing case names itself. Keying the last two also lets the annotated one narrow its return key type from the (int|string) union to string, matching every documented provider.
The other 21 documented providers in the suite open with "Data provider for testX()." naming the test they feed.
… bare. The other 60 demos that catch an interrupt carry the same one-line explanation above their exit code.
Demo scripts terminate output with PHP_EOL and spell newlines inside single-quoted sample data as chr(10), which keeps the single-quote rule. The glyph gallery echoed a raw newline instead, and the output-text demo carried a double-quoted markdown sample. Line splitting and joining of rendered views keeps its literal newline, since a view is joined that way regardless of platform.
📝 WalkthroughWalkthroughThe change updates widget constructor naming and state initialization, adjusts Unicode-aware rendering and file filtering, changes a modal exception type, centralizes rendering test themes, reorganizes PHPUnit groups, and synchronizes documentation and playground examples. ChangesWidget rendering and maintenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🚀 Deployed on https://6a699f45637bef28a9ba5bb0--tui-docs.netlify.app |
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
=======================================
Coverage 99.09% 99.09%
=======================================
Files 119 119
Lines 4645 4650 +5
=======================================
+ Hits 4603 4608 +5
Misses 42 42 ☔ View full report in Codecov by Harness. |
The nine widget constructors spelled the same seed value five ways: buffer, default, current, value and point. They all name one thing, the value a widget opens with, and every caller passes it positionally. Confirm and Suggest give up constructor promotion so the parameter can be named for what a caller passes while the property stays named for the live state it holds, and Rector is told to leave that pairing alone in Confirm.
Modal was the one Model class rejecting a declaration with an SPL exception; the other six throw the library's own 'FormException'. The constructor now documents the throw, as five of those siblings already do.
The file picker matched and ordered user-visible entry names with byte-level 'strtolower' and 'strcasecmp', which leave anything outside ASCII untouched, so a lowercase accented query never matched its uppercase entry. Every other widget folds the same kind of text through the mbstring-aware helper. A regression test covers a non-ASCII name.
The grid preview and the summary line both emitted the Unicode ellipsis whatever the display mode, so a terminal without Unicode was handed a glyph it cannot draw. The grid preview now falls back to three dots, and the summary line clips to the full width instead, which is what a table cell already does when it cannot spend a column on the marker. A layout test asserted the glyph while building an ASCII theme, so it was encoding the bug.
The group attribute followed two rival conventions: a directory name on 62 classes and a blanket 'tui' on the rest, which meant no group could select a subsystem. The directory name now decides it everywhere, so the theme, render, translation, primitive and remaining model suites are selectable on their own. Tests at the root of Unit keep 'tui', matching the facade they cover at the root of src. No configuration filtered on these names.
One helper name meant three different themes across the suite, and its body was copied into eight classes. Two of those copies had drifted apart: the output and progress primitive tests defaulted colour opposite ways, so the same call produced different themes. The trait now owns both shared themes, the output test states the colour it relies on rather than leaning on a default, and the conditional-indent helper is renamed for the one thing it varies.
The table demo listed a blackberry and the option-group demo a rhubarb, neither of which is in the reference vocabulary, and the first is also a brand name the reference bans outright. They become a plum and a cherry, which the reference already names as canonical select options. The screenshot specs, the pty recording anchor and the documentation code blocks carry the same values, so all 24 affected SVGs were regenerated and audited.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Theme/DefaultTheme.php`:
- Around line 1586-1587: Update the grid sizing performed by
measureColumnBlock() and measureValueWidth() to include the width of the
multiline marker—three columns for the ASCII '...' fallback and one for the
Unicode ellipsis—when measuring the value block. Ensure multiline values reserve
space for the appended marker, and add an assertion covering sizing when the
first line is widest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a5ba032-0408-4aef-8692-67bbd1de6d87
⛔ Files ignored due to path filters (24)
docs/assets/widget-select-groups-dark-static-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-dark-static-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-dark-static-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-dark-static.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-light-static-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-light-static-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-light-static-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-light-static.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-animated-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-animated-ascii.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-animated-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-animated.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-static-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-static-ascii.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-static-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-dark-static.svgis excluded by!**/*.svgdocs/assets/widget-table-light-animated-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-light-animated-ascii.svgis excluded by!**/*.svgdocs/assets/widget-table-light-animated-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-light-animated.svgis excluded by!**/*.svgdocs/assets/widget-table-light-static-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-light-static-ascii.svgis excluded by!**/*.svgdocs/assets/widget-table-light-static-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-table-light-static.svgis excluded by!**/*.svg
📒 Files selected for processing (91)
docs/content/widgets/option-groups.mdxdocs/content/widgets/table.mdxdocs/util/render-widget-svgs.phpdocs/util/update-assets.phpplayground/02-widgets-select-groups.phpplayground/02-widgets-table.phpplayground/11-display-modes-glyph-gallery.phpplayground/16-loading-data.phpplayground/17-query-options.phpplayground/18-output-text.phpplayground/19-dynamic-options.phpplayground/themes/OceanTheme.phprector.phpsrc/Builder/FieldBuilder.phpsrc/Builder/Form.phpsrc/Engine/Engine.phpsrc/Model/Modal.phpsrc/Render/PanelController.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Testing/TuiTester.phpsrc/Theme/DefaultTheme.phpsrc/Tui.phpsrc/Widget/CalendarWidget.phpsrc/Widget/Capability/SelectionCapableTrait.phpsrc/Widget/ConfirmWidget.phpsrc/Widget/FilePickerWidget.phpsrc/Widget/NumberWidget.phpsrc/Widget/PasswordWidget.phpsrc/Widget/PauseWidget.phpsrc/Widget/RatingWidget.phpsrc/Widget/ReorderWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SuggestWidget.phpsrc/Widget/TemplateWidget.phpsrc/Widget/TextWidget.phpsrc/Widget/TextareaWidget.phpsrc/Widget/ToggleWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/BuildsThemesTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Derive/DeriveTest.phptests/phpunit/Unit/Engine/EngineConditionalTest.phptests/phpunit/Unit/Input/HintTest.phptests/phpunit/Unit/Input/KeyMapTest.phptests/phpunit/Unit/Model/ButtonsTest.phptests/phpunit/Unit/Model/DateBoundsTest.phptests/phpunit/Unit/Model/ModalTest.phptests/phpunit/Unit/Model/PanelTest.phptests/phpunit/Unit/Model/TableSpecTest.phptests/phpunit/Unit/Primitive/OutputTest.phptests/phpunit/Unit/Primitive/ProgressTest.phptests/phpunit/Unit/ProgressableTest.phptests/phpunit/Unit/Render/AnsiTest.phptests/phpunit/Unit/Render/BoxTest.phptests/phpunit/Unit/Render/ExternalEditorTest.phptests/phpunit/Unit/Render/MarkupTest.phptests/phpunit/Unit/Render/NavigatorTest.phptests/phpunit/Unit/Render/OverlayTest.phptests/phpunit/Unit/Render/PanelControllerTest.phptests/phpunit/Unit/Render/ScrollerTest.phptests/phpunit/Unit/Render/TableTest.phptests/phpunit/Unit/Render/TerminalControlTest.phptests/phpunit/Unit/Render/TerminalTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Testing/AllWidgetsFormTest.phptests/phpunit/Unit/Testing/KeyEncoderTest.phptests/phpunit/Unit/Testing/KeyStreamTest.phptests/phpunit/Unit/Theme/BuiltinThemesTest.phptests/phpunit/Unit/Theme/OutputRenderTest.phptests/phpunit/Unit/Theme/ProgressRenderTest.phptests/phpunit/Unit/Theme/ScaleRenderTest.phptests/phpunit/Unit/Theme/ThemeConditionalIndentTest.phptests/phpunit/Unit/Theme/ThemeFullscreenTest.phptests/phpunit/Unit/Theme/ThemeLayoutTest.phptests/phpunit/Unit/Theme/ThemeManagerTest.phptests/phpunit/Unit/Theme/ThemeOptionsTest.phptests/phpunit/Unit/Theme/ThemeRenderTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Translation/ChromeCatalogTest.phptests/phpunit/Unit/Translation/TranslationRenderTest.phptests/phpunit/Unit/Translation/TranslatorTest.phptests/phpunit/Unit/TuiTest.phptests/phpunit/Unit/Widget/CalendarWidgetTest.phptests/phpunit/Unit/Widget/FilePickerWidgetTest.phptests/phpunit/Unit/Widget/NumberWidgetTest.phptests/phpunit/Unit/Widget/PasswordDisplayTest.phptests/phpunit/Unit/Widget/PasswordWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/TextareaWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
A grid cell renders one line plus a there-is-more marker, but the column was measured from the raw value lines, so a value whose first line is its widest was sized short and clipped the marker. Rendering and measuring now share one preview method, which cannot drift. The gap predates the ASCII fallback, which only widened it from one column to three.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Theme/DefaultTheme.php`:
- Line 2201: Update the width calculation in the surrounding table-rendering
method to use ANSI-aware width measurement for the rendered result of
columnValuePreview($field, $answers), replacing Strings::length() with
Ansi::width() while preserving the existing label and indentation calculation.
Add a regression case covering a colored progress, rating, or loading preview.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ce8c289-71b1-47be-ba83-8e74cdc269e5
📒 Files selected for processing (2)
src/Theme/DefaultTheme.phptests/phpunit/Unit/Theme/ThemeLayoutTest.php
A progress, rating or loading value is painted, so measuring its raw string counted every escape sequence as a column: a rating measured 30 wide where it draws 9. Both the grid cell and the label/value row now measure visible width. The row path carried the same fault before the grid cell shared its preview.
|
Summary
An idempotent consistency pass that unifies conventions which had drifted as the codebase grew. Every source, test and playground file was read in full and mapped before any change, so each decision below rests on a measured count rather than an impression.
Convergences backed by a project rule or a clear dominant majority were applied directly. The judgment calls were put to a human and answered, and their outcomes are included here. Two questions turned out not to be rename decisions at all and became follow-up issues instead: #139 and #140.
Along the way the pass surfaced four genuine defects, all of which predate this branch. They are described under Defects fixed below.
Changes
Conventions converged
usestatements alphabetically acrosssrc/,tests/andplayground/; the dominant convention was already case-insensitive alphabetical in 190 of 235 files with two or more imports. Worth recording for future passes: the sort is case-INsensitive, soorg\bovigocorrectly precedesPHPUnit. A byte-order sort would flip the 11 vfsStream test files backwards and thrash on the next run.$buffer,$default,$current,$value,$point); they are now uniformly$default. Confirm and Suggest give up constructor promotion so the parameter can be named for what a caller passes while the property stays named for the live state it holds, and Rector is told to leave that pairing alone in Confirm.#[Group]attribute followed two rival conventions, a directory name on 62 classes and a blankettuion the rest, which meant no group could select a subsystem. The directory name now decides it everywhere. Tests at the root ofUnitkeeptui, matching the facade they cover at the root ofsrc.BuildsThemesTraitnow owns both shared themes.(int|string)union tostring.Data provider.docblocks now name the test they feed, matching the 21 siblings that useData provider for testX()..Model\Modalwas the one Model class rejecting a declaration with an SPL exception; it now throws the library's ownFormExceptionlike its six siblings.// Leave quietly on Ctrl-C.explanation now carry it. Behaviour was already identical in all 64.PHP_EOL(77 of 78 siblings) and newlines inside single-quoted sample data usechr(10)(5 sibling sites), which keeps the project's single-quote rule. Line splitting and joining of rendered views deliberately keeps its literal"\n", since a view is joined that way on every platform.Defects fixed
All four predate this branch.
strtolower/strcasecmp, which leave anything outside ASCII untouched, so a lowercase accented query never matched its uppercase entry. Every other widget folds the same kind of text through the mbstring-aware helper.Deferred to follow-up issues
*violation()and*error()both earn their keep for the identical?stringcontract. Not a straightforward rename: the fragment-versus-sentence axis does not track the split, andEngine::optionsError()already uses the same suffix for a different contract.PanelControllerTesthas 80 tests and no providers;FormTesthas 20 near-identical...Throwsmethods).Verification
composer testandcomposer lint(PHPCS, PHPStan level 9, Rector) were run and green after every individual change, not just at the end. The suite grew from 2210 to 2215 tests as the four defects gained regression coverage. Playground scripts are not covered by the suite, so the changed demos were executed directly and their output confirmed, and both regenerated SVG sets were opened and visually checked.Re-running the whole convergence produces zero further changes, so the result is idempotent.
Before / After
The largest class is import ordering. A representative file had
Model\...hoisted aboveInput\...andPHPUnitaboveorg\bovigo; after the pass both are sorted case-insensitively:And the grid cell, whose column was measured without the marker it renders: