Bidirectional Linking

app/client/components/BaseView.js

@@ -132,7 +132,7 @@ 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'});
+      this.setCursorPos({rowId: rowId || 'new'}, true);
     }
   }));
 
@@ -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. isFromLink will be set when called as result of cursor linking(see Cursor.setCursorPos for info)
  */
-BaseView.prototype.setCursorPos = function(cursorPos) {
+BaseView.prototype.setCursorPos = function(cursorPos, isFromLink = false) {
   if (this.isDisposed()) {
     return;
   }
   if (!this._isLoading.peek()) {
-    this.cursor.setCursorPos(cursorPos);
+    this.cursor.setCursorPos(cursorPos, isFromLink);
   } else {
     // This is the first step; the second happens in onTableLoaded.
     this._pendingCursorPos = cursorPos;

app/client/components/Cursor.ts

@@ -16,6 +16,26 @@ function nullAsUndefined<T>(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 more clear
+// Cursors are constructed at SequenceNEVER (0). After that, changes to their sequenceNum will go through
+// NextSequenceNum(), so they'll have unique, monotonically increasing numbers for their lastEditedAt()
+// NOTE: (by the time the page loads they'll already be at nonzero numbers, the never is intended to be transient)
+export type SequenceNum = number;
+export const SequenceNEVER: SequenceNum = 0; // Cursors will start here
+let latestGlobalSequenceNum = SequenceNEVER;
+function nextSequenceNum() { // First call to this func should return 1
+  latestGlobalSequenceNum++;
+  return latestGlobalSequenceNum;
+}
+
+// NOTE: If latestGlobalSequenceNum overflows, I think it would stop incrementing because of floating point imprecision
+// However, we don't need to worry about overflow 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.
@@ -63,6 +83,14 @@ export class Cursor extends Disposable {
 
   private _properRowId: ko.Computed<UIRowId|null>;
 
+  // lastEditedAt is updated on _properRowId or fieldIndex update (including through setCursorPos)
+  // Used to determine which section takes priority for cursorLinking (specifically cycles/bidirectional linking)
+  private _lastEditedAt: ko.Observable<SequenceNum>;
+  // _silentUpdatesFlag prevents lastEditedAt from being updated, when a change in cursorPos isn't driven by the user.
+  // It's used when cursor linking calls setCursorPos, so that linked cursor moves don't trample lastEditedAt.
+  // WARNING: the flag approach relies on ko observables being resolved synchronously, may break if changed to grainjs?
+  private _silentUpdatesFlag: boolean = false;
+
   constructor(baseView: BaseView, optCursorPos?: CursorPos) {
     super();
     optCursorPos = optCursorPos || {};
@@ -83,6 +111,7 @@ export class Cursor extends Disposable {
     }));
 
     this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0);
+
     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 +122,22 @@ 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)));
+
+    // Update the cursor edit time if either the row or column change
+    // IMPORTANT: need to subscribe AFTER the properRowId->activeRowId subscription.
+    //  (Cursor-linking observables depend on lastCursorEdit, but only peek at activeRowId. Therefore, updating the
+    //   edit time triggers a re-read of activeRowId, and swapping the order will read stale values for rowId)
+    // NOTE: this may update sequence number twice for a single edit, but this shouldn't cause any issues.
+    //       For determining priority, this cursor will become the latest edited whether we call it once or twice.
+    //       For updating observables, the double-update might cause cursor-linking observables in LinkingState to
+    //       double-update, but it should be transient and get resolved immediately.
+    this.autoDispose(this._properRowId.subscribe(() => { this._cursorEdited(); }));
+    this.autoDispose(this.fieldIndex.subscribe(() => { this._cursorEdited(); }));
 
     // On dispose, save the current cursor position to the section model.
     this.onDispose(() => { baseView.viewSection.lastCursorPos = this.getCursorPos(); });
@@ -116,23 +159,65 @@ export class Cursor extends Disposable {
   /**
    * Moves the cursor to the given position. Only moves the row if rowId or rowIndex is valid,
    * preferring rowId.
+   *
+   * isFromLink prevents lastEditedAt from being updated, so lastEdit reflects only user-driven edits
    * @param cursorPos: Position as { rowId?, rowIndex?, fieldIndex? }, as from getCursorPos().
+   * @param isFromLink: should be set if this is a cascading update from cursor-linking
    */
-  public setCursorPos(cursorPos: CursorPos): void {
-    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) {
-      this.rowIndex(cursorPos.rowIndex);
-    } else {
-      // Write rowIndex to itself to force an update of rowId if needed.
-      this.rowIndex(this.rowIndex.peek());
-    }
-    if (cursorPos.fieldIndex !== undefined) {
-      this.fieldIndex(cursorPos.fieldIndex);
+  public setCursorPos(cursorPos: CursorPos, isFromLink: boolean = false): void {
+
+    try {
+      // If updating as a result of links, we want to NOT update lastEditedAt
+      if (isFromLink) { this._silentUpdatesFlag = true; }
+
+      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) {
+        this.rowIndex(cursorPos.rowIndex);
+      } else {
+        // Write rowIndex to itself to force an update of rowId if needed.
+        this.rowIndex(this.rowIndex.peek());
+      }
+
+      if (cursorPos.fieldIndex !== undefined) {
+        this.fieldIndex(cursorPos.fieldIndex);
+      }
+
+      // NOTE: _cursorEdited
+      // We primarily update cursorEdited counter from a this._properRowId.subscribe(), since that catches updates
+      //   from many sources (setCursorPos, arrowKeys, save/load, filter/sort-changes, etc)
+      // However, there's some cases where we user touches a section and properRowId doesn't change. Obvious one is
+      //   clicking in a section on the cell the cursor is already on. This doesn't change the cursor position, but it
+      //   SHOULD still update cursors to use that section as most up-to-date (user just clicked on a cell!), so we do
+      //   it here. (normally is minor issue, but can matter when a section has rows filtered out so cursors desync)
+      // Also a more subtle case: when deleting a row with several sections linked together, properRowId can fail to
+      //   update. When GridView.deleteRows calls setCursorPos to keep cursor from jumping after delete, the observable
+      //   doesn't trigger cursorEdited(), because (I think) _properRowId has already been updated that cycle.
+      //   This caused a bug when several viewSections were cursor-linked to each other and a row was deleted
+      // NOTE: Calling it explicitly here will cause cursorEdited to be called twice sometimes,
+      //   but that shouldn't cause any problems, since we don't care about edit counts, just who was edited latest.
+      this._cursorEdited();
+
+    } finally { // Make sure we reset this even on error
+      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
+  private _cursorEdited(): void {
+    // If updating as a result of links, we want to NOT update lastEdited
+    if (!this._silentUpdatesFlag)
+      { this._lastEditedAt(nextSequenceNum()); }
+  }
 }

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<boolean> {
+    silent: boolean = false,
+    visitedSections: number[] = []): Promise<boolean> {
     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
@@ -1181,7 +1188,7 @@ export class GristDoc extends DisposableWithEvents {
         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();