Skip to content

elements: number-stepper kind + sheet detents/sizing props - #72

Merged
RCmerci merged 8 commits into
mainfrom
devin/1790579276-controls
Sep 29, 2026
Merged

RCmerci merged 8 commits into
mainfrom
devin/1790579276-controls

Conversation

@RCmerci

@RCmerci RCmerci commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds two general-purpose capabilities so app code (e.g. logseq_journal's asset-settings extension) can be expressed entirely with standard elements:

number-stepper — new leaf kind for bounded numeric input (distinct from stepper/step, which is the wizard step indicator). Props: value (existing progressValue float), new min/max/step floats, text label, enabled. Emits the existing ValueChanged(node, value) event.

Lui_elements.number_stepper ~text:"Recent journal days"
  ~value:30.0 ~min:0.0 ~max:3660.0 ~step:1.0
  ~on_value_changed:(fun (ValueChanged (_, v)) -> ...)

Sheet presentation props on the existing sheet kind:

  • detents: comma-separated medium, large, or a fractional height in (0,1] ("medium,large", "0.4"); unknown tokens ignored, absent/all-unrecognized falls back to [.large] → .presentationDetents
  • sizing: form | fitted | page → .presentationSizing, applied under #available(iOS 18.0, macOS 15.0, *) (package floor is iOS 17/macOS 14) and ignored on older OSes

Both apply inside LUIModalSurfaceContent, so they compose with the navigation-form style-class path (which produces SwiftUI Form + navigation title + toolbar actions).

Notable changes

  • LUIModalPresentationPolicy gained detents(for:)/sizing(for:) (previously only showsDragIndicator) and is now @MainActor since it reads node props; the previously-unused LUIModalPresentationStyle modifier now takes the node model and applies both modifiers.
  • performValueChange accepts numberStepper and clamps to min...max instead of 0...1 on all backends.
  • Wire normalization fix: Foundation's JSONDecoder decodes integral JSON doubles (60.0) as Int, so LUIWireValue.normalized(for:) now coerces int → double for progressValue, minValue, maxValue, stepValue (alongside the existing grow/sourceX set). Without this, a legal wire value:1.0 was rejected. WinUI's backend got the same coercion (TryGetInt64 has the same habit); Qt/Flutter already treat all JSON numbers as doubles.
  • The mapsProgress test's rejection case changed from value:1 (now legitimately coerced) to value:"x".
  • Validation: number-stepper requires text or label plus a finite value, with min <= max when both present; sizing is restricted to the three wire tokens. Emit-time errors throw rather than producing partial ops.
  • Web/melange renders <input type="number">; Qt/Flutter/WinUI get schema support plus minimal renderers (QML spinner hookup, a Row of -/+ buttons in Flutter, WinUI NumberBox).

Verified: dune build, dune runtest -j 4 (32/32), make test-apple (135/135), Qt cmake build + tst_lui_qml_backend (14/14), flutter analyze clean. WinUI not compiled here (no dotnet on this machine).

Link to Devin session: https://app.devin.ai/sessions/14641ee59e124d8e853db24a05173c8b
Open in Devin Desktop: https://app.devin.ai/desktop/session/14641ee59e124d8e853db24a05173c8b?variant=devin
Requested by: @RCmerci

Adds a general-purpose numeric stepper element (kind number-stepper,
props value/min/max/step/text/enabled, emits ValueChanged) so apps can
express bounded numeric input with a step increment, and two presentation
props on sheet: detents (comma-separated medium|large|fraction) and
sizing (form|fitted|page).

Apple backend: LUINumberStepperView renders a SwiftUI Stepper over a
clamped range; sheets apply .presentationDetents and
.presentationSizing (iOS 18+/macOS 15+ gated) via
LUIModalPresentationPolicy on the shared modal surface, so navigation-form
sheets compose. Integral wire floats (e.g. value:60.0) decode as Int
through Foundation's JSONDecoder, so float-typed props now normalize
int -> double like grow/sourceX already did.

Web/melange renders an <input type=number>; qt, flutter, and winui gain
schema support and minimal renderers; winui coerces integral JSON
numbers for float props the same way.
@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T07:18:03.425634Z e0e0e68 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0e0e68737

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

case NodeKind::RadioGroup: return "LuiRadioGroup.qml";
case NodeKind::Radio: return "LuiRadio.qml";
case NodeKind::Slider: return "LuiSlider.qml";
case NodeKind::NumberStepper: return "LuiNumberStepper.qml";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Ship the Qt number-stepper QML component

When Qt renders a number-stepper, LuiNodeView.qml derives this component filename, but a repo-wide search of platform/qt/qml and the QML_FILES list in platform/qt/lib/CMakeLists.txt finds no LuiNumberStepper.qml. The QML loader therefore cannot instantiate the newly accepted node kind, leaving every Qt number-stepper blank with a component-load error.

Useful? React with 👍 / 👎.

attach_toggle_event renderer node kind dom_node
| Radio -> attach_radio_event renderer node dom_node
| Slider -> attach_slider_event renderer node dom_node
| Slider | NumberStepper -> attach_slider_event renderer node dom_node

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Sanitize web number-stepper values before dispatch

For the new number input, the reused slider listener emits valueAsNumber directly, without the finite-value check or min/max clamping implemented by every native backend. Clearing the field produces NaN, and decrementing a default stepper below zero produces a negative value because the generated input has no default min; an application that stores either event can then fail finite property validation or receive an out-of-range value. Validate and clamp number-stepper events against its retained bounds before emitting them.

Useful? React with 👍 / 👎.

Comment on lines +388 to +390
void SyncNumberStepper(LUINodeState state, LUISyncContext context)
{
if (Control is not NumberBox box) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Populate the WinUI NumberBox header from text

A number-stepper is valid when it has text but no accessibility-label, yet this sync routine only applies its numeric properties and never copies TextValue into the NumberBox.Header or another label. Consequently the common text-only usage renders an unlabeled numeric control on WinUI, unlike the Apple, web, and Flutter implementations.

Useful? React with 👍 / 👎.

Comment on lines +1601 to +1603
Flexible(child: Text(stepperState.properties['text'] as String? ?? '')),
IconButton(
icon: const Icon(Icons.remove),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor the Flutter stepper accessibility label

The validator explicitly allows a number-stepper whose accessible name comes solely from accessibility-label, but this view reads only text. With text omitted, it renders an empty label plus two icon-only buttons, and the surrounding generic wrapper applies only the accessibility identifier, so assistive technology receives no purpose for the decrement and increment controls. Apply the supplied accessibility label and meaningful button semantics here.

Useful? React with 👍 / 👎.

Comment on lines +304 to +305
(if Store.direct_toggle kind || kind = NumberStepper then
Util.child_element dom_node 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove the web stepper label from the same element

This new special case writes aria-label to the number-stepper's child input, but remove_property still treats it as an ordinary node and removes the attribute from the outer label element. If reconciliation drops an optional accessibility label while retaining nonempty text, the old input label remains and continues overriding the visible text for screen readers.

Useful? React with 👍 / 👎.

Comment thread src/lui_elements.ml
Comment on lines +1236 to +1239
Option.iter (Lui_ui.string_property context node Detents) detents;
Option.iter (Lui_ui.string_property_signal context node Detents) detents_signal;
Option.iter (Lui_ui.string_property context node Sizing) sizing;
Option.iter (Lui_ui.string_property_signal context node Sizing) sizing_signal;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Implement sheet presentation props outside Apple

When a standard sheet using either new prop runs on Web, Flutter, WinUI, or Qt, the backends accept the property but never apply its semantics: the Flutter modal route, WinUI LUIModalPresenter, and Qt LuiSheet.qml do not read either property, while Web only stores them in otherwise-unused data attributes. Applications therefore silently receive the default sheet on four supported backends; implement platform equivalents or explicitly scope/reject these props instead of exposing them as accepted no-ops.

Useful? React with 👍 / 👎.

Comment thread src/lui_protocol.ml Outdated
Comment on lines +1094 to +1100
match
( Property_map.find_opt MinValue properties,
Property_map.find_opt MaxValue properties )
with
| Some (FloatValue minimum), Some (FloatValue maximum) ->
minimum <= maximum
| _ -> true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate max against the default minimum

When only max is supplied, this fallback accepts any finite value even though every backend uses an omitted min as 0.0. Thus a valid-looking stepper such as ~max:(-1.0) has an impossible effective range: Apple, Flutter, and Qt collapse it to zero, while WinUI assigns a NumberBox.Maximum below its default minimum and may reject or coerce the control. Compare the optional endpoints after applying their defaults rather than validating only the both-present case.

Useful? React with 👍 / 👎.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

E2E-tested the new number-stepper kind and sheet detents on both real backends — temporary gallery scaffolding (reverted after testing) bound a number-stepper to the gallery's volume model and set detents:"medium,large" on the Share sheet.

Web (melange): renders <input type="number" min="0" max="1" step="0.05"> inside label.lui-number-stepper; typing 0.9 emitted ValueChanged → the label ("Volume: 0.90") and sibling slider updated; arrow keys stepped by 0.05 and clamped at max; enabled=false set disabled on the input.

iOS (LUIComponents.app on simulator): native SwiftUI Stepper renders with the model-bound label; taps step by 0.05, clamp at 1.00 (extra taps inert, + greys out), enabled=false disables it. The sheet presents at the medium detent and drags to large — both detents active. sizing is visually indistinguishable on iPhone so not asserted.

iOS number-stepper clamped at max

More evidence

iOS stepper initial
web stepper updated to 0.9
sheet medium detent
sheet dragged to large

…re detents/sizing on all backends

- add platform/qt/qml/LuiNumberStepper.qml and register it in QML_FILES
- web: clamp number-stepper value to retained min/max and drop NaN before
  dispatching ValueChanged; route aria-label removal to the child input
- apply sheet detents (height fraction) and sizing (fitted width) on the
  web, Qt, Flutter, and WinUI surfaces instead of accepting them as no-ops
- winui: copy text to NumberBox.Header; flutter: wrap the stepper in a
  Semantics label and give the step buttons tooltips
- compare stepper min/max after applying defaults so a lone negative max
  is rejected (ocaml + all backend mirrors)
@RCmerci
RCmerci merged commit cbdac18 into main Sep 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant