yangzhang75 commented on code in PR #8839:
URL: https://github.com/apache/texera/pull/8839#discussion_r4199145634
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -887,40 +894,67 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
// The schema's own title ("Attributes", "Limit", "File") -- the reader's
title when unnamed.
// Falls back to this, not the lower-camel key ("fileName"), which would
read inconsistently.
const schemaLabel = (source.props?.label as string) || binding.propertyKey;
- field.key = binding.id;
+ // The field keeps the property's own key, as on the operator property
panel: a widget that
+ // reaches a sibling or itself by key (the HuggingFace picker resets
`modelId` and its
+ // task-scoped siblings on a task change) must find the same controls and
model keys here.
field.props = {
...(field.props ?? {}),
label: binding.displayName || schemaLabel,
};
const form = new FormGroup({});
- // Seed the model with the operator's other properties as read-only
context, not just this
- // input's own value: some custom widgets read a sibling to decide what to
show -- the
- // HuggingFace model picker reads `task` to load the right models and
label the field. Only
- // this binding's value is ever written back (see below); the context is
never persisted, and
- // it is cloned so a widget that mutates it cannot reach through to the
real operator.
+ // The card's model is the operator's properties, as the panel's is: the
exposed value, and
+ // every other property as context a widget may read or change -- the
HuggingFace model picker
+ // reads `task` to load the right models and, when the task is changed,
resets the task-scoped
+ // siblings as it does on the panel. The copy is cloned so nothing reaches
through to the real
+ // operator; what changes is written back below, as one edit, the way the
panel writes.
const model: Record<string, unknown> = {
...cloneDeep(operator?.operatorProperties ?? {}),
- [binding.id]: cloneDeep(resolved.value),
+ [binding.propertyKey]: cloneDeep(resolved.value),
};
+ // The other properties ride along as plain controls (formly builds only
the exposed field), so
+ // a widget finds its siblings on the card's form exactly as on the
panel's -- the HuggingFace
+ // picker writes `task` through its control, which is also what announces
the change -- and a
+ // control-level write lands in the model, which is what is written back.
+ const schemaKeys = Object.keys((schema as { properties?: Record<string,
unknown> }).properties ?? {});
+ const siblingKeys = [...new Set([...schemaKeys,
...Object.keys(model)])].filter(key => key !== binding.propertyKey);
+ for (const key of siblingKeys) {
+ const control = new FormControl(model[key] ?? null);
+ control.valueChanges.pipe(takeUntil(this.formsRebuilt),
untilDestroyed(this)).subscribe(value => {
+ model[key] = value;
+ });
+ form.addControl(key, control);
+ }
if (this.canEdit) {
form.valueChanges
.pipe(debounceTime(FORM_DEBOUNCE_TIME_MS),
takeUntil(this.formsRebuilt), untilDestroyed(this))
.subscribe(() => {
- // Formly emits the schema's empty default while building the
control, before any edit;
- // writing that back silently wiped the operator's real value (both
views edit one
- // workflow). So only accept a dirtied form, or a value that differs
from the operator's
- // without being emptier (some controls set values without marking
dirty).
- const next = model[binding.id];
- const current =
this.formBindingService.readValue(binding.operatorID, binding.propertyKey);
+ // Whatever differs from the operator is written back: the bound
property, and the
+ // siblings a widget changed with it. Formly emits the schema's
empty default for the bound
+ // control while building it, before any edit; writing that back
silently wiped the
+ // operator's real value (both views edit one workflow). So the
bound property is taken
+ // only from a dirtied form, or when it differs without being
emptier (some controls set
+ // values without marking dirty).
+ const current =
(this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)
+ ?.operatorProperties ?? {}) as Record<string, unknown>;
const isEmpty = (v: unknown) => v === undefined || v === null || v
=== "";
- const unchanged = JSON.stringify(next ?? null) ===
JSON.stringify(current ?? null);
- if (unchanged || (!form.dirty && isEmpty(next) &&
!isEmpty(current))) {
+ // An empty string and an absent property mean the same to an
operator, and formly fills a
+ // blank for a string the operator never had: neither is a change of
the other.
+ const asJson = (value: unknown) => JSON.stringify(value === "" ?
null : value ?? null);
+ const differs = (key: string) => asJson(model[key]) !==
asJson(current[key]);
+ const bound = binding.propertyKey;
+ const blankBeforeEdit = !form.dirty && isEmpty(model[bound]) &&
!isEmpty(current[bound]);
+ const patch = Object.fromEntries(
+ Object.keys(model)
+ .filter(key => differs(key) && !(key === bound &&
blankBeforeEdit))
+ .map(key => [key, model[key]])
Review Comment:
Confirmed and fixed in 29cec391. The card now diffs against its own baseline
-- a clone of what it was built from, advanced by each write -- instead of
against the operator as it is now, so the patch is only what this card changed.
A sibling the canvas, a co-editor or another card on the same operator changed
in the meantime is no longer in it, and the blank-build-default guard moved to
the same baseline. Two tests cover it (only the changed key is sent; a second
edit sends only the second change), both deletion-checked.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -724,6 +724,13 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
if (!active || !this.host.nativeElement.contains(active)) {
return false;
}
+ // A dropdown (nz-select) keeps the focus on its search box after a pick.
That is typing only
+ // while the list is open and a filter can be typed; a closed dropdown
that merely holds the
+ // focus held every rebuild, so a change made elsewhere (the canvas, a
co-editor) never showed.
+ const dropdown = active.closest(".ant-select");
+ if (dropdown) {
+ return dropdown.classList.contains("ant-select-open");
Review Comment:
Right, fixed in 29cec391. The held rebuild is now flushed by a click or a
key anywhere on the page as well as by focusout, through the same deferred
re-check, so closing the dropdown releases it even though the focus stays in
its search box. A filter still being typed is unaffected, since the re-check
runs after the tick and the predicate still sees an open list. Covered by a
host-binding test in the rendered spec plus the negative case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
