(core) Restoring cursor position on raw data views

Summary:
This diff introduces cursor features for raw data views:
- Restoring cursor position when the browser window is reloaded
- Restoring the last edit position when the browser window is reloaded

Test Plan: Added tests

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: jarek

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

app/client/components/BaseView.js

@@ -236,6 +236,9 @@ BaseView.commonCommands = {
  * loading.
  */
 BaseView.prototype.setCursorPos = function(cursorPos) {
+  if (this.isDisposed()) {
+    return;
+  }
   if (!this._isLoading.peek()) {
     this.cursor.setCursorPos(cursorPos);
   } else {

app/client/components/CursorMonitor.ts

@@ -1,13 +1,14 @@
 import {CursorPos} from 'app/client/components/Cursor';
 import {GristDoc} from 'app/client/components/GristDoc';
 import {getStorage} from 'app/client/lib/localStorageObs';
-import {IDocPage} from 'app/common/gristUrls';
-import {Disposable} from 'grainjs';
+import {IDocPage, isViewDocPage, ViewDocPage} from 'app/common/gristUrls';
+import {Disposable, Listener, Observable} from 'grainjs';
+import {reportError} from 'app/client/models/errors';
 
 /**
  * Enriched cursor position with a view id
  */
-export type ViewCursorPos = CursorPos & { viewId: number }
+export type ViewCursorPos = CursorPos & { viewId: ViewDocPage }
 
 /**
  * Component for GristDoc that allows it to keep track of the latest cursor position.
@@ -65,27 +66,38 @@ export class CursorMonitor extends Disposable {
       return;
     }
 
+    // if we are on raw data view, we need to set the position manually
+    // as currentView observable will not be changed.
+    if (doc.activeViewId.get() === 'data') {
+      this._doRestorePosition(doc).catch((e) => reportError(e));
+      return;
+    }
+
     // on view shown
-    this.autoDispose(doc.currentView.addListener(async view => {
-      // if the position was restored for this document do nothing
-      if (this._restored) { return; }
-      // set that we already restored the position, as some view is shown to the user
-      this._restored = true;
-      // if view wasn't rendered (page is displaying history or code view) do nothing
-      if (!view) { return; }
-      const viewId = doc.activeViewId.get();
-      const position = this._restoreLastPosition(viewId);
-      if (position) {
-        await doc.recursiveMoveToCursorPos(position, true);
-      }
+    this.autoDispose(oneTimeListener(doc.currentView, async () => {
+      await this._doRestorePosition(doc);
     }));
   }
 
+  private async _doRestorePosition(doc: GristDoc) {
+    // if the position was restored for this document do nothing
+    if (this._restored) { return; }
+    // set that we already restored the position, as some view is shown to the user
+    this._restored = true;
+    const viewId = doc.activeViewId.get();
+    if (!isViewDocPage(viewId)) { return; }
+    const position = this._readPosition(viewId);
+    if (position) {
+      // Ignore error with finding desired cell.
+      await doc.recursiveMoveToCursorPos(position, true, true);
+    }
+  }
+
   private _storePosition(pos: ViewCursorPos) {
     this._store.update(this._key, pos);
   }
 
-  private _restoreLastPosition(view: IDocPage) {
+  private _readPosition(view: IDocPage) {
     const lastPosition = this._store.read(this._key);
     this._store.clear(this._key);
     if (lastPosition && lastPosition.position.viewId == view) {
@@ -128,3 +140,17 @@ class StorageWrapper {
     return `grist-last-position-${docId}`;
   }
 }
+
+export function oneTimeListener<T>(obs: Observable<T>, handler: (value: T) => any) {
+  let listener: Listener|null = obs.addListener((value) => {
+    setImmediate(dispose);
+    handler(value);
+  });
+  function dispose() {
+    if (listener) {
+      listener.dispose();
+      listener = null;
+    }
+  }
+  return { dispose };
+}

app/client/components/EditorMonitor.ts

@@ -1,8 +1,11 @@
-import { getStorage } from "app/client/lib/localStorageObs";
-import { Disposable, Emitter, Holder, IDisposableOwner } from "grainjs";
-import { GristDoc } from "app/client/components/GristDoc";
-import { FieldEditor, FieldEditorStateEvent } from "app/client/widgets/FieldEditor";
-import { CellPosition, toCursor } from "app/client/components/CellPosition";
+import {CellPosition, toCursor} from 'app/client/components/CellPosition';
+import {oneTimeListener} from 'app/client/components/CursorMonitor';
+import {GristDoc} from 'app/client/components/GristDoc';
+import {getStorage} from 'app/client/lib/localStorageObs';
+import {UserError} from 'app/client/models/errors';
+import {FieldEditor, FieldEditorStateEvent} from 'app/client/widgets/FieldEditor';
+import {isViewDocPage} from 'app/common/gristUrls';
+import {Disposable, Emitter, IDisposableOwner} from 'grainjs';
 
 /**
  * Feature for GristDoc that allows it to keep track of current editor's state.
@@ -12,9 +15,7 @@ export class EditorMonitor extends Disposable {
 
   // abstraction to work with local storage
   private _store: EditMemoryStorage;
-  // Holds a listener that is attached to the current view.
-  // It will be cleared after first trigger.
-  private _currentViewListener = Holder.create(this);
+  private _restored = false;
 
   constructor(
     doc: GristDoc,
@@ -28,7 +29,14 @@ export class EditorMonitor extends Disposable {
     this._store = new EditMemoryStorage(key, store);
 
     // listen to document events to handle view load event
-    this._listenToReload(doc);
+    this._listenToReload(doc).catch((err) => {
+      if (!(err instanceof UserError)) {
+        throw err;
+      }
+      // Don't report UserErrors for this feature (should not happen as
+      // the only error that is thrown was silenced by recursiveMoveToCursorPos)
+      console.error(`Error while restoring last edit position`, err);
+    });
   }
 
   /**
@@ -56,38 +64,36 @@ export class EditorMonitor extends Disposable {
    * When document gets reloaded, restore last cursor position and a state of the editor.
    * Returns last edited cell position and saved editor state or undefined.
    */
-  private _listenToReload(doc: GristDoc) {
-    // subscribe to the current view event on the GristDoc, but make sure that the handler
-    // will be invoked only once
-    let executed = false;
-
+  private async _listenToReload(doc: GristDoc) {
     // don't restore on readonly mode or when there is custom nav
     if (doc.isReadonly.get() || doc.hasCustomNav.get()) { return; }
-
-    // on view shown
-    this._currentViewListener.autoDispose(doc.currentView.addListener(async view => {
-      if (executed) {
-        // remove the listener - we can't do it while the listener is actively executing
-        setImmediate(() => this._currentViewListener.clear());
-        return;
-      }
-      executed = true;
-      // if view wasn't rendered (page is displaying history or code view) do nothing
-      if (!view) { return; }
-      const lastEdit = this._restorePosition();
-      if (lastEdit) {
-        // set the cursor at right cell
-        await doc.recursiveMoveToCursorPos(toCursor(lastEdit.position, doc.docModel), true);
-        // activate the editor
-        await doc.activateEditorAtCursor({ state: lastEdit.value });
-      }
-    }));
+    // if we are on raw data view, we need to set the position manually
+    // as currentView observable will not be changed.
+    if (doc.activeViewId.get() === 'data') {
+      await this._doRestorePosition(doc);
+    } else {
+      // on view shown
+      this.autoDispose(oneTimeListener(doc.currentView, async () => {
+        await this._doRestorePosition(doc);
+      }));
+    }
   }
 
-  // read the value from the storage
-  private _restorePosition() {
-    const entry = this._store.readValue();
-    return entry;
+  private async _doRestorePosition(doc: GristDoc) {
+    if (this._restored) {
+      return;
+    }
+    this._restored = true;
+    const viewId = doc.activeViewId.get();
+    // if view wasn't rendered (page is displaying history or code view) do nothing
+    if (!isViewDocPage(viewId)) { return; }
+    const lastEdit = this._store.readValue();
+    if (lastEdit) {
+      // set the cursor at right cell
+      await doc.recursiveMoveToCursorPos(toCursor(lastEdit.position, doc.docModel), true, true);
+      // activate the editor
+      await doc.activateEditorAtCursor({ state: lastEdit.value });
+    }
   }
 }
 

app/client/components/GristDoc.ts

@@ -54,7 +54,7 @@ import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
 import {isSchemaAction, UserAction} from 'app/common/DocActions';
 import {OpenLocalDocResult} from 'app/common/DocListAPI';
 import {isList, isRefListType, RecalcWhen} from 'app/common/gristTypes';
-import {HashLink, IDocPage, SpecialDocPage} from 'app/common/gristUrls';
+import {HashLink, IDocPage, isViewDocPage, SpecialDocPage, ViewDocPage} from 'app/common/gristUrls';
 import {undef, waitObs} from 'app/common/gutil';
 import {LocalPlugin} from "app/common/plugin";
 import {StringUnion} from 'app/common/StringUnion';
@@ -243,7 +243,6 @@ export class GristDoc extends DisposableWithEvents {
         tourStarting = true;
         try {
           await this._waitForView();
-          await delay(0); // we need to wait an extra bit.
 
           // Remove any tour-related hash-tags from the URL. So #repeat-welcome-tour and
           // #repeat-doc-tour are used as triggers, but will immediately disappear.
@@ -326,12 +325,13 @@ export class GristDoc extends DisposableWithEvents {
       const section = use(this.viewModel.activeSection);
       const viewId = use(activeViewId);
       const view = use(section.viewInstance);
-      return (typeof viewId === 'number') ? view : null;
+      return isViewDocPage(viewId) ? view : null;
     });
     // then listen if the view is present, because we still need to wait for it load properly
     this.autoDispose(viewInstance.addListener(async (view) => {
-      if (!view) { return; }
-      await view.getLoadingDonePromise();
+      if (view) {
+        await view.getLoadingDonePromise();
+      }
       // finally set the current view as fully loaded
       this.currentView.set(view);
     }));
@@ -343,7 +343,7 @@ export class GristDoc extends DisposableWithEvents {
       if (!view) { return undefined; }
       // get current viewId
       const viewId = use(this.activeViewId);
-      if (typeof viewId != 'number') { return undefined; }
+      if (!isViewDocPage(viewId)) { return undefined; }
       // read latest position
       const currentPosition = use(view.cursor.currentPosition);
       if (currentPosition) { return { ...currentPosition, viewId }; }
@@ -737,7 +737,10 @@ export class GristDoc extends DisposableWithEvents {
    * If setAsActiveSection is true, the section in cursorPos is set as the current
    * active section.
    */
-  public async recursiveMoveToCursorPos(cursorPos: CursorPos, setAsActiveSection: boolean): Promise<void> {
+  public async recursiveMoveToCursorPos(
+    cursorPos: CursorPos,
+    setAsActiveSection: boolean,
+    silent: boolean = false): Promise<void> {
     try {
       if (!cursorPos.sectionId) { throw new Error('sectionId required'); }
       if (!cursorPos.rowId) { throw new Error('rowId required'); }
@@ -785,12 +788,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);
+          sectionId: srcSection.id.peek(),
+        }, false, silent);
       }
       const view: ViewRec = section.view.peek();
-      const viewId = view.getRowId();
-      if (viewId != this.activeViewId.get()) { await this.openDocPage(view.getRowId()); }
+      const docPage: ViewDocPage = section.isRaw.peek() ? "data" : view.getRowId();
+      if (docPage != this.activeViewId.get()) { await this.openDocPage(docPage); }
       if (setAsActiveSection) { view.activeSectionId(cursorPos.sectionId); }
       const fieldIndex = cursorPos.fieldIndex;
       const viewInstance = await waitObs(section.viewInstance);
@@ -807,7 +810,9 @@ export class GristDoc extends DisposableWithEvents {
       await delay(0);
     } catch (e) {
       console.debug(`_recursiveMoveToCursorPos(${JSON.stringify(cursorPos)}): ${e}`);
-      throw new UserError('There was a problem finding the desired cell.');
+      if (!silent) {
+        throw new UserError('There was a problem finding the desired cell.');
+      }
     }
   }
 
@@ -826,10 +831,15 @@ export class GristDoc extends DisposableWithEvents {
   private async _waitForView() {
     // For pages like ACL's, there isn't a view instance to wait for.
     if (!this.viewModel.activeSection.peek().getRowId()) {
-      return;
+      return null;
     }
     const view = await waitObs(this.viewModel.activeSection.peek().viewInstance);
-    await view?.getLoadingDonePromise();
+    if (!view) {
+      return null;
+    }
+    await view.getLoadingDonePromise();
+    // Wait extra bit for scroll to happen.
+    await delay(0);
     return view;
   }
 

app/common/gristUrls.ts

@@ -14,6 +14,14 @@ export const SpecialDocPage = StringUnion('code', 'acl', 'data', 'GristDocTour')
 type SpecialDocPage = typeof SpecialDocPage.type;
 export type IDocPage = number | SpecialDocPage;
 
+export type ViewDocPage = number | 'data';
+/**
+ * ViewDocPage is a page that shows table data (either normal or raw data view).
+ */
+export function isViewDocPage(docPage: IDocPage): docPage is ViewDocPage {
+  return typeof docPage === 'number' || docPage === 'data';
+}
+
 // What page to show in the user's home area. Defaults to 'workspace' if a workspace is set, and
 // to 'all' otherwise.
 export const HomePage = StringUnion('all', 'workspace', 'templates', 'trash');

test/nbrowser/gristUtils.ts

@@ -2045,6 +2045,23 @@ export async function filterBy(col: IColHeader|string, save: boolean, values: (s
   await waitForServer();
 }
 
+/**
+ * Refresh browser and dismiss alert that is shown (for refreshing during edits).
+ */
+export async function refreshDismiss() {
+  await driver.navigate().refresh();
+  await (await driver.switchTo().alert()).accept();
+  await waitForDocToLoad();
+}
+
+/**
+ * Confirms that anchor link was used for navigation.
+ */
+export async function waitForAnchor() {
+  await waitForDocToLoad();
+  await driver.wait(async () => (await getTestState()).anchorApplied, 2000);
+}
+
 } // end of namespace gristUtils
 
 stackWrapOwnMethods(gristUtils);