(core) Fixing cursor position for filtered linked section.

Summary:
In a selector table, when a selected row is filtered out of view, linked widgets should update based on the newly selected row.
There were a few bugs that contributed to this wrong behavior:
- Gridview wasn't subscribing to the current row id, and the row with id 'new' was being converted to the first row
- Cursor was keeping track of the currently selected row id, it was hiding a problem behind the proper rowIndex
- Undo/redo somehow leveraged the wrong rowId from the cursor during the position restore.

The `No data` text was also changed to be more meaningful.

Test Plan: Added and updated.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3937

app/client/components/ActionLog.ts

@@ -78,12 +78,13 @@ export class ActionLog extends dispose.Disposable implements IDomComponent {
     this._displayStack = koArray<ActionGroupWithState>();
 
     // Computed for the tableId of the table currently being viewed.
-    if (!this._gristDoc) {
-      this._selectedTableId = this.autoDispose(ko.computed(() => ""));
-    } else {
-      this._selectedTableId = this.autoDispose(ko.computed(
-        () => this._gristDoc!.viewModel.activeSection().table().tableId()));
-    }
+    this._selectedTableId = this.autoDispose(ko.computed(() => {
+      if (!this._gristDoc || this._gristDoc.viewModel.isDisposed()) { return ""; }
+      const section = this._gristDoc.viewModel.activeSection();
+      if (!section || section.isDisposed()) { return ""; }
+      const table = section.table();
+      return table && !table.isDisposed() ? table.tableId() : "";
+    }));
   }
 
   public buildDom() {

app/client/components/CopySelection.ts

@@ -1,7 +1,6 @@
 import type {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec';
 import type {CellValue} from 'app/common/DocActions';
-import type {TableData} from 'app/common/TableData';
-import type {UIRowId} from 'app/common/UIRowId';
+import type {TableData, UIRowId} from 'app/common/TableData';
 
 /**
  * The CopySelection class is an abstraction for a subset of currently selected cells.

app/client/components/Cursor.ts

@@ -8,12 +8,12 @@ import BaseView from 'app/client/components/BaseView';
 import * as commands from 'app/client/components/commands';
 import BaseRowModel from 'app/client/models/BaseRowModel';
 import {LazyArrayModel} from 'app/client/models/DataTableModel';
-import type {RowId} from 'app/client/models/rowset';
+import type {UIRowId} from 'app/common/TableData';
 import {Disposable} from 'grainjs';
 import * as ko from 'knockout';
 
 export interface CursorPos {
-  rowId?: RowId;
+  rowId?: UIRowId;
   rowIndex?: number;
   fieldIndex?: number;
   sectionId?: number;
@@ -60,7 +60,7 @@ export class Cursor extends Disposable {
   public rowIndex: ko.Computed<number|null>;     // May be null when there are no rows.
   public fieldIndex: ko.Observable<number>;
 
-  private _rowId: ko.Observable<RowId|null>;     // May be null when there are no rows.
+  private _rowId: ko.Observable<UIRowId|null>;     // May be null when there are no rows.
 
   // The cursor's _rowId property is always fixed across data changes. When isLive is true,
   // the rowIndex of the cursor is recalculated to match _rowId. When false, they will
@@ -68,13 +68,15 @@ export class Cursor extends Disposable {
   private _isLive: ko.Observable<boolean> = ko.observable(true);
   private _sectionId: ko.Computed<number>;
 
+  private _properRowId: ko.Computed<UIRowId|null>;
+
   constructor(baseView: BaseView, optCursorPos?: CursorPos) {
     super();
     optCursorPos = optCursorPos || {};
     this.viewData = baseView.viewData;
 
     this._sectionId = this.autoDispose(ko.computed(() => baseView.viewSection.id()));
-    this._rowId = ko.observable<RowId|null>(optCursorPos.rowId || 0);
+    this._rowId = ko.observable<UIRowId|null>(optCursorPos.rowId || 0);
     this.rowIndex = this.autoDispose(ko.computed({
       read: () => {
         if (!this._isLive()) { return this.rowIndex.peek(); }
@@ -82,7 +84,7 @@ export class Cursor extends Disposable {
         return rowId == null ? null : this.viewData.clampIndex(this.viewData.getRowIndexWithSub(rowId));
       },
       write: (index) => {
-        const rowIndex = this.viewData.clampIndex(index!);
+        const rowIndex = index === null ? null : this.viewData.clampIndex(index);
         this._rowId(rowIndex == null ? null : this.viewData.getRowId(rowIndex));
       },
     }));
@@ -90,8 +92,16 @@ export class Cursor extends Disposable {
     this.fieldIndex = baseView.viewSection.viewFields().makeLiveIndex(optCursorPos.fieldIndex || 0);
     this.autoDispose(commands.createGroup(Cursor.editorCommands, this, baseView.viewSection.hasFocus));
 
-    // Update the section's activeRowId when the cursor's rowId changes.
-    this.autoDispose(this._rowId.subscribe((rowId) => baseView.viewSection.activeRowId(rowId)));
+    // RowId might diverge from the one stored in _rowId when the data changes (it is filtered out). So here
+    // we will calculate rowId based on rowIndex (so in reverse order), to have a proper value.
+    this._properRowId = this.autoDispose(ko.computed(() => {
+      const rowIndex = this.rowIndex();
+      const rowId = rowIndex === null ? null : this.viewData.getRowId(rowIndex);
+      return rowId;
+    }));
+
+    // Update the section's activeRowId when the cursor's rowIndex is changed.
+    this.autoDispose(this._properRowId.subscribe((rowId) => baseView.viewSection.activeRowId(rowId)));
 
     // On dispose, save the current cursor position to the section model.
     this.onDispose(() => { baseView.viewSection.lastCursorPos = this.getCursorPos(); });
@@ -103,7 +113,7 @@ export class Cursor extends Disposable {
   // Returns the cursor position with rowId, rowIndex, and fieldIndex.
   public getCursorPos(): CursorPos {
     return {
-      rowId: nullAsUndefined(this._rowId()),
+      rowId: nullAsUndefined(this._properRowId()),
       rowIndex: nullAsUndefined(this.rowIndex()),
       fieldIndex: this.fieldIndex(),
       sectionId: this._sectionId()
@@ -117,7 +127,7 @@ export class Cursor extends Disposable {
    */
   public setCursorPos(cursorPos: CursorPos): void {
     if (cursorPos.rowId !== undefined && this.viewData.getRowIndex(cursorPos.rowId) >= 0) {
-      this._rowId(cursorPos.rowId);
+      this.rowIndex(this.viewData.getRowIndex(cursorPos.rowId) );
     } else if (cursorPos.rowIndex !== undefined && cursorPos.rowIndex >= 0) {
       this.rowIndex(cursorPos.rowIndex);
     } else {

app/client/components/GridView.js

@@ -1233,9 +1233,11 @@ GridView.prototype.buildDom = function() {
         kd.style("width", ROW_NUMBER_WIDTH + 'px'),
         dom('div.gridview_data_row_info',
           kd.toggleClass('linked_dst', () => {
+            const myRowId = row.id();
+            const linkedRowId = self.linkedRowId();
             // Must ensure that linkedRowId is not null to avoid drawing on rows whose
             // row ids are null.
-            return self.linkedRowId() && self.linkedRowId() === row.getRowId();
+            return linkedRowId && linkedRowId === myRowId;
           })
         ),
         kd.text(function() { return row._index() + 1; }),

app/client/components/GristDoc.ts

@@ -476,6 +476,7 @@ export class GristDoc extends DisposableWithEvents {
       const viewId = toKo(ko, this.activeViewId)();
       if (!isViewDocPage(viewId)) { return null; }
       const section = this.viewModel.activeSection();
+      if (section?.isDisposed()) { return null; }
       const view = section.viewInstance();
       return view;
     })));
@@ -620,6 +621,11 @@ export class GristDoc extends DisposableWithEvents {
   public async setCursorPos(cursorPos: CursorPos) {
     if (cursorPos.sectionId && cursorPos.sectionId !== this.externalSectionId.get()) {
       const desiredSection: ViewSectionRec = this.docModel.viewSections.getRowModel(cursorPos.sectionId);
+      // If the section id is 0, the section doesn't exist (can happen during undo/redo), and should
+      // be fixed there. For now ignore it, to not create empty sections or views (peeking a view will create it).
+      if (!desiredSection.id.peek()) {
+        return;
+      }
       // If this is completely unknown section (without a parent), it is probably an import preview.
       if (!desiredSection.parentId.peek() && !desiredSection.isRaw.peek()) {
         const view = desiredSection.viewInstance.peek();

app/client/components/LinkingState.ts

@@ -4,7 +4,7 @@ import {DocModel} from 'app/client/models/DocModel';
 import {ColumnRec} from "app/client/models/entities/ColumnRec";
 import {TableRec} from "app/client/models/entities/TableRec";
 import {ViewSectionRec} from "app/client/models/entities/ViewSectionRec";
-import {RowId} from "app/client/models/rowset";
+import {UIRowId} from "app/common/TableData";
 import {LinkConfig} from "app/client/ui/selectBy";
 import {ClientQuery, QueryOperation} from "app/common/ActiveDocAPI";
 import {isList, isListType, isRefListType} from "app/common/gristTypes";
@@ -62,7 +62,7 @@ export type FilterColValues = Pick<ClientQuery, "filters" | "operations">;
  */
 export class LinkingState extends Disposable {
   // If linking affects target section's cursor, this will be a computed for the cursor rowId.
-  public readonly cursorPos?: ko.Computed<RowId>;
+  public readonly cursorPos?: ko.Computed<UIRowId>;
 
   // If linking affects filtering, this is a computed for the current filtering state, as a
   // {[colId]: colValues} mapping, with a dependency on srcSection.activeRowId()
@@ -136,7 +136,7 @@ export class LinkingState extends Disposable {
       const srcValueFunc = srcColId ? this._makeSrcCellGetter() : identity;
       if (srcValueFunc) {
         this.cursorPos = this.autoDispose(ko.computed(() =>
-          srcValueFunc(srcSection.activeRowId()) as RowId
+          srcValueFunc(srcSection.activeRowId()) as UIRowId
         ));
       }
 
@@ -172,7 +172,7 @@ export class LinkingState extends Disposable {
 
   // Value for this.filterColValues filtering based on a single column
   private _simpleFilter(
-    colId: string, operation: QueryOperation, valuesFunc: (rowId: RowId|null) => any[]
+    colId: string, operation: QueryOperation, valuesFunc: (rowId: UIRowId|null) => any[]
   ): ko.Computed<FilterColValues> {
     return this.autoDispose(ko.computed(() => {
       const srcRowId = this._srcSection.activeRowId();
@@ -226,7 +226,7 @@ export class LinkingState extends Disposable {
     if (!srcCellObs) {
       return null;
     }
-    return (rowId: RowId | null) => {
+    return (rowId: UIRowId | null) => {
       srcRowModel.assign(rowId);
       if (rowId === 'new') {
         return 'new';

app/client/components/buildViewSectionDom.ts

@@ -1,6 +1,7 @@
 import BaseView from 'app/client/components/BaseView';
 import {GristDoc} from 'app/client/components/GristDoc';
 import {ViewRec, ViewSectionRec} from 'app/client/models/DocModel';
+import {makeT} from 'app/client/lib/localization';
 import {filterBar} from 'app/client/ui/FilterBar';
 import {cssIcon} from 'app/client/ui/RightPanelStyles';
 import {makeCollapsedLayoutMenu} from 'app/client/ui/ViewLayoutMenu';
@@ -13,6 +14,7 @@ import {menu} from 'app/client/ui2018/menus';
 import {Computed, dom, DomElementArg, Observable, styled} from 'grainjs';
 import {defaultMenuOptions} from 'popweasel';
 
+const t = makeT('ViewSection');
 
 export function buildCollapsedSectionDom(options: {
   gristDoc: GristDoc,
@@ -69,8 +71,13 @@ export function buildViewSectionDom(options: {
 
   // Creating normal section dom
   const vs: ViewSectionRec = gristDoc.docModel.viewSections.getRowModel(sectionRowId);
+  const selectedBySectionTitle = Computed.create(null, (use) => {
+    if (!use(vs.linkSrcSectionRef)) { return null; }
+    return use(use(vs.linkSrcSection).titleDef);
+  });
   return dom('div.view_leaf.viewsection_content.flexvbox.flexauto',
     testId(`viewlayout-section-${sectionRowId}`),
+    dom.autoDispose(selectedBySectionTitle),
     !options.isResizing ? dom.autoDispose(isResizing) : null,
     cssViewLeaf.cls(''),
     cssViewLeafInactive.cls('', (use) => !vs.isDisposed() && !use(vs.hasFocus)),
@@ -96,10 +103,14 @@ export function buildViewSectionDom(options: {
       dom('div.view_data_pane_container.flexvbox',
         cssResizing.cls('', isResizing),
         dom.maybe(viewInstance.disableEditing, () =>
-          dom('div.disable_viewpane.flexvbox', 'No data')
+          dom('div.disable_viewpane.flexvbox',
+            dom.domComputed(selectedBySectionTitle, (title) => title
+              ? t(`No row selected in {{title}}`, {title})
+              : t('No data')),
+          )
         ),
         dom.maybe(viewInstance.isTruncated, () =>
-          dom('div.viewsection_truncated', 'Not all data is shown')
+          dom('div.viewsection_truncated', t('Not all data is shown'))
         ),
         dom.cls((use) => 'viewsection_type_' + use(vs.parentKey)),
         viewInstance.viewPane