Skip to content

Commit

Permalink
fix(formula): remove non-existent formula ids (#2531)
Browse files Browse the repository at this point in the history 
* fix(formula): remove non-existent formula ids

* fix(formula): offset reference update

* fix(formula): formula alert render

* fix(formula): address returns array value

* fix(formula): test address gets array
  • Loading branch information
@Dushusir
Dushusir authored Jun 21, 2024
1 parent 428ec7a commit 656c337
Show file tree
Hide file tree
Showing 9 changed files with 195 additions and 26 deletions.
5 changes: 2 additions & 3 deletions packages/engine-formula/src/functions/__tests__/nested-functions.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ import { FUNCTION_NAMES_TEXT } from '../text/function-names';
import { IFunctionService } from '../../services/function.service';
import type { ArrayValueObject } from '../../engine/value-object/array-value-object';
import type { LexerNode } from '../../engine/analysis/lexer-node';
import type { BaseValueObject } from '../../engine/value-object/base-value-object';
import { createFunctionTestBed } from './create-function-test-bed';
import { createFunctionTestBed, getObjectValue } from './create-function-test-bed';

const getFunctionsTestWorkbookData = (): IWorkbookData => {
return {
Expand Down Expand Up @@ -330,7 +329,7 @@ describe('Test nested functions', () => {

const result = await interpreter.executeAsync(astNode as BaseAstNode);

expect((result as BaseValueObject).getValue()).toBe('$B$2');
expect(getObjectValue(result)).toStrictEqual([['$B$2']]);
});
});
});
84 changes: 78 additions & 6 deletions packages/engine-formula/src/functions/lookup/address/__tests__/index.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,19 @@ import {
} from '../../../../engine/value-object/primitive-object';
import { FUNCTION_NAMES_LOOKUP } from '../../function-names';
import { Address } from '../index';
import { ArrayValueObject, transformToValue, transformToValueObject } from '../../../../engine/value-object/array-value-object';
import { getObjectValue } from '../../../__tests__/create-function-test-bed';

describe('Test address', () => {
const textFunction = new Address(FUNCTION_NAMES_LOOKUP.ADDRESS);
const testFunction = new Address(FUNCTION_NAMES_LOOKUP.ADDRESS);
const calculate = (row: number, column: number, abs?: number, a1?: boolean, sheet?: string) => {
const rowNumber = NumberValueObject.create(row);
const columnNumber = NumberValueObject.create(column);
const absNumber = abs ? NumberValueObject.create(abs) : undefined;
const absNumber = abs !== undefined ? NumberValueObject.create(abs) : undefined;
const a1Value = a1 !== undefined ? BooleanValueObject.create(a1) : undefined;
const sheetText = sheet ? StringValueObject.create(sheet) : undefined;
const result = textFunction.calculate(rowNumber, columnNumber, absNumber, a1Value, sheetText);
return result.getValue();
const sheetText = sheet !== undefined ? StringValueObject.create(sheet) : undefined;
const result = testFunction.calculate(rowNumber, columnNumber, absNumber, a1Value, sheetText);
return (getObjectValue(result) as string[][])[0][0];
};

describe('Correct situations', () => {
Expand Down Expand Up @@ -66,12 +68,82 @@ describe('Test address', () => {
it('Absolute reference to another worksheet', async () => {
expect(calculate(2, 3, 1, false, 'EXCEL SHEET')).toBe("'EXCEL SHEET'!R2C3");
});
it('Abs less than 1', async () => {
expect(calculate(2, 3, 0, false, 'EXCEL SHEET')).toBe(ErrorType.VALUE);
});
it('Abs greater than 4', async () => {
expect(calculate(2, 3, 5, false, 'EXCEL SHEET')).toBe(ErrorType.VALUE);
});

it('Array parameter', async () => {
const rowNumber = ArrayValueObject.create({
calculateValueList: transformToValueObject([
[1],
[2],
[3],
]),
rowCount: 3,
columnCount: 1,
unitId: '',
sheetId: '',
row: 0,
column: 0,
});
const columnNumber = ArrayValueObject.create({
calculateValueList: transformToValueObject([
[4, 5, 6],
]),
rowCount: 1,
columnCount: 3,
unitId: '',
sheetId: '',
row: 0,
column: 0,
});
const absNumber = ArrayValueObject.create({
calculateValueList: transformToValueObject([
[2],
[3],
]),
rowCount: 2,
columnCount: 1,
unitId: '',
sheetId: '',
row: 0,
column: 0,
});
const a1Value = ArrayValueObject.create({
calculateValueList: transformToValueObject([
[true, false],
]),
rowCount: 1,
columnCount: 2,
unitId: '',
sheetId: '',
row: 0,
column: 0,
});
const sheetText = ArrayValueObject.create({
calculateValueList: transformToValueObject([
['Sheet1'],
['Sheet4'],
]),
rowCount: 2,
columnCount: 1,
unitId: '',
sheetId: '',
row: 0,
column: 0,
});
const result = testFunction.calculate(rowNumber, columnNumber, absNumber, a1Value, sheetText);
expect(transformToValue(result.getArrayValue())).toStrictEqual([['Sheet1!D$1', 'Sheet1!R1C[5]', '#N/A'], ['Sheet4!$D2', 'Sheet4!R[2]C5', '#N/A'], ['#N/A', '#N/A', '#N/A']]);
});
});

describe('Fault situations', () => {
it('Value error', async () => {
const error = ErrorValueObject.create(ErrorType.VALUE);
const errorValue = textFunction.calculate(error, error);
const errorValue = testFunction.calculate(error, error);
expect(errorValue.isError()).toBeTruthy();
});
});
Expand Down
81 changes: 73 additions & 8 deletions packages/engine-formula/src/functions/lookup/address/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ import { ErrorType } from '../../../basics/error-type';
import { serializeRangeToR1C1 } from '../../../engine/utils/r1c1-reference';
import { needsQuoting, serializeRange } from '../../../engine/utils/reference';
import { type BaseValueObject, ErrorValueObject } from '../../../engine/value-object/base-value-object';
import { StringValueObject } from '../../../engine/value-object/primitive-object';
import { BooleanValueObject, NumberValueObject, StringValueObject } from '../../../engine/value-object/primitive-object';
import { BaseFunction } from '../../base-function';
import type { ArrayValueObject } from '../../../engine/value-object/array-value-object';
import { expandArrayValueObject } from '../../../engine/utils/array-object';

export class Address extends BaseFunction {
override minParams = 2;

override maxParams = 5;

// eslint-disable-next-line max-lines-per-function
override calculate(
rowNumber: BaseValueObject,
columnNumber: BaseValueObject,
Expand Down Expand Up @@ -55,18 +58,80 @@ export class Address extends BaseFunction {
return sheetText;
}

const row = Number(rowNumber.getValue()) - 1;
const column = Number(columnNumber.getValue()) - 1;
absNumber = absNumber ?? NumberValueObject.create(1);
a1 = a1 ?? BooleanValueObject.create(true);
sheetText = sheetText ?? StringValueObject.create('');

// get max row length
const maxRowLength = Math.max(
rowNumber.isArray() ? (rowNumber as ArrayValueObject).getRowCount() : 1,
columnNumber.isArray() ? (columnNumber as ArrayValueObject).getRowCount() : 1,
absNumber.isArray() ? (absNumber as ArrayValueObject).getRowCount() : 1,
a1.isArray() ? (a1 as ArrayValueObject).getRowCount() : 1,
sheetText.isArray() ? (sheetText as ArrayValueObject).getRowCount() : 1
);

// get max column length
const maxColumnLength = Math.max(
rowNumber.isArray() ? (rowNumber as ArrayValueObject).getColumnCount() : 1,
columnNumber.isArray() ? (columnNumber as ArrayValueObject).getColumnCount() : 1,
absNumber.isArray() ? (absNumber as ArrayValueObject).getColumnCount() : 1,
a1.isArray() ? (a1 as ArrayValueObject).getColumnCount() : 1,
sheetText.isArray() ? (sheetText as ArrayValueObject).getColumnCount() : 1
);

const rowNumArray = expandArrayValueObject(maxRowLength, maxColumnLength, rowNumber, ErrorValueObject.create(ErrorType.NA));
const columnNumArray = expandArrayValueObject(maxRowLength, maxColumnLength, columnNumber, ErrorValueObject.create(ErrorType.NA));
const absNumArray = expandArrayValueObject(maxRowLength, maxColumnLength, absNumber, ErrorValueObject.create(ErrorType.NA));
const a1Array = expandArrayValueObject(maxRowLength, maxColumnLength, a1, ErrorValueObject.create(ErrorType.NA));
const sheetTextArray = expandArrayValueObject(maxRowLength, maxColumnLength, sheetText, ErrorValueObject.create(ErrorType.NA));

return rowNumArray.map((rowNumValue, rowIndex, columnIndex) => {
const columnNumValue = columnNumArray.get(rowIndex, columnIndex) || ErrorValueObject.create(ErrorType.NA);
const absNumValue = absNumArray.get(rowIndex, columnIndex) || ErrorValueObject.create(ErrorType.NA);
const a1Value = a1Array.get(rowIndex, columnIndex) || ErrorValueObject.create(ErrorType.NA);
const sheetTextValue = sheetTextArray.get(rowIndex, columnIndex) || ErrorValueObject.create(ErrorType.NA);

if (rowNumValue.isError()) {
return rowNumValue;
}

if (columnNumValue.isError()) {
return columnNumValue;
}

if (absNumValue.isError()) {
return absNumValue;
}

if (a1Value.isError()) {
return a1Value;
}

if (sheetTextValue.isError()) {
return sheetTextValue;
}

return this._calculateSingleCell(rowNumValue, columnNumValue, absNumValue, a1Value, sheetTextValue);
});
}

if (Number.isNaN(row) || Number.isNaN(column)) {
private _calculateSingleCell(rowNumber: BaseValueObject,
columnNumber: BaseValueObject,
absNumber: BaseValueObject,
a1: BaseValueObject,
sheetText: BaseValueObject) {
const row = Number.parseInt(`${Number(rowNumber.getValue()) - 1}`);
const column = Number.parseInt(`${Number(columnNumber.getValue()) - 1}`);
const absNumberValue = Number.parseInt(`${Number(absNumber.getValue())}`);

if (Number.isNaN(row) || Number.isNaN(column) || Number.isNaN(absNumberValue) || absNumberValue < 1 || absNumberValue > 4) {
return ErrorValueObject.create(ErrorType.VALUE);
}

const absType = absNumber ? transformAbsoluteRefType(absNumber.getValue()) : AbsoluteRefType.ALL;

const absType = transformAbsoluteRefType(absNumberValue);
const a1Value = this.getZeroOrOneByOneDefault(a1);

const sheetTextValue = sheetText ? `${sheetText.getValue()}` : '';
const sheetTextValue = `${sheetText.getValue()}`;
const sheetName = needsQuoting(sheetTextValue) ? `'${sheetTextValue}'` : sheetTextValue;

const range: IRange = {
Expand Down
1 change: 1 addition & 0 deletions packages/engine-formula/src/functions/lookup/index/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export class Index extends BaseFunction {

override needsReferenceObject = true;

// eslint-disable-next-line max-lines-per-function, complexity
override calculate(reference: FunctionVariantType, rowNum?: FunctionVariantType, columnNum?: FunctionVariantType, areaNum?: FunctionVariantType) {
if (reference.isError()) {
return reference;
Expand Down
5 changes: 5 additions & 0 deletions packages/engine-formula/src/functions/lookup/offset/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ export class Offset extends BaseFunction {

override needsReferenceObject = true;

override isAddress() {
return true;
}

// eslint-disable-next-line max-lines-per-function, complexity
override calculate(
reference: FunctionVariantType,
rows: FunctionVariantType,
Expand Down
4 changes: 4 additions & 0 deletions packages/engine-formula/src/models/formula-data.model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,10 @@ export function initSheetFormulaData(
const y = r - formulaInfo.r;

sheetFormulaDataMatrix.setValue(r, c, { f, si: formulaId, x, y });
} else {
// If the formula ID is not found in the formula ID map, delete the formula ID.
// Prevent IDs without corresponding formulas from appearing
sheetFormulaDataMatrix.realDeleteValue(r, c);
}
}
});
Expand Down
15 changes: 9 additions & 6 deletions ...c/controllers/formula-alert.controller.ts → ...ollers/formula-alert-render.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
*/

import type { Workbook } from '@univerjs/core';
import { Disposable, IUniverInstanceService, LifecycleStages, LocaleService, OnLifecycle, UniverInstanceType } from '@univerjs/core';
import { Disposable, isICellData, LifecycleStages, LocaleService, OnLifecycle } from '@univerjs/core';
import { ErrorType } from '@univerjs/engine-formula';
import { CellAlertManagerService, CellAlertType, HoverManagerService } from '@univerjs/sheets-ui';
import { Inject } from '@wendellhu/redi';
import type { IRenderContext } from '@univerjs/engine-render';
import { extractFormulaError } from './utils/utils';

const ALERT_KEY = 'SHEET_FORMULA_ALERT';
Expand All @@ -38,12 +39,12 @@ const ErrorTypeToMessageMap = {
[ErrorType.NULL]: 'null',
};

@OnLifecycle(LifecycleStages.Rendered, FormulaAlertController)
export class FormulaAlertController extends Disposable {
@OnLifecycle(LifecycleStages.Rendered, FormulaAlertRenderController)
export class FormulaAlertRenderController extends Disposable {
constructor(
private readonly _context: IRenderContext<Workbook>,
@Inject(HoverManagerService) private readonly _hoverManagerService: HoverManagerService,
@Inject(CellAlertManagerService) private readonly _cellAlertManagerService: CellAlertManagerService,
@IUniverInstanceService private readonly _univerInstanceService: IUniverInstanceService,
@Inject(LocaleService) private readonly _localeService: LocaleService
) {
super();
Expand All @@ -57,13 +58,15 @@ export class FormulaAlertController extends Disposable {
private _initCellAlertPopup() {
this.disposeWithMe(this._hoverManagerService.currentCell$.subscribe((cellPos) => {
if (cellPos) {
const workbook = this._univerInstanceService.getCurrentUnitForType<Workbook>(UniverInstanceType.UNIVER_SHEET)!;
const workbook = this._context.unit;
const worksheet = workbook.getActiveSheet();

if (!worksheet) return;

const cellData = worksheet.getCell(cellPos.location.row, cellPos.location.col);

if (cellData) {
// Preventing blank object
if (isICellData(cellData)) {
const errorType = extractFormulaError(cellData);

if (!errorType) {
Expand Down
18 changes: 16 additions & 2 deletions packages/sheets-formula/src/formula-ui-plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import type { Dependency } from '@wendellhu/redi';
import { Inject, Injector } from '@wendellhu/redi';

import { UniverFormulaEnginePlugin } from '@univerjs/engine-formula';
import { IRenderManagerService } from '@univerjs/engine-render';
import { FORMULA_UI_PLUGIN_NAME } from './common/plugin-name';
import { ActiveDirtyController } from './controllers/active-dirty.controller';
import { ArrayFormulaDisplayController } from './controllers/array-formula-display.controller';
Expand All @@ -41,7 +42,7 @@ import { IRegisterFunctionService, RegisterFunctionService } from './services/re
import { DefinedNameController } from './controllers/defined-name.controller';
import { FormulaRefRangeService } from './services/formula-ref-range.service';
import { RegisterOtherFormulaService } from './services/register-other-formula.service';
import { FormulaAlertController } from './controllers/formula-alert.controller';
import { FormulaAlertRenderController } from './controllers/formula-alert-render.controller';
import { FormulaRenderManagerController } from './controllers/formula-render.controller';

/**
Expand All @@ -55,6 +56,7 @@ export class UniverSheetsFormulaPlugin extends Plugin {
constructor(
private readonly _config: Partial<IUniverSheetsFormulaConfig> = {},
@Inject(Injector) override readonly _injector: Injector,
@IRenderManagerService private readonly _renderManagerService: IRenderManagerService,
@Inject(LocaleService) private readonly _localeService: LocaleService
) {
super();
Expand All @@ -66,6 +68,19 @@ export class UniverSheetsFormulaPlugin extends Plugin {
this._init();
}

override onRendered(): void {
this._registerRenderControllers();
}

private _registerRenderControllers(): void {
([

FormulaAlertRenderController,
]).forEach((controller) => {
this.disposeWithMe(this._renderManagerService.registerRenderController(UniverInstanceType.UNIVER_SHEET, controller));
});
}

_init(): void {
const dependencies: Dependency[] = [
// services
Expand Down Expand Up @@ -99,7 +114,6 @@ export class UniverSheetsFormulaPlugin extends Plugin {
[ActiveDirtyController],
[DefinedNameController],
[FormulaRenderManagerController],
[FormulaAlertController],
];

dependencies.forEach((dependency) => this._injector.add(dependency));
Expand Down
8 changes: 7 additions & 1 deletion packages/sheets-formula/src/views/prompt/help-function/HelpFunction.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ export function HelpFunction() {
setParamIndex(paramIndex);
}

function handleClose() {
setHelpVisible(!helpVisible);

// TODO focus editor
}

return (
<>
{helpVisible
Expand All @@ -117,7 +123,7 @@ export function HelpFunction() {
</div>
<div
className={styles.formulaHelpFunctionTitleIcon}
onClick={() => setHelpVisible(!helpVisible)}
onClick={handleClose}
>
<CloseSingle />
</div>
Expand Down

0 comments on commit 656c337

Please sign in to comment.