Skip to content

forms: emit x-submitMode from the action type - #258

Open
Yaraslaut wants to merge 5 commits into
masterfrom
feat/208-submitmode-emitter
Open

forms: emit x-submitMode from the action type#258
Yaraslaut wants to merge 5 commits into
masterfrom
feat/208-submitmode-emitter

Conversation

@Yaraslaut

Copy link
Copy Markdown
Member

Closes #208.

The gap

x-submitMode was documented in the spec and implemented in DynamicForm.qml,
but nothing in C++ emitted it — so no schema generated from a compiled
action type could carry it. Ten call sites across polls, pastebin and
bookmarks hand-write the same workaround instead (DynamicForm { controller: null } plus an external Button), with comments naming each other as the source
of the copy.

The spec sentence declining to emit it refuted itself: it said an author sets the
key by hand "the same way x-layout/x-widget overrides are authored today"
but both of those are emitted from C++ declarations, and x-layout is a
top-level key emitted from a static constexpr formLayout, structurally exactly
what this needed. The mechanism already existed, twice.

What changed

struct CreatePaste {
    std::string title;
    std::string body;
    static constexpr bool explicitSubmit = true;
};

schemaJson<A>() now emits the top-level "x-submitMode": "explicit", following
formLayout's pattern.

The design question, and how I resolved it

#208 leaves opt-in trait vs. derived-by-default open. I took opt-in, and
this is the one judgement call in the change, so it is worth stating plainly:

  • Deriving from a "does this action mutate" predicate would need a predicate
    forms.hpp does not have, and would flip the rendering of every existing
    generated form at once — a shipped-behavior change, not an emitter addition.
  • Opt-in matches how formLayout, fieldSpans and formRules already work,
    keeps the decision with the author who knows whether the action has effects,
    and changes no existing schema: an action declaring nothing, or declaring
    false, emits no key.

Derived-by-default remains available later; this makes the feature reachable
without committing to it. If you'd rather have the derived version, this is cheap
to revisit.

Deliberately out of scope

Migrating the ten existing call sites off the workaround. That alters three
shipped apps' behaviour and belongs on its own change, not bundled with the
emitter.

Verification

The issue flags invariant 7 explicitly, and it is the crux here: today's only
coverage is a hand-written schema literal (tst_DynamicFormSubmitMode.qml),
which keeps passing with the emitter removed entirely. So the new fixtures
are driven through schemaJson<A>().

Mutation-checked — disabling the emitter fails 2 of the 4 new cases,
while the two "omits the key" cases correctly still pass:

CHECK( schema.find(R"("x-submitMode":"explicit")") != std::string::npos )
test cases: 4 | 2 passed | 2 failed

One case parses the schema rather than substring-matching, so a key that landed
inside properties instead of at the root fails there.

check result
morph_tests 20630 assertions in 1158 cases, all passed
new [submitmode] cases 8 assertions in 4 cases
compiler warnings 0 (initial draft used the deprecated glz::json_t; now glz::generic)
Doxygen (WARN_AS_ERROR) clean
scripts/check_spec_citations.sh pass

Note on the test fixtures

They are file-scope rather than anonymous-namespaced, deliberately: glaze's
reflection takes the address of an extern const T, which a type with no linkage
cannot have — an anonymous-namespace action fails to compile. The SM prefix
keeps them unique for the file-scope-collision CI check.

x-submitMode was documented in the spec and implemented in DynamicForm.qml, but
nothing in C++ emitted it, so no schema generated from a compiled action type
could carry it. Ten call sites across polls, pastebin and bookmarks hand-write
the same workaround instead -- `DynamicForm { controller: null }` plus an
external Button -- with comments naming each other as the source of the copy.

The spec sentence declining to emit it refuted itself: it said an author sets
the key by hand "the same way `x-layout`/`x-widget` overrides are authored
today", but both of those *are* emitted from C++ declarations, and `x-layout` is
a top-level key emitted from a `static constexpr formLayout` -- structurally
what this needed. The emitter mechanism already existed, twice.

An action now declares `static constexpr bool explicitSubmit = true` and
`schemaJson<A>()` emits the top-level `"x-submitMode": "explicit"`, following
formLayout's pattern.

Opt-in, not derived. #208 leaves that open; deriving from a "does this action
mutate" predicate would need a predicate `forms.hpp` does not have and would
flip the rendering of every existing generated form at once -- a shipped-behavior
change rather than an emitter addition. Opt-in matches how formLayout,
fieldSpans and formRules already work, keeps the decision with the author who
knows whether the action has effects, and changes no existing schema: an action
declaring nothing (or `false`) emits no key.

Migrating the ten existing call sites off the workaround is deliberately not in
this change -- it alters three shipped apps' behaviour and belongs on its own.

Tests are driven through schemaJson<A>() rather than a hand-written schema
literal, which is the point: tst_DynamicFormSubmitMode.qml already covers the
renderer against a literal and keeps passing with the emitter removed entirely.
Mutation-checked -- disabling the emitter fails 2 of the 4 new cases, while the
two "omits the key" cases correctly still pass. One case parses the schema
rather than substring-matching it, so a key that landed inside `properties`
instead of at the root would fail.

The three fixture actions are file-scope rather than anonymous-namespaced,
deliberately: glaze's reflection takes the address of an `extern const T`, which
a type with no linkage cannot have.

Closes #208
CI's clang-tidy-diff job runs with -warnings-as-errors=* over changed lines and
flagged four things, reproduced locally with the same script:

- mergeSchemaExtras was pushed over readability-function-cognitive-complexity
  (48 vs a threshold of 25). The emission moves into its own
  annotateSubmitMode<A>() free function, so the big function's complexity is
  what it was on master and the new code is trivial on its own.
- misc-use-internal-linkage wanted the three fixtures in an anonymous
  namespace, which is exactly what glaze's reflection cannot accept -- it takes
  the address of an `extern const T`. NOLINT with that reason.
- readability-container-contains: use C++23 std::string::contains rather than
  find() != npos.
- cppcoreguidelines-pro-bounds-avoid-unchecked-container-access on the glaze
  DOM's operator[], in both the new helper and the test -- the same NOLINT the
  surrounding header already uses for the same reason.

No behaviour change: 8 assertions / 4 cases unchanged, full suite green.
The WASM ladder build compiles with -Weverything -Werror, and clang's
-Wdocumentation does not consider a concept a template declaration:

  forms.hpp:1541:6: error: '@tparam' command used in a comment that is not
  attached to a template declaration [-Werror,-Wdocumentation]

HasFormRules, directly above, omits it for the same reason. Doxygen accepts
either, which is why the local docs build was clean and this only surfaced in
the WASM job.
The previous commit's explanatory comment named the doc command it was
explaining the absence of. clang parses that command even inside backticks, so
the comment reintroduced the exact error it documented.
Yaraslaut added a commit that referenced this pull request Aug 24, 2026
CI runs clang-tidy-diff with -warnings-as-errors=* over changed lines, which
#258 tripped; reproducing it locally against the same clang 22 the workflow
pins turned up the same class of findings here, before CI got to them.

Fixed rather than suppressed where the check was right:

- modernize-use-integer-sign-comparison: the two bounds arrive in different
  signednesses, so the casts were exactly the sign-mismatch the comparison
  exists to get right. Now std::cmp_greater/std::cmp_less, with a signed twin
  of the 2^53 limit for the negative side.
- The fixture models gain real state, so execute() genuinely needs `this` and
  is neither static-able nor const-able -- rather than NOLINTing
  readability-convert-member-functions-to-static.
- readability-container-contains: C++23 std::string::contains.

Suppressed only where the pattern is forced:

- misc-use-internal-linkage on the fixtures -- glaze's reflection takes the
  address of an `extern const T`, so the anonymous namespace the check asks for
  is exactly what they cannot have. Same suppression tests/
  test_shared_instances.cpp already uses.
- cert-err58-cpp / bugprone-throwing-static-initialization /
  misc-const-correctness across the BRIDGE_REGISTER_* expansions: every test
  that registers a model has this shape.
- cppcoreguidelines-pro-bounds-avoid-unchecked-container-access on the glaze
  DOM, the same NOLINT the surrounding header already carries.
- misc-no-recursion on annotateExactNumericBounds -- walking a JSON tree is
  inherently recursive; that is the function.

No behaviour change: 11 assertions / 4 cases unchanged, framework and QML
suites green.
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The original run was cancelled by the supersede-obsolete-runs concurrency rule
(#257) shortly after it merged, and re-running the cancelled workflows produced
attempts that were themselves cancelled within minutes. An empty commit gives
the PR a fresh head so its checks run from a clean slate.

No content change.
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.

x-submitMode has no C++ emitter, so ten call sites hand-write the workaround and the shipped feature is unreachable

1 participant