(core) updates from grist-core

app/client/ui/RightPanel.ts

@@ -643,7 +643,13 @@ 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";
+      // Debug info for cursor linking
+      const inPos = lstate?.incomingCursorPos ? use(lstate.incomingCursorPos) : null;
+      const cursorPosStr = (lstate?.cursorPos ? `${use(tgtSec.tableId)}[${use(lstate.cursorPos)}]` : "N/A") +
+      // TODO: the lastEdited and incomingCursorPos is kinda technical, to do with how bidirectional linking determines
+      //       priority for cyclical cursor links. Might be too technical even for the "advanced info" box
+        `\n srclastEdited: T+${use(srcSec.lastCursorEdit)} \n tgtLastEdited: T+${use(tgtSec.lastCursorEdit)}` +
+        `\n incomingCursorPos: ${inPos ? `${inPos[0]}@T+${inPos[1]}` : "N/A"}`;
 
       //Main link info as a big string, will be in a <pre></pre> block
       let preString = "No Incoming Link";

app/client/ui/selectBy.ts

@@ -49,8 +49,15 @@ interface LinkNode {
   groupbyColumns?: Set<number>;
 
   // list of ids of the sections that are ancestors to this section according to the linked section
-  // relationship
-  ancestors: Set<number>;
+  // relationship. ancestors[0] is this.section, ancestors[length-1] is oldest ancestor
+  ancestors: number[];
+
+  // For bidirectional linking, cycles are only allowed if all links on that cycle are same-table cursor-link
+  // this.ancestors only records what the ancestors are, but we need to record info about the edges between them.
+  // isAncCursLink[i]==true  means the link from ancestors[i] to ancestors[i+1] is a same-table cursor-link
+  // NOTE: (Since ancestors is a list of nodes, and this is a list of the edges between those nodes, this list will
+  //        be 1 shorter than ancestors (if there's no cycle), or will be the same length (if there is a cycle))
+  isAncestorSameTableCursorLink: boolean[];
 
   // the section record. Must be the empty record sections that are to be created.
   section: ViewSectionRec;
@@ -141,9 +148,51 @@ 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;
+    }
+
+    // We know our ancestors cycle back around to ourselves
+    // - lets walk back along the cyclic portion of the ancestor chain and verify that each link in that chain is
+    //   a cursor-link
+
+    // e.g. if the current link graph is:
+    //                     A->B->TGT->C->D->SRC
+    //    (SRC.ancestors):[5][4] [3] [2][1] [0]
+    // We're verifying the new potential link SRC->TGT, which would turn the graph into:
+    //             [from SRC] -> TGT -> C -> D -> SRC -> [to TGT]
+    // (Note that A and B will be cut away, since we change TGT's link source)
+    //
+    // We need to make sure that each link going backwards from `TGT -> C -> D -> SRC` is a same-table-cursor-link,
+    // since we disallow cycles with other kinds of links.
+    // isAncestorCursorLink[i] will tell us if the link going into ancestors[i] is a same-table-cursor-link
+    // So before we step from i=0 (SRC) to i=1 (D), we check isAncestorCursorLink[0], which tells us about D->SRC
+    let i;
+    for (i = 0; i < source.ancestors.length; i++) { // Walk backwards through the ancestors
+
+      // Once we hit the target section, we've seen all links that will be part of the cycle, and they've all been valid
+      if (source.ancestors[i] == target.section.getRowId()) {
+        break; // Success!
+      }
+
+      // Check that the link to the preceding section is valid
+      // NOTE! isAncestorSameTableCursorLink could be 1 shorter than ancestors!
+      // (e.g. if the graph looks like A->B->C, there's 3 ancestors but only two links)
+      // (however, if there's already a cycle, they'll be the same length ( [from C]->A->B->C, 3 ancestors & 3 links)
+      // If the link doesn't exist (shouldn't happen?) OR the link is not same-table-cursor, the cycle is invalid
+      if (i >= source.isAncestorSameTableCursorLink.length ||
+          !source.isAncestorSameTableCursorLink[i]) { return false; }
+    }
+
+    // If we've hit the last ancestor and haven't found target, error out (shouldn't happen!, we checked for it)
+    if (i == source.ancestors.length) { throw Error("Array doesn't include targetSection"); }
+
+
+    // Yay, this is a valid cycle of same-table cursor-links
   }
 
   return true;
@@ -224,15 +273,30 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     return [];
   }
   const table = section.table.peek();
-  const ancestors = new Set<number>();
+  const ancestors: number[] = [];
+
+  const isAncestorSameTableCursorLink: boolean[] = [];
 
   for (let sec = section; sec.getRowId(); sec = sec.linkSrcSection.peek()) {
-    if (ancestors.has(sec.getRowId())) {
-      // tslint:disable-next-line:no-console
-      console.warn(`Links should not create a cycle - section ids: ${Array.from(ancestors)}`);
+    if (ancestors.includes(sec.getRowId())) {
+      // There's a cycle in the existing link graph
+      // TODO if we're feeling fancy, can test here whether it's an all-same-table cycle and warn if not
+      //      but there's other places we check for that
       break;
     }
-    ancestors.add(sec.getRowId());
+    ancestors.push(sec.getRowId());
+
+    //Determine if this link is a same-table cursor link
+    if (sec.linkSrcSection.peek().getRowId()) { // if sec has incoming link
+      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();
+      isAncestorSameTableCursorLink.push(srcCol === 0 && tgtCol === 0 && !srcIsSummary);
+    }
+    // NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, since we might skip pushing
+    // when we hit the last ancestor (which has no incoming link)
+    // however if we have a cycle (of cursor-links), then they'll be the same length, because we won't skip last push
   }
 
   const isSummary = table.primaryTableId.peek() !== table.tableId.peek();
@@ -243,6 +307,7 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] {
     groupbyColumns: isSummary ? table.summarySourceColRefs.peek() : undefined,
     widgetType: section.parentKey.peek(),
     ancestors,
+    isAncestorSameTableCursorLink,
     section,
   };
 
@@ -280,7 +345,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: [],
+    isAncestorSameTableCursorLink: [],
     section: docModel.viewSections.getRowModel(pageWidget.section),
   };