yangzhang75 commented on code in PR #8442:
URL: https://github.com/apache/texera/pull/8442#discussion_r3982951833
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -631,10 +636,19 @@ export class OperatorPropertyEditFrameComponent
implements OnInit, OnChanges, On
this.currentOperatorSchema =
this.dynamicSchemaService.getDynamicSchema(this.currentOperatorId);
this.currentOperatorStatus =
this.workflowStatusSerivce.getCurrentStatus()[this.currentOperatorId];
-
this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ if (this.broadcastEditing) {
+ this.workflowActionService
+ .getTexeraGraph()
+ .updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ }
const operator =
this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId);
- // set the operator data needed
- this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
+ // Syncing the operator to the current schema version writes the new
version into the Yjs shared
+ // model (changeOperatorVersion), which broadcasts and persists. That is
right on the canvas, but
+ // a read-only inspect (broadcastEditing=false) must not mutate the
workflow just by opening a
+ // step, so skip the sync there and show the version as stored.
+ if (this.broadcastEditing) {
+ this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
Review Comment:
Fixed, and you were right about the reach of it. The write is real, not
theoretical: ajv with useDefaults fills in any default the stored operator
lacks, and the Python UDF branch just above sets defaultEnv=true when it is
undefined, so opening any UDF saved before that field existed made formData
differ from the graph and the debounced write went through. Every write is now
gated on one input: awareness, version sync, both property paths (the
form-change sink and the ui-parameter sync), and setInteractivity, which clamps
so neither the modification-enabled stream after a run nor the runtime unlock
button can make a viewer editable; the unlock button is hidden there rather
than left present and inert. The property writes are gated at their single sink
rather than per caller so a new path cannot escape. Renamed to actsAsEditor per
the naming thread. Added the fake-timer regression test you asked for (opens a
step, ticks past FORM_DEBOUNCE_TIME_MS, asserts no awareness, no version,
no property write, and that the stored properties are untouched), plus the
positive counterpart proving the same change does write when the flag is on,
and a viewer test for the ui-parameter sync.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -236,8 +243,48 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
// Give the result tables a realistic height to page against, so they show
a screenful of rows
// instead of one. (~7 rows; the card scrolls for the rest.)
this.panelResizeService.changePanelSize(900, 560);
+ // Highlighting is off by default; turning it on is what makes a click on
a step select it,
+ // which is how a reader opens that step's panel to inspect it (and,
later, an author to expose).
+ this.workflowActionService.setHighlightingEnabled(true);
this.load(wid);
+ // Selecting a step on the embedded (read-only) canvas opens its property
panel read-only. The
+ // canvas is not editable, but highlighting still works, so reuse it
rather than teach the editor
+ // a second click mode.
+ this.workflowActionService
+ .getJointGraphWrapper()
+ .getJointOperatorHighlightStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => {
+ // The stream emits only the newly-highlighted ids, not the whole
selection, so read the
+ // current selection to decide -- the same source the property panel
uses. Exactly one
+ // highlighted step opens the panel; a shift-click multi-select opens
nothing (the panel
+ // shows no single operator either), rather than opening whichever
step was clicked last.
+ const selected =
this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs();
+ if (selected.length === 1) {
+ this.onOperatorClicked(selected[0]);
+ } else {
+ this.clearSelection();
+ }
+ // The panel is mounted with [broadcastEditing]="false", so opening a
step here never
+ // announces "currently editing this operator" on the shared co-editor
channel -- a reader
+ // inspecting a step is not editing the graph, and broadcasting would
print the reader's own
+ // name in colour over that operator on everyone else's canvas.
Suppressed at the frame (the
+ // only place that writes it), not here, so it cannot be re-set after
this handler runs.
+ });
+
+ // Clicking empty canvas clears the highlight; the panel should go with it.
+ this.workflowActionService
+ .getJointGraphWrapper()
+ .getJointOperatorUnhighlightStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => {
+ if
(this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs().length
=== 0) {
+ this.clearSelection();
+ this.cdr.detectChanges();
+ }
+ });
Review Comment:
Fixed. Both streams now run one rule that reads the resulting selection
rather than the delta, so dropping one of two selected steps opens the panel on
the one that is left. Test added for that transition and for the case where
more than one is still highlighted.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -1043,6 +1090,34 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return this.rendered.some(r => hasRequiredError(r.form));
}
+ //
---------------------------------------------------------------------------
+ // Inspecting a step: its property panel, opened read-only from the preview
+ //
---------------------------------------------------------------------------
+
+ /** Clicking a step opens the workflow's own property panel for it,
read-only. */
+ public onOperatorClicked(operatorID: string): void {
+ const graph = this.workflowActionService.getTexeraGraph();
+ if (!graph.hasOperator(operatorID)) {
+ this.clearSelection();
+ return;
+ }
+ this.selectedOperatorId = operatorID;
+ this.cdr.detectChanges();
+ }
+
+ /** Dismiss the panel: the selection is what holds it open, so drop the
highlight and the selection. */
+ public closeOperatorPanel(): void {
+ const wrapper = this.workflowActionService.getJointGraphWrapper();
+
wrapper.unhighlightOperators(...wrapper.getCurrentHighlightedOperatorIDs());
Review Comment:
Fixed, now through WorkflowActionService.unhighlightOperators. The spec
harness models the two as the real pair (the wrapper drops the ids, the service
also publishes the highlighted awareness field) so the test can tell them
apart, and it asserts the broadcast.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts:
##########
@@ -75,6 +77,19 @@ import { GuiConfigService } from
"../../../common/service/gui-config.service";
* name/avatar row, the Canvas switch actually firing, the loading/body swap,
and the co-editor
* row -- which is the review's evidence of the rendered page in place of a
screenshot.
*/
+// A stand-in for the always-mounted property panel. The real one is heavy --
its ngOnInit
+// subscribes to the full JointJS highlight-stream set and the panel service
-- and it has its
+// own spec. This page only needs the panel present (it lives behind [hidden],
not *ngIf, so it
+// is mounted from the start to catch the highlight that opens it), so swap in
a stub carrying the
+// two inputs the template binds and nothing else. The swap is on a child of
the page, so the
+// page's own template still renders as shipped and stays covered.
+@Component({ selector: "texera-property-editor", template: "", standalone:
true })
+class MockPropertyEditorComponent {
+ @Input() exposeChoosing = false;
+ @Input() persistPlacement = true;
+ @Input() broadcastEditing = true;
Review Comment:
Fixed. The rendered spec now asserts the panel is mounted before anything is
selected, the [hidden] flip when a step is selected, the inert attribute, and
all three input bindings read back off the stub. The authoring PR adds the
counterpart for edit mode.
--
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]
