yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117888474
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +854,99 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined):
void {
- if (this.workflowSettings === workflowSettings) {
+ const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
+ // Skip a redundant write: setting the same value would still cut a Yjs
update and observer
+ // churn for every collaborator.
+ if (isEqual(this.getWorkflowSettings(), newSettings)) {
return;
}
-
- const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
- this.workflowSettings = newSettings;
+ this.texeraGraph.sharedModel.contentMetaMap.set("settings", newSettings);
}
public getWorkflowSettings(): WorkflowSettings {
- return this.workflowSettings;
+ return (this.readContentMeta("settings") as WorkflowSettings) ??
this.getDefaultSettings();
+ }
+
+ /**
+ * Seed workflowSettings into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the seed is not announced as an edit.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void
{
+ this.seedContentMeta("settings", workflowSettings);
}
/**
- * Load a definition without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * Load a definition into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the shared-map observer skips this
seed and opening a
+ * workflow is not announced as an edit.
*/
public hydrateFormBinding(formBinding: FormBindingConfig | undefined): void {
- this.formBindingLoaded = formBinding !== undefined;
- this.formBinding = formBinding ?? getDefaultFormBinding();
+ this.seedContentMeta("formBinding", formBinding);
+ }
+
+ /**
+ * Put the database's copy of a content-meta value into the shared model as
a workflow opens,
+ * without taking a co-editor's newer one away.
+ *
+ * The document is created and connected just before this runs, so the
provider has usually not
+ * synced yet. Writing straight away is a concurrent whole-value write, and
Yjs resolves those
+ * by client id rather than by recency: the copy the database handed us
could win over an edit a
+ * co-editor has not saved yet -- the lost update this all exists to
prevent. So wait for the
+ * first sync, then write only when the key is still absent; a value that
came from the room is
+ * authoritative, whether it is a co-editor's edit or our own from another
tab.
+ *
+ * Meanwhile the value is not lost: reads fall back to the copy held here,
so the page shows it
+ * and an autosave that fires before the sync carries it rather than an
empty one. The wait is
+ * also bounded, so an unreachable y-websocket still ends up with the value
in the document.
+ *
+ * `undefined` clears the key, so a workflow that carries no settings or no
form definition does
+ * not inherit the one the document already held (reloading a version into
the open document);
+ * a value the room put there is left alone here too.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue |
undefined): void {
+ // Whatever the previously open workflow left waiting is not this
workflow's.
+ this.pendingContentMeta.delete(key);
+ const shared = this.texeraGraph.sharedModel;
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const seed = () => {
+ // The workflow may have been closed or swapped while we waited; that
document's seed, and
+ // that workflow's value, are no longer ours to write.
+ if (this.texeraGraph.sharedModel !== shared ||
this.pendingContentMeta.get(key) !== value) {
Review Comment:
Right: with nothing in the pending map for a cleared key, the seed could not
tell "still waiting" from "superseded by an edit". Fixed in 6667f6962: each
seed has an identity of its own (`contentMetaSeeds`), and a local write to the
key (`setFormBinding`, `setWorkflowSettings`, anything but the seed itself)
cancels it in the observer, so the sync lands on a cancelled seed and neither
writes over nor deletes the author's value. Spec:
`hydrateFormBinding(undefined)`, `setFormBinding(config)`, sync -> the document
still holds `config` (red before).
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +854,99 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined):
void {
- if (this.workflowSettings === workflowSettings) {
+ const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
+ // Skip a redundant write: setting the same value would still cut a Yjs
update and observer
+ // churn for every collaborator.
+ if (isEqual(this.getWorkflowSettings(), newSettings)) {
return;
}
-
- const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
- this.workflowSettings = newSettings;
+ this.texeraGraph.sharedModel.contentMetaMap.set("settings", newSettings);
}
public getWorkflowSettings(): WorkflowSettings {
- return this.workflowSettings;
+ return (this.readContentMeta("settings") as WorkflowSettings) ??
this.getDefaultSettings();
+ }
+
+ /**
+ * Seed workflowSettings into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the seed is not announced as an edit.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void
{
+ this.seedContentMeta("settings", workflowSettings);
}
/**
- * Load a definition without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * Load a definition into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the shared-map observer skips this
seed and opening a
+ * workflow is not announced as an edit.
*/
public hydrateFormBinding(formBinding: FormBindingConfig | undefined): void {
- this.formBindingLoaded = formBinding !== undefined;
- this.formBinding = formBinding ?? getDefaultFormBinding();
+ this.seedContentMeta("formBinding", formBinding);
+ }
+
+ /**
+ * Put the database's copy of a content-meta value into the shared model as
a workflow opens,
+ * without taking a co-editor's newer one away.
+ *
+ * The document is created and connected just before this runs, so the
provider has usually not
+ * synced yet. Writing straight away is a concurrent whole-value write, and
Yjs resolves those
+ * by client id rather than by recency: the copy the database handed us
could win over an edit a
+ * co-editor has not saved yet -- the lost update this all exists to
prevent. So wait for the
+ * first sync, then write only when the key is still absent; a value that
came from the room is
+ * authoritative, whether it is a co-editor's edit or our own from another
tab.
+ *
+ * Meanwhile the value is not lost: reads fall back to the copy held here,
so the page shows it
+ * and an autosave that fires before the sync carries it rather than an
empty one. The wait is
+ * also bounded, so an unreachable y-websocket still ends up with the value
in the document.
+ *
+ * `undefined` clears the key, so a workflow that carries no settings or no
form definition does
+ * not inherit the one the document already held (reloading a version into
the open document);
+ * a value the room put there is left alone here too.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue |
undefined): void {
+ // Whatever the previously open workflow left waiting is not this
workflow's.
+ this.pendingContentMeta.delete(key);
+ const shared = this.texeraGraph.sharedModel;
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const seed = () => {
+ // The workflow may have been closed or swapped while we waited; that
document's seed, and
+ // that workflow's value, are no longer ours to write.
+ if (this.texeraGraph.sharedModel !== shared ||
this.pendingContentMeta.get(key) !== value) {
+ return;
+ }
+ this.pendingContentMeta.delete(key);
+ // The room's value wins, whether it is a co-editor's edit or this
client's from another tab.
+ if (this.roomOwnedContentMeta.has(key)) {
+ return;
+ }
+ // Opening a workflow is not an edit, so the seed is not announced (see
observeContentMeta).
+ this.seedingContentMetaKeys.add(key);
+ try {
+ if (value === undefined) {
+ shared.contentMetaMap.delete(key);
+ } else if (!shared.contentMetaMap.has(key)) {
+ shared.contentMetaMap.set(key, value);
+ }
+ } finally {
+ this.seedingContentMetaKeys.delete(key);
+ }
+ };
+ if (!shared.wsProvider.shouldConnect || shared.wsProvider.synced) {
+ seed();
+ return;
+ }
+ const fallback = setTimeout(seed, CONTENT_META_SEED_TIMEOUT_MS);
Review Comment:
Agreed: the fallback wrote before the sync on a slow room, which is the
concurrent write the wait exists to avoid. Removed in 6667f6962: the copy goes
into the document only on the first sync (or at once when the document is not
connecting, or has already synced); until then reads and every save carry it,
and never seeding it loses nothing, since nobody else was in that document
either. Spec: 60 s without a sync -> document untouched while
`getFormBinding()` and `getWorkflowContent().formBinding` still answer the
copy; then the sync -> the document holds it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +151,53 @@ export class WorkflowActionService {
);
this.sharedModelChangeHandler.setConfigService(this.config);
this.workflowMetadata = DEFAULT_WORKFLOW;
- this.workflowSettings = this.getDefaultSettings();
this.undoRedoService.setUndoManager(this.texeraGraph.sharedModel.undoManager);
+ // Watch the shared content map, re-attaching whenever the shared model is
recreated
+ // (opening another workflow), the same way SharedModelChangeHandler
re-attaches its
+ // graph observers. A formBinding change from a local edit or a co-editor
is republished
+ // on formBindingChanged$ so the Form View re-renders and the existing
autosave picks it
+ // up. The reload seed is skipped -- like the graph seed -- so opening a
workflow is not
+ // announced as an edit and does not save on every open.
+ this.observeContentMeta();
+ this.texeraGraph.newYDocLoadedSubject.subscribe(() =>
this.observeContentMeta());
+
this.handleJointElementDrag();
}
+ private observeContentMeta(): void {
+ // Detach the observer from the previous shared model before attaching to
the new one, so
+ // re-attaching on each opened workflow does not stack listeners on the
same map.
+ this.observedContentMetaMap?.unobserve(this.contentMetaObserver!);
+ // A new document is a new room: nothing in it belongs to the previous
one's co-editors.
+ this.roomOwnedContentMeta.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ // A value that arrived from the room belongs to the room: a seed must
not overwrite or
+ // delete it (see seedContentMeta). The first sync delivers a
co-editor's state as a remote
+ // transaction, which is how an opening client learns the room already
holds one.
+ if (!event.transaction.local) {
+ for (const key of event.changes.keys.keys()) {
+ this.roomOwnedContentMeta.add(key as ContentMetaKey);
+ }
+ }
+ if (this.jointGraphWrapper.getReloadingWorkflow()) {
+ return;
+ }
+ if (event.changes.keys.has("formBinding") &&
!this.seedingContentMetaKeys.has("formBinding")) {
Review Comment:
Manual steps are in the PR body now (two windows on one workflow: A exposes
a field in the Form View or changes a setting, B drags an operator, reload A).
A side-by-side before/after video is being recorded and will be added to the
body; leaving this open until it is.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -718,7 +774,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings ||
this.getDefaultSettings();
+ this.hydrateSettings(workflowContent.settings);
Review Comment:
Done in 6667f6962: `hydrateSettings(undefined)` now sits beside
`hydrateFormBinding(undefined)` in the blank branch, and `clearWorkflow` no
longer calls `setWorkflowSettings(undefined)`, so the settings go back to the
defaults through the seed under the reloading flag, with nothing written into
the destroyed document or announced. Spec: non-default settings,
`clearWorkflow()` -> nothing on `workflowChanged()`, settings back to the
defaults (red before: one announcement). The pending-seed half is with
@copilot's thread: each seed has an identity now, and a local edit of the key
cancels it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -865,11 +1002,12 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or
an author
- // populated it), so a plain workflow's content is unchanged and its
save cuts no
- // needless version.
- ...(this.formBindingLoaded ||
this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or
an author
+ // populated it since), so a plain workflow's content is unchanged and
its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate
sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") ||
this.isFormBindingNonEmpty(formBinding)
Review Comment:
Done in 6667f6962: `hasContentMeta(key)` mirrors `readContentMeta` (the
document, or the copy still waiting), and `getWorkflowContent` uses it, so
presence and value come from the same place. Spec: `hydrateFormBinding({
fields: [] })` before the sync -> `getWorkflowContent().formBinding` is `{
fields: [] }`, not dropped (red before).
--
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]
