diff --git a/.agent/plans/dense_css_property_ids_and_bitset.md b/.agent/plans/dense_css_property_ids_and_bitset.md deleted file mode 100644 index c89549afe..000000000 --- a/.agent/plans/dense_css_property_ids_and_bitset.md +++ /dev/null @@ -1,560 +0,0 @@ -# Dense CSS Property and Shorthand IDs / Bitset Migration Plan - -Status: implementation-ready plan, 2026-08-05. - -## Goal - -Replace hash-valued CSS `PropertyId` and `ShorthandId` identifiers with compact, contiguous IDs, -and replace the heap-allocating `PropertyIdSet` hash set with a fixed-capacity bitset. Preserve -string/hash lookup at parsing and public name-based entry points, while making all resolved-property -identity, set algebra, and state-change traversal allocation-free. - -The implementation must also remove the existing accidental dependency between CSS property -application order and replaced-image sizing. A bitset has a deterministic numeric iteration order, -which differs from the current `UnorderedSet` bucket order. The migration is not allowed to encode -the old bucket order into enum values or otherwise depend on a lucky order. - -This plan includes a separate dense `ShorthandId` namespace and name map. Shorthand IDs must never -be cast to `PropertyId`, even when a shorthand and a longhand share a spelling such as `transition`, -`flex`, `grid`, or `gap`. - -## Decisions Already Made - -The implementer must use these choices; none are left open: - -1. `PropertyId` uses `Uint16`, not `Uint8`. eepp currently registers 307 longhand definitions and - the enum currently contains 316 names. Even before custom properties, that cannot fit in an - 8-bit ID. Splitting widget-specific longhands into unrelated ID spaces would prevent one - `PropertyIdSet` and make dispatch substantially more complex, so it is explicitly rejected. -2. `PropertyId::Invalid` is zero. Built-ins are contiguous from one through the last built-in. - `PropertyId::NumDefinedIds` is the exclusive end of built-ins, and - `PropertyId::FirstCustomId` has the same numeric value. -3. `PropertyId::MaxNumIds` is 512 and is an exclusive upper bound. Valid IDs are `[1, 511]`. - The initial tree therefore has room for at least 195 runtime-registered properties. Exhaustion - logs an error and registration fails without aliasing or wrapping an ID. -4. `ShorthandId` uses `Uint8`. `Invalid` is zero, built-ins are contiguous, `NumDefinedIds` and - `FirstCustomId` mark the exclusive built-in end, and `MaxNumIds` is 255 (exclusive). Valid IDs - are `[1, 254]`. The 37 current shorthand registrations leave ample custom capacity. -5. Numeric IDs are process-local implementation identifiers. They are not persisted, serialized, - sent over IPC, or exposed as stable values. Names are the stable external representation. -6. Built-in IDs are explicitly supplied at registration. Custom IDs are assigned monotonically by - the corresponding name map after all built-ins have been registered. Runtime unregistration and - ID reuse are not supported. -7. Existing name hashes remain where they serve a different purpose: `StyleSheetProperties`, - animation action tags, CSS variables, selectors, and data properties remain keyed by - `String::HashType`. `PropertyDefinition::getId()` and `ShorthandDefinition::getId()` continue to - return the canonical name hash for source compatibility. Dense identity is returned only by - `getPropertyId()` and `getShorthandId()`. -8. Unknown CSS names and arbitrary `data-*` names do not consume dense IDs. Only successfully - registered property and shorthand definitions receive IDs. CSS variables remain entirely - outside both ID spaces. -9. `PropertyIdSet` stores exactly `std::bitset<512>`. It accepts and yields `PropertyId`, never raw - hashes or `Uint32`. Its iteration order is ascending dense ID order. -10. Property application must be correct for any traversal order. The pre-migration sizing fix in - Stage 1 is mandatory and must land before the set implementation changes. - -## Confirmed Iteration-Order Bug - -The prior `SmallVector` experiment did not expose a WebP decoder or test-CWD issue. It exposed a -real ordering dependency in style application: - -- `UIStyle::onStateChange()` builds a set of changed properties and iterates it while an attributes - transaction is active. -- `UIWidget::beginAttributesTransaction()` / `endAttributesTransaction()` only coalesce layout - notifications. They do not defer property setters or make the property update atomic. -- `UIHTMLImage::onSizeChange()` and `onSizePolicyChange()` call `autoSizeImage()` immediately. -- `UIWidget::setMaxWidthEq()` calls the virtual `onSizeChange()` immediately. -- `autoSizeImage()` reads the current width policy, height policy, `mMaxWidthEq`, `mMaxHeightEq`, - padding, drawable dimensions, and current size. During the loop these values may represent a - partially applied style. -- The regression fixture combines HTML `width="2560" height="1436"` with CSS `height:auto` and - `max-width:100%`. A changed iteration order can therefore calculate from an intermediate fixed - width/height state and leave zero or stale geometry. The outer transaction does not currently - perform a final sizing reconciliation. - -The correct fix is a final-state reconciliation at the outermost transaction boundary. Preserving -the old unordered bucket order, insertion order, or a hand-selected enum order is forbidden: those -approaches leave the same bug available for stylesheet changes, aliases, custom registrations, and -different standard-library hash layouts. - -## Target Types and APIs - -### `PropertyId` and `ShorthandId` - -Move both enum declarations into a new public header: - -`include/eepp/ui/css/propertyids.hpp` - -Use the following shape (with all built-in enumerators listed explicitly between the markers): - -```cpp -enum class PropertyId : Uint16 { - Invalid = 0, - Id, - Class, - // Every registered longhand, exactly once. - Defer, - NumDefinedIds, - FirstCustomId = NumDefinedIds, - MaxNumIds = 512, -}; - -enum class ShorthandId : Uint8 { - Invalid = 0, - Margin, - // Every registered shorthand, exactly once. - PlaceContent, - NumDefinedIds, - FirstCustomId = NumDefinedIds, - MaxNumIds = 255, -}; -``` - -Keep the current `PropertyId` declaration order to minimize switch/source churn, but reconcile it -against `StyleSheetSpecification::registerDefaultProperties()` before assigning the final list: - -- add an enum member for every registered longhand missing from the current enum; -- remove from `PropertyId` any name that is only a shorthand and has no longhand registration; -- keep names that are both a registered longhand and a shorthand in both enums; -- update every switch/caller for any member moved out of `PropertyId`; -- add compile-time checks that `NumDefinedIds < MaxNumIds` for both enums. - -The built-in `ShorthandId` list must exactly match these 37 current registrations, including names -missing from today's hash enum: - -`Margin`, `LayoutMargin`, `LayoutMarginUnderscore`, `Padding`, `Background`, `Foreground`, -`BoxMargin`, `BackgroundPosition`, `ForegroundPosition`, `BorderColor`, `BorderWidth`, -`BorderStyle`, `BorderRadius`, `ForegroundRadius`, `RotationOriginPoint`, `RotateOriginPoint`, -`ScaleOriginPoint`, `MinSize`, `MaxSize`, `Border`, `TextShadow`, `HintShadow`, `BorderLeft`, -`BorderRight`, `BorderTop`, `BorderBottom`, `ListStyle`, `Font`, `VerticalAlign`, `FlexFlow`, `Flex`, -`Gap`, `GridTemplate`, `Grid`, `PlaceItems`, `PlaceSelf`, and `PlaceContent`. - -Use `ListStyle` as the corrected C++ spelling; remove the current `ListStye` typo and update its -callers. `layout-margin` and `layout_margin`, and `rotation-origin-point` and -`rotate-origin-point`, remain distinct built-in shorthand IDs because they are independently -registered definitions today. - -### Generic `IdNameMap` - -Add the reusable header: - -`include/eepp/ui/css/idnamemap.hpp` - -Implement `template class IdNameMap`. It owns: - -- `std::vector mNames`, indexed by the underlying numeric ID; -- `UnorderedMap mIdsByName`, using exact normalized canonical/alias names; -- the next custom ID, initialized to `Id::FirstCustomId` after built-in registration is finalized. - -Required operations and behavior: - -```cpp -bool addBuiltin( Id id, const std::string& canonicalName ); -bool addAlias( const std::string& alias, Id target ); -bool finalizeBuiltins(); -Id getId( std::string_view name ) const; -const std::string& getName( Id id ) const; -Id getOrCreateId( const std::string& canonicalName ); -bool contains( Id id ) const; -bool contains( std::string_view name ) const; -``` - -Implementation invariants: - -- construct index zero as `Invalid` with an empty name; -- `addBuiltin()` is valid only before custom allocation begins, rejects `Invalid`, rejects IDs at - or beyond `MaxIds`, rejects duplicate names, and rejects assigning a different name to an - occupied ID; -- it resizes `mNames` to `id + 1`, so explicitly registered built-ins may be checked even if a - mistake introduces a gap; -- `addAlias()` adds only the reverse entry and does not occupy an ID or replace the canonical name; -- `finalizeBuiltins()` verifies every slot `[1, FirstCustomId)` is populated, sets the next custom - ID to `FirstCustomId`, and permanently rejects later `addBuiltin()` calls; -- `getOrCreateId()` is rejected until `finalizeBuiltins()` succeeds; -- `getId()` returns `Invalid` for an unknown name; -- `getName()` returns the empty invalid name for invalid/out-of-range/unassigned IDs; -- `getOrCreateId()` returns an existing canonical/alias target when present; otherwise it assigns - `mNextCustomId`, increments it, and appends the canonical name; -- reaching `MaxIds` returns `Invalid` and logs a clear capacity error containing the rejected name; -- all names passed into the map are already lowercase and trimmed by the caller. The map performs - no hidden normalization and no hash-only equality. - -Do not copy RmlUi's source verbatim. Reimplement this behavior using eepp containers, types, -logging, and formatting conventions. - -### Definition and specification ownership - -Change constructors to receive dense identity: - -```cpp -PropertyDefinition( PropertyId propertyId, const std::string& name, - const std::string& defaultValue, bool inherited = false ); -ShorthandDefinition( ShorthandId shorthandId, const std::string& name, - const std::vector& properties, - const std::string& shorthandFuncName ); -``` - -Each definition stores both identities: - -- `mPropertyId` / `mShorthandId`: dense dispatch identity; -- `mId`: existing canonical name hash used by name-keyed structures and action tags. - -`StyleSheetProperty::getId()` remains the key for `StyleSheetProperties`: return the canonical -definition hash for a resolved property or shorthand, and return `mNameHash` for an unresolved -name. This preserves canonical alias coalescing while ensuring distinct unknown/`data-*` names do -not collapse onto `Invalid`/zero. Add explicit `getPropertyId()` and `getShorthandId()` accessors -that return their respective dense IDs or `Invalid`; callers must use the accessor matching the -definition kind. - -`PropertySpecification` owns: - -- `IdNameMap mPropertyIds`; -- `IdNameMap mShorthandIds`; -- `std::vector> mPropertiesById` indexed by dense ID; -- `std::vector> mShorthandsById` indexed by dense ID; -- `PropertyIdSet mInheritableProperties` instead of `SmallVector`. - -Resize the definition vectors on registration to `id + 1`; do not preconstruct 512 strings or 512 -`shared_ptr`s. Name lookup first resolves the name through the appropriate `IdNameMap`, then indexes -the definition vector. ID lookup indexes directly after validating range. - -Expose these exact overloads through both `PropertySpecification` and -`StyleSheetSpecification`: - -```cpp -PropertyDefinition& registerProperty( PropertyId id, const std::string& name, - const std::string& defaultValue, - bool inherited = false ); -PropertyDefinition* registerProperty( const std::string& name, - const std::string& defaultValue, - bool inherited = false ); -const PropertyDefinition* getProperty( PropertyId id ) const; -const PropertyDefinition* getProperty( const std::string& name ) const; - -ShorthandDefinition& registerShorthand( ShorthandId id, const std::string& name, - const std::vector& properties, - const std::string& parserName ); -ShorthandDefinition* registerShorthand( const std::string& name, - const std::vector& properties, - const std::string& parserName ); -const ShorthandDefinition* getShorthand( ShorthandId id ) const; -const ShorthandDefinition* getShorthand( const std::string& name ) const; -``` - -Remove public `getProperty(Uint32)`, `getShorthand(Uint32)`, and `isShorthand(Uint32)` overloads so -a name hash cannot accidentally bind to a dense-ID lookup. Callers that possess both a cached hash -and the full name must use the full-name overload; do not add a hash-only fast path to `IdNameMap`. - -Built-in registration returns references and treats a mismatch as a programming error: log an -error and assert in debug if an explicit ID is already bound to another name, a name is bound to -another ID, or a built-in slot is skipped. Runtime registration returns a pointer so exhaustion can -return `nullptr`. Preserve current duplicate semantics: a duplicate non-prefixed name warns and -returns the existing definition; a duplicate name beginning with `-` replaces its definition in -the same dense slot (without allocating another ID). Do not duplicate its inheritable-set entry. - -`PropertyDefinition::addAlias()` must call `PropertySpecification::addPropertyAlias(alias, -mPropertyId)`. Preserve the definition's existing alias strings/hashes for `isAlias()` callers, -but make the specification's full-string reverse map authoritative. Apply the equivalent behavior -to `ShorthandDefinition::addAlias()` if shorthand aliases are added later; no `ShorthandIdMap` -class separate from the generic `IdNameMap` is needed. - -### Built-in registration - -Convert every call in `StyleSheetSpecification::registerDefaultProperties()` from name-only form -to explicit form: - -```cpp -registerProperty( PropertyId::Width, "width", "" )...; -registerShorthand( ShorthandId::Margin, "margin", {...}, "box" ); -``` - -The enum spelling, canonical string, and registration call form one audited table even though they -remain written as enum declarations plus registration code. Add a debug-only final validation at -the end of default registration that: - -- every numeric built-in slot `[1, NumDefinedIds)` has exactly one definition; -- every definition's dense ID round-trips through its canonical name; -- every canonical name round-trips to the same definition; -- the first custom ID equals `NumDefinedIds`; -- the count of registrations equals `NumDefinedIds - 1` independently for properties and - shorthands. - -This validation prevents a newly added enum or registration from silently shifting custom IDs or -leaving an unregistered bit. - -### `PropertyIdSet` - -Rewrite `include/eepp/ui/css/propertyidset.hpp` around `std::bitset<512>`. Keep it header-only. -The public API is: - -```cpp -void insert( PropertyId id ); -void clear(); -void erase( PropertyId id ); -bool empty() const; -bool contains( PropertyId id ) const; -std::size_t size() const; -PropertyIdSet& operator|=( const PropertyIdSet& other ); -PropertyIdSet operator|( const PropertyIdSet& other ) const; -PropertyIdSet& operator&=( const PropertyIdSet& other ); -PropertyIdSet operator&( const PropertyIdSet& other ) const; -bool operator==( const PropertyIdSet& other ) const; -bool operator!=( const PropertyIdSet& other ) const; -PropertyIdSetIterator begin() const; -PropertyIdSetIterator end() const; -PropertyIdSetIterator erase( PropertyIdSetIterator it ); -``` - -`Invalid` insertion/erasure is a no-op, `contains(Invalid)` is false, and out-of-capacity values -assert in debug while returning safely in release. The iterator stores the owning set and a -`std::size_t` bit index. Construction and increment scan forward to the next set bit; dereference -returns `PropertyId`. `erase(iterator)` clears the current bit and returns an iterator positioned at -the next set bit. There is no custom-ID side container and no heap fallback. - -Add: - -```cpp -static_assert( sizeof( PropertyIdSet ) == 64 ); -``` - -on the supported standard libraries/builds. If visibility or padding makes this fail on a supported -toolchain, assert `sizeof(std::bitset<512>) == 64` and `sizeof(PropertyIdSet) <= 72` instead; this is -the only permitted toolchain adaptation. - -Change all `PropertyIdSet` loops and calls to `PropertyId`. In particular, remove the -`static_cast(prop)` conversions in `UIStyle::onStateChange()` and reject raw name hashes -at compile time. - -## Implementation Stages - -### Stage 1 - Make property application order-independent - -Complete this stage and run its focused tests before changing any ID or set representation. - -1. Add a protected virtual `UIWidget::onAttributesTransactionEnd()` with a default empty - implementation. -2. In `UIWidget::endAttributesTransaction()`, assert that the transaction count is positive before - decrementing it. When the count reaches zero, call `onAttributesTransactionEnd()` before - emitting the accumulated self/parent layout notifications. Changes triggered by reconciliation - are therefore folded into the same pending flags. -3. Override the hook in `UIHTMLWidget`. Call `updateCSSContentBoxFixedSize()` there so final width, - height, padding/border, and `box-sizing` state is reconciled independent of setter order. -4. Override the hook in `UIHTMLImage`. Call `UIHTMLWidget::onAttributesTransactionEnd()` first and - `autoSizeImage()` second so the final width/height policies, CSS content-box size, min/max - equations, padding, and drawable ratio are all present before replaced sizing is finalized. -5. Keep existing immediate setter behavior outside transactions. Do not globally suppress - `onSizeChange()` or `onSizePolicyChange()`; native/direct setters rely on it. -6. Add a test helper that applies the same image properties inside an attribute transaction in - these orders: `width,height,max-width`; `max-width,height,width`; `height,width,max-width`; and - reverse dense-ID order. Each case must end with identical non-zero size and the 1436/2560 aspect - ratio under the same containing block. -7. Keep and run `UIHTML.ImageMaxWidthConstrainsWebpWithHeightAuto`. Add a second transition test - that starts with fixed HTML dimensions, applies `height:auto;max-width:100%` through a style - state change, then removes and reapplies the rule. Assert identical final geometry each time. - -The acceptance criterion is explicit: randomizing or reversing changed-property traversal in a -test-only helper must not alter final widget geometry. Only after this passes may Stage 5 switch to -ascending bitset iteration. - -### Stage 2 - Introduce dense enum declarations and maps - -1. Add `propertyids.hpp`; move both enum declarations out of `propertydefinition.hpp` and - `shorthanddefinition.hpp`; include the new header from both. -2. Reconcile enum members with the actual default registrations as specified above. -3. Add `idnamemap.hpp` and focused unit tests before integrating it. -4. Add dense members to both definition classes while retaining their canonical name hashes. -5. Replace the two hash-to-definition ownership maps in `PropertySpecification` with the two name - maps and two dense definition vectors. -6. Convert string constructors/lookups in `StyleSheetProperty` to full-name lookup. Continue to - compute/cache `mNameHash`; never use a hash alone to select a definition. -7. Convert attribute-selector lookup in `stylesheetselectorrule.cpp` and every remaining - hash-only property/shorthand lookup to either a known dense ID or full string lookup. -8. Verify aliases resolve to the canonical definition and dense ID while preserving the alias text - in the parsed `StyleSheetProperty` where current diagnostics/serialization expect it. - -### Stage 3 - Explicitly register all built-ins - -1. Convert all longhand calls in `stylesheetspecification.cpp` to explicit `PropertyId` overloads. -2. Convert all shorthand calls to explicit `ShorthandId` overloads. -3. Add the final registration validation. -4. Search the entire repository for `registerProperty` and `registerShorthand`. Keep external/tool - registrations on the runtime overload and add null handling for capacity failure. -5. Search for casts between hashes, `Uint32`, `PropertyId`, and `ShorthandId`; eliminate every cast - that treats one identity domain as another. - -### Stage 4 - Migrate resolved-property APIs to dense IDs - -Keep `StyleSheetProperties` name-hash keyed. Change APIs that semantically address a registered -property to accept `PropertyId`: - -- `ElementDefinition::getProperty(PropertyId)`; -- `StyleSheetStyle::hasProperty(PropertyId)` and a new `getProperty(PropertyId)` helper; -- `UIStyle::getProperty`, `hasProperty`, `hasLocalProperty`, `getLocalProperty`, and - `getResolvedLocalProperty` dense overloads; -- transition/animation definition maps whose keys represent registered properties; -- all widget dispatch and `getPropertyString` paths. - -Implement a dense lookup into a hash-keyed `StyleSheetProperties` by retrieving the -`PropertyDefinition` from `PropertySpecification`, then finding `definition->getId()` in the hash -map. Do not add a 512-entry pointer array to every `ElementDefinition`; that would trade one small -lookup for several kilobytes per cached style definition. - -Keep explicit name/hash APIs only where they address arbitrary names (`data-*`, CSS variables, -selectors, serialization, or animation action tags). Rename ambiguous helpers such as -`getPropertyById(Uint32)` to `getPropertyByNameHash(String::HashType)` when they truly remain -hash-based. - -When `ElementDefinition::refresh()` builds its ID set, insert -`property.getPropertyDefinition()->getPropertyId()` only when a definition exists. Preserve the -current separate handling of unknown and `data-*` properties; `StyleSheetProperty::getId()` must -key each by its own `mNameHash`, while neither `PropertyIdSet` nor either `IdNameMap` receives it. - -### Stage 5 - Replace `PropertyIdSet` and remove the render-time allocation workaround - -1. Land the bitset implementation and its unit tests. -2. Convert `ElementDefinition`, `UIStyle`, inheritance propagation, and all other set users to - typed `PropertyId` operations. -3. Change `PropertySpecification::mInheritableProperties` and its getter to `PropertyIdSet`. -4. Remove `UIStyle::mChangedProperties` and its `std::optional` cache. Restore - `PropertyIdSet changedProperties;` as a stack local in `onStateChange()`; the 64-byte bitset has - no constructor allocation and does not retain per-widget heap capacity. -5. Keep ascending numeric iteration. Do not add insertion-order storage, sorting, or a second - vector. -6. Re-run the Stage 1 permutation tests with the real bitset traversal and the WebP regression. - -### Stage 6 - Cleanup and documentation - -1. Remove obsolete raw-`Uint32` overloads, hash casts, unordered-set iterator machinery, and - comments describing hash values as property IDs. -2. Document in `propertyids.hpp` that enum numeric values are unstable internal values and names - must be used for persistence/plugins crossing binary boundaries. -3. Document runtime capacity and registration-before-parsing requirements on both registration - APIs. -4. Add a short comment at the transaction-end reconciliation hook explaining that the hook makes - interdependent property setters observe the complete style and protects set-order independence. -5. Update Doxygen for `getId()`, `getPropertyId()`, `getShorthandId()`, and renamed name-hash APIs so - the three identity domains cannot be confused. - -## Required Tests - -Add focused tests under `src/tests/unit_tests/` and register them with the existing unit-test build: - -### `IdNameMap` - -- zero is invalid and returns the empty name; -- explicit built-ins round-trip by ID and full string; -- aliases resolve to the target without consuming an ID; -- duplicate ID/name and conflicting aliases are rejected; -- the first custom ID is exactly `FirstCustomId`; -- repeated custom lookup returns the same ID; -- IDs increase contiguously; -- the final valid slot succeeds and the next registration returns `Invalid`; -- two different strings are never equated only because a hash collides. - -### Definition registration - -- every built-in property and shorthand slot is populated; -- canonical name and dense ID round-trip for every slot; -- all automatically generated dash/underscore-free property aliases resolve to the same ID; -- explicit aliases such as `bgcolor`, `align`, `layout_width`, `lw`, and `rotate` resolve correctly; -- a runtime property receives `FirstCustomId`, can be parsed/applied/dispatched, and appears in a - `PropertyIdSet`; -- a runtime shorthand receives `ShorthandId::FirstCustomId`, expands to registered longhands, and - never collides with an equal-valued `PropertyId`; -- duplicate and `-`-prefixed replacement behavior remains as specified; -- capacity failure is safe and observable. - -### `PropertyIdSet` - -- default empty state performs no heap allocation under the debug memory manager; -- insert, duplicate insert, contains, erase-by-ID, clear, size, equality, union, intersection, and - self-union/intersection; -- `Invalid` behavior and upper boundary behavior; -- custom IDs work identically to built-ins; -- iteration yields only present IDs in ascending numeric order; -- iterator erase returns the next present ID and supports erasing every element in one loop; -- copy/move operations preserve bits and allocate nothing; -- exact/maximum object-size assertion described above. - -### Ordering and integration - -- all Stage 1 image property permutations produce identical final geometry; -- `UIHTML.ImageMaxWidthConstrainsWebpWithHeightAuto` passes; -- the state-transition variant passes; -- shorthand parsing for every built-in shorthand still produces the same longhand names/values; -- style specificity, indexed properties, aliases, inheritance, transitions, animations, CSS - variables, custom registered properties, and `data-*` properties retain coverage; -- add a regression proving two definitions with the same shorthand/property spelling occupy their - separate ID spaces and resolve through the correct map. - -## Build and Verification Sequence - -After each stage that changes C++: - -1. From the repository root, regenerate using the current debug command from - `.agent/rules/build-project.md`. Use `premake4` when installed, include `--with-mold-linker` when - `mold` is installed, and otherwise use the documented fallback. -2. Format all changed C/C++ files with the repository `clang-format` command. -3. Build with `make -C make/linux -j$(nproc)` exactly. -4. Run focused tests through: - - `projects/scripts/xvfb-run-eepp bin/unit_tests/eepp-unit_tests-debug --filter=""` - -Before completion: - -1. Run the complete unit-test suite through - `projects/scripts/xvfb-run-eepp bin/unit_tests/eepp-unit_tests-debug`. -2. Build the normal eepp targets and ecode, not only the unit-test target, because public CSS APIs - and plugin-facing registration signatures change. -3. Run `git diff --check` and inspect the full diff for accidental generated/build artifacts. - -## Performance and Allocation Acceptance Criteria - -- Constructing, clearing, copying, unioning, intersecting, and iterating `PropertyIdSet` performs - zero heap allocations. -- `UIStyle::onStateChange()` has no allocation attributable to the changed-property set, including - its first call on a widget. -- No per-`ElementDefinition` 512-entry lookup table is introduced. -- Full-string name lookup and custom ID assignment occur during registration/parsing, not during - property-set iteration or widget dispatch. -- Dense property lookup is vector indexing; set membership/algebra is fixed-size bitwise work. -- The transaction-end reconciliation adds no heap allocation. It runs only when the outermost - transaction closes and must avoid duplicate layout messages by executing before pending flags - are flushed. -- Perform the repository-required allocation audit over every touched constructor, container - insertion, string copy, lambda, and transition/animation path. - -## Compatibility and Failure Policy - -- This is an ABI break for enum values and for APIs that previously accepted raw `Uint32` IDs. - Treat it as an intentional eepp library ABI change; do not provide implicit integer overloads - that reintroduce hash/dense ambiguity. -- Source code comparing `getId()` to name hashes continues to work. Source code casting - `PropertyId`/`ShorthandId` to persisted integers must migrate to names. -- Existing external custom registrations continue through the name-only runtime overload but must - handle its nullable return on capacity exhaustion. -- Registration must finish before styles using custom names are parsed. Registering a definition - does not retroactively repair already parsed unknown properties. -- Capacity exhaustion is never allowed to wrap, reuse `Invalid`, overwrite another definition, or - silently drop a built-in. Built-in mismatch is a debug assertion/programming error; runtime - exhaustion is a logged recoverable failure. -- Hash collisions in existing name-keyed `StyleSheetProperties` remain a pre-existing concern - outside this refactor. The new ID maps themselves must use full-string equality and must not add - any new collision-based identity. - -## Completion Checklist - -- [ ] Stage 1 proves style application order-independent before bitset migration. -- [ ] `PropertyId` is dense `Uint16`, complete, and below 512. -- [ ] `ShorthandId` is dense `Uint8`, complete, and below 255. -- [ ] Separate property and shorthand `IdNameMap` instances own full-string reverse lookup. -- [ ] Every built-in registration supplies an explicit enum ID and passes round-trip validation. -- [ ] Runtime custom property and shorthand registration has deterministic IDs and exhaustion - handling. -- [ ] Hash identity and dense identity have unambiguous API names/types. -- [ ] `PropertyIdSet` is a fixed 512-bit allocation-free set with typed ascending iteration. -- [ ] `UIStyle` no longer retains `mChangedProperties` merely to avoid set allocation. -- [ ] Image sizing, shorthand expansion, inheritance, animations/transitions, variables, aliases, - data properties, and custom properties pass focused coverage. -- [ ] Debug eepp, unit tests, ecode, and the full wrapped suite build/pass using `-j$(nproc)`. -- [ ] Formatting, diff check, allocation audit, and final self-review are complete. diff --git a/.agent/plans/ui_data_handling_modernization_plan.md b/.agent/plans/ui_data_handling_modernization_plan.md new file mode 100644 index 000000000..67be48f6e --- /dev/null +++ b/.agent/plans/ui_data_handling_modernization_plan.md @@ -0,0 +1,682 @@ +# UI Data Handling Modernization Plan + +Status: Stage 1 implemented and under review; Stage 2 is next, 2026-08-09. + +Baseline commit: `01d5614a7 ui: add scoped event and observable value bindings` + +## Goal + +Build a modern, predictable data-handling layer for eepp's retained-mode UI without imposing a +React-like component model, virtual DOM, immutable application state, or mandatory reactive +architecture. + +The system must preserve direct widget manipulation and the existing model/view APIs while making +safe value synchronization, validation, derived state, commands, background delivery, forms, and +diagnostics available as composable C++ tools. + +The target layering is: + +```text +Application state UI-local state Existing raw state +ObservableValue UIProperty T* + | | | +UIValueBinding UIDataBind <--------------+ + | | + +------------- UIValueConverter + + | + UIWidget + +Collections -> Model adapters -> UIAbstractView +Commands ---------------------> buttons / menus / shortcuts +``` + +The current classes retain distinct purposes: + +- `EventConnection`: scoped lifetime for a `Node` listener. It does not replace widget-level + `Event::OnClose` lifecycle notification. +- `UIDataBind`: low-level adaptation of an externally owned `T*`. The caller controls and must + prove the value lifetime. +- `UIProperty`: inexpensive owned UI-local value and bidirectional widget binding. +- `ObservableValue`: UI-independent owned state whose observer types are unknown to the model. +- `UIValueBinding`: scoped bidirectional adapter between an `ObservableValue` and a widget. +- `UIValueConverter`: conversion policy shared by raw and observable bindings. +- Existing `Model` / view classes: structured and potentially large collection presentation. + +Do not collapse these classes merely to share implementation. Their ownership and coupling models +are intentionally different. + +## Decisions Already Made + +1. Typed conversion results and observable field error state were completed in Stage 1. +2. Layout-update batching is out of scope. eepp already queues/coalesces layout invalidation, so a + generic observable transaction would add complexity without a demonstrated problem. +3. Scripting is out of scope for this roadmap. The C++ lifetime, validation, command, collection, + and inspection foundations come first. +4. The system remains retained-mode. Reactive features update persistent widgets and models; they + do not reconstruct a virtual widget tree. +5. All current event, observable, binding, and widget mutation remains single-threaded unless an + explicit UI-thread delivery adapter is used. +6. New facilities must be opt-in. Existing event handlers, widget setters, and custom `Model` + implementations remain valid and are often the clearest solution. +7. Every scoped observer or binding must be safe when either endpoint is destroyed first. +8. Conversion, validation, and binding errors must be inspectable. Silent failure is not an + acceptable final design. + +## Priority Order + +1. Rich conversion and validation results. +2. Form/binding groups and aggregate validation. +3. Computed/derived observable values. +4. Explicit UI-thread observation and delivery. +5. Commands and reactive command state. +6. Observable collections and incremental model adapters. +7. Binding/observable inspection tooling. + +This order is dependency-driven: form groups consume validation state; commands benefit from +computed values; inspection should understand every final primitive rather than being repeatedly +redesigned. + +--- + +# Stage 1: Typed Conversion and Field Error State + +Status: implemented locally; build and 924-test ASAN suite pass. + +## Objective + +Replace output-parameter converters and their bare `bool` result with typed results that either +contain an accepted value or identify a failure. Expose the current error state from both +`UIDataBind` and `UIValueBinding` without adding a second validator pass or a general-purpose +binding pipeline. + +`UIValueConverter` is intentionally the only input policy. Its `toValue()` callback performs +whatever parsing and field-local acceptance a use case needs, while `fromValue()` formats +authoritative model values. More specialized composition belongs in application code until a +repeated concrete use case justifies another shared abstraction. + +Validation errors should be machine-readable first. UI code normally maps a stable numeric error +code to localized text; the optional string is a technical diagnostic for logs, tests, and +inspection rather than the default user-facing message. This stage must not force converters to +allocate an error string on success or on ordinary coded failures. + +## Implemented result type + +The public result and observable error state live in: + +```text +include/eepp/ui/uivaluevalidation.hpp +``` + +Implemented result shape: + +```cpp +struct UIValueValidationResult { + using Code = Uint32; + + bool valid{ true }; + std::optional code; + std::optional debugMessage; + + static UIValueValidationResult success(); + static UIValueValidationResult error( Code code ); + static UIValueValidationResult error( Code code, std::string debugMessage ); + static UIValueValidationResult error( std::string debugMessage ); + explicit operator bool() const { return valid; } +}; +``` + +`Code` is intentionally numeric and `0` is not overloaded to mean “no code”; the disengaged +`std::optional` represents absence. Codes are defined by the subsystem or application that owns +the acceptance rule. `UIValueValidationError` reserves documented values for built-in converter +failures. Codes are not otherwise globally unique or stable for serialization unless a later API +introduces an error domain. + +An error may have only a code, only a diagnostic, or both. Coded errors are the normal application +path. Diagnostic-only errors remain useful for ad-hoc acceptance rules and converter migration, but UI +presentation must not depend on English diagnostic text. + +The selected public name is `UIValueValidationResult`: converter failures can represent syntax, +range, or other field-local acceptance errors without introducing nearly identical result types. +Success and code-only errors are allocation-free; diagnostics allocate only when supplied. + +## Converter migration + +Change `UIValueConverter` callbacks from: + +```cpp +std::function +std::function +``` + +to: + +```cpp +std::function( const PropertyDefinition*, const std::string& )> +std::function( const PropertyDefinition*, const T& )> +``` + +Returning values instead of mutating output parameters prevents failed converters from leaking +partial output and makes the binding's early-return behavior explicit. + +This is intentionally source-breaking for custom converters. A legacy `return false` is ambiguous +once conversion must return a typed value and could accidentally become a successful boolean or +numeric value. Migrate custom converters explicitly and do not retain duplicate output-parameter +adapters. Infallible converters may return a raw `T`; fallible converters use +`UIValueResult::error()`. + +Default parse failures should use documented core error codes and may additionally produce useful +diagnostics containing the rejected text and expected type/category when practical. Do not +localize low-level converter diagnostics; preserve technical text suitable for logs and inspector +tooling. Applications translate the code (plus their binding/form context) through their own i18n +layer. + +## Property conversion + +Bindings call `UIValueConverter` directly in both directions: + +```text +model output: T -> widget property string +widget input: property string -> accepted T or error +``` + +Presentation-specific formatting such as currencies, percentages, and units belongs in +`UIValueConverter`. More elaborate typed adaptation can be implemented by applications if a +concrete use case requires it. + +## Field-local acceptance + +Parsing and semantic acceptance are conceptually different, but both are part of the converter's +single widget-input operation: + +```text +widget string -> UIValueConverter::toValue() -> accepted T or error +``` + +Examples: + +- `"abc"` cannot convert to an integer. +- `-1` parses as an integer but may still be rejected by the converter's acceptance rules. +- A path converts to a string but may not exist. +- A return date converts successfully but may precede the departure date. + +Do not add a second validation pass to every binding. A custom converter can reuse parsing or +validation helpers internally when an application needs them. Do not put asynchronous validation +in Stage 1. + +## Binding state and API + +Both `UIDataBind` and `UIValueBinding` must expose their current validity without requiring +knowledge of the other class. The common state is: + +```cpp +class UIValueValidationState { + public: + bool isValid() const; + const std::optional& code() const; + const std::optional& debugMessage() const; + Connection observe( Callback ); +}; +``` + +Validation observation reuses `ObservableValue` internally and allocates its observer storage only +when observed. `ObservableValue` does not depend on UI headers. + +Required binding behavior: + +1. Successful input acceptance updates the model value and clears the previous error. +2. Rejected input leaves the last accepted model value unchanged. +3. The originating widget may retain its invalid text so the user can correct it. +4. Other widgets bound to the same value must continue showing the last valid model value; invalid + text must not propagate to them. +5. Converter acceptance applies to UI-originated proposals. Programmatic `set()` and external + `ObservableValue` changes are authoritative model updates; formatting can still fail and be + reported. +6. Model-to-widget conversion failure must not apply an empty or partial property string. +7. Repeated identical validation errors should not emit duplicate state-change notifications. +8. Widget destruction clears its validation contribution safely. + +## Widget presentation policy + +Stage 1 deliberately does not hard-code an error class, tooltip, or localized message into the +binding core. Numeric codes and optional diagnostics remain independently observable. Stage 2 must +decide how a form maps field and cross-field errors to localized presentation after auditing the +existing invalid/error widget states and theme conventions. + +## API compatibility audit + +Audit and migrate: + +- all `UIValueConverter` construction; +- `UIDataBind::converterDefault/String/Bool()` compatibility forwarders; +- ecode's `ProjectOutputParserTypes` converter; +- `UIProperty` constructor defaults; +- all unit tests and examples; +- any downstream-style public callback signatures exposed in headers. + +Document source-breaking changes clearly because `UIDataBind::Converter` was public before this +roadmap. + +## Stage 1 verification + +Implemented focused coverage includes: + +- successful default conversion; +- conversion failure with a code and no diagnostic allocation; +- diagnostic-only and code-plus-diagnostic failures; +- field-local acceptance failure after parsing; +- model-originated values remain authoritative even when equivalent widget input would be rejected; +- invalid widget text does not change the model; +- invalid widget text does not propagate to sibling widgets; +- later valid input clears the error and updates every widget; +- failed model-to-widget conversion does not apply a property; +- programmatic set validation behavior; +- repeated identical error suppression, comparing validity, code, and diagnostic; +- widget-first, binding-first, and value-first destruction while invalid; +- custom converter parsing, formatting, and acceptance; +- formatted currency-style values in both directions. + +The project builds with the ASAN debug configuration, all focused binding tests pass, and the full +suite passes 924/924. Presentation/localization and cross-field behavior remain Stage 2 concerns. +Use Flight Booker as a design reference for that phase, but do not migrate it until the resulting +form API is clearly shorter and more expressive than its current explicit validation function. + +--- + +# Stage 2: Form and Binding Groups + +## Objective + +Provide ownership and aggregate validation for related bindings without turning every form into a +new framework. + +The group must solve two separate concerns: + +1. Stable ownership of heterogeneous bindings. +2. Aggregate state such as valid, dirty, commit, reset, and first error. + +## Proposed API direction + +Explore extending or replacing the narrowly typed `UIDataBindHolder` classes with a type-erased +scoped binding interface: + +```cpp +class UIValueBindingBase { + public: + virtual ~UIValueBindingBase() = default; + virtual void disconnect() = 0; + virtual bool isConnected() const = 0; + virtual bool isValid() const = 0; + virtual const std::optional& validationCode() const = 0; + virtual const std::optional& validationDebugMessage() const = 0; +}; +``` + +Avoid virtual dispatch if a small type-erased value holder can provide the same ownership cleanly. +Measure complexity before choosing. + +Candidate user API: + +```cpp +UIBindingGroup form; +form += bindValue( config.name, nameInput, stringConverter, "text", Event::OnTextChanged ); +form += bindValue( config.path, pathInput, stringConverter, "text", Event::OnTextChanged ); + +saveButton->setEnabled( form.isValid() && form.isDirty() ); +form.onValidationChange( ... ); +``` + +## Required semantics + +- Destruction or `clear()` disconnects every binding. +- A group may contain `UIValueBinding`, `UIDataBind`, and optionally non-binding validators. +- Aggregate validity updates when a child binding changes validity or disappears. +- The group exposes every child's code, optional diagnostic, and originating binding/widget, plus + the first invalid widget for focus/navigation. The group must preserve the context needed to map + application-defined codes to localized messages. +- Dirty state compares against an explicit baseline, not merely “received an event.” +- `markClean()` establishes a new baseline after save. +- `reset()` restores the baseline where values are copyable. +- Commit/rollback must be opt-in; not every live configuration form uses temporary state. +- A group must not own widgets or observable model values. + +## Tests + +- heterogeneous binding ownership; +- aggregate validity transitions; +- first invalid widget behavior; +- widget destruction while invalid; +- dirty/clean baseline behavior; +- reset and mark-clean; +- group destruction before and after endpoints; +- no duplicate aggregate notification when state is unchanged. + +--- + +# Stage 3: Computed and Derived Observable Values + +## Objective + +Represent read-only state derived from one or more observables while preserving synchronous, +deterministic retained-mode updates. + +## Design constraints + +- Do not implement implicit dependency tracking by executing arbitrary lambdas and recording reads. +- Dependencies must be explicit in the initial implementation. +- Computed values are read-only to consumers. +- Dependency connections are scoped and expire safely. +- Equality suppression should match `ObservableValue` behavior. +- Reentrant updates and cycles must have defined behavior before merging. + +## Candidate API + +```cpp +auto fullName = computedValue( + firstName, lastName, + []( const std::string& first, const std::string& last ) { + return first + " " + last; + } ); + +auto canSave = computedValue( + formValid, formDirty, + []( bool valid, bool dirty ) { return valid && dirty; } ); +``` + +The returned type should expose the read/observe subset of `ObservableValue`, not `set()`. + +## Reentrancy decision required + +The current synchronous `ObservableValue` allows an observer to set the same value during +notification. Before computed values are implemented, define and test one policy: + +1. Nested immediate notifications. +2. Queue the latest value until the current notification completes. +3. Reject/assert reentrant mutation. + +Recommended direction: queue the latest distinct value and drain synchronously after the current +observer snapshot completes. This avoids observers seeing later values twice during an older +notification while retaining synchronous completion before `set()` returns. + +Cycle detection must report a clear diagnostic in debug builds rather than recurse indefinitely. + +## Tests + +- one and multiple dependencies; +- registration-order observation; +- equality suppression; +- dependency destruction; +- computed destruction; +- chained computed values; +- diamond dependency graph behavior; +- reentrant source updates; +- direct and indirect cycles; +- binding a computed value one-way to a widget. + +--- + +# Stage 4: Explicit UI-Thread Delivery + +## Objective + +Allow state produced on worker threads to be delivered safely to the UI without making +`ObservableValue`, `EventConnection`, or widgets internally thread-safe. + +## Proposed direction + +Use existing `Node::runOnMainThread()` / `ensureMainThread()` infrastructure. Candidate APIs: + +```cpp +auto connection = observeOnUIThread( value, widget, callback ); +``` + +or: + +```cpp +auto uiValue = deliverOnUIThread( source, uiSceneOrNode ); +``` + +The adapter must capture only lifetime-safe handles. It must not queue a raw widget pointer that can +die before execution. + +## Required semantics + +- Source observation may occur on the source's owning thread. +- Widget mutation occurs only on the UI thread. +- Destruction before queued delivery makes the delivery a no-op. +- Define whether every value is delivered or only the latest pending value. Provide explicit names + if both modes are needed. +- Preserve order for non-coalesced delivery. +- No blocking cross-thread calls. +- Clearly document that base `ObservableValue` remains single-threaded; a producer must serialize + mutation or use a separate synchronized source adapter. + +## Tests + +- worker-to-UI delivery; +- endpoint destruction before execution; +- connection destruction before execution; +- ordered delivery; +- latest-value coalescing, if supported; +- UI-thread immediate fast path; +- sanitizer coverage where available. + +--- + +# Stage 5: Commands + +## Objective + +Represent user actions separately from values and bind one action consistently to buttons, menus, +keyboard shortcuts, toolbars, and command palettes. + +## Candidate API + +```cpp +Command save{ + [&] { saveProject(); }, + canSave +}; + +auto buttonBinding = bindCommand( save, saveButton ); +auto menuBinding = bindCommand( save, saveMenuItem ); +``` + +## Command state + +Explore: + +- enabled; +- checked/toggled; +- visible, only if a real use case requires it; +- label and icon metadata; +- shortcut metadata; +- execution callback; +- optional parameter type for reusable commands. + +Prefer observable/computed state inputs over command-owned ad hoc listener APIs. + +## Required behavior + +- Disabled commands cannot execute through any bound endpoint. +- Multiple UI representations stay synchronized. +- Endpoint and command destruction are safe in either order. +- Reentrant execution policy is explicit. +- Asynchronous command progress/cancellation is deferred unless a concrete ecode workflow requires + it during implementation. + +## Tests and candidate migrations + +- bind one command to button and menu item; +- enabled and checked propagation; +- shortcut execution; +- endpoint destruction; +- command destruction; +- duplicate execution prevention; +- consider ecode actions already represented in menus/toolbars as the primary real-world audit; +- Circle Drawer undo/redo is a useful small example for enabled-state command binding. + +--- + +# Stage 6: Observable Collections and Model Adapters + +## Objective + +Bridge ordinary application collections to eepp model/view components with incremental updates, +without replacing custom models such as Cells or forcing every collection to be observable. + +## Research first + +Audit existing `Model` invalidation/update flags and selection preservation before defining new +collection notifications. Reuse established model vocabulary where possible. + +## Candidate change vocabulary + +```cpp +inserted( index, count ); +removed( index, count ); +moved( from, to, count ); +changed( index, count ); +reset(); +``` + +Candidate types: + +```cpp +ObservableVector +ObservableCollection +CollectionModelAdapter +``` + +Avoid exposing mutable container references that bypass notifications. Mutation should happen +through explicit operations or an edit guard that emits one well-defined change. + +## Required behavior + +- Incremental model updates preserve unaffected indexes and selection. +- Removal invalidates only affected indexes according to existing model contracts. +- Batch operations emit one range update where possible. +- Collection and view/model adapter destruction are safe in either order. +- Large collections do not copy their contents for every notification. +- Thread behavior is explicit and uses Stage 4 adapters when needed. + +## Candidate audits + +- CRUD's people list for basic insertion/removal/filtering. +- Application configuration lists in ecode. +- Do not migrate Cells: it has a specialized formula dependency graph and custom model semantics. + +## Tests + +- insert/remove/move/change/reset; +- selection preservation; +- filtering/sorting proxy interaction; +- adapter destruction; +- large range changes; +- mutation during model notification; +- stable identity where rows represent long-lived objects. + +--- + +# Stage 7: Inspection and Diagnostics + +## Objective + +Make invisible data flow understandable in the existing widget inspector and debug tooling. + +This stage follows the functional primitives so it can expose one coherent model. + +## Inspectable information + +For a widget: + +- active event connections by type and registration ID; +- active `UIDataBind` / `UIValueBinding` property bindings; +- binding direction and converter type/name where available; +- current model value in a safe string representation; +- last widget value received; +- validity, numeric validation code, and optional diagnostic message; +- connected/expired endpoint status; +- owning binding/form group; +- last propagation timestamp or sequence number in debug builds; +- command bindings; +- model/collection adapter information. + +For an observable: + +- observer count; +- computed dependencies and dependents; +- current notification/reentrancy state; +- thread-affinity owner in debug builds; +- last validation or delivery error. + +## Instrumentation constraints + +- Release builds should not pay for names, timestamps, graph edges, or stack traces unless an + existing debug/inspection flag enables them. +- Do not expose raw pointers as stable identities in user-facing output. +- Inspector observation must not change lifetime or keep endpoints alive. +- Diagnostics must not recursively trigger the binding being inspected. + +## Tests + +- inspection does not retain endpoints; +- disconnected and expired state visibility; +- validation code and optional diagnostic visibility; +- command and computed dependency display; +- debug instrumentation compiled out or minimized in release configuration. + +--- + +# Explicitly Deferred Work + +## Generic observable transactions + +Deferred because eepp already coalesces layout invalidation. Reconsider only with profiling evidence +of expensive non-layout observers repeatedly recomputing during bulk configuration changes. + +## Scripting + +Deferred until the C++ APIs and inspection model stabilize. A future scripting bridge should expose +the same primitives rather than inventing a separate lifetime system: + +- `EventConnection`; +- observable values and computed values; +- UI bindings and validation; +- commands; +- models/collection adapters. + +The scripting design must explicitly solve VM/context destruction, callback disconnection, dynamic +type conversion, error reporting, and UI-thread delivery. + +## Virtual DOM / mandatory declarative components + +Not planned. Users may build a React-like layer on these primitives, but eepp's core remains a +retained widget tree with direct, predictable C++ control. + +--- + +# Cross-Stage Quality Requirements + +Every stage must: + +1. Preserve current direct widget/event APIs. +2. Document ownership, thread affinity, notification order, and destruction behavior. +3. Use scoped connections for all callbacks that capture object addresses. +4. Avoid per-update allocation on successful common paths where practical. +5. Preserve registration-order dispatch. +6. Define reentrancy before exposing APIs that can form cycles. +7. Add focused destruction-order and mutation-during-notification tests. +8. Run clang-format on modified C/C++ files. +9. Run `git diff --check`. +10. Build with the project's ASAN debug configuration. +11. Run focused tests first, followed by the full unit-test suite before each stage is considered + complete. +12. Audit at least one real eepp or ecode workflow before accepting a new abstraction. + +# Immediate Next Action + +Review and commit Stage 1, then design Stage 2 around the implementation that now exists. Audit +`UIDataBindHolder`, existing form-like screens, invalid/error widget styling, and theme conventions. +The Stage 2 design must keep field-local converter errors separate from cross-field/form errors, +aggregate observable `UIValueValidationState` instances without retaining widgets, and avoid +adding machinery to ordinary bindings solely for form use cases. diff --git a/include/eepp/ui.hpp b/include/eepp/ui.hpp index 767f3d653..7d687e58f 100644 --- a/include/eepp/ui.hpp +++ b/include/eepp/ui.hpp @@ -174,6 +174,7 @@ #include #include #include +#include #include #include #include diff --git a/include/eepp/ui/uidatabind.hpp b/include/eepp/ui/uidatabind.hpp index 121987d2c..2c8706cc6 100644 --- a/include/eepp/ui/uidatabind.hpp +++ b/include/eepp/ui/uidatabind.hpp @@ -3,7 +3,6 @@ #include #include -#include #include #include #include @@ -18,6 +17,10 @@ namespace EE { namespace UI { * object supplied at construction. Calling set() updates that object and propagates the converted * value to every bound widget. * + * The converter maps directly between T and the widget property string. Its toValue() callback + * decides whether widget input is acceptable. Values passed to set() are authoritative model state + * and are formatted through fromValue(). + * * @warning The external object is not owned. It must outlive the UIDataBind, or reset() must be * called before that object is destroyed. UIProperty is the owning alternative when the value * should have the same lifetime as its binding. @@ -50,7 +53,7 @@ template class UIDataBind { static std::unique_ptr> New( T* t, const UnorderedSet& widgets, - const Converter& converter = UIDataBind::converterDefault(), + const Converter& converter = Converter::converterDefault(), const std::string& valueKey = "value", const Event::EventType& eventType = Event::OnValueChange ) { return std::unique_ptr>( @@ -58,7 +61,7 @@ template class UIDataBind { } static std::unique_ptr> - New( T* t, UIWidget* widget, const Converter& converter = UIDataBind::converterDefault(), + New( T* t, UIWidget* widget, const Converter& converter = Converter::converterDefault(), const std::string& valueKey = "value", const Event::EventType& eventType = Event::OnValueChange ) { return std::unique_ptr>( @@ -72,21 +75,20 @@ template class UIDataBind { UIDataBind& operator=( UIDataBind&& ) = delete; UIDataBind( T* t, const UnorderedSet& widgets, - const Converter& converter = UIDataBind::converterDefault(), + const Converter& converter = Converter::converterDefault(), const std::string& valueKey = "value", const Event::EventType& eventType = Event::OnValueChange ) { init( t, widgets, converter, valueKey, eventType ); } - UIDataBind( T* t, UIWidget* widget, - const Converter& converter = UIDataBind::converterDefault(), + UIDataBind( T* t, UIWidget* widget, const Converter& converter = Converter::converterDefault(), const std::string& valueKey = "value", const Event::EventType& eventType = Event::OnValueChange ) { init( t, { widget }, converter, valueKey, eventType ); } void init( T* t, const UnorderedSet& widgets, - const Converter& converter = UIDataBind::converterDefault(), + const Converter& converter = Converter::converterDefault(), const std::string& valueKey = "value", const Event::EventType& eventType = Event::OnValueChange ) { eeASSERT( t != nullptr ); @@ -104,29 +106,11 @@ template class UIDataBind { dataInitialized = true; } - void set( const T& t ) { - eeASSERT( isInitialized() ); - if ( dataInitialized && t == *data ) - return; - inSetValue = true; - *data = t; - setValueChange(); - inSetValue = false; - if ( onValueChangeCb ) - onValueChangeCb( t ); - } + /** Propagates the authoritative model value and reports formatting failures. */ + UIValueValidationResult set( const T& t ) { return setData( t ); } - void set( T&& t ) { - eeASSERT( isInitialized() ); - if ( dataInitialized && t == *data ) - return; - inSetValue = true; - *data = std::move( t ); - setValueChange(); - inSetValue = false; - if ( onValueChangeCb ) - onValueChangeCb( *data ); - } + /** Propagates the authoritative model value and reports formatting failures. */ + UIValueValidationResult set( T&& t ) { return setData( std::move( t ) ); } const T& get() const { eeASSERT( isInitialized() ); @@ -147,6 +131,8 @@ template class UIDataBind { connections.clear(); widgets.clear(); converter = Converter(); + validation.clear(); + validationEmitter = nullptr; inSetValue = false; dataInitialized = false; property = nullptr; @@ -161,9 +147,14 @@ template class UIDataBind { return; bindListeners( widget ); widgets.insert( widget ); - inSetValue = true; - widget->applyProperty( StyleSheetProperty( property, dataToString() ) ); - inSetValue = false; + std::string string; + auto result = dataToString( string ); + if ( result ) { + inSetValue = true; + widget->applyProperty( StyleSheetProperty( property, string ) ); + inSetValue = false; + } + setValidationResult( std::move( result ) ); } /** @brief Disconnects and removes @p widget from the synchronized widget set. */ @@ -172,9 +163,18 @@ template class UIDataBind { return; connections.erase( widget ); widgets.erase( widget ); + if ( validationEmitter == widget ) { + validationEmitter = nullptr; + validation.clear(); + } } - ~UIDataBind() { reset(); } + ~UIDataBind() { + // Do not publish a final "valid" transition while the binding itself is being destroyed. + // Validation connections expire safely with validation after widget listeners are removed. + connections.clear(); + widgets.clear(); + } const PropertyDefinition* getPropertyDefinition() const { return property; } @@ -182,7 +182,31 @@ template class UIDataBind { const UnorderedSet& getWidgets() const { return widgets; } + /** @return Observable converter error state for this binding. */ + UIValueValidationState& validationState() { return validation; } + const UIValueValidationState& validationState() const { return validation; } + bool isValid() const { return validation.isValid(); } + protected: + template UIValueValidationResult setData( U&& t ) { + eeASSERT( isInitialized() ); + if ( dataInitialized && t == *data ) { + inSetValue = true; + auto result = setValueChange(); + inSetValue = false; + setValidationResult( result ); + return result; + } + inSetValue = true; + *data = std::forward( t ); + auto result = setValueChange(); + inSetValue = false; + if ( onValueChangeCb ) + onValueChangeCb( *data ); + setValidationResult( result ); + return result; + } + T* data{ nullptr }; UnorderedSet widgets; UnorderedMap connections; @@ -190,6 +214,8 @@ template class UIDataBind { bool dataInitialized{ false }; const PropertyDefinition* property{ nullptr }; Converter converter; + UIValueValidationState validation; + UIWidget* validationEmitter{ nullptr }; Event::EventType eventType{ Event::OnValueChange }; void bindListeners( UIWidget* widget ) { @@ -201,17 +227,25 @@ template class UIDataBind { auto widget = event->getNode()->asType(); connections.erase( widget ); widgets.erase( widget ); + if ( validationEmitter == widget ) { + validationEmitter = nullptr; + validation.clear(); + } } ); } - std::string dataToString() const { + UIValueValidationResult dataToString( std::string& string ) const { eeASSERT( isInitialized() ); - std::string str; - if ( !converter.fromValue( property, str, *data ) ) { - Log::error( "UIDataBind::dataToString converter::fromValue: unable to convert value " - "to string." ); - } - return str; + auto converted = converter.fromValue( property, *data ); + if ( !converted ) + return converted.validation; + string = std::move( *converted.value ); + return UIValueValidationResult::success(); + } + + void setValidationResult( UIValueValidationResult result, UIWidget* emitter = nullptr ) { + validationEmitter = result ? nullptr : emitter; + validation.set( std::move( result ) ); } void processValueChange( UIWidget* emitter ) { @@ -219,28 +253,40 @@ template class UIDataBind { eeASSERT( emitter != nullptr ); if ( inSetValue ) return; - bool success = false; - T val; - success = converter.toValue( property, val, emitter->getPropertyString( property ) ); - - if ( success ) { - *data = val; - StyleSheetProperty prop( property, dataToString(), 0, false ); - inSetValue = true; - for ( auto widget : widgets ) { - if ( widget != emitter ) - widget->applyProperty( prop ); - } - inSetValue = false; - if ( onValueChangeCb ) - onValueChangeCb( val ); + auto proposed = converter.toValue( property, emitter->getPropertyString( property ) ); + if ( !proposed ) { + setValidationResult( std::move( proposed.validation ), emitter ); + return; } + + auto canonicalString = converter.fromValue( property, *proposed.value ); + if ( !canonicalString ) { + setValidationResult( std::move( canonicalString.validation ), emitter ); + return; + } + *data = std::move( *proposed.value ); + StyleSheetProperty prop( property, *canonicalString.value, 0, false ); + inSetValue = true; + for ( auto widget : widgets ) { + if ( widget != emitter ) + widget->applyProperty( prop ); + } + inSetValue = false; + validationEmitter = nullptr; + validation.clear(); + if ( onValueChangeCb ) + onValueChangeCb( *data ); } - void setValueChange() { - StyleSheetProperty prop( property, dataToString(), 0, false ); + UIValueValidationResult setValueChange() { + std::string string; + auto result = dataToString( string ); + if ( !result ) + return result; + StyleSheetProperty prop( property, string, 0, false ); for ( auto widget : widgets ) widget->applyProperty( prop ); + return result; } }; diff --git a/include/eepp/ui/uiproperty.hpp b/include/eepp/ui/uiproperty.hpp index 6783fec44..b1a5d8462 100644 --- a/include/eepp/ui/uiproperty.hpp +++ b/include/eepp/ui/uiproperty.hpp @@ -16,7 +16,7 @@ namespace EE { namespace UI { * * Use UIProperty for concise UI-local state when the value and its widgets naturally share a * lifetime. It avoids the shared state required by ObservableValue and owns its UIDataBind - * directly. + * directly. A custom UIValueConverter can provide presentation-specific parsing and formatting. * * @code * UIProperty celsius( 0.0, celsiusInput ); @@ -143,6 +143,10 @@ template class UIProperty { const T& value() const { return mBindedData.get(); } const UIDataBind& databind() const { return mBindedData; } + UIDataBind& databind() { return mBindedData; } + + /** @return Current converter error state. */ + const UIValueValidationState& validationState() const { return mBindedData.validationState(); } /** @brief Connects another widget to this property's value. */ UIProperty& connect( UIWidget* widget ) { diff --git a/include/eepp/ui/uivaluebinding.hpp b/include/eepp/ui/uivaluebinding.hpp index 83ae23ad7..bc38efb64 100644 --- a/include/eepp/ui/uivaluebinding.hpp +++ b/include/eepp/ui/uivaluebinding.hpp @@ -2,7 +2,6 @@ #define EE_UI_UIVALUEBINDING_HPP #include -#include #include #include @@ -11,10 +10,12 @@ namespace EE { namespace UI { /** * @brief Move-only two-way binding between an ObservableValue and a UIWidget property. * - * The binding synchronizes the observable's current value into the widget immediately. Later - * observable changes update the widget, and the selected widget event converts the property back - * into the observable. Destroying the binding disconnects both directions. Destroying either the - * observable or widget first is safe and does not keep that endpoint alive. + * The converter maps directly between T and the widget property string. Its toValue() callback + * decides whether widget input may enter the model. Model-originated values are authoritative and + * are formatted through fromValue(). + * + * Destroying the binding disconnects both directions. Destroying either the observable or widget + * first is safe and does not keep that endpoint alive. * * Synchronization is immediate and single-threaded. The observable, widget, and binding must all be * used on the widget's owning UI thread. @@ -50,6 +51,13 @@ template class UIValueBinding { void disconnect() { mState.reset(); } explicit operator bool() const { return mState && mState->widget && mState->value; } + bool isValid() const { return !mState || mState->validation.isValid(); } + + /** @return Observable conversion and input-validation state. */ + UIValueValidationState* validationState() { return mState ? &mState->validation : nullptr; } + const UIValueValidationState* validationState() const { + return mState ? &mState->validation : nullptr; + } private: struct State { @@ -57,6 +65,7 @@ template class UIValueBinding { UIWidget* widget{ nullptr }; const PropertyDefinition* property{ nullptr }; Converter converter; + UIValueValidationState validation; bool synchronizing{ false }; typename ObservableValue::Connection valueConnection; EventConnectionList widgetConnections; @@ -64,14 +73,15 @@ template class UIValueBinding { bool applyToWidget( const T& newValue ) { if ( !widget ) return false; - std::string string; - if ( !converter.fromValue( property, string, newValue ) ) { - Log::error( "UIValueBinding: unable to convert observable value to string." ); + auto converted = converter.fromValue( property, newValue ); + if ( !converted ) { + validation.set( std::move( converted.validation ) ); return false; } synchronizing = true; - widget->applyProperty( StyleSheetProperty( property, string ) ); + widget->applyProperty( StyleSheetProperty( property, *converted.value ) ); synchronizing = false; + validation.clear(); return true; } }; @@ -94,12 +104,15 @@ template class UIValueBinding { } ); state->widgetConnections += widget->connect( eventType, [weakState]( const Event* event ) { if ( auto state = weakState.lock(); state && !state->synchronizing ) { - T newValue; - if ( state->converter.toValue( - state->property, newValue, - event->getNode()->asType()->getPropertyString( - state->property ) ) && - !state->value.set( std::move( newValue ) ) ) { + auto proposed = state->converter.toValue( + state->property, + event->getNode()->asType()->getPropertyString( state->property ) ); + if ( !proposed ) { + state->validation.set( std::move( proposed.validation ) ); + return; + } + state->validation.clear(); + if ( !state->value.set( std::move( *proposed.value ) ) ) { state->widget = nullptr; state->widgetConnections.clear(); } @@ -109,6 +122,7 @@ template class UIValueBinding { if ( auto state = weakState.lock() ) { state->widget = nullptr; state->valueConnection.disconnect(); + state->validation.clear(); state->widgetConnections.clear(); } } ); @@ -121,10 +135,11 @@ template class UIValueBinding { /** @brief Creates a scoped two-way binding between @p value and @p widget. */ template -UIValueBinding bindValue( - ObservableValue& value, UIWidget* widget, - const typename UIValueBinding::Converter& converter = UIValueBinding::converterDefault(), - const std::string& propertyName = "value", Event::EventType eventType = Event::OnValueChange ) { +UIValueBinding +bindValue( ObservableValue& value, UIWidget* widget, + const UIValueConverter& converter = UIValueConverter::converterDefault(), + const std::string& propertyName = "value", + Event::EventType eventType = Event::OnValueChange ) { return UIValueBinding( value, widget, converter, propertyName, eventType ); } diff --git a/include/eepp/ui/uivalueconverter.hpp b/include/eepp/ui/uivalueconverter.hpp index fdfdd688f..ae98143d3 100644 --- a/include/eepp/ui/uivalueconverter.hpp +++ b/include/eepp/ui/uivalueconverter.hpp @@ -3,9 +3,9 @@ #include #include +#include #include #include -#include #include #include @@ -20,18 +20,21 @@ namespace EE { namespace UI { * * @code * auto converter = UIValueConverter( - * []( const CSS::PropertyDefinition*, MyEnum& value, const std::string& text ) { - * return enumFromString( value, text ); + * []( const CSS::PropertyDefinition*, const std::string& text ) { + * MyEnum value; + * return enumFromString( value, text ) ? UIValueResult::success( value ) + * : UIValueResult::error( 1 ); * }, - * []( const CSS::PropertyDefinition*, std::string& text, const MyEnum& value ) { - * text = enumToString( value ); - * return true; + * []( const CSS::PropertyDefinition*, const MyEnum& value ) { + * return UIValueResult::success( enumToString( value ) ); * } ); * @endcode */ template struct UIValueConverter { - using ToValue = std::function; - using FromValue = std::function; + using ToValue = + std::function( const CSS::PropertyDefinition*, const std::string& )>; + using FromValue = + std::function( const CSS::PropertyDefinition*, const T& )>; UIValueConverter() = default; UIValueConverter( ToValue toValue, FromValue fromValue ) : @@ -42,18 +45,22 @@ template struct UIValueConverter { static UIValueConverter converterDefault() { return UIValueConverter( - []( const CSS::PropertyDefinition* property, T& value, const std::string& string ) { + []( const CSS::PropertyDefinition* property, const std::string& string ) { if constexpr ( std::is_same_v || std::is_same_v ) { - value = T( string ); - return true; + return UIValueResult::success( T( string ) ); } else if constexpr ( std::is_same_v ) { - value = CSS::StyleSheetProperty( property, string ).asBool(); - return true; + return UIValueResult::success( + CSS::StyleSheetProperty( property, string ).asBool() ); } else { - return String::fromString( value, string ); + T value; + return String::fromString( value, string ) + ? UIValueResult::success( std::move( value ) ) + : UIValueResult::error( static_cast( + UIValueValidationError::ConversionFailed ) ); } }, - []( const CSS::PropertyDefinition*, std::string& string, const T& value ) { + []( const CSS::PropertyDefinition*, const T& value ) { + std::string string; if constexpr ( std::is_same_v ) { string = value; } else if constexpr ( std::is_same_v ) { @@ -67,34 +74,31 @@ template struct UIValueConverter { } else { string = String::toString( value ); } - return true; + return UIValueResult::success( std::move( string ) ); } ); } static UIValueConverter converterString() { return UIValueConverter( - []( const CSS::PropertyDefinition*, T& value, const std::string& string ) { - value = T( string ); - return true; + []( const CSS::PropertyDefinition*, const std::string& string ) { + return UIValueResult::success( T( string ) ); }, - []( const CSS::PropertyDefinition*, std::string& string, const T& value ) { + []( const CSS::PropertyDefinition*, const T& value ) { if constexpr ( std::is_same_v ) - string = value.toUtf8(); + return UIValueResult::success( value.toUtf8() ); else - string = value; - return true; + return UIValueResult::success( value ); } ); } static UIValueConverter converterBool() { return UIValueConverter( - []( const CSS::PropertyDefinition* property, T& value, const std::string& string ) { - value = CSS::StyleSheetProperty( property, string ).asBool(); - return true; + []( const CSS::PropertyDefinition* property, const std::string& string ) { + return UIValueResult::success( + CSS::StyleSheetProperty( property, string ).asBool() ); }, - []( const CSS::PropertyDefinition*, std::string& string, const T& value ) { - string = value ? "true" : "false"; - return true; + []( const CSS::PropertyDefinition*, const T& value ) { + return UIValueResult::success( value ? "true" : "false" ); } ); } }; diff --git a/include/eepp/ui/uivaluevalidation.hpp b/include/eepp/ui/uivaluevalidation.hpp new file mode 100644 index 000000000..0a8646c71 --- /dev/null +++ b/include/eepp/ui/uivaluevalidation.hpp @@ -0,0 +1,150 @@ +#ifndef EE_UI_UIVALUEVALIDATION_HPP +#define EE_UI_UIVALUEVALIDATION_HPP + +#include +#include +#include +#include +#include +#include +#include + +namespace EE { namespace UI { + +/** Numeric error codes reserved by eepp's built-in value converters. */ +enum class UIValueValidationError : Uint32 { + ConversionFailed = 1, +}; + +/** + * @brief Machine-readable result of converting or accepting a widget value. + * + * A numeric code is the normal way to identify an error. Applications can map that code, together + * with the binding or form context, to localized UI text. debugMessage is optional diagnostic + * information for logs, tests, and inspection; it should not be shown to users implicitly. + * + * Codes are owned by the subsystem or application defining the validation rule. Apart from values + * in UIValueValidationError, eepp does not require codes to be globally unique or stable for + * serialization. Code 0 is valid: the optional itself represents the absence of a code. + */ +struct UIValueValidationResult { + using Code = Uint32; + + bool valid{ true }; + std::optional code; + std::optional debugMessage; + + UIValueValidationResult() = default; + + static UIValueValidationResult success() { return {}; } + + static UIValueValidationResult error( Code code ) { + UIValueValidationResult result; + result.valid = false; + result.code = code; + return result; + } + + static UIValueValidationResult error( Code code, std::string debugMessage ) { + UIValueValidationResult result = error( code ); + result.debugMessage = std::move( debugMessage ); + return result; + } + + static UIValueValidationResult error( std::string debugMessage ) { + UIValueValidationResult result; + result.valid = false; + result.debugMessage = std::move( debugMessage ); + return result; + } + + explicit operator bool() const { return valid; } + + bool operator==( const UIValueValidationResult& other ) const { + return valid == other.valid && code == other.code && debugMessage == other.debugMessage; + } + + bool operator!=( const UIValueValidationResult& other ) const { return !( *this == other ); } +}; + +/** + * @brief A converted value together with its failure information. + * + * Successful results always contain a value. Failures contain no value and preserve the numeric + * code and optional diagnostic produced by a converter. + */ +template struct UIValueResult { + std::optional value; + UIValueValidationResult validation; + + UIValueResult() = default; + UIValueResult( T value ) : value( std::move( value ) ) {} + + static UIValueResult success( T value ) { return UIValueResult( std::move( value ) ); } + + static UIValueResult error( UIValueValidationResult validation ) { + UIValueResult result; + result.validation = std::move( validation ); + return result; + } + + static UIValueResult error( UIValueValidationResult::Code code ) { + return error( UIValueValidationResult::error( code ) ); + } + + static UIValueResult error( UIValueValidationResult::Code code, std::string debugMessage ) { + return error( UIValueValidationResult::error( code, std::move( debugMessage ) ) ); + } + + explicit operator bool() const { return validation.valid && value.has_value(); } +}; + +/** + * @brief Observable current validation result shared by UI value binding implementations. + * + * Reading validity never allocates. Observer storage is created lazily by observe(), keeping the + * ordinary unobserved binding path inexpensive. Notifications are synchronous and use the same + * snapshot semantics as ObservableValue. + */ +class UIValueValidationState { + public: + using Callback = std::function; + using Connection = ObservableValue::Connection; + + UIValueValidationState() = default; + UIValueValidationState( const UIValueValidationState& ) = delete; + UIValueValidationState& operator=( const UIValueValidationState& ) = delete; + UIValueValidationState( UIValueValidationState&& ) = delete; + UIValueValidationState& operator=( UIValueValidationState&& ) = delete; + + bool isValid() const { return mResult.valid; } + const UIValueValidationResult& result() const { return mResult; } + const std::optional& code() const { return mResult.code; } + const std::optional& debugMessage() const { return mResult.debugMessage; } + + /** Observes later result changes. The current result is available through result(). */ + Connection observe( Callback callback ) { + if ( !mObservable ) + mObservable = std::make_unique>( mResult ); + return mObservable->observe( std::move( callback ) ); + } + + /** Updates the current result and notifies observers only when it actually changed. */ + void set( UIValueValidationResult result ) { + if ( mResult == result ) + return; + mResult = std::move( result ); + if ( mObservable ) + mObservable->set( mResult ); + } + + void clear() { set( UIValueValidationResult::success() ); } + + private: + UIValueValidationResult mResult; + std::unique_ptr> mObservable; +}; + +}} // namespace EE::UI + +#endif diff --git a/src/tests/unit_tests/observablevalue_tests.cpp b/src/tests/unit_tests/observablevalue_tests.cpp index a9c8ca4eb..36977b840 100644 --- a/src/tests/unit_tests/observablevalue_tests.cpp +++ b/src/tests/unit_tests/observablevalue_tests.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include using namespace EE; @@ -90,3 +91,79 @@ UTEST( UIValueBinding, synchronizesBothDirectionsAndHandlesEndpointLifetimes ) { eeDelete( widget ); EXPECT_FALSE( static_cast( binding ) ); } + +UTEST( UIValueBinding, validatesWidgetProposalsButAcceptsAuthoritativeObservableValues ) { + UIApplication app( + WindowSettings( 320, 240, "eepp - UIValueBinding Validation Test", WindowStyle::Default, + WindowBackend::Default, 32 ), + UIApplication::Settings( Sys::getProcessPath() + ".." + FileSystem::getOSSlash(), 1 ) ); + auto widget = UITextInput::New(); + ObservableValue value( "initial" ); + UIValueConverter nonEmpty( + []( const CSS::PropertyDefinition*, + const std::string& candidate ) -> UIValueResult { + return candidate.empty() ? UIValueResult::error( 300, "empty value" ) + : UIValueResult::success( candidate ); + }, + UIValueConverter::converterString().fromValue ); + auto binding = bindValue( value, widget, nonEmpty, "text", Event::OnTextChanged ); + + widget->setText( "" ); + EXPECT_TRUE( value.get() == "initial" ); + EXPECT_FALSE( binding.isValid() ); + EXPECT_EQ( *binding.validationState()->code(), 300u ); + + widget->setText( "valid" ); + EXPECT_TRUE( value.get() == "valid" ); + EXPECT_TRUE( binding.isValid() ); + + value = ""; + EXPECT_TRUE( widget->getText().empty() ); + EXPECT_TRUE( binding.isValid() ); + + eeDelete( widget ); + EXPECT_TRUE( binding.isValid() ); +} + +UTEST( UIValueBinding, convertsFormattedModelValues ) { + UIApplication app( + WindowSettings( 320, 240, "eepp - UIValueBinding Converter Test", WindowStyle::Default, + WindowBackend::Default, 32 ), + UIApplication::Settings( Sys::getProcessPath() + ".." + FileSystem::getOSSlash(), 1 ) ); + auto widget = UITextInput::New(); + ObservableValue amount( 10.0 ); + UIValueConverter euro( + []( const CSS::PropertyDefinition*, const std::string& text ) -> UIValueResult { + const std::string prefix( "€" ); + double value; + if ( text.rfind( prefix, 0 ) != 0 || + !String::fromString( value, text.substr( prefix.size() ) ) ) + return UIValueResult::error( 400, "expected an EUR amount" ); + if ( value < 0 ) + return UIValueResult::error( 401 ); + return value; + }, + []( const CSS::PropertyDefinition*, double value ) { + return UIValueResult( "€" + String::fromDouble( value ) ); + } ); + auto binding = bindValue( amount, widget, euro, "text", Event::OnTextChanged ); + + EXPECT_TRUE( widget->getText() == "€10" ); + widget->setText( "€25" ); + EXPECT_EQ( amount.get(), 25.0 ); + EXPECT_TRUE( binding.isValid() ); + + widget->setText( "USD 30" ); + EXPECT_EQ( amount.get(), 25.0 ); + EXPECT_EQ( *binding.validationState()->code(), 400u ); + + widget->setText( "€-5" ); + EXPECT_EQ( amount.get(), 25.0 ); + EXPECT_EQ( *binding.validationState()->code(), 401u ); + + amount = -5; + EXPECT_TRUE( widget->getText() == "€-5" ); + EXPECT_TRUE( binding.isValid() ); + + eeDelete( widget ); +} diff --git a/src/tests/unit_tests/uidatabind_tests.cpp b/src/tests/unit_tests/uidatabind_tests.cpp index 6ff34cea0..e49cac259 100644 --- a/src/tests/unit_tests/uidatabind_tests.cpp +++ b/src/tests/unit_tests/uidatabind_tests.cpp @@ -3,10 +3,45 @@ #include #include #include +#include using namespace EE; using namespace EE::UI; +UTEST( UIValueValidation, supportsCodesAndOptionalDiagnostics ) { + auto success = UIValueValidationResult::success(); + EXPECT_TRUE( static_cast( success ) ); + EXPECT_FALSE( success.code.has_value() ); + EXPECT_FALSE( success.debugMessage.has_value() ); + + auto coded = UIValueValidationResult::error( 42 ); + EXPECT_FALSE( static_cast( coded ) ); + EXPECT_TRUE( coded.code.has_value() ); + EXPECT_EQ( *coded.code, 42u ); + EXPECT_FALSE( coded.debugMessage.has_value() ); + + auto diagnosed = UIValueValidationResult::error( 43, "technical detail" ); + EXPECT_EQ( *diagnosed.code, 43u ); + EXPECT_TRUE( *diagnosed.debugMessage == "technical detail" ); + auto diagnosticOnly = UIValueValidationResult::error( std::string( "only diagnostic" ) ); + EXPECT_FALSE( diagnosticOnly.code.has_value() ); + EXPECT_TRUE( *diagnosticOnly.debugMessage == "only diagnostic" ); +} + +UTEST( UIValueValidation, observesOnlyDistinctResultChanges ) { + UIValueValidationState state; + int notifications = 0; + auto connection = state.observe( [&]( const UIValueValidationResult& ) { ++notifications; } ); + + state.set( UIValueValidationResult::error( 7 ) ); + state.set( UIValueValidationResult::error( 7 ) ); + state.set( UIValueValidationResult::error( 7, "detail" ) ); + state.clear(); + + EXPECT_EQ( notifications, 3 ); + EXPECT_TRUE( state.isValid() ); +} + UTEST( UIProperty, defaultConstructionOwnsUsableValue ) { UIProperty property; EXPECT_EQ( property.value(), 0 ); @@ -69,9 +104,9 @@ UTEST( UIProperty, stringConcatenationPropagatesForStandardAndEEStrings ) { UTEST( UIDataBind, defaultStringConverterReadsWidgetValue ) { auto converter = UIDataBind::converterDefault(); - std::string value; - EXPECT_TRUE( converter.toValue( nullptr, value, "widget value" ) ); - EXPECT_TRUE( value == "widget value" ); + auto value = converter.toValue( nullptr, "widget value" ); + EXPECT_TRUE( value ); + EXPECT_TRUE( *value.value == "widget value" ); } UTEST( UIDataBind, lateBoundWidgetReceivesValueAndCanDieFirst ) { @@ -119,3 +154,98 @@ UTEST( UIDataBind, supportsMultipleWidgetsAndDisconnectsWhenBindingDiesFirst ) { eeDelete( firstWidget ); eeDelete( secondWidget ); } + +UTEST( UIDataBind, validatesWidgetProposalsButAcceptsAuthoritativeModelValues ) { + UIApplication app( + WindowSettings( 320, 240, "eepp - UIDataBind Validation Test", WindowStyle::Default, + WindowBackend::Default, 32 ), + UIApplication::Settings( Sys::getProcessPath() + ".." + FileSystem::getOSSlash(), 1 ) ); + std::string value( "initial" ); + auto firstWidget = UITextInput::New(); + auto secondWidget = UITextInput::New(); + UIValueConverter converter( + []( const CSS::PropertyDefinition*, + const std::string& candidate ) -> UIValueResult { + return candidate == "invalid" ? UIValueResult::error( 100 ) + : UIValueResult::success( candidate ); + }, + UIValueConverter::converterString().fromValue ); + UIDataBind binding( &value, UnorderedSet{ firstWidget, secondWidget }, + converter, "text", Event::OnTextChanged ); + + firstWidget->setText( "invalid" ); + EXPECT_TRUE( value == "initial" ); + EXPECT_TRUE( firstWidget->getText() == "invalid" ); + EXPECT_TRUE( secondWidget->getText() == "initial" ); + EXPECT_FALSE( binding.isValid() ); + EXPECT_EQ( *binding.validationState().code(), 100u ); + + auto result = binding.set( std::string( "invalid" ) ); + EXPECT_TRUE( static_cast( result ) ); + EXPECT_TRUE( value == "invalid" ); + EXPECT_TRUE( binding.isValid() ); + + EXPECT_TRUE( binding.set( std::string( "initial" ) ) ); + firstWidget->setText( "initial" ); + firstWidget->setText( "invalid" ); + eeDelete( firstWidget ); + firstWidget = nullptr; + EXPECT_TRUE( binding.isValid() ); + + firstWidget = UITextInput::New(); + binding.bind( firstWidget ); + firstWidget->setText( "valid" ); + EXPECT_TRUE( value == "valid" ); + EXPECT_TRUE( secondWidget->getText() == "valid" ); + EXPECT_TRUE( binding.isValid() ); + + eeDelete( firstWidget ); + eeDelete( secondWidget ); +} + +UTEST( UIDataBind, defaultNumericConversionFailurePreservesModel ) { + UIApplication app( + WindowSettings( 320, 240, "eepp - UIDataBind Parse Validation Test", WindowStyle::Default, + WindowBackend::Default, 32 ), + UIApplication::Settings( Sys::getProcessPath() + ".." + FileSystem::getOSSlash(), 1 ) ); + auto widget = UITextInput::New(); + int value = 5; + UIDataBind binding( &value, widget, UIDataBind::converterDefault(), "text", + Event::OnTextChanged ); + + widget->setText( "not a number" ); + EXPECT_EQ( value, 5 ); + EXPECT_FALSE( binding.isValid() ); + EXPECT_EQ( *binding.validationState().code(), + static_cast( UIValueValidationError::ConversionFailed ) ); + + eeDelete( widget ); +} + +UTEST( UIDataBind, failedModelToWidgetConversionDoesNotApplyPartialText ) { + UIApplication app( + WindowSettings( 320, 240, "eepp - UIDataBind Conversion Test", WindowStyle::Default, + WindowBackend::Default, 32 ), + UIApplication::Settings( Sys::getProcessPath() + ".." + FileSystem::getOSSlash(), 1 ) ); + auto widget = UITextInput::New(); + int value = 1; + UIValueConverter converter( + []( const CSS::PropertyDefinition*, const std::string& ) { + return UIValueResult::success( 1 ); + }, + []( const CSS::PropertyDefinition*, const int& converted ) { + return converted == 2 ? UIValueResult::error( 200 ) + : UIValueResult::success( "partial" ); + } ); + UIDataBind binding( &value, widget, converter, "text", Event::OnTextChanged ); + EXPECT_TRUE( widget->getText() == "partial" ); + + auto result = binding.set( 2 ); + EXPECT_FALSE( static_cast( result ) ); + EXPECT_TRUE( widget->getText() == "partial" ); + EXPECT_EQ( value, 2 ); + EXPECT_FALSE( binding.set( 2 ) ); + EXPECT_FALSE( binding.isValid() ); + + eeDelete( widget ); +} diff --git a/src/tools/ecode/uibuildsettings.cpp b/src/tools/ecode/uibuildsettings.cpp index ea3c0ee71..5983d6cfd 100644 --- a/src/tools/ecode/uibuildsettings.cpp +++ b/src/tools/ecode/uibuildsettings.cpp @@ -127,20 +127,18 @@ class UICustomOutputParserWindow : public UIWindow { UIDropDownList* cpTypeddl = find( "custom_parser_type" ); UIDataBind::Converter projectOutputParserTypesConverter( - []( const PropertyDefinition* property, ProjectOutputParserTypes& val, - const std::string& str ) -> bool { + []( const PropertyDefinition* property, const std::string& str ) { auto v = StyleSheetProperty( property, str ).asString(); Uint32 idx; - if ( String::fromString( idx, v ) && idx >= 0 && idx <= 2 ) { - val = (ProjectOutputParserTypes)idx; - return true; - } - return false; + if ( String::fromString( idx, v ) && idx <= 2 ) + return UIValueResult::success( + static_cast( idx ) ); + return UIValueResult::error( + static_cast( UIValueValidationError::ConversionFailed ) ); }, - [cpTypeddl]( const PropertyDefinition*, std::string& str, - const ProjectOutputParserTypes& val ) -> bool { - str = cpTypeddl->getListBox()->getItem( (Uint32)val )->getText(); - return true; + [cpTypeddl]( const PropertyDefinition*, const ProjectOutputParserTypes& val ) { + return UIValueResult::success( + cpTypeddl->getListBox()->getItem( static_cast( val ) )->getText() ); } ); mDataBindHolder += UIDataBind::New(