yangzhang75 commented on code in PR #8455: URL: https://github.com/apache/texera/pull/8455#discussion_r3984318387
########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.ts: ########## @@ -0,0 +1,75 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component } from "@angular/core"; +import { NgIf } from "@angular/common"; +import { NzIconDirective } from "ng-zorro-antd/icon"; +import { FieldWrapper, FormlyFieldConfig } from "@ngx-formly/core"; +import { merge } from "lodash-es"; + +/** + * Lets an author rename or hide one field of the form in place: the label itself becomes + * the input, so what they type is exactly what the reader sees, where they see it (the + * schema's own labels -- "File Key", "Alias" -- describe the operator, not the reader's + * task). Renders as a plain label for anyone not authoring. + */ +@Component({ + selector: "texera-editable-label-wrapper", + templateUrl: "./editable-label-wrapper.component.html", + styleUrls: ["./editable-label-wrapper.component.scss"], Review Comment: Not needed on this codebase: Angular 21 (package.json pins 21.2.19) makes components standalone by default, so imports is valid without the flag, and the rest of the repo omits it the same way (e.g. DatasetSelectionModalComponent). It compiles and AOT builds as is. ########## frontend/src/app/workspace/component/property-editor/property-editor.component.ts: ########## @@ -205,6 +228,11 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { @HostListener("window:beforeunload") ngOnDestroy(): void { + // The Form View's read-only copy (persistPlacement=false) must not persist geometry: it is not + // the docked canvas panel, so writing these keys would overwrite the real panel's saved size. + if (!this.persistPlacement) { + return; + } localStorage.setItem("right-panel-width", String(this.width)); localStorage.setItem("right-panel-height", String(this.height)); Review Comment: Done, in #8442 where that code lives (this PR sits on it): the persistence block is now inside if (this.persistPlacement) and nothing returns early, so anything added to ngOnDestroy later runs for both mounts. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -221,6 +361,136 @@ <h2>{{ instructionTitle || "How to use this" }}</h2> <texera-mini-map *ngIf="workflowEverOpened" class="box"></texera-mini-map> + + <!-- Sibling of the panel, not nested inside it: the property editor opens its own stacking + context, and a button inside that context paints under its content -- visible but + unclickable. As a sibling the close button is simply above. --> + <button + class="panel-close" + *ngIf="selectedOperatorId" + type="button" + aria-label="Close step details" + (click)="closeOperatorPanel()"> + <i + nz-icon + nzType="close" + aria-hidden="true"></i> + </button> + + <!-- Reader / inspect: the property editor carries the `inert` attribute (blocks pointer AND + keyboard AND focus) and does not broadcast or write, so a step opens read-only. + Authoring: the panel goes live -- inert off, exposeChoosing on (tick boxes to pick what + the form exposes), and it broadcasts/edits like the canvas, since edit mode has enabled + workflow modification. persistPlacement stays off either way (this is not the docked + canvas panel). + [hidden], not *ngIf: the property editor shows its operator by REACTING to the highlight + stream (no initial pull), so it must already be mounted and subscribed when the click + highlights a step. Mounting it on selection (*ngIf) subscribes too late, misses that + emission, and the panel opens empty. So keep it mounted and just hide it. --> + <div + class="panel" + [hidden]="!selectedOperatorId"> + <texera-property-editor + [exposeChoosing]="authoring" + [persistPlacement]="false" + [broadcastEditing]="authoring" + [attr.inert]="authoring ? null : ''"></texera-property-editor> + </div> + </div> + </section> + + <!-- Results, under the workflow that produced them. The section keeps its place before a run so + the answer has a visible destination. --> + <section class="results"> + <div class="pc-section-head"><span class="label">Results</span></div> + <p + class="results-empty" + *ngIf="resultIdsToShow.length === 0"> + {{ isRunning ? "Working…" : hasRunFinished ? "This run produced no results to show." : "Press Run and the + results appear here." }} + </p> + <!-- A card only for a step that actually produced a result. Whether a Python UDF + yields one cannot be told from the graph, so a step earns its card at runtime rather than + sitting on a permanent "No result yet." (a download/publish step never would). --> + <ng-container *ngFor="let id of resultIdsToShow"> + <div class="card result"> + <div class="result-head"> + <span>{{ resultLabel(id) }}</span> + <!-- Zooming only means something once there is a picture to zoom. --> + <span + class="result-zoom" + *ngIf="vizHasContent(id)"> + <button + (click)="zoomResult(id, -1)" + [disabled]="resultZoom(id) <= 0" + nz-tooltip="Smaller" + aria-label="Smaller"> Review Comment: This is a supported form: ng-zorro NzTooltipDirective declares the nz-tooltip attribute itself as an input (directiveTitle: ["nz-tooltip", ...] in ng-zorro-antd-tooltip.mjs), so nz-tooltip="Smaller" sets the title. The repo uses it everywhere, e.g. nz-tooltip="Customize Operator Name" on the property panel. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -221,6 +361,136 @@ <h2>{{ instructionTitle || "How to use this" }}</h2> <texera-mini-map *ngIf="workflowEverOpened" class="box"></texera-mini-map> + + <!-- Sibling of the panel, not nested inside it: the property editor opens its own stacking + context, and a button inside that context paints under its content -- visible but + unclickable. As a sibling the close button is simply above. --> + <button + class="panel-close" + *ngIf="selectedOperatorId" + type="button" + aria-label="Close step details" + (click)="closeOperatorPanel()"> + <i + nz-icon + nzType="close" + aria-hidden="true"></i> + </button> + + <!-- Reader / inspect: the property editor carries the `inert` attribute (blocks pointer AND + keyboard AND focus) and does not broadcast or write, so a step opens read-only. + Authoring: the panel goes live -- inert off, exposeChoosing on (tick boxes to pick what + the form exposes), and it broadcasts/edits like the canvas, since edit mode has enabled + workflow modification. persistPlacement stays off either way (this is not the docked + canvas panel). + [hidden], not *ngIf: the property editor shows its operator by REACTING to the highlight + stream (no initial pull), so it must already be mounted and subscribed when the click + highlights a step. Mounting it on selection (*ngIf) subscribes too late, misses that + emission, and the panel opens empty. So keep it mounted and just hide it. --> + <div + class="panel" + [hidden]="!selectedOperatorId"> + <texera-property-editor + [exposeChoosing]="authoring" + [persistPlacement]="false" + [broadcastEditing]="authoring" + [attr.inert]="authoring ? null : ''"></texera-property-editor> + </div> + </div> + </section> + + <!-- Results, under the workflow that produced them. The section keeps its place before a run so + the answer has a visible destination. --> + <section class="results"> + <div class="pc-section-head"><span class="label">Results</span></div> + <p + class="results-empty" + *ngIf="resultIdsToShow.length === 0"> + {{ isRunning ? "Working…" : hasRunFinished ? "This run produced no results to show." : "Press Run and the + results appear here." }} + </p> + <!-- A card only for a step that actually produced a result. Whether a Python UDF + yields one cannot be told from the graph, so a step earns its card at runtime rather than + sitting on a permanent "No result yet." (a download/publish step never would). --> + <ng-container *ngFor="let id of resultIdsToShow"> + <div class="card result"> + <div class="result-head"> + <span>{{ resultLabel(id) }}</span> + <!-- Zooming only means something once there is a picture to zoom. --> + <span + class="result-zoom" + *ngIf="vizHasContent(id)"> + <button + (click)="zoomResult(id, -1)" + [disabled]="resultZoom(id) <= 0" + nz-tooltip="Smaller" + aria-label="Smaller"> + <i + nz-icon + nzType="minus" + aria-hidden="true"></i> + </button> + <button + (click)="zoomResult(id, 1)" + [disabled]="resultZoom(id) >= 2" + nz-tooltip="Bigger" + aria-label="Bigger"> Review Comment: Same as the sibling thread: nz-tooltip="..." is the directive title input in ng-zorro (directiveTitle), not just an enabler, and it is the form used across the repo. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } Review Comment: Fixed, both halves. toggleAuthoring now refuses to enter edit mode without canEdit (leaving is always allowed), so the guard is at the method rather than only on the button; the test you noticed that entered edit mode as a reader now asserts it cannot. And pruneBrokenBindings is gone altogether (see the #8456 thread on it): it was deleting a broken binding before resolveFields could show it as the removable card this PR describes, so the only remaining config writes in edit mode are explicit author actions. ########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.html: ########## @@ -0,0 +1,58 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<!-- Authoring: the label is the input, so what you type is what the reader reads. --> +<div + class="lbl-row" + *ngIf="props.authoring"> + <input + class="lbl-input" + [value]="props.authorName" + [placeholder]="props.schemaLabel" + (change)="onRename($event)" Review Comment: Deliberate: the name commits on blur or Enter, the same way the workflow name input does (change on the canvas title). Writing on every keystroke would persist half-typed names into the shared config and rebuild the form under the cursor; there is no live preview elsewhere on the page that lags because of it. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } + + /** + * The author's picker for the extra results: every non-terminal step with view-result ("the eye") + * on the canvas is offered, plus any already-chosen step (so a pick never vanishes from its own + * picker). The terminal step is not offered -- its result always shows and is not the author's to + * toggle. The shown flag mirrors the saved resultOperatorIds. + */ + private rebuildResultChoices(): void { + // The picker is only shown while authoring, so a reader does no per-operator work. + if (!this.authoring) { + this.resultChoices = []; + return; + } + const viewed = this.workflowActionService.getTexeraGraph().getOperatorsToViewResult(); + const chosen = this.formBindingService.getConfig().resultOperatorIds; + // The terminal result always shows and is not the author's to toggle, so it is not offered here. + // The picker curates only the extra intermediate steps -- those given view-result (the eye) on the + // canvas. Reuse the one terminal rule (terminalOperatorIds) rather than a second copy. Already-chosen + // ids stay listed so the author can un-pick them. + const terminals = new Set(this.terminalOperatorIds()); + this.resultChoices = this.operators() + .filter(op => !terminals.has(op.operatorID) && (viewed.has(op.operatorID) || chosen.includes(op.operatorID))) + .map(op => ({ + operatorID: op.operatorID, + label: this.formBindingService.operatorLabel(op), + shown: chosen.includes(op.operatorID), Review Comment: Done: chosen is a Set now, has() in both places. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } + + /** + * The author's picker for the extra results: every non-terminal step with view-result ("the eye") + * on the canvas is offered, plus any already-chosen step (so a pick never vanishes from its own + * picker). The terminal step is not offered -- its result always shows and is not the author's to + * toggle. The shown flag mirrors the saved resultOperatorIds. + */ + private rebuildResultChoices(): void { + // The picker is only shown while authoring, so a reader does no per-operator work. + if (!this.authoring) { + this.resultChoices = []; + return; + } + const viewed = this.workflowActionService.getTexeraGraph().getOperatorsToViewResult(); + const chosen = this.formBindingService.getConfig().resultOperatorIds; + // The terminal result always shows and is not the author's to toggle, so it is not offered here. + // The picker curates only the extra intermediate steps -- those given view-result (the eye) on the + // canvas. Reuse the one terminal rule (terminalOperatorIds) rather than a second copy. Already-chosen + // ids stay listed so the author can un-pick them. + const terminals = new Set(this.terminalOperatorIds()); + this.resultChoices = this.operators() + .filter(op => !terminals.has(op.operatorID) && (viewed.has(op.operatorID) || chosen.includes(op.operatorID))) + .map(op => ({ + operatorID: op.operatorID, + label: this.formBindingService.operatorLabel(op), + shown: chosen.includes(op.operatorID), Review Comment: Done together with the sibling thread: one Set, has() in both places. -- 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]
