yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117998964
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +861,117 @@ 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();
}
/**
- * 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.
+ * 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 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
+ * not bounded: a copy published before the sync would be the concurrent
write above, on a room
+ * that is slow to answer as much as on one that never does. For as long as
the room stays out
+ * of reach the copy is read from here and every save carries it, and it
goes into the document
+ * when the sync eventually comes -- or never, which loses nothing, since
nobody else was in that
+ * document either.
+ *
+ * A local edit of the key while the seed waits cancels it (see
observeContentMeta): the document
+ * then carries the author's value, which the seed must neither write over
nor delete.
+ *
+ * `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 {
+ // This seed replaces whatever the previously open workflow left waiting
for the key. It has an
+ // identity of its own, since a cleared key (undefined) has no value to be
told apart by.
+ const seed = {};
+ this.contentMetaSeeds.set(key, seed);
+ this.pendingContentMeta.delete(key);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ const land = () => {
+ // Not ours to write any more: the workflow was closed or swapped while
we waited (the seed
+ // or the document was replaced), or the author edited the key meanwhile
(the seed was
+ // cancelled).
+ if (this.texeraGraph.sharedModel !== shared ||
this.contentMetaSeeds.get(key) !== seed) {
+ return;
+ }
+ this.cancelContentMetaSeed(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;
Review Comment:
Right: the room's precedence outlived the open and reached every later
reload into the same document. Fixed in aabcbe763: it applies to the first seed
of a key on a document only (`seededContentMetaKeys`, reset with the document);
a later reload's seed -- a version shown or returned from
(`displayReadonlyWorkflow` / `closeReadonlyWorkflowDisplay`), an agent's
rewrite -- replaces the value, the room's included, or clears it for a version
that carries none. Spec: a co-editor's value survives the open; a reload then
brings another and the document holds that one; a reload without one clears it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +861,117 @@ 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();
}
/**
- * 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.
+ * 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 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
+ * not bounded: a copy published before the sync would be the concurrent
write above, on a room
+ * that is slow to answer as much as on one that never does. For as long as
the room stays out
+ * of reach the copy is read from here and every save carries it, and it
goes into the document
+ * when the sync eventually comes -- or never, which loses nothing, since
nobody else was in that
+ * document either.
+ *
+ * A local edit of the key while the seed waits cancels it (see
observeContentMeta): the document
+ * then carries the author's value, which the seed must neither write over
nor delete.
+ *
+ * `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 {
+ // This seed replaces whatever the previously open workflow left waiting
for the key. It has an
+ // identity of its own, since a cleared key (undefined) has no value to be
told apart by.
+ const seed = {};
+ this.contentMetaSeeds.set(key, seed);
+ this.pendingContentMeta.delete(key);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ const land = () => {
+ // Not ours to write any more: the workflow was closed or swapped while
we waited (the seed
+ // or the document was replaced), or the author edited the key meanwhile
(the seed was
+ // cancelled).
+ if (this.texeraGraph.sharedModel !== shared ||
this.contentMetaSeeds.get(key) !== seed) {
+ return;
+ }
+ this.cancelContentMetaSeed(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) {
+ land();
+ return;
+ }
+ shared.wsProvider.once("sync", land);
Review Comment:
Done in aabcbe763: each seed owns its listener (`on` with a `detach` on the
seed, rather than `once`) and drops it when it lands, is superseded or is
cancelled; loading a new document cancels every seed of the old one. Spec: one
sync listener per waiting key after two reloads, none after a cancelling edit,
none once the sync has landed.
##########
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:
One change since the reply above, in aabcbe763: rather than seeding both
keys in the blank branch, the per-document seed state (waiting copies, seeds,
room ownership, seeded keys) is reset when a new document is loaded, which is
what the blank reload does. So the blank branch seeds nothing and
`clearWorkflow` writes nothing; the same two tests pin it (clearing announces
nothing and writes nothing; a blank workflow does not read the previous one's
waiting copy).
--
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]
