diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 6726d97c..cb67c000 100644 --- a/app/client/components/BaseView.js +++ b/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 { diff --git a/app/client/components/CursorMonitor.ts b/app/client/components/CursorMonitor.ts index 27e91375..f777e39d 100644 --- a/app/client/components/CursorMonitor.ts +++ b/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(obs: Observable, 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 }; +} diff --git a/app/client/components/EditorMonitor.ts b/app/client/components/EditorMonitor.ts index 6d78c732..26826c4e 100644 --- a/app/client/components/EditorMonitor.ts +++ b/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 }); + } } } diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index c791ce5a..5c38ac18 100644 --- a/app/client/components/GristDoc.ts +++ b/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 { + public async recursiveMoveToCursorPos( + cursorPos: CursorPos, + setAsActiveSection: boolean, + silent: boolean = false): Promise { 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; } diff --git a/app/common/gristUrls.ts b/app/common/gristUrls.ts index 3c7d29ee..19b9ac4b 100644 --- a/app/common/gristUrls.ts +++ b/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'); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 7ae18513..d3a9754f 100644 --- a/test/nbrowser/gristUtils.ts +++ b/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);