From d3126bec6201ff2544281c34ccae336b885cdb5c Mon Sep 17 00:00:00 2001 From: Janet Vorobyeva Date: Tue, 5 Sep 2023 09:22:55 -0700 Subject: [PATCH] Added bidirectional linking (BIG DIFF) --- app/client/components/BaseView.js | 10 +-- app/client/components/Cursor.ts | 54 ++++++++++++- app/client/components/GristDoc.ts | 13 +++- app/client/components/LinkingState.ts | 81 ++++++++++++++++++-- app/client/models/entities/ViewSectionRec.ts | 4 + app/client/ui/RightPanel.ts | 9 ++- app/client/ui/selectBy.ts | 70 ++++++++++++++--- 7 files changed, 215 insertions(+), 26 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 445c1700..42f1c9f0 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -131,8 +131,8 @@ function BaseView(gristDoc, viewSectionModel, options) { // Update the cursor whenever linkedRowId() changes (but only if we have any linking). this.autoDispose(this.linkedRowId.subscribe(rowId => { - if (this.viewSection.linkingState.peek()) { - this.setCursorPos({rowId: rowId || 'new'}); + if (this.viewSection.linkingState.peek() && rowId != null) { //TODO JV: used to be that null meant "new", now it means "no cursor linking" + this.setCursorPos({rowId: rowId || 'new'}, true); //true b/c not a user-edit (caused by linking) } })); @@ -282,14 +282,14 @@ BaseView.prototype.deleteRecords = function(source) { /** * Sets the cursor to the given position, deferring if necessary until the current query finishes - * loading. + * loading. silentUpdate will be set if updating as a result of cursor linking(see Cursor.setCursorPos for info) */ -BaseView.prototype.setCursorPos = function(cursorPos) { +BaseView.prototype.setCursorPos = function(cursorPos, silentUpdate = false) { if (this.isDisposed()) { return; } if (!this._isLoading.peek()) { - this.cursor.setCursorPos(cursorPos); + this.cursor.setCursorPos(cursorPos, silentUpdate); } else { // This is the first step; the second happens in onTableLoaded. this._pendingCursorPos = cursorPos; diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index ad35bcc7..1d3f07cc 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -16,6 +16,21 @@ function nullAsUndefined(value: T|null|undefined): T|undefined { return value == null ? undefined : value; } +// ================ SequenceNum: used to keep track of cursor edits (lastEditedAt) +// basically just a global auto-incrementing counter, with some types to make intent a bit more clear +// Each cursor starts at SequenceNEVER (0), and after that all cursors using NextSequenceNum() will have +// a unique, monotonically increasing number for their lastEditedAt() +export type SequenceNum = number; +export const SequenceNEVER: SequenceNum = 0; +let latestGlobalSequenceNum = SequenceNEVER; +const nextSequenceNum = () => { return latestGlobalSequenceNum++; }; + +// Note: we don't make any provisions for handling overflow. It's fine because: +// - Number.MAX_SAFE_INTEGER is 9,007,199,254,740,991 (9 * 10^15) +// - even at 1000 cursor-edits per second, it would take ~300,000 yrs to overflow +// - Plus it's client-side, so that's a single continuous 300-millenia-long session, which would be impressive uptime + + /** * Cursor represents the location of the cursor in the viewsection. It is maintained by BaseView, * and implements the shared functionality related to the cursor cell. @@ -62,6 +77,12 @@ export class Cursor extends Disposable { private _sectionId: ko.Computed; private _properRowId: ko.Computed; + private _lastEditedAt: ko.Observable; + + private _silentUpdatesFlag: boolean = false; + // lastEditedAt is updated on rowIndex or fieldIndex update (including through setCursorPos) + // _silentUpdatesFlag disables this, set when setCursorPos called from cursor link to prevent infinite loops + // WARNING: the flag approach will only work if ko observables work synchronously, which they appear to do. constructor(baseView: BaseView, optCursorPos?: CursorPos) { super(); @@ -79,10 +100,13 @@ export class Cursor extends Disposable { write: (index) => { const rowIndex = index === null ? null : this.viewData.clampIndex(index); this._rowId(rowIndex == null ? null : this.viewData.getRowId(rowIndex)); + this.cursorEdited(); }, })); this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0); + this.fieldIndex.subscribe(() => { this.cursorEdited(); }); + this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.hasFocus)); // RowId might diverge from the one stored in _rowId when the data changes (it is filtered out). So here @@ -93,8 +117,11 @@ export class Cursor extends Disposable { return rowId; })); - // Update the section's activeRowId when the cursor's rowIndex is changed. + this._lastEditedAt = ko.observable(SequenceNEVER); + + // update the section's activeRowId and lastCursorEdit when needed this.autoDispose(this._properRowId.subscribe((rowId) => baseView.viewSection.activeRowId(rowId))); + this.autoDispose(this._lastEditedAt.subscribe((seqNum) => baseView.viewSection.lastCursorEdit(seqNum))); // On dispose, save the current cursor position to the section model. this.onDispose(() => { baseView.viewSection.lastCursorPos = this.getCursorPos(); }); @@ -116,9 +143,16 @@ export class Cursor extends Disposable { /** * Moves the cursor to the given position. Only moves the row if rowId or rowIndex is valid, * preferring rowId. + * + * silentUpdate prevents lastEditedAt from being updated, so linking doesn't cause an infinite loop of updates * @param cursorPos: Position as { rowId?, rowIndex?, fieldIndex? }, as from getCursorPos(). + * @param silentUpdate: should only be set if this is a cascading update from cursor-linking */ - public setCursorPos(cursorPos: CursorPos): void { + public setCursorPos(cursorPos: CursorPos, silentUpdate: boolean = false): void { + //If updating as a result of links, we want to NOT update lastEditedAt + if(silentUpdate) { this._silentUpdatesFlag = true; } + //console.log(`CURSOR: ${silentUpdate}, silentUpdate=${this._silentUpdatesFlag}, lastUpdated = ${this.lastUpdated.peek()}`) //TODO JV DEBUG TEMP + if (cursorPos.rowId !== undefined && this.viewData.getRowIndex(cursorPos.rowId) >= 0) { this.rowIndex(this.viewData.getRowIndex(cursorPos.rowId) ); } else if (cursorPos.rowIndex !== undefined && cursorPos.rowIndex >= 0) { @@ -130,9 +164,25 @@ export class Cursor extends Disposable { if (cursorPos.fieldIndex !== undefined) { this.fieldIndex(cursorPos.fieldIndex); } + + //console.log(`CURSOR-END: silentUpdate=${this._silentUpdatesFlag}, lastEditedAt = ${this._lastEditedAt.peek()} `); //TODO JV DEBUG TEMP + this._silentUpdatesFlag = false; } + + + public setLive(isLive: boolean): void { this._isLive(isLive); } + + //Should be called whenever the cursor is updated + //EXCEPT FOR: when cursor is set by linking + //this is used to determine which widget/cursor has most recently been touched, + //and therefore which one should be used to drive linking if there's a conflict + public cursorEdited(): void { + //If updating as a result of links, we want to NOT update lastEdited + if(!this._silentUpdatesFlag) + { this._lastEditedAt(nextSequenceNum()); } + } } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index d46ae3ce..4eeddfa2 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -1120,7 +1120,8 @@ export class GristDoc extends DisposableWithEvents { public async recursiveMoveToCursorPos( cursorPos: CursorPos, setAsActiveSection: boolean, - silent: boolean = false): Promise { + silent: boolean = false, + visitedSections: number[] = []): Promise { try { if (!cursorPos.sectionId) { throw new Error('sectionId required'); @@ -1132,6 +1133,12 @@ export class GristDoc extends DisposableWithEvents { if (!section.id.peek()) { throw new Error(`Section ${cursorPos.sectionId} does not exist`); } + + if (visitedSections.includes(section.id.peek())) { + //We've already been here (we hit a cycle), just return immediately + return true; + } + const srcSection = section.linkSrcSection.peek(); if (srcSection.id.peek()) { // We're in a linked section, so we need to recurse to make sure the row we want @@ -1178,10 +1185,12 @@ export class GristDoc extends DisposableWithEvents { if (!srcRowId || typeof srcRowId !== 'number') { throw new Error('cannot trace rowId'); } + await this.recursiveMoveToCursorPos({ rowId: srcRowId, sectionId: srcSection.id.peek(), - }, false, silent); + }, false, silent, visitedSections.concat([section.id.peek()])); + } const view: ViewRec = section.view.peek(); const docPage: ViewDocPage = section.isRaw.peek() ? "data" : view.getRowId(); diff --git a/app/client/components/LinkingState.ts b/app/client/components/LinkingState.ts index ef532816..70ce4560 100644 --- a/app/client/components/LinkingState.ts +++ b/app/client/components/LinkingState.ts @@ -17,6 +17,7 @@ import merge = require('lodash/merge'); import mapValues = require('lodash/mapValues'); import pick = require('lodash/pick'); import pickBy = require('lodash/pickBy'); +import {SequenceNEVER, SequenceNum} from "./Cursor"; // Descriptive string enum for each case of linking @@ -51,7 +52,13 @@ export const EmptyFilterColValues: FilterColValues = FilterStateToColValues(Empt export class LinkingState extends Disposable { // If linking affects target section's cursor, this will be a computed for the cursor rowId. // Is undefined if not cursor-linked - public readonly cursorPos?: ko.Computed; + public readonly cursorPos?: ko.Computed; + + //TODO: JV bidirectional linking stuff + //It's a pair of [position, version] + //NOTE: observables don't do deep-equality check, so MUST NEVER change the value of the components individually, + //have to update the whole array so that `==` can catch the change + public readonly incomingCursorPos: ko.Computed<[UIRowId|null, SequenceNum]>; // If linking affects filtering, this is a computed for the current filtering state, including user-facing // labels for filter values and types of the filtered columns @@ -71,6 +78,7 @@ export class LinkingState extends Disposable { private _docModel: DocModel; private _srcSection: ViewSectionRec; + private _tgtSection: ViewSectionRec; private _srcTableModel: DataTableModel; private _srcColId: string | undefined; @@ -79,10 +87,14 @@ export class LinkingState extends Disposable { const {srcSection, srcCol, srcColId, tgtSection, tgtCol, tgtColId} = linkConfig; this._docModel = docModel; this._srcSection = srcSection; + this._tgtSection = tgtSection; this._srcColId = srcColId; this._srcTableModel = docModel.dataTables[srcSection.table().tableId()]; const srcTableData = this._srcTableModel.tableData; + console.log(`============ LinkingState: Re-running constructor; tgtSec:${tgtSection.id()}: ${tgtSection.titleDef()}`); //TODO JV TEMP; + + // === IMPORTANT NOTE! (this applies throughout this file) // srcCol and tgtCol can be the "empty column" // - emptyCol.getRowId() === 0 @@ -194,13 +206,70 @@ export class LinkingState extends Disposable { // either same-table cursor-link (!srcCol && !tgtCol, so do activeRowId -> cursorPos) // or cursor-link by reference ( srcCol && !tgtCol, so do srcCol -> cursorPos) - //colVal, or rowId if no srcCol + // gets the relevant col value for the passed-in rowId, or return rowId unchanged if same-table link const srcValueFunc = this._makeValGetter(this._srcSection.table(), this._srcColId); - if (srcValueFunc) { // if makeValGetter succeeded, set up cursorPos - this.cursorPos = this.autoDispose(ko.computed(() => - srcValueFunc(srcSection.activeRowId()) as UIRowId - )); + if (srcValueFunc) { + //Incoming-cursor-pos determines what the linked cursor position should be only considering the previous + //linked section (srcSection) and all upstream sections (through srcSection.linkingState) + //does NOT take into account tgtSection, so will be out of date if tgtSection has been updated more recently + this.incomingCursorPos = this.autoDispose((ko.computed(() => { + // Note: prevLink is the link info for 2 hops behind the current (tgt) section. 1 hop back is this.srcSec; + // this.srcSec.linkingState is 2 hops back, (i.e. it reads from from this.srcSec.linkSrcSec) + const prevLink = this._srcSection.linkingState?.(); + const prevLinkHasCursor = prevLink && + (prevLink.linkTypeDescription() == "Cursor:Same-Table" || + prevLink.linkTypeDescription() == "Cursor:Reference"); + + const [prevLinkedPos, prevLinkedVersion] = prevLinkHasCursor ? prevLink.incomingCursorPos(): [null, SequenceNEVER]; + + const srcSecPos = this._srcSection.activeRowId.peek(); //we don't depend on this, only on its cursor version + const srcSecVersion = this._srcSection.lastCursorEdit(); + + // is NEVER if viewSection's cursor hasn't yet initialized (shouldn't happen?)? + //TODO JV when will this happen? do some checks. Should get set on page load / setCursorPos to saved pos + // maybe more correct to just interpret NEVER as "never updated", which is already handled correctly + if(srcSecVersion == SequenceNEVER) { + console.log("=== linkingState: cursor-linking, srcSecVersion = NEVER"); + return [null, SequenceNEVER] as [UIRowId|null, SequenceNum]; + } + + // ==== Determine whose info to use: + // If prevLinkedVersion < srcSecVersion, then the prev linked data is stale, don't use it + // If prevLinkedVersion == srcSecVersion, then srcSec is the driver for this link cycle (i.e. we're its first + // outgoing link), AND the link cycle has come all the way around + const useLinked = prevLinkHasCursor && prevLinkedVersion > srcSecVersion; + + // srcSec/prevLinkedPos is rowId from srcSec. However if "Cursor:Reference", need to follow the ref in srcCol + // srcValueFunc will get the appropriate value based on this._srcColId + const tgtCursorPos = (srcValueFunc(useLinked ? prevLinkedPos : srcSecPos) || "new") as UIRowId; + // NOTE: srcValueFunc returns 'null' if rowId is the add-row, so we coerce that back into "new" + // NOTE: cursor linking is only ever done by the id column (for same-table) or by single Ref col (cursor:ref), + // so we'll never have to worry about `null` showing up as an actual cell-value, since null refs are `0` + + return [ + tgtCursorPos, + useLinked ? prevLinkedVersion : srcSecVersion, //propagate which version our cursorPos is from + ] as [UIRowId|null, SequenceNum]; + }))); + //public readonly incomingCursorPos: ko.Computed; + + + //This is the cursorPos that's directly applied to tgtSection, should be null if incoming link is outdated + //where null means "cursorPos does not apply to tgtSection and should be ignored" + this.cursorPos = this.autoDispose(ko.computed(() => { + const [incomingPos, incomingVersion]: [UIRowId|null, SequenceNum] = this.incomingCursorPos(); + const tgtSecVersion = this._tgtSection.lastCursorEdit(); + + //if(!tgtSecVersion) { return null; } + + if(incomingVersion > tgtSecVersion) { // if linked cursor newer that current sec, use it + return incomingPos; + } else { // else, there's no linked cursor, since current section is driving the linking + return null; + } + + })); } if (!srcColId) { // If same-table cursor-link, copy getDefaultColValues from the source if possible diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index 71f8226b..9023b8b1 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -1,4 +1,5 @@ import BaseView from 'app/client/components/BaseView'; +import {SequenceNEVER, SequenceNum} from 'app/client/components/Cursor'; import {EmptyFilterColValues, LinkingState} from 'app/client/components/LinkingState'; import {KoArray} from 'app/client/lib/koArray'; import {ColumnToMapImpl} from 'app/client/models/ColumnToMap'; @@ -155,6 +156,8 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section">, RuleO activeRowId: ko.Observable; // May be null when there are no rows. + lastCursorEdit: ko.Observable; + // If the view instance for section is instantiated, it will be accessible here. viewInstance: ko.Observable; @@ -618,6 +621,7 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): this.linkTargetCol = refRecord(docModel.columns, this.linkTargetColRef); this.activeRowId = ko.observable(null); + this.lastCursorEdit = ko.observable(SequenceNEVER); this._linkingState = Holder.create(this); this.linkingState = this.autoDispose(ko.pureComputed(() => { diff --git a/app/client/ui/RightPanel.ts b/app/client/ui/RightPanel.ts index 3574fc31..aa04876d 100644 --- a/app/client/ui/RightPanel.ts +++ b/app/client/ui/RightPanel.ts @@ -644,7 +644,14 @@ export class RightPanel extends Disposable { const lstate = use(tgtSec.linkingState); const lfilter = lstate?.filterState ? use(lstate.filterState) : undefined; - const cursorPosStr = lstate?.cursorPos ? `${tgtSec.tableId()}[${use(lstate.cursorPos)}]` : "N/A"; + //TODO JV TEMP DEBUG: srcLastUpdated is temporary + const cursorPosStr = (lstate?.cursorPos ? `${tgtSec.tableId()}[${use(lstate.cursorPos)}]` : "N/A") + + `\n srclastEdited: ${use(srcSec.lastCursorEdit)} \n tgtLastEdited: ${use(tgtSec.lastCursorEdit)}` + + `\n incomingCursorPos: ${lstate?.incomingCursorPos ? `${lstate.incomingCursorPos()}` : "N/A"}` + + (() => { + //const; + return ''; + })(); //Main link info as a big string, will be in a 
 block
       let preString = "No Incoming Link";
diff --git a/app/client/ui/selectBy.ts b/app/client/ui/selectBy.ts
index a6664781..ca8629ed 100644
--- a/app/client/ui/selectBy.ts
+++ b/app/client/ui/selectBy.ts
@@ -49,8 +49,12 @@ interface LinkNode {
   groupbyColumns?: Set;
 
   // list of ids of the sections that are ancestors to this section according to the linked section
-  // relationship
-  ancestors: Set;
+  // relationship. ancestors[0] is this.section, ancestors[last] is oldest ancestor
+  ancestors: number[];
+
+  //corresponds to ancestors array, but is 1 shorter.
+  // if isAncCursLink[0] == true, that means the link from ancestors[0] to ancestors[1] is a same-table cursor-link
+  isAncestorCursorLink: boolean[];
 
   // the section record. Must be the empty record sections that are to be created.
   section: ViewSectionRec;
@@ -141,9 +145,39 @@ function isValidLink(source: LinkNode, target: LinkNode) {
     }
   }
 
-  // The link must not create a cycle
-  if (source.ancestors.has(target.section.getRowId())) {
-    return false;
+  //The link must not create a cycle, unless it's only same-table cursor-links all the way to target
+  if (source.ancestors.includes(target.section.getRowId())) {
+
+    //cycles only allowed for cursor links
+    if(source.column || target.column || source.isSummary) {
+      return false;
+    }
+
+    // Walk backwards along the chain of ancestors
+    // - if we hit a non-cursor link before reaching target, then that would be an illegal cycle
+    // - when we hit target, we've verified that this is a legal cycle, so break
+    //   (ancestors further up the hierarchy past target don't matter, since once we set target.linkSrcSec = src.sec,
+    //    they would stop being ancestors of src)
+    // NOTE: we're guaranteed to hit target before the end of the array (because of the `if` above)
+    //     but I'm paranoid so let's check and throw if it happens
+    // ALSO NOTE: isAncestorCursorLink may be 1 shorter than ancestors, but it's accounted for by the above
+
+    for(let i = 0; ; i++) {
+      //We made it! All is well
+      if(source.ancestors[i] == target.section.getRowId()) {
+        break;
+      }
+
+      //If we've hit the last ancestor and haven't found target, error out (shouldn't happen)
+      if(i == source.ancestors.length-1) { throw Error("Error: Array doesn't include targetSection"); }
+
+      //Need to keep following links back, make sure this one is cursorLink
+      if(!source.isAncestorCursorLink[i]) {
+        return false;
+      }
+    }
+    console.log("===== selectBy found valid cycle", JSON.stringify(source)); //TODO JV TEMP DEBUG
+    //Yay, this is a valid cycle of same-table cursor-links
   }
 
   return true;
@@ -224,15 +258,29 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     return [];
   }
   const table = section.table.peek();
-  const ancestors = new Set();
+  const ancestors: number[] = [];
+
+  const isAncestorCursorLink: boolean[] = [];
 
   for (let sec = section; sec.getRowId(); sec = sec.linkSrcSection.peek()) {
-    if (ancestors.has(sec.getRowId())) {
+    if (ancestors.includes(sec.getRowId())) {
       // tslint:disable-next-line:no-console
-      console.warn(`Links should not create a cycle - section ids: ${Array.from(ancestors)}`);
+      console.warn(`Links should not create a cycle - section ids: ${ancestors}`);
+      //TODO JV: change this to only warn if cycles aren't all Cursor:Same-Table
       break;
     }
-    ancestors.add(sec.getRowId());
+    ancestors.push(sec.getRowId());
+
+    //isAncestorCursorLink may be 1 shorter than ancestors, since last ancestor has no incoming link
+    // however if we have a cycle (of cursor-links), then they'll be the same length
+    if(sec.linkSrcSection.peek().getRowId()) {
+      //TODO JV TEMP: Dear god determining if something is a cursor link or not is a nightmare
+      const srcCol = sec.linkSrcCol.peek().getRowId();
+      const tgtCol = sec.linkTargetCol.peek().getRowId();
+      const srcTable = sec.linkSrcSection.peek().table.peek();
+      const srcIsSummary = srcTable.primaryTableId.peek() !== srcTable.tableId.peek();
+      isAncestorCursorLink.push(srcCol == 0 && tgtCol == 0 && !srcIsSummary);
+    }
   }
 
   const isSummary = table.primaryTableId.peek() !== table.tableId.peek();
@@ -243,6 +291,7 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     groupbyColumns: isSummary ? table.summarySourceColRefs.peek() : undefined,
     widgetType: section.parentKey.peek(),
     ancestors,
+    isAncestorCursorLink,
     section,
   };
 
@@ -280,7 +329,8 @@ function fromPageWidget(docModel: DocModel, pageWidget: IPageWidget): LinkNode[]
     // (e.g.: link from summary table with Attachments in group-by) but it seems to work fine as is
     groupbyColumns,
     widgetType: pageWidget.type,
-    ancestors: new Set(),
+    ancestors: [],
+    isAncestorCursorLink: [],
     section: docModel.viewSections.getRowModel(pageWidget.section),
   };