Skip to content

Commit

Permalink
fix: do not use move_end$ to set a selection, use ONLY_SET
Browse files Browse the repository at this point in the history
  • Loading branch information
@lumixraku
lumixraku committed Nov 14, 2024
1 parent 528fcb0 commit 90d8c06
Show file tree
Hide file tree
Showing 14 changed files with 123 additions and 116 deletions.
2 changes: 1 addition & 1 deletion e2e/disposing/disposing.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ test('no error on constructing and disposing sheet unit', async ({ page }) => {
errored = true;
});

await page.goto('http://localhost:3000/sheets/', { waitUntil: 'networkidle' });
await page.goto('http://localhost:3000/sheets/');
await page.evaluate(() => window.E2EControllerAPI.disposeCurrSheetUnit());
await page.evaluate(() => window.E2EControllerAPI.loadDefaultSheet());

Expand Down
20 changes: 16 additions & 4 deletions examples-node/src/locales.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,45 +17,57 @@
import { Tools } from '@univerjs/core';

import sheetsenUS from '@univerjs/sheets/locale/en-US';
import sheetsfaIR from '@univerjs/sheets/locale/fa-IR';
import sheetsruRU from '@univerjs/sheets/locale/ru-RU';
import sheetsviVN from '@univerjs/sheets/locale/vi-VN';
import sheetszhCN from '@univerjs/sheets/locale/zh-CN';
import sheetszhTW from '@univerjs/sheets/locale/zh-TW';
import sheetsviVN from '@univerjs/sheets/locale/vi-VN';
import sheetsfaIR from '@univerjs/sheets/locale/fa-IR';
import sheetsformulaenUS from '@univerjs/sheets-formula/locale/en-US';
import sheetsformularuRU from '@univerjs/sheets-formula/locale/ru-RU';
import sheetsformulazhCN from '@univerjs/sheets-formula/locale/zh-CN';
import sheetsformulazhTW from '@univerjs/sheets-formula/locale/zh-TW';
import sheetsformulaviVN from '@univerjs/sheets-formula/locale/vi-VN';
import sheetsformulafaIR from '@univerjs/sheets-formula/locale/fa-IR';
import sheetssortenUS from '@univerjs/sheets-sort/locale/en-US';
import sheetssortfaIR from '@univerjs/sheets-sort/locale/fa-IR';
import sheetssortruRU from '@univerjs/sheets-sort/locale/ru-RU';
import sheetssortviVN from '@univerjs/sheets-sort/locale/vi-VN';
import sheetssortzhCN from '@univerjs/sheets-sort/locale/zh-CN';
import sheetssortzhTW from '@univerjs/sheets-sort/locale/zh-TW';
import sheetssortviVN from '@univerjs/sheets-sort/locale/vi-VN';
import sheetssortfaIR from '@univerjs/sheets-sort/locale/fa-IR';

export const enUS = Tools.deepMerge(
{},
sheetsenUS,
sheetsformulaenUS,
sheetssortenUS
);
export const ruRU = Tools.deepMerge(
{},
sheetsruRU,
sheetsformularuRU,
sheetssortruRU
);
export const zhCN = Tools.deepMerge(
{},
sheetszhCN,
sheetsformulazhCN,
sheetssortzhCN
);
export const zhTW = Tools.deepMerge(
{},
sheetszhTW,
sheetsformulazhTW,
sheetssortzhTW
);
export const viVN = Tools.deepMerge(
{},
sheetsviVN,
sheetsformulaviVN,
sheetssortviVN
);
export const faIR = Tools.deepMerge(
{},
sheetsfaIR,
sheetsformulafaIR,
sheetssortfaIR
);
43 changes: 20 additions & 23 deletions packages/sheets-formula-ui/src/services/render-services/ref-selections.render-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { ScrollTimerType, SHEET_VIEWPORT_KEY, Vector2 } from '@univerjs/engine-r
import { convertSelectionDataToRange, IRefSelectionsService, SelectionMoveType } from '@univerjs/sheets';
import { attachSelectionWithCoord, BaseSelectionRenderService, checkInHeaderRanges, genNormalSelectionStyle, getAllSelection, getCoordByOffset, getSheetObject, SelectionControl, SheetSkeletonManagerService } from '@univerjs/sheets-ui';
import { IShortcutService } from '@univerjs/ui';
import { merge } from 'rxjs';

/**
* This service extends the existing `SelectionRenderService` to provide the rendering of prompt selections
Expand Down Expand Up @@ -175,21 +174,7 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
const scene = this._scene;

selectionWithStyle.style = style;
const selectionWithCoord = attachSelectionWithCoord(selectionWithStyle, skeleton);
const control = this.newSelectionControl(scene, skeleton, selectionWithCoord);
// TODO: memory leak? This extension seems never released.

control.setControlExtension({
skeleton,
scene,
themeService: this._themeService,
injector: this._injector,
selectionHooks: {
selectionMoveEnd: (): void => {
this._selectionMoveEnd$.next(this.getSelectionDataWithStyle());
},
},
});
const control = this.newSelectionControl(scene, skeleton, selectionWithStyle);
return control;
}

Expand Down Expand Up @@ -225,7 +210,7 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
// Changing the selection area through the 8 control points of the ref selection will not trigger this subscriber.

// beforeSelectionMoveEnd$ & selectionMoveEnd$ would triggered when change skeleton(change sheet).
this.disposeWithMe(merge(this._workbookSelections.selectionMoveEnd$, this._workbookSelections.selectionSet$).subscribe((selectionsWithStyles) => {
this.disposeWithMe(this._workbookSelections.selectionSet$.subscribe((selectionsWithStyles) => {
this._reset();
const skeleton = this._skeleton;
if (!skeleton) return;
Expand Down Expand Up @@ -334,11 +319,10 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
}

const selectionWithStyle: ISelectionWithStyle = { range: selectCell, primary: selectCell, style: null };

selectionWithStyle.range.rangeType = rangeType;
// const selectionCellWithCoord = this._getSelectionWithCoordByOffset(offsetX, offsetY, scaleX, scaleY, scrollXY);
const selectionCellWithCoord = attachSelectionWithCoord(selectionWithStyle, this._skeleton);
selectionCellWithCoord.rangeWithCoord.rangeType = rangeType;
this._startRangeWhenPointerDown = { ...selectionCellWithCoord.rangeWithCoord, rangeType };
this._startRangeWhenPointerDown = { ...selectionCellWithCoord.rangeWithCoord };

const cursorCellRangeWithRangeType: IRangeWithCoord = { ...selectionCellWithCoord.rangeWithCoord, rangeType };

Expand Down Expand Up @@ -384,7 +368,7 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
activeSelectionControl.updateRangeBySelectionWithCoord(selectionCellWithCoord);
} else {
// In normal situation, pointerdown ---> Create new SelectionControl,
activeSelectionControl = this.newSelectionControl(scene, skeleton, selectionCellWithCoord);
activeSelectionControl = this.newSelectionControl(scene, skeleton, selectionWithStyle);
}
// clear highlight except last one.
for (let i = 0; i < this.getSelectionControls().length - 1; i++) {
Expand Down Expand Up @@ -432,9 +416,9 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
* @param scene
* @param skeleton
* @param selectionWithCoord
* @returns
* @returns {SelectionControl} selectionControl just created
*/
override newSelectionControl(scene: Scene, skeleton: SpreadsheetSkeleton, selectionWithCoord: ISelectionWithCoord): SelectionControl {
override newSelectionControl(scene: Scene, skeleton: SpreadsheetSkeleton, selection: ISelectionWithStyle): SelectionControl {
const zIndex = this.getSelectionControls().length;
const { rowHeaderWidth, columnHeaderHeight } = skeleton;
const control = new SelectionControl(scene, zIndex, this._themeService, {
Expand All @@ -443,8 +427,21 @@ export class RefSelectionsRenderService extends BaseSelectionRenderService imple
rowHeaderWidth,
columnHeaderHeight,
});
const selectionWithCoord = attachSelectionWithCoord(selection, skeleton);
control.updateRangeBySelectionWithCoord(selectionWithCoord);
this._selectionControls.push(control);

control.setControlExtension({
skeleton,
scene,
themeService: this._themeService,
injector: this._injector,
selectionHooks: {
selectionMoveEnd: (): void => {
this._selectionMoveEnd$.next(this.getSelectionDataWithStyle());
},
},
});
return control;
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/sheets-ui/src/commands/commands/__tests__/set-format-painter.command.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import type { DependencyIdentifier, Injector, Univer, Workbook } from '@univerjs/core';
import type { ISelectionWithCoord } from '@univerjs/sheets';
import {
Disposable,
ICommandService,
Expand All @@ -26,7 +27,6 @@ import {
UniverInstanceType,
} from '@univerjs/core';
import { IRenderManagerService } from '@univerjs/engine-render';
import type { ISelectionWithCoordAndStyle } from '@univerjs/sheets';
import {
AddWorksheetMergeMutation,
RemoveWorksheetMergeMutation,
Expand All @@ -39,14 +39,14 @@ import { beforeEach, describe, expect, it } from 'vitest';

import { FormatPainterController } from '../../../controllers/format-painter/format-painter.controller';
import { FormatPainterService, IFormatPainterService } from '../../../services/format-painter/format-painter.service';
import { IMarkSelectionService } from '../../../services/mark-selection/mark-selection.service';
import { ISheetSelectionRenderService } from '../../../services/selection/base-selection-render.service';
import { SetFormatPainterOperation } from '../../operations/set-format-painter.operation';
import {
ApplyFormatPainterCommand,
SetInfiniteFormatPainterCommand,
SetOnceFormatPainterCommand,
} from '../set-format-painter.command';
import { IMarkSelectionService } from '../../../services/mark-selection/mark-selection.service';
import { ISheetSelectionRenderService } from '../../../services/selection/base-selection-render.service';
import { createCommandTestBed } from './create-command-test-bed';

const theme = {
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('Test format painter rules in controller', () => {

beforeEach(() => {
class SheetSelectionRenderService {
private readonly _selectionMoveEnd$ = new BehaviorSubject<ISelectionWithCoordAndStyle[]>([]);
private readonly _selectionMoveEnd$ = new BehaviorSubject<ISelectionWithCoord[]>([]);
readonly selectionMoveEnd$ = this._selectionMoveEnd$.asObservable();
}

Expand Down
2 changes: 1 addition & 1 deletion packages/sheets-ui/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export {
export { MarkSelectionService } from './services/mark-selection/mark-selection.service';
export { IMarkSelectionService } from './services/mark-selection/mark-selection.service';
export { SheetSelectionRenderService } from './services/selection/selection-render.service';
export { genSelectionByRange, getAllSelection, getTopLeftSelectionOfCurrSheet } from './services/selection/base-selection-render.service';
export { genSelectionByRange, getTopLeftSelectionOfCurrSheet, selectionDataForSelectAll as getAllSelection } from './services/selection/base-selection-render.service';
export { BaseSelectionRenderService, ISheetSelectionRenderService } from './services/selection/base-selection-render.service';
export { SelectionControl as SelectionShape, SelectionControl } from './services/selection/selection-control';
export { SelectionShapeExtension } from './services/selection/selection-shape-extension';
Expand Down
51 changes: 20 additions & 31 deletions packages/sheets-ui/src/services/selection/base-selection-render.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
this._singleSelectionEnabled = enabled;
}

newSelectionControl(scene: Scene, skeleton: SpreadsheetSkeleton, selectionWithCoord: ISelectionWithCoord): SelectionControl {
newSelectionControl(scene: Scene, skeleton: SpreadsheetSkeleton, selection: ISelectionWithStyle): SelectionControl {
const zIndex = this.getSelectionControls().length;
const { rowHeaderWidth, columnHeaderHeight } = skeleton;
const control = new SelectionControl(scene, zIndex, this._themeService, {
Expand All @@ -232,9 +232,21 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
columnHeaderHeight,
});
this._selectionControls.push(control);
selectionWithCoord.rangeWithCoord.rangeType = selectionWithCoord.rangeWithCoord.rangeType || RANGE_TYPE.NORMAL;
const selectionWithCoord = attachSelectionWithCoord(selection, skeleton);
control.updateRangeBySelectionWithCoord(selectionWithCoord);

control.setControlExtension({
skeleton,
scene,
themeService: this._themeService,
injector: this._injector,
selectionHooks: {
selectionMoveEnd: (): void => {
this._selectionMoveEnd$.next(this.getSelectionDataWithStyle());
},
},
});

return control;
}

Expand All @@ -257,7 +269,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
if (control) {
control.updateRangeBySelectionWithCoord(selectionWithCoord);
} else {
this.newSelectionControl(this._scene!, skeleton, selectionWithCoord);
this.newSelectionControl(this._scene!, skeleton, selectionWithStyle);
}
}
if (selectionsWithStyleList.length < allSelectionControls.length) {
Expand Down Expand Up @@ -320,21 +332,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
const scene = this._scene;

selectionWithStyle.style = style;
const selectionWithCoord = attachSelectionWithCoord(selectionWithStyle, skeleton);
const control = this.newSelectionControl(scene, skeleton, selectionWithCoord);

// TODO: memory leak? This extension seems never released.
control.setControlExtension({
skeleton,
scene,
themeService: this._themeService,
injector: this._injector,
selectionHooks: {
selectionMoveEnd: (): void => {
this._selectionMoveEnd$.next(this.getSelectionDataWithStyle());
},
},
});
const control = this.newSelectionControl(scene, skeleton, selectionWithStyle);
return control;
}

Expand Down Expand Up @@ -694,7 +692,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
if (this._shouldDetectMergedCells) {
newSelectionRange = skeleton.expandRangeByMerge(newSelectionRange);
}
const newSelection: ISelectionWithStyle = { range: newSelectionRange, style: null, primary: null };
const newSelection: ISelectionWithStyle = { range: newSelectionRange, primary: undefined, style: null };
const newSelectionRangeWithCoord = attachSelectionWithCoord(newSelection, skeleton);
newSelectionRangeWithCoord.rangeWithCoord.unitId = unitId;
newSelectionRangeWithCoord.rangeWithCoord.sheetId = sheetId;
Expand Down Expand Up @@ -846,17 +844,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
activeControl: SelectionControl
): void {
const { actualRow, actualColumn, mergeInfo: actualMergeInfo } = currentCell;
this._startRangeWhenPointerDown = {
startColumn: currentCell.mergeInfo.startColumn,
startRow: currentCell.mergeInfo.startRow,
endColumn: currentCell.mergeInfo.endColumn,
endRow: currentCell.mergeInfo.endRow,
startY: currentCell.mergeInfo.startY || 0,
endY: currentCell.mergeInfo.endY || 0,
startX: currentCell.mergeInfo.startX || 0,
endX: currentCell.mergeInfo.endX || 0,
rangeType,
};
this._startRangeWhenPointerDown = { ...currentCell.mergeInfo };

// Get the maximum range selected based on the two cells selected with Shift key.
const newStartRow = Math.min(actualRow, startSelectionRange.startRow, actualMergeInfo.startRow);
Expand Down Expand Up @@ -907,7 +895,7 @@ export class BaseSelectionRenderService extends Disposable implements ISheetSele
}
}

export function getAllSelection(skeleton: SpreadsheetSkeleton): ISelectionWithStyle {
export function selectionDataForSelectAll(skeleton: SpreadsheetSkeleton): ISelectionWithStyle {
return {
range: {
startRow: 0,
Expand Down Expand Up @@ -945,6 +933,7 @@ export function genSelectionByRange(skeleton: SpreadsheetSkeleton, range: IRange
startColumn: topLeftCell.startColumn,
endRow: bottomRightCell.endRow,
endColumn: bottomRightCell.endColumn,
rangeType: RANGE_TYPE.NORMAL,
},
primary: {
actualRow: topLeftCell.startRow,
Expand Down
Loading

0 comments on commit 90d8c06

Please sign in to comment.