-
-
Notifications
You must be signed in to change notification settings - Fork 0
[#7] Added fuzzy matching, highlighting and paging to the choice widgets. #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a835c55
7819bf6
6e2819b
23ef844
15d9633
8fbb102
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ | |
| use DrevOps\Tui\Input\KeyMapManager; | ||
| use DrevOps\Tui\Input\Scope; | ||
| use DrevOps\Tui\Input\ScopedKeyMap; | ||
| use DrevOps\Tui\Render\Scroller; | ||
| use DrevOps\Tui\Render\Viewport; | ||
| use DrevOps\Tui\Theme\ThemeInterface; | ||
|
|
||
| /** | ||
|
|
@@ -18,6 +20,11 @@ | |
| */ | ||
| abstract class AbstractWidget implements WidgetInterface { | ||
|
|
||
| /** | ||
| * The page size applied when a field declares none. | ||
| */ | ||
| public const int DEFAULT_PAGE_SIZE = 10; | ||
|
|
||
| /** | ||
| * The resolved key bindings for this widget's scope. | ||
| * | ||
|
|
@@ -26,6 +33,21 @@ abstract class AbstractWidget implements WidgetInterface { | |
| */ | ||
| protected ?ScopedKeyMap $scoped = NULL; | ||
|
|
||
| /** | ||
| * The number of option rows shown at once before the list pages. | ||
| */ | ||
| protected int $pageSize = self::DEFAULT_PAGE_SIZE; | ||
|
|
||
| /** | ||
| * The index of the first visible option row under paging. | ||
| */ | ||
| protected int $offset = 0; | ||
|
|
||
| /** | ||
| * The fuzzy matcher, created on first use. | ||
| */ | ||
| protected ?Matcher $matcher = NULL; | ||
|
|
||
| /** | ||
| * Whether a valid value has been accepted. | ||
| */ | ||
|
|
@@ -189,6 +211,125 @@ protected function renderRadioRow(ThemeInterface $theme, string $label, bool $cu | |
| return $theme->radio($current) . ' ' . $this->highlightLabel($theme, $label, $current); | ||
| } | ||
|
|
||
| /** | ||
| * The shared fuzzy matcher. | ||
| * | ||
| * @return \DrevOps\Tui\Widget\Matcher | ||
| * The matcher. | ||
| */ | ||
| protected function matcher(): Matcher { | ||
| return $this->matcher ??= new Matcher(); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the effective page size, rejecting a non-positive declared value. | ||
| * | ||
| * The builder rejects a non-positive page size, but a widget may be | ||
| * constructed directly, so the invariant is enforced here too. | ||
| * | ||
| * @param int|null $pageSize | ||
| * The declared page size, or NULL to use the default. | ||
| * | ||
| * @return int | ||
| * The effective page size. | ||
| * | ||
| * @throws \InvalidArgumentException | ||
| * When a declared page size is not positive. | ||
| */ | ||
| protected function resolvePageSize(?int $pageSize): int { | ||
| if ($pageSize !== NULL && $pageSize < 1) { | ||
| throw new \InvalidArgumentException(sprintf('Page size must be a positive integer, %d given.', $pageSize)); | ||
| } | ||
|
|
||
| return $pageSize ?? self::DEFAULT_PAGE_SIZE; | ||
| } | ||
|
Comment on lines
+224
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Add a Static analysis flags the inline fully-qualified 🧹 Proposed fix+use InvalidArgumentException;
+
abstract class AbstractWidget implements WidgetInterface {- throw new \InvalidArgumentException(sprintf('Page size must be a positive integer, %d given.', $pageSize));
+ throw new InvalidArgumentException(sprintf('Page size must be a positive integer, %d given.', $pageSize));🧰 Tools🪛 PHPMD (2.15.0)[error] 241-241: Missing class import via use statement (line '241', column '17'). (undefined) (MissingImport) 🤖 Prompt for AI AgentsSource: Linters/SAST tools
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the fully-qualified This matches the existing convention in the codebase - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n 'InvalidArgumentException' src/Theme/DefaultTheme.php src/**/*.php -g '*.php' | head -50Length of output: 8229
🧬 Code Graph Analysis ResultsRelevant snippet:
|
||
|
|
||
| /** | ||
| * Compute the cursor-visible paging window, storing its offset. | ||
| * | ||
| * @param int $total | ||
| * The total number of option rows. | ||
| * @param int $cursor | ||
| * The cursor row index (a negative cursor pins the window to the top). | ||
| * | ||
| * @return \DrevOps\Tui\Render\Viewport | ||
| * The window: its offset and whether rows are scrolled off above or below. | ||
| */ | ||
| protected function pageViewport(int $total, int $cursor): Viewport { | ||
| $viewport = (new Scroller())->compute($total, $this->pageSize, max(0, $cursor), $this->offset); | ||
| $this->offset = $viewport->offset; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| return $viewport; | ||
| } | ||
|
|
||
| /** | ||
| * Style an option label, emphasising the query-matched characters. | ||
| * | ||
| * The label is split into runs of matched and unmatched characters, each run | ||
| * styled on its own so no SGR code nests inside another: matched runs get the | ||
| * match colour, and on the cursor row the rest keeps the highlight colour. | ||
| * With no matched positions this is exactly {@see highlightLabel()}. | ||
| * | ||
| * @param \DrevOps\Tui\Theme\ThemeInterface $theme | ||
| * The theme. | ||
| * @param string $label | ||
| * The option label. | ||
| * @param list<int> $positions | ||
| * The zero-based indices of the matched characters. | ||
| * @param bool $current | ||
| * Whether the option's row holds the cursor. | ||
| * | ||
| * @return string | ||
| * The styled label. | ||
| */ | ||
| protected function renderMatchedLabel(ThemeInterface $theme, string $label, array $positions, bool $current): string { | ||
| if ($positions === []) { | ||
| return $this->highlightLabel($theme, $label, $current); | ||
| } | ||
|
|
||
| $matched = array_fill_keys($positions, TRUE); | ||
| $out = ''; | ||
| $run = ''; | ||
| $run_matched = FALSE; | ||
|
|
||
| foreach (mb_str_split($label) as $index => $char) { | ||
| $is_matched = isset($matched[$index]); | ||
|
|
||
| if ($run !== '' && $is_matched !== $run_matched) { | ||
| $out .= $this->styleRun($theme, $run, $run_matched, $current); | ||
| $run = ''; | ||
| } | ||
|
|
||
| $run .= $char; | ||
| $run_matched = $is_matched; | ||
| } | ||
|
|
||
| return $out . $this->styleRun($theme, $run, $run_matched, $current); | ||
| } | ||
|
|
||
| /** | ||
| * Style one run of same-kind characters for {@see renderMatchedLabel()}. | ||
| * | ||
| * @param \DrevOps\Tui\Theme\ThemeInterface $theme | ||
| * The theme. | ||
| * @param string $run | ||
| * The run of characters. | ||
| * @param bool $matched | ||
| * Whether the run's characters matched the query. | ||
| * @param bool $current | ||
| * Whether the option's row holds the cursor. | ||
| * | ||
| * @return string | ||
| * The styled run. | ||
| */ | ||
| protected function styleRun(ThemeInterface $theme, string $run, bool $matched, bool $current): string { | ||
| if ($matched) { | ||
| return $theme->highlightMatch($run); | ||
| } | ||
|
|
||
| return $current ? $theme->highlight($run) : $run; | ||
| } | ||
|
|
||
| /** | ||
| * Validate and, when valid, transform a value and complete the widget. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace DrevOps\Tui\Widget; | ||
|
|
||
| /** | ||
| * The outcome of matching a query against a candidate label. | ||
| * | ||
| * Carries the relevance score used to rank a candidate against its peers and | ||
| * the label character indices the query matched, used to highlight them. | ||
| * | ||
| * @package DrevOps\Tui\Widget | ||
| */ | ||
| final readonly class MatchResult { | ||
|
|
||
| /** | ||
| * Construct a match result. | ||
| * | ||
| * @param int $score | ||
| * The relevance score: higher ranks ahead. Tiered so an exact match always | ||
| * outranks a prefix, a prefix a substring, and a substring a looser | ||
| * subsequence, whatever the finer refinement within a tier. | ||
| * @param list<int> $positions | ||
| * The zero-based indices of the matched characters in the candidate, in | ||
| * ascending order, for highlighting. | ||
| */ | ||
| public function __construct( | ||
| public int $score, | ||
| public array $positions, | ||
| ) { | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace DrevOps\Tui\Widget; | ||
|
|
||
| /** | ||
| * How closely a candidate matches a query, coarsest band first. | ||
| * | ||
| * A tighter tier always outranks a looser one regardless of the finer | ||
| * within-tier refinement, so the tier's weight dominates the match score. | ||
| * | ||
| * @package DrevOps\Tui\Widget | ||
| */ | ||
| enum MatchTier { | ||
|
|
||
| // The candidate equals the query (case-insensitively). | ||
| case Exact; | ||
|
|
||
| // The query is a prefix of the candidate. | ||
| case Prefix; | ||
|
|
||
| // The query appears in the candidate as a contiguous substring. | ||
| case Substring; | ||
|
|
||
| // The query's characters appear in order but not contiguously. | ||
| case Subsequence; | ||
|
|
||
| /** | ||
| * The ranking weight: a higher weight ranks ahead. | ||
| * | ||
| * @return int | ||
| * The weight, from 4 (exact) down to 1 (subsequence). | ||
| */ | ||
| public function weight(): int { | ||
| return match ($this) { | ||
| self::Exact => 4, | ||
| self::Prefix => 3, | ||
| self::Substring => 2, | ||
| self::Subsequence => 1, | ||
| }; | ||
| } | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: drevops/tui
Length of output: 5700
🏁 Script executed:
Repository: drevops/tui
Length of output: 6579
🏁 Script executed:
Repository: drevops/tui
Length of output: 13629
🏁 Script executed:
Repository: drevops/tui
Length of output: 25702
🏁 Script executed:
Repository: drevops/tui
Length of output: 16851
Keep
ThemeInterfacebackward compatibleAdding
highlightMatch()here breaks any custom theme implementation. Make it optional via a separate interface/trait, or treat this as a major-version API break and document the migration.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping
highlightMatch()onThemeInterface.The documented and only supported way to build a theme is to extend
DefaultTheme(see its class docblock), which implementshighlightMatch(), so consumer themes are unaffected.ThemeInterfacedeliberately declares every themeable atom as a required method -title,value,marker,radio,check,caret,heading,dividerand ~40 others - and each widget feature has added its atom here as the library has grown;highlightMatch()follows that same established pattern rather than being a special case.The package is pre-1.0 with no published stable-API contract yet, so there is no compatibility guarantee to break. Splitting a single atom into an optional secondary interface (forcing every call site to
instanceof-check the theme) would be inconsistent with the other atoms and add complexity for no practical gain given the extend-DefaultThememodel.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexSkrypnykThanks for the clarification — that makes sense. Given the pre-1.0 status, the documented "extendDefaultTheme" convention, and the consistency with how every other themeable atom is declared as a required method onThemeInterface, addinghighlightMatch()here is the right call. Withdrawing this concern.