From 98ac2f7e5bd74841d167499f5bc83c7eb96f6ead Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Thu, 7 Apr 2022 12:10:58 +0200 Subject: [PATCH] (core) add 'Aggregate Values' option to chart config Summary: - adds the `aggregate values` option to chart config - aggregation is performed by turning table into summary table - change columns options of xaxis and split series selectors to be the source table columns when `aggregate values` is on - change xAxis and split series computed to hold colId instead of column id - change GristDoc saveViewSection routine to preserve old sections viewFields and options - Rename `Group data` into `split series` quip doc: https://grist.quip.com/tAsCAuv8RiMa/Charts-data-aggregation#temp:C:QcK0ce13e1e8ae64048988f44f9c Test Plan: Adds ChartAggregate.ts nbrowser test. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D3336 --- app/client/components/ChartView.ts | 200 ++++++++++++++++++++++------ app/client/components/GristDoc.ts | 67 +++++++++- app/client/components/ViewLayout.ts | 4 +- app/client/ui/PageWidgetPicker.ts | 2 +- test/nbrowser/gristUtils.ts | 7 + 5 files changed, 231 insertions(+), 49 deletions(-) diff --git a/app/client/components/ChartView.ts b/app/client/components/ChartView.ts index e316d812..70d1da70 100644 --- a/app/client/components/ChartView.ts +++ b/app/client/components/ChartView.ts @@ -11,13 +11,15 @@ import {ColumnRec, ViewFieldRec, ViewSectionRec} from 'app/client/models/DocMode import {reportError} from 'app/client/models/errors'; import {KoSaveableObservable, ObjObservable, setSaveValue} from 'app/client/models/modelUtil'; import {SortedRowSet} from 'app/client/models/rowset'; +import {toPageWidget} from 'app/client/ui/PageWidgetPicker'; import {cssLabel, cssRow, cssSeparator} from 'app/client/ui/RightPanel'; import {cssFieldEntry, cssFieldLabel, IField, VisibleFieldsConfig } from 'app/client/ui/VisibleFieldsConfig'; +import {IconName} from 'app/client/ui2018/IconList'; import {squareCheckbox} from 'app/client/ui2018/checkbox'; import {colors, vars} from 'app/client/ui2018/cssVars'; import {cssDragger} from 'app/client/ui2018/draggableList'; import {icon} from 'app/client/ui2018/icons'; -import {linkSelect, menu, menuItem, menuText, select} from 'app/client/ui2018/menus'; +import {IOptionFull, linkSelect, menu, menuItem, menuText, select} from 'app/client/ui2018/menus'; import {nativeCompare, unwrap} from 'app/common/gutil'; import {Sort} from 'app/common/SortSpec'; import {BaseFormatter} from 'app/common/ValueFormatter'; @@ -81,6 +83,7 @@ interface ChartOptions { textSize?: number; isXAxisUndefined?: boolean; orientation?: 'v'|'h'; + aggregate?: boolean; } // tslint:disable:no-console @@ -443,6 +446,8 @@ function getPlotlyLayout(options: ChartOptions): Partial { */ export class ChartConfig extends GrainJSDisposable { + private static _instanceMap = new WeakMap(); + // helper to build the draggable field list private _configFieldsHelper = VisibleFieldsConfig.create(this, this._gristDoc, this._section); @@ -456,13 +461,13 @@ export class ChartConfig extends GrainJSDisposable { ) ); - // The column id of the grouping column, or -1 if multiseries is disabled or there are no viewFields, + // The colId of the grouping column, or "" if multiseries is disabled or there are no viewFields, // for example during section removal. - private _groupDataColId: Computed = Computed.create(this, (use) => { + private _groupDataColId: Computed = Computed.create(this, (use) => { const multiseries = use(this._optionsObj.prop('multiseries')); const viewFields = use(use(this._section.viewFields).getObservable()); - if (!multiseries || viewFields.length === 0) { return -1; } - return use(viewFields[0].column).getRowId(); + if (!multiseries || viewFields.length === 0) { return ""; } + return use(use(viewFields[0].column).colId); }) .onWrite((colId) => this._setGroupDataColumn(colId)); @@ -473,30 +478,38 @@ export class ChartConfig extends GrainJSDisposable { private _freezeYAxis = Observable.create(this, false); - // The column id of the x-axis. - private _xAxis: Computed = Computed.create( + // The colId of the x-axis, or "" is x axis is undefined. + private _xAxis: Computed = Computed.create( this, this._xAxisFieldIndex, this._freezeXAxis, (use, i, freeze) => { if (freeze) { return this._xAxis.get(); } const viewFields = use(use(this._section.viewFields).getObservable()); if (-1 < i && i < viewFields.length) { - return use(viewFields[i].column).getRowId(); + return use(use(viewFields[i].column).colId); } - return -1; + return ""; }) .onWrite((colId) => this._setXAxis(colId)); - private _columns = Computed.create(this, (use) => use(use(use(this._section.table).columns).getObservable())); - - // The list of available columns for the group data picker. Picking the actual x-axis is not - // permitted. - private _groupDataOptions = Computed.create>>(this, (use) => [ - {value: -1, label: 'Pick a column'}, - ...use(this._columns) + // Columns options + private _columnsOptions: Computed>> = Computed.create( + this, this._freezeXAxis, (use, freeze) => { + if (freeze) { return this._columnsOptions.get(); } + const columns = use(this._optionsObj.prop('aggregate')) ? + this._getSummarySourceColumns(use) : + this._getColumns(use); + return columns // filter out hidden column (ie: manualsort ...) - .filter((col) => !col.isHiddenCol.peek()) - .map((col) => ({ - value: col.getRowId(), label: col.label.peek(), icon: 'FieldColumn', - })) + .filter((col) => !col.isHiddenCol.peek()) + .map((col) => ({ + value: col.colId(), label: col.label.peek(), icon: 'FieldColumn' as IconName, + })); + } + ); + + // The list of available columns for the group data picker. + private _groupDataOptions = Computed.create>>(this, (use) => [ + {value: "", label: 'Pick a column'}, + ...use(this._columnsOptions) ]); // Force checking/unchecking of the group data checkbox option. @@ -505,12 +518,12 @@ export class ChartConfig extends GrainJSDisposable { // State for the group data option checkbox. True, if a group data column is set or if the user // forced it. False otherwise. private _groupData = Computed.create( - this, this._groupDataColId, this._groupDataForce, (_use, col, force) => { - if (col > -1) { return true; } + this, this._groupDataColId, this._groupDataForce, (_use, colId, force) => { + if (colId) { return true; } return force; }).onWrite((val) => { if (val === false) { - this._groupDataColId.set(-1); + this._groupDataColId.set(""); } this._groupDataForce.set(val); }); @@ -528,15 +541,18 @@ export class ChartConfig extends GrainJSDisposable { await this._section.chartTypeDef.saveOnly(val); // When switching chart type to 'pie' makes sure to remove the group data option. if (isPieLike(val)) { - await this._setGroupDataColumn(-1); + await this._setGroupDataColumn(""); this._groupDataForce.set(false); } }); }); + private _isValueAggregated = Computed.create(this, (use) => use(this._optionsObj.prop('aggregate'))) + .onWrite((val) => this._setAggregation(val)); constructor(private _gristDoc: GristDoc, private _section: ViewSectionRec) { super(); + ChartConfig._instanceMap.set(_section, this); } private get _optionsObj() { return this._section.optionsObj; } @@ -560,7 +576,7 @@ export class ChartConfig extends GrainJSDisposable { ), dom.maybe((use) => !isPieLike(use(this._section.chartTypeDef)), () => [ // These options don't make much sense for a pie chart. - cssCheckboxRowObs('Group data', this._groupData), + cssCheckboxRowObs('Split series', this._groupData), cssCheckboxRow('Invert Y-axis', this._optionsObj.prop('invertYAxis')), cssRow( cssRowLabel('Orientation'), @@ -614,7 +630,7 @@ export class ChartConfig extends GrainJSDisposable { cssSeparator(), dom.maybe(this._groupData, () => [ - cssLabel('Group data'), + cssLabel('Split Series'), cssRow( select(this._groupDataColId, this._groupDataOptions), testId('group-by-column'), @@ -626,15 +642,12 @@ export class ChartConfig extends GrainJSDisposable { cssLabel(dom.text(this._firstFieldLabel), testId('first-field-label')), cssRow( select( - this._xAxis, Computed.create(this, (use) => use(this._columns) - .filter((col) => !col.isHiddenCol.peek()) - .map((col) => ({ - value: col.getRowId(), label: col.label.peek(), icon: 'FieldColumn', - }))), + this._xAxis, this._columnsOptions, { defaultLabel: 'Pick a column' } ), testId('x-axis'), ), + cssCheckboxRowObs('Aggregate values', this._isValueAggregated), cssLabel('SERIES'), this._buildYAxis(), @@ -669,15 +682,25 @@ export class ChartConfig extends GrainJSDisposable { ]; } - private async _setXAxis(colId: number) { + private async _setXAxis(colId: string) { const optionsObj = this._section.optionsObj; - const col = this._gristDoc.docModel.columns.getRowModel(colId); + const findColumn = () => this._getColumns().find((c) => c.colId() === colId); const viewFields = this._section.viewFields.peek(); await this._gristDoc.docData.bundleActions('selected new x-axis', async () => { this._freezeYAxis.set(true); + this._freezeXAxis.set(true); try { + // if values aggregation is 'on' update the grouped by columns first. This will make sure + // that colId is not missing from the summary table's columns (as could happen if it were a + // non-numeric for instance). + if (this._optionsObj.prop('aggregate')()) { + const splitColId = this._groupDataColId.get(); + const cols = splitColId === colId ? [colId] : [colId, splitColId]; + await this._setGroupByColumns(cols); + } + // first remove the current field if (this._xAxisFieldIndex.get() !== -1 && this._xAxisFieldIndex.get() < viewFields.peek().length) { await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]); @@ -687,7 +710,7 @@ export class ChartConfig extends GrainJSDisposable { await setSaveValue(this._optionsObj.prop('isXAxisUndefined'), false); // if new field was used to group by column series, disable multiseries - const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().getRowId() === colId); + const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().colId() === colId); if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) { await optionsObj.prop('multiseries').setAndSave(false); return; @@ -701,29 +724,43 @@ export class ChartConfig extends GrainJSDisposable { if (fieldIndex > -1) { await this._configFieldsHelper.changeFieldPosition(viewFields.peek()[fieldIndex], xAxisField); } else { - await this._configFieldsHelper.addField(col, xAxisField); + const col = findColumn(); + if (col) { + await this._configFieldsHelper.addField(col, xAxisField); + } } } finally { this._freezeYAxis.set(false); + this._freezeXAxis.set(false); } }); } - private async _setGroupDataColumn(colId: number) { + private async _setGroupDataColumn(colId: string) { const viewFields = this._section.viewFields.peek().peek(); await this._gristDoc.docData.bundleActions('selected new group data columnd', async () => { this._freezeXAxis.set(true); this._freezeYAxis.set(true); try { + // if grouping was already set, first remove the current field - if (this._groupDataColId.get() > -1) { + if (this._groupDataColId.get()) { await this._configFieldsHelper.removeField(viewFields[0]); } - if (colId > -1) { - const col = this._gristDoc.docModel.columns.getRowModel(colId); - const field = viewFields.find((f) => f.column.peek().getRowId() === colId); + // if values aggregation is 'on' update the grouped by columns first. This will make sure + // that colId is not missing from the summary table's columns (as could happen if it were a + // non-numeric for instance). + if (this._optionsObj.prop('aggregate')()) { + const xAxisColId = this._xAxis.get(); + const cols = xAxisColId === colId ? [colId] : [xAxisColId, colId]; + await this._setGroupByColumns(cols); + } + + if (colId) { + const col = this._getColumns().find((c) => c.colId() === colId)!; + const field = viewFields.find((f) => f.column.peek().colId() === colId); // if new field is already visible, moves the fields to the first place else add the field to the first // place @@ -739,7 +776,8 @@ export class ChartConfig extends GrainJSDisposable { } } - await this._optionsObj.prop('multiseries').setAndSave(colId > -1); + await this._optionsObj.prop('multiseries').setAndSave(Boolean(colId)); + } finally { this._freezeXAxis.set(false); this._freezeYAxis.set(false); @@ -747,6 +785,17 @@ export class ChartConfig extends GrainJSDisposable { }, {nestInActiveBundle: true}); } + private _getColumns(use: UseCB = unwrap) { + const table = use(this._section.table); + return use(use(table.columns).getObservable()); + } + + private _getSummarySourceColumns(use: UseCB = unwrap) { + let table = use(this._section.table); + table = use(table.summarySource); + return use(use(table.columns).getObservable()); + } + private _buildField(col: IField) { return cssFieldEntry( cssFieldLabel(dom.text(col.label)), @@ -786,6 +835,75 @@ export class ChartConfig extends GrainJSDisposable { return isNumericOnly(use(this._chartType)) ? isNumericLike(col, use) : true; } + private async _setAggregation(val: boolean) { + try { + this._freezeXAxis.set(true); + await this._gristDoc.docData.bundleActions(`Toggle chart aggregation`, async () => { + await this._optionsObj.prop('aggregate').saveOnly(val); + if (val) { + await this._doAggregation(); + } else { + await this._undoAggregation(); + } + }); + } finally { + if (!this.isDisposed()) { + this._freezeXAxis.set(false); + } + } + } + + // Do the aggregation: if not a summary table, turns into one; else update groupby columns to + // match the X-Axis and Split-series columns. + private async _doAggregation(): Promise { + if (!this._isSummaryTable()) { + await this._toggleSummaryTable(); + } else { + await this._setGroupByColumns([this._xAxis.get(), this._groupDataColId.get()]); + } + } + + // Undo the aggregation. + private async _undoAggregation() { + if (this._isSummaryTable()) { + await this._toggleSummaryTable(); + } + } + + private _isSummaryTable(use: UseCB = unwrap) { + return Boolean(use(use(this._section.table).summarySourceTable)); + } + + // Toggle whether section table is a summary table. Must use with care: this function calls + // `this.dispose()` as a side effect. Conveniently returns the ChartConfig instance of the new + // view section that replaces the old one. + private async _toggleSummaryTable(): Promise { + const colIds = [this._xAxis.get(), this._groupDataColId.get()]; + const pageWidget = toPageWidget(this._section); + pageWidget.summarize = !this._isSummaryTable(); + pageWidget.columns = this._getColumnIds(colIds); + const newSection = await this._gristDoc.saveViewSection(this._section, pageWidget); + return ChartConfig._instanceMap.get(newSection)!; + } + + private async _setGroupByColumns(groupByCols: string[]) { + const pageWidget = toPageWidget(this._section); + pageWidget.columns = this._getColumnIds(groupByCols); + return this._gristDoc.saveViewSection(this._section, pageWidget); + } + + private _getColumnIds(colIds: string[]) { + const cols = this._isSummaryTable() ? + this._section.table().summarySource().columns().all() : + this._section.table().columns().all(); + const columns = colIds + .map((colId) => colId && cols.find(c => c.colId() === colId)) + .filter((col): col is ColumnRec => Boolean(col)) + .map(col => col.id()); + return columns; + } + + } // Row for a numeric option. User can change value using spinners or directly using keyboard. In diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 8cf212ee..a1c822f3 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -588,18 +588,17 @@ export class GristDoc extends DisposableWithEvents { if (isEqual(oldVal, newVal)) { // nothing to be done - return; + return section; } - await this._viewLayout!.freezeUntil(docData.bundleActions( + return await this._viewLayout!.freezeUntil(docData.bundleActions( `Saved linked section ${section.title()} in view ${viewModel.name()}`, async () => { // if table changes or a table is made a summary table, let's replace the view section by a // new one, and return. if (oldVal.table !== newVal.table || oldVal.summarize !== newVal.summarize) { - await this._replaceViewSection(section, newVal); - return; + return await this._replaceViewSection(section, newVal); } // if type changes, let's save it. @@ -618,10 +617,53 @@ export class GristDoc extends DisposableWithEvents { if (oldVal.link !== newVal.link) { await this.saveLink(linkFromId(newVal.link)); } - } + return section; + }, + {nestInActiveBundle: true} )); } + // Set section's viewFields to be colIds in that order. Omit any colum id that do not belong to + // section's table. + public async setSectionViewFieldsFromArray(section: ViewSectionRec, colIds: string[]) { + + // remove old view fields + await Promise.all(section.viewFields.peek().all().map((viewField) => ( + this.docModel.viewFields.sendTableAction(['RemoveRecord', viewField.id()]) + ))); + + // create map + const mapColIdToColumn = new Map(); + for (const col of section.table().columns().all()) { + mapColIdToColumn.set(col.colId(), col); + } + + // If split series and/or x-axis do not exist any more in new table, update options to make them + // undefined + if (!mapColIdToColumn.has(colIds[0])) { + if (section.optionsObj.prop('multiseries')()) { + await section.optionsObj.prop('multiseries').saveOnly(false); + if (!mapColIdToColumn.has(colIds[0])) { + await section.optionsObj.prop('isXAxisUndefined').saveOnly(true); + } + } else { + await section.optionsObj.prop('isXAxisUndefined').saveOnly(true); + } + } + + // adds new view fields; ignore colIds that do not exist in new table. + await Promise.all(colIds.map((colId, i) => { + if (!mapColIdToColumn.has(colId)) { return; } + const colInfo = { + parentId: section.id(), + colRef: mapColIdToColumn.get(colId).id(), + parentPos: i + }; + const action = ['AddRecord', null, colInfo]; + return this.docModel.viewFields.sendTableAction(action); + })); + } + // Save link for the active section. public async saveLink(link: IPageWidgetLink) { const viewModel = this.viewModel; @@ -864,6 +906,10 @@ export class GristDoc extends DisposableWithEvents { const docModel = this.docModel; const viewModel = section.view(); const docData = this.docModel.docData; + const options = section.options(); + const colIds = section.viewFields().all().map((f) => f.column().colId()); + const chartType = section.chartType(); + const theme = section.theme(); // we must read the current layout from the view layout because it can override the one in // `section.layoutSpec` (in particular it provides a default layout when missing from the @@ -888,11 +934,22 @@ export class GristDoc extends DisposableWithEvents { }); await viewModel.layoutSpec.saveOnly(JSON.stringify(newLayoutSpec)); + // persist options + await newSection.options.saveOnly(options); + + // persist view fields if possible + await this.setSectionViewFieldsFromArray(newSection, colIds); + + // update theme, and chart type + await newSection.theme.saveOnly(theme); + await newSection.chartType.saveOnly(chartType); + // The newly-added section should be given focus. this.viewModel.activeSectionId(newSection.getRowId()); // remove old section await docData.sendAction(['RemoveViewSection', sectionId]); + return newSection; } /** diff --git a/app/client/components/ViewLayout.ts b/app/client/components/ViewLayout.ts index c36103d7..eb31fb81 100644 --- a/app/client/components/ViewLayout.ts +++ b/app/client/components/ViewLayout.ts @@ -166,10 +166,10 @@ export class ViewLayout extends DisposableWithEvents implements IDomComponent { // Freezes the layout until the passed in promise resolves. This is useful to achieve a single // layout rebuild when multiple user actions needs to apply, simply pass in a promise that resolves // when all user actions have resolved. - public async freezeUntil(promise: Promise): Promise { + public async freezeUntil(promise: Promise): Promise { this._freeze = true; try { - await promise; + return await promise; } finally { this._freeze = false; this._rebuildLayout(this.layoutSpec.peek()); diff --git a/app/client/ui/PageWidgetPicker.ts b/app/client/ui/PageWidgetPicker.ts index 586f7a7f..59040d8f 100644 --- a/app/client/ui/PageWidgetPicker.ts +++ b/app/client/ui/PageWidgetPicker.ts @@ -86,7 +86,7 @@ function isValidSelection(table: TableId, type: IWidgetType, isNewPage: boolean| return table !== null && getCompatibleTypes(table, isNewPage).includes(type); } -export type ISaveFunc = (val: IPageWidget) => Promise; +export type ISaveFunc = (val: IPageWidget) => Promise; // Delay in milliseconds, after a user click on the save btn, before we start showing a modal // spinner. If saving completes before this time elapses (which is likely to happen for regular diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index b770f77f..7cbc6c13 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -1654,6 +1654,13 @@ export async function addColumn(name: string) { await waitForServer(); } +export async function showColumn(name: string) { + await scrollIntoView(await driver.find('.active_section .mod-add-column')); + await driver.find('.active_section .mod-add-column').click(); + await driver.findContent('.grist-floating-menu li', `Show column ${name}`).click(); + await waitForServer(); +} + // Select a range of columns, clicking on col1 and dragging to col2. export async function selectColumnRange(col1: string, col2: string) { await getColumnHeader({col: col1}).mouseMove();