From 2997434815a6d7ec39be7fcbdbd29565026d3370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 24 Aug 2022 13:43:15 +0200 Subject: [PATCH] (core) Showing a raw data section on a popup Summary: Show raw data will now open a popup with raw section instead of redirecting to raw data page. Adding new anchor link type "a2" that is able to open any section in a popup on a current view. Not related: Fixing highlightMatches function, after merging core PR. Test Plan: Updated tests Reviewers: alexmojaki, georgegevoian Reviewed By: alexmojaki, georgegevoian Subscribers: georgegevoian, alexmojaki Differential Revision: https://phab.getgrist.com/D3592 --- app/client/components/BaseView.js | 5 +- app/client/components/GristDoc.ts | 154 ++++++++++++++---- .../components/{RawData.ts => RawDataPage.ts} | 64 +++++--- app/client/components/RecordLayout.js | 1 + app/client/models/entities/ViewSectionRec.ts | 6 +- app/client/widgets/FieldBuilder.ts | 2 +- app/common/gristUrls.ts | 10 +- test/nbrowser/gristUtils.ts | 11 +- 8 files changed, 186 insertions(+), 67 deletions(-) rename app/client/components/{RawData.ts => RawDataPage.ts} (74%) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 0ffb384f..ea5bb0a7 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -307,7 +307,7 @@ BaseView.prototype.getAnchorLinkForSection = function(sectionId) { // Note that this case only happens in combination with the widget linking mentioned. // If the table is empty but the 'new record' row is selected, the `viewData.getRowId` line above works. || 'new'; - const colRef = this.viewSection.viewFields().peek()[this.cursor.fieldIndex()].colRef(); + const colRef = this.viewSection.viewFields().peek()[this.cursor.fieldIndex.peek()].colRef.peek(); return {hash: {sectionId, rowId, colRef}}; } @@ -328,7 +328,8 @@ BaseView.prototype.copyLink = async function() { BaseView.prototype.showRawData = async function() { const sectionId = this.schemaModel.rawViewSectionRef.peek(); const anchorUrlState = this.getAnchorLinkForSection(sectionId); - await urlState().pushUrl({...anchorUrlState, docPage: 'data'}); + anchorUrlState.hash.popup = true; + await urlState().pushUrl(anchorUrlState, {replace: true, avoidReload: true}); } BaseView.prototype.filterByThisCellValue = function() { diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 4a2ee69b..3d4b754e 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -17,7 +17,7 @@ import {Drafts} from "app/client/components/Drafts"; import {EditorMonitor} from "app/client/components/EditorMonitor"; import * as GridView from 'app/client/components/GridView'; import {Importer} from 'app/client/components/Importer'; -import {RawData} from 'app/client/components/RawData'; +import {RawDataPage, RawDataPopup} from 'app/client/components/RawDataPage'; import {ActionGroupWithCursorPos, UndoStack} from 'app/client/components/UndoStack'; import {ViewLayout} from 'app/client/components/ViewLayout'; import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals'; @@ -67,6 +67,7 @@ import { Computed, dom, Emitter, + fromKo, Holder, IDisposable, IDomComponent, @@ -108,6 +109,12 @@ export interface IExtraTool { content: TabContent[]|IDomComponent; } +interface PopupOptions { + viewSection: ViewSectionRec; + hash: HashLink; + close: () => void; +} + export class GristDoc extends DisposableWithEvents { public docModel: DocModel; public viewModel: ViewRec; @@ -159,6 +166,9 @@ export class GristDoc extends DisposableWithEvents { private _viewLayout: ViewLayout|null = null; private _showGristTour = getUserOrgPrefObs(this.userOrgPrefs, 'showGristTour'); private _seenDocTours = getUserOrgPrefObs(this.userOrgPrefs, 'seenDocTours'); + private _popupOptions: Observable = Observable.create(this, null); + private _activeContent: Computed; + constructor( public readonly app: App, @@ -204,6 +214,8 @@ export class GristDoc extends DisposableWithEvents { return viewId || use(defaultViewId); }); + this._activeContent = Computed.create(this, use => use(this._popupOptions) ?? use(this.activeViewId)); + // This viewModel reflects the currently active view, relying on the fact that // createFloatingRowModel() supports an observable rowId for its argument. // Although typings don't reflect it, createFloatingRowModel() accepts non-numeric values, @@ -224,12 +236,17 @@ export class GristDoc extends DisposableWithEvents { } })); - // Navigate to an anchor if one is present in the url hash. + // Subscribe to URL state, and navigate to anchor or open a popup if necessary. this.autoDispose(subscribe(urlState().state, async (use, state) => { if (state.hash) { try { - const cursorPos = this._getCursorPosFromHash(state.hash); - await this.recursiveMoveToCursorPos(cursorPos, true); + if (state.hash.popup) { + await this.openPopup(state.hash); + } else { + // Navigate to an anchor if one is present in the url hash. + const cursorPos = this._getCursorPosFromHash(state.hash); + await this.recursiveMoveToCursorPos(cursorPos, true); + } } catch (e) { reportError(e); } finally { @@ -332,22 +349,23 @@ export class GristDoc extends DisposableWithEvents { // create computed observable for viewInstance - if it is loaded or not - // Add an artificial intermediary computed only to delay the evaluation of currentView, so - // that it happens after section.viewInstance is set. If it happens before, then - // section.viewInstance is seen as null, and as it gets updated, GrainJS refuses to - // recalculate this computed since it was already calculated in the same tick. - const activeViewId = Computed.create(this, (use) => use(this.activeViewId)); - const viewInstance = Computed.create(this, (use) => { - const section = use(this.viewModel.activeSection); - const viewId = use(activeViewId); - const view = use(section.viewInstance); - return isViewDocPage(viewId) ? view : null; - }); + // GrainJS will not recalculate section.viewInstance correctly because it will be + // modified (updated from null to a correct instance) in the same tick. We need to + // switch for a moment to knockout to fix this. + const viewInstance = fromKo(this.autoDispose(ko.pureComputed(() => { + const viewId = toKo(ko, this.activeViewId)(); + if (!isViewDocPage(viewId)) { return null; } + const section = this.viewModel.activeSection(); + const view = section.viewInstance(); + return view; + }))); + // 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) { await view.getLoadingDonePromise(); } + if (view?.isDisposed()) { return; } // finally set the current view as fully loaded this.currentView.set(view); })); @@ -355,9 +373,8 @@ export class GristDoc extends DisposableWithEvents { // create observable for current cursor position this.cursorPosition = Computed.create(this, use => { // get the BaseView - const view = use(viewInstance); + const view = use(this.currentView); if (!view) { return undefined; } - // get current viewId const viewId = use(this.activeViewId); if (!isViewDocPage(viewId)) { return undefined; } // read latest position @@ -393,15 +410,25 @@ export class GristDoc extends DisposableWithEvents { * Builds the DOM for this GristDoc. */ public buildDom() { - return cssViewContentPane(testId('gristdoc'), - cssViewContentPane.cls("-contents", use => use(this.activeViewId) === 'data'), - dom.domComputed(this.activeViewId, (viewId) => ( - viewId === 'code' ? dom.create(CodeEditorPanel, this) : - viewId === 'acl' ? dom.create(AccessRules, this) : - viewId === 'data' ? dom.create(RawData, this) : - viewId === 'GristDocTour' ? null : - dom.create((owner) => (this._viewLayout = ViewLayout.create(owner, this, viewId))) - )), + return cssViewContentPane( + testId('gristdoc'), + cssViewContentPane.cls("-contents", use => use(this.activeViewId) === 'data' || use(this._popupOptions) !== null), + dom.domComputed(this._activeContent, (content) => { + return ( + content === 'code' ? dom.create(CodeEditorPanel, this) : + content === 'acl' ? dom.create(AccessRules, this) : + content === 'data' ? dom.create(RawDataPage, this) : + content === 'GristDocTour' ? null : + typeof content === 'object' ? dom.create(owner => { + // In case user changes a page, close the popup. + owner.autoDispose(this.activeViewId.addListener(content.close)); + // In case the section is removed, close the popup. + content.viewSection.autoDispose({dispose: content.close}); + return dom.create(RawDataPopup, this, content.viewSection, content.close); + }) : + dom.create((owner) => (this._viewLayout = ViewLayout.create(owner, this, content))) + ); + }), ); } @@ -934,17 +961,82 @@ export class GristDoc extends DisposableWithEvents { await tableRec.tableName.saveOnly(newTableName); } + /** + * Opens popup with a section data (used by Raw Data view). + */ + public async openPopup(hash: HashLink) { + // We can only open a popup for a section. + if (!hash.sectionId) { return; } + // We will borrow active viewModel and will trick him into believing that + // the section from the link is his viewSection and it is active. Fortunately + // he doesn't care. After popup is closed, we will restore the original. + const prevSection = this.viewModel.activeSection.peek(); + this.viewModel.activeSectionId(hash.sectionId); + // Now we have view section we want to show in the popup. + const popupSection = this.viewModel.activeSection.peek(); + // We need to make it active, so that cursor on this section will be the + // active one. This will change activeViewSectionId on a parent view of this section, + // which might be a diffrent view from what we currently have. If the section is + // a raw data section it will use `EmptyRowModel` as raw sections don't have parents. + popupSection.hasFocus(true); + this._popupOptions.set({ + hash, + viewSection: popupSection, + close: () => { + // In case we are already close, do nothing. + if (!this._popupOptions.get()) { return; } + if (popupSection !== prevSection) { + // We need to blur raw view section. Otherwise it will automatically be opened + // on raw data view. Note: raw data section doesn't have its own view, it uses + // empty row model as a parent (which feels like a hack). + if (!popupSection.isDisposed()) { popupSection.hasFocus(false); } + // We need to restore active viewSection for a view that we borrowed. + // When this popup was opened we tricked active view by setting its activeViewSection + // to our viewSection (which might be a completely diffrent section or a raw data section) not + // connected to this view. + if (!prevSection.isDisposed()) { prevSection.hasFocus(true); } + } + // Clearing popup data will close this popup. + this._popupOptions.set(null); + } + }); + // If the anchor link is valid, set the cursor. + if (hash.colRef && hash.rowId) { + const fieldIndex = popupSection.viewFields.peek().all().findIndex(f => f.colRef.peek() === hash.colRef); + if (fieldIndex >= 0) { + const view = await this._waitForView(popupSection); + view?.setCursorPos({ sectionId: hash.sectionId, rowId: hash.rowId, fieldIndex }); + } + } + } + /** * Waits for a view to be ready */ - private async _waitForView() { + private async _waitForView(popupSection?: ViewSectionRec) { + const sectionToCheck = popupSection ?? this.viewModel.activeSection.peek(); // For pages like ACL's, there isn't a view instance to wait for. - if (!this.viewModel.activeSection.peek().getRowId()) { + if (!sectionToCheck.getRowId()) { return null; } - const view = await waitObs(this.viewModel.activeSection.peek().viewInstance); - if (!view) { - return null; + async function singleWait(s: ViewSectionRec): Promise { + const view = await waitObs( + sectionToCheck.viewInstance, + vsi => Boolean(vsi && !vsi.isDisposed()) + ); + return view!; + } + let view = await singleWait(sectionToCheck); + if (view.isDisposed()) { + // If the view is disposed (it can happen, as wait is not reliable enough, because it uses + // subscription for testing the predicate, which might dispose object before we have a chance to test it). + // This can happen when section is recreating itself on a popup. + if (popupSection) { + view = await singleWait(popupSection); + } + if (view.isDisposed()) { + return null; + } } await view.getLoadingDonePromise(); // Wait extra bit for scroll to happen. diff --git a/app/client/components/RawData.ts b/app/client/components/RawDataPage.ts similarity index 74% rename from app/client/components/RawData.ts rename to app/client/components/RawDataPage.ts index 4e11ec20..fcc414c0 100644 --- a/app/client/components/RawData.ts +++ b/app/client/components/RawDataPage.ts @@ -8,15 +8,15 @@ import {colors, mediaSmall, vars} from 'app/client/ui2018/cssVars'; import {icon} from 'app/client/ui2018/icons'; import {Computed, Disposable, dom, fromKo, makeTestId, Observable, styled} from 'grainjs'; import {reportError} from 'app/client/models/errors'; +import {ViewSectionRec} from 'app/client/models/DocModel'; const testId = makeTestId('test-raw-data-'); -export class RawData extends Disposable { +export class RawDataPage extends Disposable { private _lightboxVisible: Observable; constructor(private _gristDoc: GristDoc) { super(); const commandGroup = { - cancel: () => { this._close(); }, printSection: () => { printViewSection(null, this._gristDoc.viewModel.activeSection()).catch(reportError); }, }; this.autoDispose(commands.createGroup(commandGroup, this, true)); @@ -41,9 +41,6 @@ export class RawData extends Disposable { } public buildDom() { - // Handler to close the lightbox. - const close = this._close.bind(this); - return cssContainer( dom('div', dom.create(DataTables, this._gristDoc), @@ -52,30 +49,12 @@ export class RawData extends Disposable { dom.hide(this._lightboxVisible) ), /*************** Lightbox section **********/ - dom.domComputedOwned(fromKo(this._gristDoc.viewModel.activeSection), (owner, viewSection) => { + dom.domComputed(fromKo(this._gristDoc.viewModel.activeSection), (viewSection) => { const sectionId = viewSection.getRowId(); if (!sectionId || !viewSection.isRaw.peek()) { return null; } - ViewSectionHelper.create(owner, this._gristDoc, viewSection); - return cssOverlay( - testId('overlay'), - cssSectionWrapper( - buildViewSectionDom({ - gristDoc: this._gristDoc, - sectionRowId: viewSection.getRowId(), - draggable: false, - focusable: false, - widgetNameHidden: true - }) - ), - cssCloseButton('CrossBig', - testId('close-button'), - dom.on('click', close) - ), - // Close the lightbox when user clicks exactly on the overlay. - dom.on('click', (ev, elem) => void (ev.target === elem ? close() : null)) - ); + return dom.create(RawDataPopup, this._gristDoc, viewSection, () => this._close()); }), ); } @@ -85,6 +64,41 @@ export class RawData extends Disposable { } } +export class RawDataPopup extends Disposable { + constructor( + private _gristDoc: GristDoc, + private _viewSection: ViewSectionRec, + private _onClose: () => void, + ) { + super(); + const commandGroup = { + cancel: () => { this._onClose(); }, + }; + this.autoDispose(commands.createGroup(commandGroup, this, true)); + } + public buildDom() { + ViewSectionHelper.create(this, this._gristDoc, this._viewSection); + return cssOverlay( + testId('overlay'), + cssSectionWrapper( + buildViewSectionDom({ + gristDoc: this._gristDoc, + sectionRowId: this._viewSection.getRowId(), + draggable: false, + focusable: false, + widgetNameHidden: true + }) + ), + cssCloseButton('CrossBig', + testId('close-button'), + dom.on('click', () => this._onClose()) + ), + // Close the lightbox when user clicks exactly on the overlay. + dom.on('click', (ev, elem) => void (ev.target === elem ? this._onClose() : null)) + ); + } +} + const cssContainer = styled('div', ` overflow-y: auto; position: relative; diff --git a/app/client/components/RecordLayout.js b/app/client/components/RecordLayout.js index 4817e818..0360340f 100644 --- a/app/client/components/RecordLayout.js +++ b/app/client/components/RecordLayout.js @@ -69,6 +69,7 @@ function RecordLayout(options) { // Update the stored layoutSpecObj with any missing fields that are present in viewFields. this.layoutSpec = this.autoDispose(ko.computed(function() { + if (this.viewSection.isDisposed()) { return null; } return RecordLayout.updateLayoutSpecWithFields( this.viewSection.layoutSpecObj(), this.viewSection.viewFields().all()); }, this).extend({rateLimit: 0})); // layoutSpecObj and viewFields should be updated together. diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index 3acd3037..b324c0ac 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -474,7 +474,7 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): this.hasFocus = ko.pureComputed({ // Read may occur for recently disposed sections, must check condition first. read: () => !this.isDisposed() && this.view().activeSectionId() === this.id(), - write: (val) => { if (val) { this.view().activeSectionId(this.id()); } } + write: (val) => { this.view().activeSectionId(val ? this.id() : 0); } }); this.activeLinkSrcSectionRef = modelUtil.customValue(this.linkSrcSectionRef); @@ -595,8 +595,8 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel): this.allowSelectBy = Observable.create(this, false); this.selectedRows = Observable.create(this, []); - this.tableId = ko.pureComputed(() => this.table().tableId()); - const rawSection = ko.pureComputed(() => this.table().rawViewSection()); + this.tableId = this.autoDispose(ko.pureComputed(() => this.table().tableId())); + const rawSection = this.autoDispose(ko.pureComputed(() => this.table().rawViewSection())); this.rulesCols = refListRecords(docModel.columns, ko.pureComputed(() => rawSection().rules())); this.rulesColsIds = ko.pureComputed(() => this.rulesCols().map(c => c.colId())); this.rulesStyles = modelUtil.savingComputed({ diff --git a/app/client/widgets/FieldBuilder.ts b/app/client/widgets/FieldBuilder.ts index 00c38779..70e0d8e3 100644 --- a/app/client/widgets/FieldBuilder.ts +++ b/app/client/widgets/FieldBuilder.ts @@ -436,7 +436,7 @@ export class FieldBuilder extends Disposable { * Builds the cell and editor DOM for the chosen UserType. Calls the buildDom and * buildEditorDom functions of its widgetImpl. */ - public buildDomWithCursor(row: DataRowModel, isActive: boolean, isSelected: boolean) { + public buildDomWithCursor(row: DataRowModel, isActive: ko.Computed, isSelected: ko.Computed) { const computedFlags = koUtil.withKoUtils(ko.pureComputed(() => { return this.field.rulesColsIds().map(colRef => row.cells[colRef]?.() ?? false); }, this).extend({ deferred: true })); diff --git a/app/common/gristUrls.ts b/app/common/gristUrls.ts index 6f58ced7..469acbbd 100644 --- a/app/common/gristUrls.ts +++ b/app/common/gristUrls.ts @@ -250,7 +250,7 @@ export function encodeUrl(gristConfig: Partial, const hashParts: string[] = []; if (state.hash && state.hash.rowId) { const hash = state.hash; - hashParts.push(`a1`); + hashParts.push(state.hash?.popup ? 'a2' : `a1`); for (const key of ['sectionId', 'rowId', 'colRef'] as Array) { if (hash[key]) { hashParts.push(`${key[0]}${hash[key]}`); } } @@ -372,9 +372,9 @@ export function decodeUrl(gristConfig: Partial, location: Locat for (const part of hashParts) { hashMap.set(part.slice(0, 1), part.slice(1)); } - if (hashMap.has('#') && hashMap.get('#') === 'a1') { + if (hashMap.has('#') && ['a1', 'a2'].includes(hashMap.get('#') || '')) { const link: HashLink = {}; - for (const key of ['sectionId', 'rowId', 'colRef'] as Array) { + for (const key of ['sectionId', 'rowId', 'colRef'] as Array>) { const ch = key.substr(0, 1); if (!hashMap.has(ch)) { continue; } const value = hashMap.get(ch); @@ -384,6 +384,9 @@ export function decodeUrl(gristConfig: Partial, location: Locat link[key] = parseInt(value!, 10); } } + if (hashMap.get('#') === 'a2') { + link.popup = true; + } state.hash = link; } state.welcomeTour = hashMap.get('#') === 'repeat-welcome-tour'; @@ -773,6 +776,7 @@ export interface HashLink { sectionId?: number; rowId?: UIRowId; colRef?: number; + popup?: boolean; } // Check whether a urlId is a prefix of the docId, and adequately long to be diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 94dbfaa5..d8e8b5cd 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -536,16 +536,18 @@ export async function getFormulaText() { /** * Check that formula editor is shown and its value matches the given regexp. */ -export async function checkFormulaEditor(valueRe: RegExp) { +export async function checkFormulaEditor(value: RegExp|string) { assert.equal(await driver.findWait('.test-formula-editor', 500).isDisplayed(), true); + const valueRe = typeof value === 'string' ? exactMatch(value) : value; assert.match(await driver.find('.code_editor_container').getText(), valueRe); } /** * Check that plain text editor is shown and its value matches the given regexp. */ -export async function checkTextEditor(valueRe: RegExp) { +export async function checkTextEditor(value: RegExp|string) { assert.equal(await driver.findWait('.test-widget-text-editor', 500).isDisplayed(), true); + const valueRe = typeof value === 'string' ? exactMatch(value) : value; assert.match(await driver.find('.celleditor_text_editor').value(), valueRe); } @@ -2384,6 +2386,11 @@ export async function waitForAnchor() { await driver.wait(async () => (await getTestState()).anchorApplied, 2000); } +export async function getAnchor() { + await driver.find('body').sendKeys(Key.chord(Key.SHIFT, await modKey(), 'a')); + return (await getTestState()).clipboard || ''; +} + export async function getActiveSectionTitle(timeout?: number) { return await driver.findWait('.active_section .test-viewsection-title', timeout ?? 0).getText(); }