Skip to content

Commit

Permalink
fix(sheets): fix value type casting in set range values (#1646)
Browse files Browse the repository at this point in the history 
* fix(sheets): fix value type casting in set range values

* fix: fix insert number on boolean cell

* fix: fix unit test

* fix: fix paste with formula should preserve boolean

* fix: fix auto fill with boolean
  • Loading branch information
@wzhudev
wzhudev committed Apr 2, 2024
1 parent 1876e68 commit 227f5b0
Show file tree
Hide file tree
Showing 9 changed files with 193 additions and 102 deletions.
94 changes: 36 additions & 58 deletions packages/sheets-formula/src/controllers/formula-clipboard.controller.ts
View file
Expand Up @@ -27,7 +27,7 @@ import {
import { LexerTreeBuilder } from '@univerjs/engine-formula';
import type { ISetRangeValuesMutationParams } from '@univerjs/sheets';
import { SetRangeValuesMutation, SetRangeValuesUndoMutationFactory } from '@univerjs/sheets';
import type { ICellDataWithSpanInfo, ISheetClipboardHook } from '@univerjs/sheets-ui';
import type { ICellDataWithSpanInfo, ICopyPastePayload, ISheetClipboardHook, ISheetRangeLocation } from '@univerjs/sheets-ui';
import { COPY_TYPE, ISheetClipboardService } from '@univerjs/sheets-ui';
import type { IAccessor } from '@wendellhu/redi';
import { Inject, Injector } from '@wendellhu/redi';
Expand Down Expand Up @@ -59,71 +59,49 @@ export class FormulaClipboardController extends Disposable {
}

private _pasteFormulaHook(): ISheetClipboardHook {
const specialPasteFormulaHook: ISheetClipboardHook = {
return {
id: SPECIAL_PASTE_FORMULA,
priority: 10,
specialPasteInfo: {
label: 'specialPaste.formula',
},
onPasteCells: (pasteFrom, pasteTo, data, payload) => {
const copyInfo = {
copyType: payload.copyType || COPY_TYPE.COPY,
copyRange: pasteFrom?.range,
};
const pastedRange = pasteTo.range;
const matrix = data;
const workbook = this._currentUniverSheet.getCurrentUniverSheetInstance();
const unitId = workbook.getUnitId();
const subUnitId = workbook.getActiveSheet().getSheetId();

return this._injector.invoke((accessor) =>
getSetCellFormulaMutations(
unitId,
subUnitId,
pastedRange,
matrix,
accessor,
copyInfo,
this._lexerTreeBuilder,
true
)
);
},
specialPasteInfo: { label: 'specialPaste.formula' },
onPasteCells: (pasteFrom, pasteTo, data, payload) => this._onPasteCells(pasteFrom, pasteTo, data, payload, true),
};

return specialPasteFormulaHook;
}

private _pasteWithFormulaHook(): ISheetClipboardHook {
const specialPasteFormulaHook: ISheetClipboardHook = {
return {
id: DEFAULT_PASTE_FORMULA,
priority: 10,
onPasteCells: (pasteFrom, pasteTo, data, payload) => {
const copyInfo = {
copyType: payload.copyType || COPY_TYPE.COPY,
copyRange: pasteFrom?.range,
};
const pastedRange = pasteTo.range;
const matrix = data;
const workbook = this._currentUniverSheet.getCurrentUniverSheetInstance();
const unitId = workbook.getUnitId();
const subUnitId = workbook.getActiveSheet().getSheetId();

return this._injector.invoke((accessor) =>
getSetCellFormulaMutations(
unitId,
subUnitId,
pastedRange,
matrix,
accessor,
copyInfo,
this._lexerTreeBuilder
)
);
},
onPasteCells: (pasteFrom, pasteTo, data, payload) => this._onPasteCells(pasteFrom, pasteTo, data, payload, false),
};
}

return specialPasteFormulaHook;
private _onPasteCells(
pasteFrom: ISheetRangeLocation | null,
pasteTo: ISheetRangeLocation,
data: ObjectMatrix<ICellDataWithSpanInfo>,
payload: ICopyPastePayload,
isSpecialPaste: boolean
) {
const copyInfo = {
copyType: payload.copyType || COPY_TYPE.COPY,
copyRange: pasteFrom?.range,
};
const pastedRange = pasteTo.range;
const matrix = data;
const workbook = this._currentUniverSheet.getCurrentUniverSheetInstance();
const unitId = workbook.getUnitId();
const subUnitId = workbook.getActiveSheet().getSheetId();

return this._injector.invoke((accessor) => getSetCellFormulaMutations(
unitId,
subUnitId,
pastedRange,
matrix,
accessor,
copyInfo,
this._lexerTreeBuilder,
isSpecialPaste
));
}
}

Expand Down Expand Up @@ -165,11 +143,11 @@ export function getSetCellFormulaMutations(

matrix.forValue((row, col, value) => {
const originalFormula = value.f || '';
const valueObject: ICellDataWithSpanInfo = {};

let valueObject: ICellDataWithSpanInfo = {};
// Paste the formula only, you also need to process some regular values
if (isSpecialPaste) {
valueObject.v = value.v;
valueObject = Tools.deepClone(value);
}

if (isFormulaString(originalFormula) && copyRowLength && copyColumnLength) {
Expand Down
13 changes: 1 addition & 12 deletions packages/sheets-ui/src/controllers/clipboard/utils.ts
View file
Expand Up @@ -287,18 +287,7 @@ export function getSetCellValueMutations(
const valueMatrix = new ObjectMatrix<ICellData>();

matrix.forValue((row, col, value) => {
valueMatrix.setValue(row + startRow, col + startColumn, {
v: value.v,
});
// if (value.p?.body) {
// valueMatrix.setValue(row + startRow, col + startColumn, {
// p: {
// body: {
// dataStream: value.p.body.dataStream,
// },
// },
// });
// }
valueMatrix.setValue(row + startRow, col + startColumn, Tools.deepClone(value));
});
// set cell value and style
const setValuesMutation: ISetRangeValuesMutationParams = {
Expand Down
3 changes: 2 additions & 1 deletion packages/sheets-ui/src/controllers/editor/__tests__/end-edit.controller.spec.ts
View file
Expand Up @@ -164,6 +164,7 @@ describe('Test EndEditController', () => {
v: 0,
t: 2,
};

const inputCell = {
v: '',
};
Expand All @@ -175,7 +176,7 @@ describe('Test EndEditController', () => {
}

const cellData = getCellDataByInput(cell, documentLayoutObject, lexerTreeBuilder);
expect(cellData).toEqual({ v: '', f: null, si: null, p: null, t: 2 });
expect(cellData).toEqual({ v: '', f: null, si: null, p: null, t: undefined });
});
});
});
4 changes: 2 additions & 2 deletions packages/sheets-ui/src/controllers/editor/end-edit.controller.ts
View file
Expand Up @@ -392,19 +392,19 @@ export function getCellDataByInput(
cellData = Tools.deepClone(cellData);

const { documentModel } = documentLayoutObject;

if (documentModel == null) {
return null;
}

const snapshot = documentModel.getSnapshot();

const { body } = snapshot;

if (body == null) {
return null;
}

cellData.t = undefined;

const data = body.dataStream;
const lastString = data.substring(data.length - 2, data.length);
let newDataStream = lastString === DEFAULT_EMPTY_DOCUMENT_VALUE ? data.substring(0, data.length - 2) : data;
Expand Down
2 changes: 1 addition & 1 deletion packages/sheets-ui/src/index.ts
View file
Expand Up @@ -57,7 +57,7 @@ export {
PREDEFINED_HOOK_NAME,
SheetClipboardService,
} from './services/clipboard/clipboard.service';
export type { ICellDataWithSpanInfo, ISheetClipboardHook } from './services/clipboard/type';
export type { ICellDataWithSpanInfo, ISheetClipboardHook, ISheetRangeLocation, ICopyPastePayload } from './services/clipboard/type';
export { COPY_TYPE } from './services/clipboard/type';
export { getRepeatRange } from './services/clipboard/utils';
export { CellEditorManagerService, ICellEditorManagerService } from './services/editor/cell-editor-manager.service';
Expand Down
11 changes: 8 additions & 3 deletions packages/sheets-ui/src/services/auto-fill/tools.ts
View file
Expand Up @@ -15,7 +15,7 @@
*/

import type { ICellData, IObjectMatrixPrimitiveType, IRange, Nullable } from '@univerjs/core';
import { Direction, ObjectMatrix, Tools } from '@univerjs/core';
import { CellValueType, Direction, ObjectMatrix, Tools } from '@univerjs/core';

export const chnNumChar = { 零: 0, 一: 1, 二: 2, 三: 3, 四: 4, 五: 5, 六: 6, 七: 7, 八: 8, 九: 9 };
export const chnNumChar2 = ['零', '一', '二', '三', '四', '五', '六', '七', '八', '九'];
Expand Down Expand Up @@ -394,7 +394,9 @@ export function fillSeries(data: Array<Nullable<ICellData>>, len: number, direct
const num = Number(data[data.length - 1]?.v) * (Number(data[1]?.v) / Number(data[0]?.v)) ** i;

if (d) {
d.v = num;
if (d.t !== CellValueType.BOOLEAN) {
d.v = num;
}
applyData.push(d);
}
}
Expand All @@ -408,11 +410,14 @@ export function fillSeries(data: Array<Nullable<ICellData>>, len: number, direct
const y = forecast(data.length + i, dataNumArr, xArr, forward);

if (d) {
d.v = y;
if (d.t !== CellValueType.BOOLEAN) {
d.v = y;
}
applyData.push(d);
}
}
}

return applyData;
}

Expand Down
50 changes: 50 additions & 0 deletions packages/sheets/src/commands/mutations/__tests__/set-range-values.mutation.spec.ts
View file
@@ -0,0 +1,50 @@
/**
* Copyright 2023-present DreamNum Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { describe, expect, it } from 'vitest';
import { CellValueType } from '@univerjs/core';
import { checkCellValueType, extractBooleanValue } from '../set-range-values.mutation';

describe('test "SetRangeValuesMutation" ', () => {
describe('test type related utils', () => {
it('should be able to get the correct type of cell value from "checkCellValueType"', () => {
expect(checkCellValueType('string', CellValueType.BOOLEAN)).toBe(CellValueType.STRING);
expect(checkCellValueType('string', CellValueType.NUMBER)).toBe(CellValueType.STRING);
expect(checkCellValueType('string', CellValueType.STRING)).toBe(CellValueType.STRING);

expect(checkCellValueType(123, CellValueType.STRING)).toBe(CellValueType.NUMBER);
expect(checkCellValueType(123, CellValueType.BOOLEAN)).toBe(CellValueType.NUMBER); // not a valid boolean number, casted to number
expect(checkCellValueType(123, CellValueType.NUMBER)).toBe(CellValueType.NUMBER);
expect(checkCellValueType(1, CellValueType.NUMBER)).toBe(CellValueType.NUMBER);
expect(checkCellValueType(0, CellValueType.NUMBER)).toBe(CellValueType.NUMBER);

expect(checkCellValueType(1, CellValueType.BOOLEAN)).toBe(CellValueType.BOOLEAN); // it is valid boolean value
expect(checkCellValueType(0, CellValueType.BOOLEAN)).toBe(CellValueType.BOOLEAN); // it is valid boolean value
});

it('should be able to cast values that can be casted to boolean', () => {
expect(extractBooleanValue(1)).toBe(true);
expect(extractBooleanValue(0)).toBe(false);
expect(extractBooleanValue(-123)).toBe(null);
expect(extractBooleanValue('1')).toBe(true);
expect(extractBooleanValue('0')).toBe(false);
expect(extractBooleanValue('89757')).toBe(null);
expect(extractBooleanValue('true')).toBe(true);
expect(extractBooleanValue('false')).toBe(false);
expect(extractBooleanValue('Michael Jackson')).toBe(null);
});
});
});
76 changes: 69 additions & 7 deletions packages/sheets/src/commands/mutations/set-range-values.mutation.ts
View file
Expand Up @@ -173,10 +173,12 @@ export const SetRangeValuesMutation: IMutation<ISetRangeValuesMutationParams, bo
} else {
const oldVal = cellMatrix.getValue(row, col) || {};

const type =
newVal.t === CellValueType.FORCE_STRING
? newVal.t
: checkCellValueType(newVal.v === undefined ? oldVal.v : newVal.v);
// NOTE: we may need to take `p` into account
const type = (newVal.t === CellValueType.FORCE_STRING)
? newVal.t
: newVal.v !== undefined
? checkCellValueType(newVal.v, newVal.t)
: checkCellValueType(oldVal.v, oldVal.t);

if (newVal.f !== undefined) {
oldVal.f = newVal.f;
Expand All @@ -195,8 +197,7 @@ export const SetRangeValuesMutation: IMutation<ISetRangeValuesMutationParams, bo
oldVal.v = type === CellValueType.NUMBER
? Number(newVal.v)
: type === CellValueType.BOOLEAN
// if the value is a boolean, we should store it as 1 or 0
? (newVal.v!.toString()).toUpperCase() === 'TRUE' ? 1 : 0
? extractBooleanValue(newVal.v) ? 1 : 0
: newVal.v;
}

Expand Down Expand Up @@ -244,26 +245,87 @@ export const SetRangeValuesMutation: IMutation<ISetRangeValuesMutationParams, bo
},
};

function checkCellValueType(v: Nullable<CellValue>): Nullable<CellValueType> {
/**
* Get the correct type after setting values to a cell.
*
* @param v the new value
* @param oldType the old type
* @returns the new type
*/
export function checkCellValueType(v: Nullable<CellValue>, oldType: Nullable<CellValueType>): Nullable<CellValueType> {
if (v === null) return null;

if (typeof v === 'string') {
if (isSafeNumeric(v)) {
if ((+v === 0 || +v === 1) && oldType === CellValueType.BOOLEAN) {
return CellValueType.BOOLEAN;
}

return CellValueType.NUMBER;
} else if (isBooleanString(v)) {
return CellValueType.BOOLEAN;
}
return CellValueType.STRING;
}

if (typeof v === 'number') {
if ((v === 0 || v === 1) && oldType === CellValueType.BOOLEAN) {
return CellValueType.BOOLEAN;
}
return CellValueType.NUMBER;
}

if (typeof v === 'boolean') {
return CellValueType.BOOLEAN;
}

return CellValueType.FORCE_STRING;
}

/**
* Check if the value can be casted to a boolean.
* @internal
* @param value
* @returns It would return null if the value cannot be casted to a boolean, and would return the boolean value if it can be casted.
*/
export function extractBooleanValue(value: Nullable<string | number | boolean>): Nullable<boolean> {
if (typeof value === 'string') {
if (value.toUpperCase() === 'TRUE') {
return true;
};

if (value.toUpperCase() === 'FALSE') {
return false;
}

if (isSafeNumeric(value)) {
if (Number(value) === 0) {
return false;
}

if (Number(value) === 1) {
return true;
}
}
}

if (typeof value === 'number') {
if (value === 0) {
return false;
}

if (value === 1) {
return true;
}
}

if (typeof value === 'boolean') {
return value;
}

return null;
}

/**
* Convert old style data for storage
* @param style
Expand Down

0 comments on commit 227f5b0

Please sign in to comment.