diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index b48c0f38..2857ef60 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -17,15 +17,20 @@ function nullAsUndefined(value: T|null|undefined): T|undefined { } // ================ 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() +// 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; +export const SequenceNEVER: SequenceNum = 0; // Cursors will start here let latestGlobalSequenceNum = SequenceNEVER; -const nextSequenceNum = () => { return latestGlobalSequenceNum++; }; +function nextSequenceNum() { // First call to this func should return 1 + latestGlobalSequenceNum++; + return latestGlobalSequenceNum; +} -// Note: we don't make any provisions for handling overflow. It's fine because: +// 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 @@ -127,6 +132,10 @@ export class Cursor extends Disposable { // 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(); })); @@ -156,26 +165,28 @@ export class Cursor extends Disposable { * @param isFromLink: should be set if this is a cascading update from cursor-linking */ public setCursorPos(cursorPos: CursorPos, isFromLink: boolean = false): void { - //If updating as a result of links, we want to NOT update lastEditedAt - if(isFromLink) { 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) { - 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); + 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); + } + + } finally { // Make sure we reset this even on error + this._silentUpdatesFlag = false; } - //console.log(`CURSOR-END: silentUpdate=${this._silentUpdatesFlag}, - // lastEditedAt = ${this._lastEditedAt.peek()} `); //TODO JV DEBUG TEMP - this._silentUpdatesFlag = false; } @@ -185,13 +196,13 @@ export class Cursor extends Disposable { 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) + // 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()); } } } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index a5c4a03e..b7e23928 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -1135,7 +1135,7 @@ export class GristDoc extends DisposableWithEvents { } if (visitedSections.includes(section.id.peek())) { - //We've already been here (we hit a cycle), just return immediately + // We've already been here (we hit a cycle), just return immediately return true; } diff --git a/app/client/components/LinkingState.ts b/app/client/components/LinkingState.ts index 305ff24a..115e127a 100644 --- a/app/client/components/LinkingState.ts +++ b/app/client/components/LinkingState.ts @@ -1,3 +1,4 @@ +import {SequenceNEVER, SequenceNum} from "app/client/components/Cursor"; import {DataRowModel} from "app/client/models/DataRowModel"; import DataTableModel from "app/client/models/DataTableModel"; import {DocModel} from 'app/client/models/DocModel'; @@ -17,7 +18,6 @@ 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 @@ -211,7 +211,7 @@ export class LinkingState extends Disposable { // [ A ]--------/ [ B ] --------------/ [ C ] | // A.actRowId B.actRowId | // - // However, if e.g. viewSec B is filtered, the correct rowId might not exist in B, and so it's rowId would be + // However, if e.g. viewSec B is filtered, the correct rowId might not exist in B, and so its activeRowId would be // on a different row, and therefore the cursor linking would set C to a different row from A, even if it existed // in C // @@ -239,9 +239,10 @@ export class LinkingState extends Disposable { // viewSection.lastCursorEdit). incomingCursorPos is a pair of [rowId, sequenceNum], so each linkingState sets its // incomingCursorPos to whichever is most recent between its srcSection, and the previous LS's incCursPos. // - // The end result is that there is that because the lastCursorEdits are guaranteed to be unique, there is always - // a stable configuration of links, where even in the case of a cycle the incomingCursorPos will all take their - // rowId and version from the most recently edited viewSection in the cycle, which is what the user expects + // If we do this right, the end result is that because the lastCursorEdits are guaranteed to be unique, + // there is always a stable configuration of links, where even in the case of a cycle the incomingCursorPos-es + // will all take their rowId and version from the most recently edited viewSection in the cycle, + // which is what the user expects // // ...from C--> [A.LS] --------> [B.LS] --> [C.LS] ----->...to A | // | | / | | @@ -252,8 +253,10 @@ export class LinkingState extends Disposable { // Once the incomingCursorPos-es are determined correctly, the cursorPos-es just need to pull out the rowId, // and that will drive the cursors of the associated tgt section for each LS. // - // NOTE: setting cursorPos will change the viewSections' cursor, but it's special-cased to - // not affect their lastCursorEdit times. + // NOTE: setting cursorPos *WILL* change the viewSections' cursor, but it's special-cased to + // so that cursor-driven linking doesn't modify their lastCursorEdit times, so that lastCursorEdit + // reflects only changes driven by external factors + // (e.g. page load, user moving cursor, user changing linking settings/filter settings) // ============================= // gets the relevant col value for the passed-in rowId, or return rowId unchanged if same-table link @@ -264,25 +267,31 @@ export class LinkingState extends Disposable { //Incoming-cursor-pos determines what the linked cursor position should be, considering the previous //linked section (srcSection) and all upstream sections (through srcSection.linkingState) this.incomingCursorPos = this.autoDispose((ko.computed(() => { - // Get previous linkingstate's info - // NOTE: prevlink = this.srcSec.linkingState is 2 hops back, (i.e. it reads from from this.srcSec.linkSrcSec) - // The section 1 link back from the current (tgt) section is this.srcSec; - 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]; - // Get srcSection's info + // NOTE: This computed primarily decides between srcSec and prevLink. Here's what those mean: + // e.g. consider sections A->B->C, (where this === C) + // We need to decide between taking cursor info from B, our srcSection (1 hop back) + // vs taking cursor info from further back, e.g. A, or before (2+ hops back) + // To take cursor info from further back, we rely on B's linkingState, since B's linkingState will + // be looking at the preceding sections, either A or whatever is behind A. + // Therefore: we either use srcSection (1 back), or prevLink = srcSection.linkingState (2+ back) + + // Get srcSection's info (1 hop back) const srcSecPos = this._srcSection.activeRowId.peek(); //we don't depend on this, only on its cursor version const srcSecVersion = this._srcSection.lastCursorEdit(); - // Sanity check: version might be NEVER if viewSection's cursor hasn't initialized (shouldn't happen I think) - if(srcSecVersion == SequenceNEVER) { - console.warn("=== linkingState: cursor-linking, srcSecVersion = NEVER"); + // If cursors haven't been initialized, cursor-linking doesn't make sense, so don't do it + if(srcSecVersion === SequenceNEVER) { return [null, SequenceNEVER] as [UIRowId|null, SequenceNum]; } + // Get previous linkingstate's info, if applicable (2 or more hops back) + 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]; + // ==== 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 diff --git a/app/client/ui/RightPanel.ts b/app/client/ui/RightPanel.ts index e892fac7..6da909a0 100644 --- a/app/client/ui/RightPanel.ts +++ b/app/client/ui/RightPanel.ts @@ -645,8 +645,8 @@ export class RightPanel extends Disposable { const lfilter = lstate?.filterState ? use(lstate.filterState) : undefined; // Debug info for cursor linking - const inPos = lstate?.incomingCursorPos?.(); - const cursorPosStr = (lstate?.cursorPos ? `${tgtSec.tableId()}[${use(lstate.cursorPos)}]` : "N/A") + + 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)}` + diff --git a/app/client/ui/selectBy.ts b/app/client/ui/selectBy.ts index 3e197ef3..5b23e405 100644 --- a/app/client/ui/selectBy.ts +++ b/app/client/ui/selectBy.ts @@ -52,9 +52,11 @@ interface LinkNode { // relationship. ancestors[0] is this.section, ancestors[length-1] is oldest ancestor ancestors: number[]; - // Records which links between ancestors are same-table cursor-links - // if isAncCursLink[0] == true, that means the link from ancestors[0] to ancestors[1] is a same-table cursor-link - // NOTE: (it is therefore 1 shorter than ancestors, unless the links form a cycle in which case it's the same len) + // 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. @@ -149,8 +151,8 @@ function isValidLink(source: LinkNode, target: LinkNode) { // 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) { + // cycles only allowed for cursor links + if (source.column || target.column || source.isSummary) { return false; } @@ -161,19 +163,20 @@ function isValidLink(source: LinkNode, target: LinkNode) { // they would stop being ancestors of src) // NOTE: we're guaranteed to hit target before the end of the array (because of the `if(...includes...)` above) // ALSO NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, but it's accounted for by the above - - for(let i = 0; ; i++) { - if(source.ancestors[i] == target.section.getRowId()) { + let i = 0 + while(true) { + if (source.ancestors[i] == target.section.getRowId()) { // We made it! All is well break; } // 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-1) { throw Error("Error: Array doesn't include targetSection"); } + if (i == source.ancestors.length-1) { throw Error("Array doesn't include targetSection"); } - // Need to follow another link back, verify that it's a same-table cursor-link - if(!source.isAncestorSameTableCursorLink[i]) { return false; } + // We're not done with the cycle. Check next link, if it's non-same-table-cursor-link, the cycle is invalid. + if (!source.isAncestorSameTableCursorLink[i]) { return false; } + + i++; // Walk back } - console.log("===== selectBy found valid cycle", JSON.stringify(source)); //TODO JV TEMP DEBUG // Yay, this is a valid cycle of same-table cursor-links } @@ -269,15 +272,16 @@ function fromViewSectionRec(section: ViewSectionRec): LinkNode[] { ancestors.push(sec.getRowId()); //Determine if this link is a same-table cursor link - if(sec.linkSrcSection.peek().getRowId()) { + 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); + isAncestorSameTableCursorLink.push(srcCol === 0 && tgtCol === 0 && !srcIsSummary); } - // NOTE: isAncestorSameTableCursorLink 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 + // 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(); diff --git a/test/nbrowser/RightPanelSelectBy.ts b/test/nbrowser/RightPanelSelectBy.ts index a78ce60a..e53a98ae 100644 --- a/test/nbrowser/RightPanelSelectBy.ts +++ b/test/nbrowser/RightPanelSelectBy.ts @@ -78,7 +78,7 @@ describe('RightPanelSelectBy', function() { it('should allow creating cursor-linked-cycles', async function() { assert.equal(await driver.findContent('.test-select-row', /Performances detail/).isPresent(), true); - // undo, link is expected to be unset for next test + // undo, the operation from the previous test; link is expected to be unset for next test await gu.undo(); });