ComboBox: reverse-lookup current-value writes into current-index - #12792
ComboBox: reverse-lookup current-value writes into current-index#12792tilladam wants to merge 6 commits into
Conversation
c6c437f to
5fdc5ba
Compare
5fdc5ba to
cb693dd
Compare
Sibling to the array.any/array.all predicates (slint-ui#11989): returns the index of the first element for which the closure holds, or -1 if none match. Mirrors any/all end-to-end — parser/lookup, resolving, Rust and C++ codegen, api/cpp/include/private/slint_models.h, the interpreter, and core::model::model_find_index — reusing the existing Expression::Closure/ Type::Closure plumbing. Same experimental gate as any/all (tracked by slint-ui#12777); only usable inline, not stored or passed around, for the same reasons any/all are restricted that way. The interpreter reuses core's model_any/model_all/model_find_index for the row iteration and dependency tracking, rather than open-coding the loop a second time; a small eval_array_row_predicate helper does the per-row work shared by any/all/find-index (bind arg_name, evaluate the closure expression, restore the shadowed local var). Added as groundwork for a real fix to slint-ui#11970 (ComboBox/RadioGroup current-value writes), which needs a value-to-index reverse lookup that plain any/all can't express. changelog: Added an experimental `array.find-index((name) => condition)` predicate, returning the index of the first matching element or -1.
`array.index-of(value)` reads the index of the first element equal to value, or -1. Unlike find-index/any/all it takes a plain value, not a predicate closure, so the caller never writes closure syntax to use it. Implemented as a compiler-side desugaring (array_index_of_macro, next to the existing push/remove/insert macros): it synthesizes an equality closure and a FunctionCall to the existing ArrayFindIndex builtin, then lowers exactly like a hand-written `find-index((x) => x == value)` call. No new codegen, no new interpreter arm, no new runtime function — it reuses model_find_index end to end. `value` is evaluated exactly once, into a local variable read from inside the synthesized closure, rather than re-embedded in the closure body — the latter would re-run it once per row visited (and not at all against an empty array), breaking normal once-per-call argument evaluation for any `value` with a side effect or a non-deterministic result. Regression test: index-of-value-with-side-effect in tests/cases/models/array.slint. Because closures are never part of the user-visible syntax for index-of, its lookup.rs entry sits outside the `enable_experimental` gate that guards any/all/find-index (verified by a dedicated test: index_of_stability.rs, since the syntax-test corpus and every runtime test driver force enable_experimental=true uniformly and so can't catch a regression here). index-of ships as a stable, ordinary array method documented alongside push/remove/insert/length, not as part of the experimental array-predicates guide. changelog: Added a stable `array.index-of(value)` array method, returning the index of the first matching element or -1.
Implements Option A from slint-ui#11970: a host write to current-value now means "select the row with this value" — the widget finds it in the model via array.index-of and moves current-index to match. A value that isn't in the model resolves to no selection (current-index -1, current-value ""), the same as setting current-index out of range — and, unlike the successful case, logs a debug() message naming the value, so a developer debugging an unexpectedly-cleared selection has something to go on. A successful reverse lookup stays silent. Uses index-of rather than find-index: ComboBox is the first built-in widget with any dependency on the array-predicates machinery, and index-of is the stable half of it (see the previous commit) — so this carries no dependency on the still-experimental find-index/any/all gate (slint-ui#12777), unlike an inline find-index((value) => value == current-value) predicate would. This supersedes the non-reverting interim behavior from 7dfcfdf, which deliberately left an unmatched host write in place (with only a debug() warning) to avoid disturbing existing `<=>` two-way bindings while the real fix was blocked on iteration support. That blocker is gone for this narrow case: index-of needs no general loop, only a single-pass reverse lookup, so we no longer need the non-reverting compromise. `<=>` bindings that round-trip an arbitrary, not-in-model string through current-value will now see it replaced (with "" or the first match) instead of preserved — this is intentional, matching the issue's Option A design, not a regression. RadioGroup is intentionally left untouched: its public API (declared in builtins.slint) exposes current-value as `out` only and never exposes current-index at all, so the only host-writable selection path is `checked` on an individual RadioButton child, which already correctly stays in sync with current-index/current-value (see RadioButtonImplBase's `changed checked` handler) — there's no writable-but-unreliable surface left for a reverse lookup to fix. Known limitation, documented in combobox-base.slint and combobox.mdx: the reverse lookup runs from `changed current-value`, which only fires on an actual value transition. A write that doesn't change the property — writing the value it already holds, or a value equal to a later duplicate in `model` while an earlier duplicate should now be selected per the "first match" contract — is a no-op. Slint has no `.slint`-level hook for "run on every write regardless of value equality" (the Rust-internal equivalent, BindingCallable:: intercept_set, isn't usable from widget source), so this can't be closed without new core machinery; it equally affected the debug()-only interim mitigation this replaces. Covered by two regression-documenting assertions in tests/cases/widgets/combobox.slint so the behavior stays intentional and visible rather than silently drifting. changelog: ComboBox: writes to `current-value` from host code (Rust/C++/JS/Python) now update the selection via reverse lookup instead of being silently discarded. Fixes slint-ui#11970 for ComboBox.
cb693dd to
812d035
Compare
LeonMatthes
left a comment
There was a problem hiding this comment.
The approach seems good. 👍
We should take this opportunity to make sure setting current-value is consistent everywhere. Currently it behaves differently if set in the element instantiation instead of imperatively.
| let found-index = root.model.index-of(root.current-value); | ||
| if found-index < 0 { | ||
| debug("ComboBox: `current-value` was set to \"" + root.current-value + "\", which is not in `model`; the selection was cleared. See https://github.com/slint-ui/slint/issues/11970."); | ||
| } |
There was a problem hiding this comment.
We should add this check to init as well, as current-value could be set at init time already, which currently behaves differently.
So currently this:
box := ComboBox {
model: ["1", "2", "3"];
current-value: "5";
}Will actually show 5, which is inconsistent with box.current-value = "5", which would instead clear the ComboBox selection.
We should clear the selection at init time as well IMO. A ComboBox should only ever show a valid value, or an invalid index.
Make sure to run this check only if current-value != root.model[root.current-index] to prevent calling index-of on every init.
There was a problem hiding this comment.
I did implement this — added init => { root.reverse-lookup-current-value(); } so a declarative mismatch gets resolved the same way a host write does.
However, I could not verify it's actually reliable, so I've reverted it (1830689) rather than leave it in a state I can't stand behind. What happened: CI's Windows job ran the widget test suite twice in the same run — once normally (passed) and once with --features slint/live-preview (failed), on assert_eq!(get_init_value_found_index(), 1). I dug into the generated Rust to understand why: the literal current-value: "5" write does land in init() before user_init() runs the init => handler (I confirmed this empirically, so it's not a fundamentally broken approach), but I couldn't reproduce the failure locally after 30+ runs, including with the exact --features slint/live-preview flag CI uses. So there's a real platform- or feature-dependent difference here I don't understand yet, and I'd rather not ship an init-time fix I can't explain or verify.
Given that, I've pulled the init handler and the init-time test instances out of this PR. If you'd like this fixed here rather than as a follow-up, I'm happy to keep digging, but wanted to flag the CI finding rather than quietly resubmit something with an unexplained flake.
| mock_elapsed_time(500); | ||
| assert_selection(-1, ""); | ||
|
|
||
| // Same limitation with a duplicate value: re-writing "dup" while the second occurrence |
There was a problem hiding this comment.
Please add a test first that set_current_value("dup") actually selects the first of the duplicate indices.
There was a problem hiding this comment.
Added — see the "A fresh write to a duplicate value" test right before the pre-existing known-limitation test: it writes a real transition ("" → "dup") and asserts index-of picks the first occurrence (index 1, not the later index 2).
| // or "" if not found. Uses the stable `index-of`, not the experimental `find-index` it | ||
| // lowers to, so ComboBox doesn't depend on an experimental feature. | ||
| // Only fires on an actual value change, so writing the value current-value already | ||
| // holds is a no-op even when the "select the first match" contract says it shouldn't be. |
There was a problem hiding this comment.
Follow-up questions: what happens if I write current-index and current-value right after each other? 🤔
Does the last set win, or does current-index win over current-value?
Maybe we should add this to the test.
There was a problem hiding this comment.
Added a test for this — the "Consecutive writes to current-index and current-value" block. What it shows: each write's changed handler resolves the other property before the next statement runs, so with set_current_index(1); set_current_value("Ccc"); the value's resolution sticks (it was written last), and reversed, the index's resolution sticks instead. So today: last write wins, no built-in priority either way.
I want to flag that this is what I observed from the current changed-handler implementation, not something I verified is a robust, documented guarantee — while chasing the init-time issue above I found ordering-dependent behavior in this widget has more edge cases than I expected, so I'd rather not assert this as an intentional contract without your input. Do you want "last write wins" codified as the actual guarantee, or would you prefer e.g. current-index to always take priority over current-value regardless of write order? Happy to implement whichever you'd rather have.
…stency
A declarative `ComboBox { current-value: "..."; }` replaces the default
`model[current-index]` binding and never fires `changed current-value`, so
the reverse lookup added for host writes never ran for it — the selection
stayed on an unresolved value instead of being looked up or cleared like an
imperative write. Run the same lookup from `init`, once, after model and
current-index have their initial values; factor the shared logic into
`reverse-lookup-current-value()` so both call sites stay in sync.
The lookup now calls `update-current-value()` explicitly instead of relying
solely on the `changed current-index` cascade: a property write made while
the tree is still under construction (i.e. from `init`) isn't guaranteed to
have already propagated through that handler by the time `init` returns.
Also: a regression test proving a fresh write to a duplicate model value
picks the first occurrence (not just the already-covered re-write-same-value
limitation), two tests establishing that current-index and current-value
have no inherent priority over each other (whichever is written last wins),
and documentation of both in combobox.mdx.
changelog: ComboBox: `current-value` set declaratively at instantiation is now resolved against `model` from `init`, consistent with a host write.
The `init =>` handler that reverse-looked-up a declaratively-set `current-value` ran before the literal was guaranteed to have been applied to inlined sub-components in the generated code, and only happened to pass in CI's non-live-preview build; the live-preview build of the same test flaked on Windows. Since this wasn't part of what was requested and can't be reliably verified from here, drop it along with the associated test instances and assertions, and stop documenting the unverified change-handler-ordering guarantee. changelog: ComboBox: `current-value` set declaratively at instantiation is no longer resolved against `model` at construction time (reverts part of the previous commit); it is still resolved on every subsequent host or imperative write.
Summary
Stacked on #12790 and #12791 (
array.find-index/array.index-of) — this branch contains both commits plus one new one on top. Please review commit-by-commit via the "Commits" tab, or wait for the first two to merge and this will show a clean single-commit diff against master.Implements Option A from #11970: a host write to
current-valuenow means "select the row with this value" — the widget finds it in the model viaarray.index-ofand movescurrent-indexto match. A value that isn't in the model resolves to no selection (current-index-1,current-value""), the same as settingcurrent-indexout of range — and, unlike the successful case, logs adebug()message naming the value, so a developer debugging an unexpectedly-cleared selection has something to go on. A successful reverse lookup stays silent.Uses
index-ofrather thanfind-index: ComboBox is the first built-in widget with any dependency on the array-predicates machinery, andindex-ofis the stable half of it (#12791) — so this carries no dependency on the still-experimentalfind-index/any/allgate (#12777), unlike an inlinefind-index((value) => value == current-value)predicate would.This supersedes the non-reverting interim behavior from
7dfcfdf93, which deliberately left an unmatched host write in place (with only adebug()warning) to avoid disturbing existing<=>two-way bindings while the real fix was blocked on iteration support. That blocker is gone for this narrow case:index-ofneeds no general loop, only a single-pass reverse lookup, so we no longer need the non-reverting compromise.<=>bindings that round-trip an arbitrary, not-in-model string throughcurrent-valuewill now see it replaced (with""or the first match) instead of preserved — this is intentional, matching the issue's Option A design, not a regression.RadioGroup is intentionally left untouched: its public API (declared in
builtins.slint) exposescurrent-valueasoutonly and never exposescurrent-indexat all, so the only host-writable selection path ischeckedon an individualRadioButtonchild, which already correctly stays in sync withcurrent-index/current-value(seeRadioButtonImplBase'schanged checkedhandler) — there's no writable-but-unreliable surface left for a reverse lookup to fix.Known limitation, documented in
combobox-base.slintandcombobox.mdx: the reverse lookup runs fromchanged current-value, which only fires on an actual value transition. A write that doesn't change the property — writing the value it already holds, or a value equal to a later duplicate inmodelwhile an earlier duplicate should now be selected per the "first match" contract — is a no-op. Slint has no.slint-level hook for "run on every write regardless of value equality" (the Rust-internal equivalent,BindingCallable::intercept_set, isn't usable from widget source), so this can't be closed without new core machinery; it equally affected thedebug()-only interim mitigation this replaces. Covered by two regression-documenting assertions intests/cases/widgets/combobox.slintso the behavior stays intentional and visible rather than silently drifting.Closes #11970 (for ComboBox — the issue also covered RadioGroup, addressed separately as described above).
Part 3 of 3:
array.find-index, the experimental predicate.array.index-of, the stable value-based sibling.index-of, closing ComboBox and RadioGroup: writes tocurrent-valueare silently overridden #11970.Test plan
SLINT_TEST_FILTER=combobox cargo test --manifest-path tests/Cargo.toml -p test-driver-{interpreter,rust,cpp}— all 5 widget styles pass.