yangzhang75 commented on code in PR #8437:
URL: https://github.com/apache/texera/pull/8437#discussion_r3943267758


##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -175,6 +240,149 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     this.workflowActionService.disableWorkflowModification();
   }
 
+  // 
---------------------------------------------------------------------------
+  // Inputs: the exposed properties, rendered as their operators' own fields
+  // 
---------------------------------------------------------------------------
+
+  /** Whether the cursor is currently inside one of this page's inputs. */
+  private isTypingInTheForm(): boolean {
+    const active = document.activeElement as HTMLElement | null;
+    if (!active || !this.host.nativeElement.contains(active)) {
+      return false;
+    }
+    return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || 
active.isContentEditable;
+  }
+
+  private readConfig(): void {
+    this.parameters = this.formBindingService.resolveFields();
+    this.buildForm();
+  }
+
+  /**
+   * Build the form from the operators' JSON schemas (FormlyJsonschema), 
keeping the one field per
+   * exposed property. Each input gets its own form keyed by binding id.
+   */
+  private buildForm(): void {
+    this.formsRebuilt.next();
+    this.rendered = this.visibleFields
+      .map(field => this.renderField(field))
+      .filter((r): r is RenderedField => r !== undefined);
+  }
+
+  private renderField(resolved: ResolvedField): RenderedField | undefined {
+    const { binding } = resolved;
+    const schema = this.operatorSchemaFor(binding.operatorID);
+    if (!schema) {
+      return undefined;
+    }
+    const operatorType = 
this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)?.operatorType;
+    const full = this.formlyJsonschema.toFieldConfig(cloneDeep(schema) as 
never, {
+      map: (mapped, source) => {
+        // Render the exact custom widget the operator property panel would 
(file/model/dataset
+        // pickers, image/audio uploaders, ...), shared via 
customFormlyFieldType so an exposed
+        // property shows its real control instead of degrading to a text box.
+        const customType = customFormlyFieldType({
+          key: mapped.key,
+          operatorType,
+          description: (source as { description?: string })?.description,
+          currentType: mapped.type,
+        });
+        // Canvas-only widgets (code editor, drag-reorder) do not work here; 
an older workflow may
+        // already carry one, so leave it to formly's default editable control 
rather than a widget
+        // that cannot function on a form.
+        if (customType && !CANVAS_ONLY_FORMLY_TYPES.has(customType)) {
+          mapped.type = customType;
+        }
+        return mapped;
+      },
+    });
+    const source = (full.fieldGroup ?? []).find(child => child.key === 
binding.propertyKey);
+    if (!source) {
+      return undefined;
+    }
+
+    const field = cloneDeep(source);
+    // 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;
+    field.props = {
+      ...(field.props ?? {}),
+      label: binding.displayName || schemaLabel,
+      // The schema's own description is the operator author's note to whoever 
wired the operator
+      // up; it is not guidance to a form reader, and formly shows it once per 
scalar field. Drop it
+      // here so it does not appear unbidden under the input.
+      description: "",
+    };
+
+    const form = new FormGroup({});
+    const model: Record<string, unknown> = { [binding.id]: 
cloneDeep(resolved.value) };

Review Comment:
   Fixed. The field's model is now seeded with the operator's other properties 
(`task` included) as read-only context, so the picker loads the right models 
and labels the field. Only the bound property is written back, and the context 
is cloned so the widget cannot reach the real operator. The remaining piece -- 
a form-view mode that also hides/locks the task selector so a task change 
cannot desync -- touches the shared widget and is tracked in #8439.



##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -121,6 +162,28 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     }
     this.wid = wid;
     this.load(wid);
+
+    // Attribute boxes become dropdowns only after compilation writes the 
column enums into each
+    // operator's dynamic schema -- which lands after these cards were built. 
Rebuild on the
+    // compilation-state stream, a ReplaySubject(1) so a late subscriber (this 
page reloads fresh
+    // on every Canvas<->Form switch) gets the current state at once. Skip it 
while someone is
+    // typing, so a rebuild does not throw away a half-entered value under the 
cursor.
+    this.workflowCompilingService
+      .getCompilationStateInfoChangedStream()
+      .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this))
+      .subscribe(() => {
+        if (this.isTypingInTheForm()) {
+          return;

Review Comment:
   Acknowledged; left as an edge for now (the next compilation refreshes it). 
The pending-rebuild-on-blur fix is tracked in #8439.



##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -121,6 +162,28 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     }
     this.wid = wid;
     this.load(wid);
+
+    // Attribute boxes become dropdowns only after compilation writes the 
column enums into each
+    // operator's dynamic schema -- which lands after these cards were built. 
Rebuild on the
+    // compilation-state stream, a ReplaySubject(1) so a late subscriber (this 
page reloads fresh
+    // on every Canvas<->Form switch) gets the current state at once. Skip it 
while someone is
+    // typing, so a rebuild does not throw away a half-entered value under the 
cursor.
+    this.workflowCompilingService
+      .getCompilationStateInfoChangedStream()
+      .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this))
+      .subscribe(() => {
+        if (this.isTypingInTheForm()) {
+          return;
+        }
+        this.readConfig();
+      });
+
+    // Exposing or un-exposing a property (from the panel, or a co-editor) 
changes the definition;
+    // the inputs above have to follow at once, which is the whole point of 
editing side by side.
+    
this.workflowActionService.formBindingChanged$.pipe(untilDestroyed(this)).subscribe(()
 => {

Review Comment:
   Intended: this stream only fires for a co-editor once #8351 moves 
formBinding into the shared model, so the subscription is here to work the 
moment that lands. The comment now says it fires for this client today and for 
co-editors after #8351, and I added the `isTypingInTheForm()` skip so a remote 
change cannot clobber a half-entered value.



##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -175,6 +240,149 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     this.workflowActionService.disableWorkflowModification();
   }
 
+  // 
---------------------------------------------------------------------------
+  // Inputs: the exposed properties, rendered as their operators' own fields
+  // 
---------------------------------------------------------------------------
+
+  /** Whether the cursor is currently inside one of this page's inputs. */
+  private isTypingInTheForm(): boolean {
+    const active = document.activeElement as HTMLElement | null;
+    if (!active || !this.host.nativeElement.contains(active)) {
+      return false;
+    }
+    return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || 
active.isContentEditable;
+  }
+
+  private readConfig(): void {
+    this.parameters = this.formBindingService.resolveFields();
+    this.buildForm();
+  }
+
+  /**
+   * Build the form from the operators' JSON schemas (FormlyJsonschema), 
keeping the one field per
+   * exposed property. Each input gets its own form keyed by binding id.
+   */
+  private buildForm(): void {
+    this.formsRebuilt.next();
+    this.rendered = this.visibleFields
+      .map(field => this.renderField(field))
+      .filter((r): r is RenderedField => r !== undefined);
+  }
+
+  private renderField(resolved: ResolvedField): RenderedField | undefined {
+    const { binding } = resolved;
+    const schema = this.operatorSchemaFor(binding.operatorID);
+    if (!schema) {
+      return undefined;
+    }
+    const operatorType = 
this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)?.operatorType;
+    const full = this.formlyJsonschema.toFieldConfig(cloneDeep(schema) as 
never, {
+      map: (mapped, source) => {
+        // Render the exact custom widget the operator property panel would 
(file/model/dataset
+        // pickers, image/audio uploaders, ...), shared via 
customFormlyFieldType so an exposed
+        // property shows its real control instead of degrading to a text box.
+        const customType = customFormlyFieldType({
+          key: mapped.key,
+          operatorType,
+          description: (source as { description?: string })?.description,
+          currentType: mapped.type,
+        });
+        // Canvas-only widgets (code editor, drag-reorder) do not work here; 
an older workflow may
+        // already carry one, so leave it to formly's default editable control 
rather than a widget
+        // that cannot function on a form.
+        if (customType && !CANVAS_ONLY_FORMLY_TYPES.has(customType)) {
+          mapped.type = customType;
+        }
+        return mapped;
+      },
+    });
+    const source = (full.fieldGroup ?? []).find(child => child.key === 
binding.propertyKey);
+    if (!source) {
+      return undefined;
+    }
+
+    const field = cloneDeep(source);
+    // 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;
+    field.props = {
+      ...(field.props ?? {}),
+      label: binding.displayName || schemaLabel,
+      // The schema's own description is the operator author's note to whoever 
wired the operator
+      // up; it is not guidance to a form reader, and formly shows it once per 
scalar field. Drop it
+      // here so it does not appear unbidden under the input.
+      description: "",
+    };
+
+    const form = new FormGroup({});
+    const model: Record<string, unknown> = { [binding.id]: 
cloneDeep(resolved.value) };
+    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);
+          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))) {
+            return;
+          }
+          // Write straight onto the operator (the same edit the canvas makes) 
and refresh this
+          // card's snapshot, which the template reads.
+          this.formBindingService.writeValue(binding, next);
+          this.parameters = this.formBindingService.resolveFields();
+          const refreshed = this.parameters.find(p => p.binding.id === 
binding.id);
+          const card = this.rendered.find(r => r.resolved.binding.id === 
binding.id);
+          if (refreshed && card) {
+            card.resolved = refreshed;
+          }
+          this.cdr.detectChanges();
+        });
+    } else {
+      // A read-only viewer sees the author's values and can run with them, 
but cannot change them:
+      // disable the control so it renders non-editable, and wire no 
write-back at all.
+      form.disable();

Review Comment:
   Fixed. `form.disable()` on the still-empty group was a no-op -- formly 
builds its controls after it, and a control added to a disabled group comes 
back enabled (verified: `control.disabled` false, and the group re-enables 
itself). Switched to `field.props.disabled = true` (formly honours it, and it 
cascades to a nested property's sub-fields); the read-only card also carries 
`pointer-events: none`, so the extra elements a widget draws of its own (a file 
picker's Browse button, an uploader) are inert too.



##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -175,6 +240,149 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     this.workflowActionService.disableWorkflowModification();
   }
 
+  // 
---------------------------------------------------------------------------
+  // Inputs: the exposed properties, rendered as their operators' own fields
+  // 
---------------------------------------------------------------------------
+
+  /** Whether the cursor is currently inside one of this page's inputs. */
+  private isTypingInTheForm(): boolean {
+    const active = document.activeElement as HTMLElement | null;
+    if (!active || !this.host.nativeElement.contains(active)) {
+      return false;
+    }
+    return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || 
active.isContentEditable;
+  }
+
+  private readConfig(): void {
+    this.parameters = this.formBindingService.resolveFields();
+    this.buildForm();
+  }
+
+  /**
+   * Build the form from the operators' JSON schemas (FormlyJsonschema), 
keeping the one field per
+   * exposed property. Each input gets its own form keyed by binding id.
+   */
+  private buildForm(): void {
+    this.formsRebuilt.next();
+    this.rendered = this.visibleFields
+      .map(field => this.renderField(field))
+      .filter((r): r is RenderedField => r !== undefined);
+  }
+
+  private renderField(resolved: ResolvedField): RenderedField | undefined {
+    const { binding } = resolved;
+    const schema = this.operatorSchemaFor(binding.operatorID);
+    if (!schema) {
+      return undefined;
+    }
+    const operatorType = 
this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)?.operatorType;
+    const full = this.formlyJsonschema.toFieldConfig(cloneDeep(schema) as 
never, {
+      map: (mapped, source) => {
+        // Render the exact custom widget the operator property panel would 
(file/model/dataset
+        // pickers, image/audio uploaders, ...), shared via 
customFormlyFieldType so an exposed
+        // property shows its real control instead of degrading to a text box.
+        const customType = customFormlyFieldType({
+          key: mapped.key,
+          operatorType,
+          description: (source as { description?: string })?.description,
+          currentType: mapped.type,
+        });
+        // Canvas-only widgets (code editor, drag-reorder) do not work here; 
an older workflow may
+        // already carry one, so leave it to formly's default editable control 
rather than a widget
+        // that cannot function on a form.
+        if (customType && !CANVAS_ONLY_FORMLY_TYPES.has(customType)) {
+          mapped.type = customType;
+        }
+        return mapped;
+      },
+    });
+    const source = (full.fieldGroup ?? []).find(child => child.key === 
binding.propertyKey);
+    if (!source) {
+      return undefined;
+    }
+
+    const field = cloneDeep(source);
+    // 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;
+    field.props = {
+      ...(field.props ?? {}),
+      label: binding.displayName || schemaLabel,
+      // The schema's own description is the operator author's note to whoever 
wired the operator
+      // up; it is not guidance to a form reader, and formly shows it once per 
scalar field. Drop it
+      // here so it does not appear unbidden under the input.
+      description: "",
+    };
+
+    const form = new FormGroup({});
+    const model: Record<string, unknown> = { [binding.id]: 
cloneDeep(resolved.value) };
+    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);
+          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))) {
+            return;
+          }
+          // Write straight onto the operator (the same edit the canvas makes) 
and refresh this
+          // card's snapshot, which the template reads.
+          this.formBindingService.writeValue(binding, next);
+          this.parameters = this.formBindingService.resolveFields();
+          const refreshed = this.parameters.find(p => p.binding.id === 
binding.id);
+          const card = this.rendered.find(r => r.resolved.binding.id === 
binding.id);
+          if (refreshed && card) {
+            card.resolved = refreshed;
+          }
+          this.cdr.detectChanges();
+        });
+    } else {
+      // A read-only viewer sees the author's values and can run with them, 
but cannot change them:
+      // disable the control so it renders non-editable, and wire no 
write-back at all.
+      form.disable();
+    }
+
+    return { resolved, fields: [field], form, model };
+  }
+
+  private operatorSchemaFor(operatorID: string): object | undefined {
+    const graph = this.workflowActionService.getTexeraGraph();
+    if (!graph.hasOperator(operatorID)) {
+      return undefined;
+    }
+    try {
+      // Prefer the per-instance schema: it carries the upstream column names, 
so an attribute
+      // picker renders as a dropdown of real columns rather than a text box.
+      return this.dynamicSchemaService.getDynamicSchema(operatorID).jsonSchema;
+    } catch {
+      try {
+        return 
this.operatorMetadataService.getOperatorSchema(graph.getOperator(operatorID).operatorType).jsonSchema;
+      } catch {
+        return undefined;
+      }
+    }
+  }
+
+  /**
+   * The inputs a reader is offered. Broken bindings (the operator was 
deleted, or the property key
+   * no longer exists) are left out, since filling one in could not affect a 
run; the author's view
+   * of them, to repair them, is added by the authoring PR.
+   */
+  public get visibleFields(): ResolvedField[] {

Review Comment:
   There is no binding-level `hidden` on `FormFieldBinding` today -- `hidden` 
lives on the per-sub-field `FormFieldOverride`, which #8438 honours in the 
sub-field walk. So there is nothing at the binding level to filter yet; when 
authoring adds a way to hide a whole input, the filter goes in with it.



##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -121,6 +162,28 @@ export class WorkflowFormComponent implements OnInit, 
OnDestroy {
     }
     this.wid = wid;
     this.load(wid);
+
+    // Attribute boxes become dropdowns only after compilation writes the 
column enums into each
+    // operator's dynamic schema -- which lands after these cards were built. 
Rebuild on the
+    // compilation-state stream, a ReplaySubject(1) so a late subscriber (this 
page reloads fresh
+    // on every Canvas<->Form switch) gets the current state at once. Skip it 
while someone is
+    // typing, so a rebuild does not throw away a half-entered value under the 
cursor.
+    this.workflowCompilingService
+      .getCompilationStateInfoChangedStream()
+      .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this))
+      .subscribe(() => {
+        if (this.isTypingInTheForm()) {
+          return;
+        }
+        this.readConfig();
+      });
+
+    // Exposing or un-exposing a property (from the panel, or a co-editor) 
changes the definition;
+    // the inputs above have to follow at once, which is the whole point of 
editing side by side.
+    
this.workflowActionService.formBindingChanged$.pipe(untilDestroyed(this)).subscribe(()
 => {

Review Comment:
   Added -- the formBindingChanged path now has the same `isTypingInTheForm()` 
skip as the compilation path, and the comment notes it goes live for co-editors 
once #8351 lands.



-- 
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]

Reply via email to