diff --git a/docs/spec/forms/forms.md b/docs/spec/forms/forms.md index 4c259186a..aeda45efb 100644 --- a/docs/spec/forms/forms.md +++ b/docs/spec/forms/forms.md @@ -1182,6 +1182,61 @@ which is what `maximum: Infinity` already meant, and matches JSON Schema giving a null numeric keyword no meaning. Without that, a null bound would read as the bound `0` and reject every positive value. +### What `ready` claims + +`DynamicForm.ready` is a claim about the **payload**: `true` only when the body +the form would submit satisfies the schema the form was generated from. It is +deliberately *not* the weaker "every control the renderer drew is filled". Under +that reading the flag would be `true` for a body the action must reject, and the +rejection would surface at the action boundary rather than in the form, where the +user could still correct it. Two obligations stated earlier are instances of the +same rule: a value outside a [closed set](#closed-sets--a-reflected-enum-class) +leaves the form not ready even though the combo box holds it, and a `boolean` +field refuses anything but `true`/`false`. + +So a member this renderer has **no encoding for** keeps the form short of +`ready`. `DynamicForm.qml` calls such a member *unrepresentable*, and recognises +two shapes: + +- an **object-typed** member that no typed control claims — a + [nested aggregate](#nested-aggregates-recursive-cycle-safe), whose one scalar + control collects text where the schema asks for an object; +- an **array whose `items` are objects** (or arrays), which takes the + [`type: "array"` control](#array-fields--type-array) and encodes each entry as + a JSON string. A control *was* drawn and the member is unrepresentable anyway, + which is precisely why "a control was filled" cannot be what `ready` means. + +A `Quantity` and a `Choice` are `"type": "object"` in the schema too and are +**not** unrepresentable: their own controls encode the shape their schema asks +for. The test is whether an encoding exists, not what the JSON type is. + +Two seams carry the reason, because a gate that only says "no" is not +actionable: + +| Read | Where | Meaning | +|---|---|---| +| `fields[i].unrepresentable` | field descriptor | Why no control here can collect what the schema asks for, or `""`. A property of the *schema*, so it is readable before anything is typed — a caller can decline the form up front. | +| `unrepresentableReason` | form | `": "` for the member submission is currently stuck on, or `""`. Written by `revalidate()` in the same pass that writes `ready`, so the two cannot disagree. Empty while the form is ready **and** while it is merely unfilled: a blank required field or an unsatisfied rule is the ordinary submit gate, which the user can act on. | + +The form's status label shows that reason in place of "fill the required (\*) +fields" — advice no input can act on — and, being the label the +[accessibility slice](#renderer-conformance-kit) mirrors into +`Accessible.description`, announces it rather than merely tinting it. + +**An unrepresentable member the payload may legitimately omit does not block +submission.** Optional and left blank, it is simply absent from the body, and +that body is one the schema accepts; `unrepresentable` still names it on the +descriptor, so the member is declined rather than dropped silently. Typing into +its control is what makes the form unready, since that text has no encoding. + +What this does **not** decide is what the renderer should eventually *draw* for +such a member — a sub-form, a refusal with a diagnostic, or the flattening it +does today. The readiness answer is the same under all three, so it does not +wait on that one. `src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml` pins +both directions: the unready cases with their reasons, and a flat form +(integer, string, array-of-string, `Quantity`) that names nothing +unrepresentable and reaches `ready`. + ## Renderer conformance kit A renderer proves it honors the contract above by consuming a **schema @@ -2788,28 +2843,40 @@ Four statements, each asserted by that suite: `address` object with `street`/`city` members yields `field_address` and nothing for `street` or `city`. The cycle is not what stops the renderer — nesting is. -4. **The payload is wrong and the form reports itself ready.** The object-typed - member is submitted as a JSON *string* (`{"id":7,"root":"anything"}`), and a - recursive collection member takes the `type: "array"` control and submits an - array of strings (`{"name":"top","children":["a","b"]}`) where the schema - asks for an array of objects. `ready` is `true` in both cases. +4. **The form does not report itself ready, and says which member stopped it.** + The flattened control collects text, and text is a JSON *string* where the + schema asks for an object; a recursive collection member takes the + `type: "array"` control, which encodes each entry as a string where the + schema asks for an array of objects. Neither member has an encoding, so both + are *unrepresentable* (see [What `ready` claims](#what-ready-claims)): while + the payload would have to carry one, `ready` is `false`, `previewLine` is + empty, `submitIfValid` is never called, and `unrepresentableReason` names the + member. An optional nested member left blank is not an exception to this: the + body omits it, and that is a body the schema accepts. Measured on Qt 6.11.2, `QT_QPA_PLATFORM=offscreen`, against the real `DynamicForm` with a mock controller. The suite was shown to measure the -renderer rather than pass vacuously: filtering object-typed properties out of -`DynamicForm.qml`'s `fields` builder — the "decline" alternative — turns 4 of -its 5 cases red. - -Point 4 is a description of today's behaviour, **not** an endorsement of it: a -`ready` that is `true` for a payload the action must reject is the one part of -this contract that is arguably wrong, and whether the renderer should draw the -sub-form, decline the schema with a diagnostic, or keep flattening it is -undecided. Nothing in this repository has a nested-aggregate member today, so -nothing depends on the answer yet. +renderer rather than pass vacuously, on each half of point 4 separately: of its +7 cases, removing `fieldJsonLiteral`'s unrepresentable check — which restores a +`ready` of `true` for both wrong payloads — turns 4 red on the `ready` +comparison, and removing `revalidate()`'s `unrepresentableReason` assignment +turns the same 4 red on the reason instead. What stays green under both is the +two structural cases — the cycle builds a form, and the member is one control +rather than a sub-form — and the flat form, which is what makes those four +failures a statement about readiness rather than about the suite. + +Point 4 is the readiness contract applied to a nesting the renderer does not +draw, **not** a concession to it. A renderer may legitimately decline to draw a +member it cannot represent; it may not then report that the resulting body is +acceptable. What stays undecided is the other half — whether the renderer should +draw the sub-form, decline the schema with a diagnostic, or keep flattening — +and the readiness answer is the same under all three, so it does not wait on +that. Nothing in this repository has a nested-aggregate member today, so nothing +depends on the rendering answer yet. So: an action with a nested-aggregate member — cyclic or otherwise — is a -document morph generates completely and a form morph draws only down to the -nesting. +document morph generates completely, a form morph draws only down to the +nesting, and a body morph declines to assemble. Computed fields, `formLayout`/`fieldSpans`, and `formRules` remain **top-level only** regardless of nesting depth: a nested aggregate declaring any of those diff --git a/src/qt/forms/qml/DynamicForm.qml b/src/qt/forms/qml/DynamicForm.qml index d6e854c93..71de74d7b 100644 --- a/src/qt/forms/qml/DynamicForm.qml +++ b/src/qt/forms/qml/DynamicForm.qml @@ -17,9 +17,21 @@ // an explicit Submit button (enabled only while ready) // instead -- see "Explicit submit mode" below // +// A member this vocabulary has no control for -- an object-typed member, or a +// collection whose items are objects -- is *unrepresentable*: no typed text +// encodes to the shape the schema asks for. Such a member is named in its +// field descriptor's `unrepresentable` and keeps the form short of `ready` +// (docs/spec/forms/forms.md, "What `ready` claims"). +// // Quantity payloads are assembled as JSON text from the typed digit string, // so they are exact at any magnitude (same contract as the HTML renderer). // +// `ready` is a claim about the *payload*: true only when the assembled body +// satisfies the schema the form was generated from, not merely when every +// control the renderer happened to draw is filled. `unrepresentableReason` +// carries the live reason when a member no input can satisfy is what blocks +// submission. +// // By default, the form calls controller.submitIfValid(...) automatically // the instant every field/rule is satisfied (safe for a read-only query // action). A schema for a side-effectful action should set the top-level @@ -80,6 +92,18 @@ Frame { property int optionsRevision: 0 property bool ready: false + // Why no input can make this form ready, or "" when none applies: the + // first member the renderer cannot represent that submission currently + // waits on (`": "`). Written by revalidate() from the + // same pass that writes `ready`, so the two cannot disagree. + // + // Empty while the form is ready, and also empty while it is merely + // unfilled -- a blank required field or an unsatisfied rule is the + // ordinary submit gate, which the user can act on, and the status label + // below says so. This property exists for the case the user cannot act + // on, which is otherwise indistinguishable from it. + property string unrepresentableReason: "" + // Non-zero while values are being written programmatically rather than // edited by a user -- restoring a control the layout just recreated, or // clearing the form between rows. revalidate() keeps recomputing validity @@ -273,6 +297,45 @@ Frame { return p } + // A resolved property's declared JSON types, as an array however the + // schema spells it: one string, a list (`["integer","null"]`), or no + // `type` key at all, which is no declaration rather than a type. + function jsonTypes(p) { + return Array.isArray(p.type) ? p.type : (p.type === undefined ? [] : [p.type]) + } + + // Why this renderer cannot represent `p`, or "" when it can. + // + // The form draws flat fields. A member whose value is an *object* reaches + // one scalar control, and a collection whose items are objects reaches the + // comma-separated array control, so whatever a user types encodes as a + // JSON string (or an array of strings) where the schema asks for an object + // or an array of them. No input closes that gap: the member is + // unrepresentable, not merely unfilled. + // + // Naming it is what keeps `ready` a claim about the payload rather than + // about which controls happen to be filled. A form that reported ready + // here would assemble a body the action must reject, and the rejection + // would surface at the action boundary instead of in the form, where the + // user could see it. Which of drawing a sub-form, declining the schema or + // going on flattening it is right is a separate question; the readiness + // answer is the same under all three. + // + // `typed` says one of the kind flags already claims the property for a + // control that encodes a shape of its own. That is what keeps a Quantity + // and a Choice -- both `"type": "object"` in the schema, both with an + // encoder that produces the right shape -- out of this. + function unrepresentableMemberReason(p, types, typed) { + if (!typed && types.indexOf("object") !== -1) + return "a nested object member, which this renderer does not draw" + if (types.indexOf("array") !== -1) { + const itemTypes = jsonTypes(resolveProp(p.items)) + if (itemTypes.indexOf("object") !== -1 || itemTypes.indexOf("array") !== -1) + return "a collection of nested objects, which this renderer does not draw" + } + return "" + } + // Value/label pairs for a property that states a **closed set of values** // outright, or [] for one that does not. Two spellings, both handled: // @@ -380,7 +443,7 @@ Frame { .map(function (name) { const raw = props[name] const p = resolveProp(raw) - const types = Array.isArray(p.type) ? p.type : (p.type === undefined ? [] : [p.type]) + const types = jsonTypes(p) const dp = opt(raw["x-decimalPlaces"], p["x-decimalPlaces"]) const optionsAction = opt(raw["x-optionsAction"], p["x-optionsAction"]) // A closed set stated by the schema itself. Read @@ -417,6 +480,16 @@ Frame { const literalTitle = opt(raw["title"], opt(p.title, name)) const literalHelp = opt(p.description, "") const literalPlaceholder = opt(raw["x-placeholder"], opt(p["x-placeholder"], "")) + // The kind flags below that hand the property to a control + // with an encoder of its own, restated here because + // unrepresentableMemberReason has to know whether anything + // claimed the property before it judges its declared type. + // `isArray` is deliberately absent: the array control claims + // the property but encodes its items as strings, so an array + // of objects is unrepresentable even though a control drew it. + const typedControl = dp !== undefined || optionsAction !== undefined + || enumOptionRows.length > 0 || p.format === "date-time" + || types.indexOf("integer") !== -1 || types.indexOf("boolean") !== -1 return { name: name, title: literalTitle, @@ -466,6 +539,12 @@ Frame { // but each entry is encoded as a JSON string, same as an // array of strings, rather than silently misencoding. isArray: types.indexOf("array") !== -1, + // Why no control here can collect what the schema asks + // for, or "" for every member this renderer represents -- + // which is every member of a flat action. A non-empty + // reason makes the member unencodable, so the form reports + // ready only for a payload that legitimately omits it. + unrepresentable: unrepresentableMemberReason(p, types, typedControl), required: required.indexOf(name) !== -1, // `resolveRef` merges the property node *over* the `$def` // it points at, so these three read a per-field bound @@ -1162,6 +1241,12 @@ Frame { const text = (opt(fieldValues[f.name], "")).trim() if (text === "") return null + // A member no control can collect has no literal, whatever was typed: + // every encoding below would produce a value of the wrong JSON type, + // and a wrong literal is worse than none, because it is the one that + // makes the form report ready. + if (f.unrepresentable !== "") + return null if (f.isArray) { return arrayJsonLiteral(text) } @@ -1271,13 +1356,24 @@ Frame { // int64-sized integers stay exact. const parts = [] let ok = true + let blocker = "" for (let i = 0; i < fields.length; ++i) { const f = fields[i] const text = (opt(fieldValues[f.name], "")).trim() const literal = fieldJsonLiteral(f) if (literal === null) { - if (text !== "" || f.required || isDynamicallyRequired(f.name)) + if (text !== "" || f.required || isDynamicallyRequired(f.name)) { ok = false + // An unrepresentable member blocks submission only when + // the payload would have to carry it -- the schema + // requires it, or the user typed into it anyway. One left + // blank and optional is legitimately omitted, and a + // payload the schema accepts is not something to report. + // First one wins: the caller wants a reason, and the whole + // set is on the field descriptors. + if (blocker === "" && f.unrepresentable !== "") + blocker = f.name + ": " + f.unrepresentable + } continue } parts.push(JSON.stringify(f.name) + ":" + literal) @@ -1295,6 +1391,7 @@ Frame { } } ready = ok + unrepresentableReason = ok ? "" : blocker previewLine = ok ? "{" + parts.join(",") + "}" : "" rulesRevision++ // In explicit-submit mode the renderer never fires on its own -- @@ -2018,8 +2115,16 @@ Frame { Label { Layout.topMargin: 8 text: { - if (!form.ready) + if (!form.ready) { + // Filling fields in is the usual remedy, but it is not the + // remedy for a member this renderer cannot represent, and + // telling the user to fill something that would not help + // is the worse half of the same lie a `ready` of true + // would be. Name the member instead. + if (form.unrepresentableReason !== "") + return "cannot be submitted -- " + form.unrepresentableReason return "fill the required (*) fields" + } return form.explicitSubmitMode ? "✓ ready -- press Submit" : "✓ executes automatically as you type" } opacity: 0.6 diff --git a/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml b/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml index 457c08730..a504810f2 100644 --- a/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml +++ b/src/qt/forms/tests/tst_DynamicFormNestedAggregate.qml @@ -19,13 +19,19 @@ // the cycle is not what stops the renderer, nesting is. The acyclic case // is in here as the control -- without it, (2) reads as a cycle-specific // defect rather than the general limit it is. -// 4. The payload is wrong and the form does not say so: an object-typed -// member submits as a JSON *string*, an array-of-objects member as an -// array of strings, and `ready` is true for both. +// 4. The form does not claim a payload it cannot assemble. Such a member is +// unrepresentable -- the control that was drawn collects text, and text +// encodes as a JSON *string* where the schema asks for an object -- so +// the form stays short of `ready`, submits nothing, and names the member +// in `unrepresentableReason`. // -// (4) is the part worth arguing about, and this file is not the place to -// argue it. This suite states the current behaviour so that a change to it is -// visible as a failing test rather than as a silent difference. +// (4) is the readiness claim, and it is about the payload rather than about +// which controls happen to be filled: whether the renderer should eventually +// draw the sub-form, decline the schema or go on flattening it, the answer to +// "does the body satisfy the schema" is the same. The last two cases here are +// the boundary in the other direction -- a member the payload legitimately +// omits, and a flat form -- because the way to break this is to make an +// ordinary form unready. import QtQuick import QtTest @@ -124,23 +130,32 @@ TestCase { compare(findChild(form, "field_root_children"), null) } - // (4) The wrong payload, stated exactly. `root` must be an object; what - // the form offers is a string, and it reports itself ready to send it. - function test_the_cyclic_member_submits_as_a_json_string_and_the_form_says_ready() { + // (4) `root` must be an object; the only thing the drawn control collects + // is text. So the form never reports ready, never assembles a body, and + // says which member it is stuck on. Filling the control is not a remedy, + // which is exactly why the reason has to be reachable: "fill the required + // fields" is advice no input can act on here. + function test_the_cyclic_member_leaves_the_form_unready_and_says_why() { var form = createTemporaryObject(cyclicComponent, testCase) verify(form !== null) + var submitsBefore = mockController.submitCount compare(form.ready, false) // both members are required, both blank + // The reason is a property of the schema, so it is readable before + // anything is typed -- a caller can refuse the form up front. + compare(form.fields[0].unrepresentable, "") + verify(form.fields[1].unrepresentable !== "") + verify(form.unrepresentableReason.indexOf("root: ") === 0) + findChild(form, "field_id").text = "7" compare(form.ready, false) // `root` still blank findChild(form, "field_root").text = "anything" - compare(form.ready, true) - - var parsed = JSON.parse(form.previewLine) - compare(parsed.id, 7) - compare(typeof parsed.root, "string") - compare(parsed.root, "anything") + compare(form.ready, false) + verify(form.unrepresentableReason.indexOf("root: ") === 0) + // Nothing to preview, because there is no body to send. + compare(form.previewLine, "") + compare(mockController.submitCount, submitsBefore) } // --- The acyclic control --------------------------------------------- @@ -177,10 +192,12 @@ TestCase { } // (3) Same outcome with no cycle anywhere: one control for the whole - // sub-object, none for its members, and a string payload. + // sub-object, none for its members -- and the same refusal to call the + // result ready. function test_an_acyclic_nested_aggregate_is_flattened_the_same_way() { var form = createTemporaryObject(acyclicComponent, testCase) verify(form !== null) + var submitsBefore = mockController.submitCount compare(form.fields.length, 2) verify(findChild(form, "field_address") !== null) compare(findChild(form, "field_street"), null) @@ -188,11 +205,10 @@ TestCase { findChild(form, "field_id").text = "7" findChild(form, "field_address").text = "somewhere" - compare(form.ready, true) - - var parsed = JSON.parse(form.previewLine) - compare(typeof parsed.address, "string") - compare(parsed.address, "somewhere") + compare(form.ready, false) + verify(form.unrepresentableReason.indexOf("address: ") === 0) + compare(form.previewLine, "") + compare(mockController.submitCount, submitsBefore) } // --- A recursive collection at the root ------------------------------- @@ -200,7 +216,9 @@ TestCase { // `TreeNode` used as the action type itself: `children` is an array whose // items `$ref` back to `TreeNode`. The array control is chosen by // `type: "array"` alone and encodes every comma-separated entry as a JSON - // string, so an array of objects arrives as an array of strings. + // string, so it cannot produce the array of *objects* the schema asks for. + // A control was drawn, and the member is unrepresentable anyway -- which is + // why "a control the renderer drew is filled" is not what `ready` claims. property var rootIsCyclicSchema: ({ type: "object", properties: { @@ -240,25 +258,124 @@ TestCase { } } - function test_a_recursive_collection_gets_the_array_control_and_string_items() { + function test_a_recursive_collection_gets_the_array_control_but_no_payload() { var form = createTemporaryObject(rootCyclicComponent, testCase) verify(form !== null) + var submitsBefore = mockController.submitCount compare(form.fields.length, 2) // Declaration order, not JSON key order: `name` carries x-order 0. compare(form.fields[0].name, "name") compare(form.fields[1].name, "children") + // The array control is still the one drawn: what changed is the claim + // about the payload, not the rendering. compare(form.fields[1].isArray, true) + compare(form.fields[0].unrepresentable, "") + verify(form.fields[1].unrepresentable !== "") findChild(form, "field_name").text = "top" findChild(form, "field_children").text = "a, b" + compare(form.ready, false) + verify(form.unrepresentableReason.indexOf("children: ") === 0) + compare(form.previewLine, "") + compare(mockController.submitCount, submitsBefore) + } + + // --- The boundary: what is still ready -------------------------------- + // + // An *optional* nested member is one the payload may legitimately leave + // out, so leaving it out is a body the schema accepts and the form says so. + // That the member is undrawable is still reachable on its descriptor -- + // it is not dropped silently, it is declined. Typing into its control + // anyway is the case the gate must catch: there is no encoding for that + // text, so the form goes unready rather than quoting it into the body. + property var optionalNestedSchema: ({ + type: "object", + properties: { + id: { type: "integer", "x-order": 0, title: "Id" }, + note: { type: "string", "x-order": 1, title: "Note" }, + address: { + type: "object", + properties: { street: { type: "string", "x-order": 0, title: "Street" } }, + required: ["street"], + "x-order": 2, + title: "Address" + } + }, + required: ["id"] + }) + + Component { + id: optionalNestedComponent + DynamicForm { + actionType: "OptionalNestedAction" + schema: testCase.optionalNestedSchema + controller: mockController + } + } + + function test_an_optional_unrepresentable_member_may_be_omitted() { + var form = createTemporaryObject(optionalNestedComponent, testCase) + verify(form !== null) + verify(form.fields[2].unrepresentable !== "") + + findChild(form, "field_id").text = "7" + compare(form.ready, true) + compare(form.unrepresentableReason, "") + var parsed = JSON.parse(form.previewLine) + compare(parsed.id, 7) + compare(parsed.address, undefined) + + // ... and the moment something is typed into it, there is no literal + // for it and the form stops claiming the body is acceptable. + findChild(form, "field_address").text = "somewhere" + compare(form.ready, false) + verify(form.unrepresentableReason.indexOf("address: ") === 0) + } + + // The regression this contract most easily causes: an ordinary flat form + // judged unready because a member's schema was misread as a nested + // aggregate. A Quantity is `"type": "object"` in the schema and must not + // be caught by it, and neither must an array of strings. + property var flatSchema: ({ + type: "object", + properties: { + id: { type: "integer", "x-order": 0, title: "Id" }, + note: { type: "string", "x-order": 1, title: "Note" }, + tags: { type: "array", items: { type: "string" }, "x-order": 2, title: "Tags" }, + weight: { type: "object", "x-order": 3, "x-decimalPlaces": 2, title: "Weight", + ExtUnits: { unitAscii: "kg", unitUnicode: "kg" } } + }, + required: ["id", "note", "tags", "weight"] + }) + + Component { + id: flatComponent + DynamicForm { + actionType: "FlatAction" + schema: testCase.flatSchema + controller: mockController + } + } + + function test_a_flat_form_names_nothing_unrepresentable_and_reaches_ready() { + var form = createTemporaryObject(flatComponent, testCase) + verify(form !== null) + compare(form.fields.length, 4) + for (var i = 0; i < form.fields.length; ++i) + compare(form.fields[i].unrepresentable, "") + + findChild(form, "field_id").text = "7" + findChild(form, "field_note").text = "hello" + findChild(form, "field_tags").text = "a, b" + findChild(form, "field_weight").text = "1.25" compare(form.ready, true) + compare(form.unrepresentableReason, "") var parsed = JSON.parse(form.previewLine) - compare(parsed.name, "top") - verify(Array.isArray(parsed.children)) - compare(parsed.children.length, 2) - compare(typeof parsed.children[0], "string") - compare(parsed.children[0], "a") - compare(parsed.children[1], "b") + compare(parsed.id, 7) + compare(parsed.note, "hello") + verify(Array.isArray(parsed.tags)) + compare(parsed.tags[0], "a") + compare(parsed.weight.num, 125) } }