yangzhang75 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4074692014
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -629,6 +629,42 @@ export class WorkflowActionService {
this.texeraGraph.destroyYModel();
}
+ /**
+ * Whether this page already holds `workflowId` open: the shared document is
in that workflow's
+ * co-editing room, so the graph, the undo history and the room membership
are the live ones.
+ *
+ * The operator canvas and the Form View are two views of one open workflow
and hand the session
+ * over between them rather than each building its own. The arriving view
asks this before
+ * loading: seeding a second document for the same workflow would leave the
room and rejoin it,
+ * which is what used to leave a ghost of yourself in the co-editor list.
+ *
+ * A workflow that was open and has since been left does not answer true
here, and the reason is
+ * worth stating because it is not local: `destroyYModel` destroys the
document but keeps the
+ * object, `wid` and all. What clears it is `clearWorkflow` going on to
`reloadWorkflow(undefined)`,
+ * which seeds a fresh model with no `wid`. Were that to stop happening, a
canvas re-entered from
+ * the dashboard would attach to a destroyed document instead of loading.
Pinned by a test.
+ */
+ public hasWorkflowOpen(workflowId: number): boolean {
+ return this.texeraGraph.sharedModel.wid === workflowId;
Review Comment:
Fixed, along the lines @mengw15 laid out below rather than the first
suggestion here. Both sides key the hand-over on the room the shared document
is in now: `WorkflowActionService.getOpenWorkflowId()` returns the room's wid
or undefined, `isLeavingWorkspace` is fed that on the way out, and
`hasWorkflowOpen` no longer matches on a falsy id. A workflow created in this
session -- whose document sits in a uuid room while its metadata carries the
assigned id -- is torn down and rebuilt on its first switch, as it is today.
Recognising the metadata id on the arriving side was not taken: it would have
had the Form View co-editing in a private room under the workflow's name,
invisible to anyone else opening it. Tests: the departing canvas and form both
tear down when the document is in no workflow's room even though the
destination is this workflow's other view; `getOpenWorkflowId` names the room
or nothing; `hasWorkflowOpen(undefined)` and `(0)` are false. All
deletion-checked. f9e9d49
##########
frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts:
##########
@@ -88,8 +95,26 @@ export class MiniMapComponent implements AfterViewInit,
OnDestroy {
this.panelService.resetPanelStream.pipe(untilDestroyed(this)).subscribe(()
=> (this.hidden = false));
}
+ /**
+ * The browser is leaving this document: remember whether the mini-map was
hidden, and destroy
+ * nothing. The document may be kept in the back/forward cache and restored
with its JavaScript
+ * state exactly as it was left, re-running nothing, so a paper disposed
here would stay disposed
+ * on a page that looks live (the same reason the workspace stopped tearing
down here, #8599).
+ */
@HostListener("window:beforeunload")
+ onBeforeUnload(): void {
+ this.rememberVisibility();
+ }
+
ngOnDestroy(): void {
Review Comment:
Fixed. One bound reference registered on the main paper, removed when a new
paper arrives and on destroy, so the departing mini-map -- which, as you say,
still receives the arriving paper through the replaying stream -- leaves
nothing on it. Test: handlers on the first paper are gone once a second
arrives, and gone from the second on destroy; deletion-checked. f9e9d49
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -197,17 +197,14 @@ export class MenuComponent implements OnInit, OnDestroy {
private jupyterPanelService: JupyterPanelService,
private fileSaverService: FileSaverService
) {
- workflowWebsocketService
- .subscribeToEvent("ExecutionDurationUpdateEvent")
- .pipe(
- tap(event => (this.executionDuration = event.duration)),
- // restart the 1s timer on each event, only while running
- switchMap(event => (event.isRunning ? timer(1000, 1000) : EMPTY)),
- untilDestroyed(this)
- )
- .subscribe(() => {
- this.executionDuration += 1000;
- });
+ // From the service, not from the engine's event directly: that event
arrives twice in a whole
+ // run, so a timer hung off it never started for a menu that mounted in
between -- which is what
+ // a routed switch between the canvas and the Form View makes. The service
anchors the clock and
+ // ticks it, and replays the current value to whoever subscribes.
+ executeWorkflowService
+ .getExecutionDurationStream()
Review Comment:
Removed: `switchMap`, `EMPTY` and `timer` from the menu, `switchMap` from
the Form View (`EMPTY`, `timer` and `tap` are still used there). Thanks for the
note on why CI let it through. f9e9d49
--
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]
