Skip to content

Commit

Permalink
fix: fix render unit and interceptor memory leak
Browse files Browse the repository at this point in the history
  • Loading branch information
@wzhudev
wzhudev committed Feb 7, 2025
1 parent 9732f7b commit 121c8e3
Show file tree
Hide file tree
Showing 12 changed files with 113 additions and 119 deletions.
1 change: 1 addition & 0 deletions packages/engine-render/src/render-manager/render-manager.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export interface IRenderManagerService extends IDisposable {
getRenderUnitById(unitId: string): Nullable<IRender>;
getAllRenderersOfType(type: UniverInstanceType): RenderUnit[];
getCurrentTypeOfRenderer(type: UniverInstanceType): Nullable<RenderUnit>;
getCurrentTypeOfRenderer$(type: UniverInstanceType): Observable<Nullable<RenderUnit>>;
getRenderAll(): Map<string, IRender>;
defaultEngine: Engine;

Expand Down
9 changes: 0 additions & 9 deletions packages/engine-render/src/render-manager/render-unit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,6 @@ export class RenderUnit extends Disposable implements IRender {

this._activated$.next(false);
this._activated$.complete();

// Avoid memory leak. Basically it is because RenderUnit itself is leaking.
// We use this as a temporary solution to make CI pass.
// @ts-ignore
this._renderContext.activated$ = null;
// @ts-ignore
this._renderContext.activate = null;
// @ts-ignore
this._renderContext.deactivate = null;
}

/**
Expand Down
27 changes: 16 additions & 11 deletions packages/sheets-drawing-ui/src/services/canvas-float-dom-manager.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { COMMAND_LISTENER_SKELETON_CHANGE, getSheetCommandTarget, SetFrozenMutat
import { DrawingApplyType, ISheetDrawingService, SetDrawingApplyMutation } from '@univerjs/sheets-drawing';
import { ISheetSelectionRenderService, SetScrollOperation, SetZoomRatioOperation, SheetSkeletonManagerService } from '@univerjs/sheets-ui';
import { CanvasFloatDomService } from '@univerjs/ui';
import { BehaviorSubject, filter, map, Subject, switchMap, take } from 'rxjs';
import { BehaviorSubject, filter, map, of, Subject, switchMap, take } from 'rxjs';
import { InsertSheetDrawingCommand } from '../commands/commands/insert-sheet-drawing.command';

export interface ICanvasFloatDom {
Expand Down Expand Up @@ -516,22 +516,27 @@ export class SheetCanvasFloatDomManagerService extends Disposable {
// #region scroll
this.disposeWithMe(
this._univerInstanceService.getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET).pipe(
filter((workbook) => !!workbook),
switchMap((workbook) => workbook.activeSheet$),
filter((sheet) => !!sheet),
map((sheet) => {
const render = this._renderManagerService.getRenderById(sheet.getUnitId());
return render ? { render, unitId: sheet.getUnitId(), subUnitId: sheet.getSheetId() } : null;
switchMap((workbook) => workbook ? workbook.activeSheet$ : of(null)),
map((worksheet) => {
if (!worksheet) return null;
const unitId = worksheet.getUnitId();
const render = this._renderManagerService.getRenderById(unitId);
return render ? { render, unitId, subUnitId: worksheet.getSheetId() } : null;
}),
filter((render) => !!render),
switchMap((render) =>
fromEventSubject(render.render.scene.getViewport(SHEET_VIEWPORT_KEY.VIEW_MAIN)!.onScrollAfter$)
.pipe(map(() => ({ unitId: render.unitId, subUnitId: render.subUnitId })))
render
? fromEventSubject(render.render.scene.getViewport(SHEET_VIEWPORT_KEY.VIEW_MAIN)!.onScrollAfter$)
.pipe(map(() => ({ unitId: render.unitId, subUnitId: render.subUnitId })))
: of(null)
)
).subscribe(({ unitId, subUnitId }) => {
).subscribe((value) => {
if (!value) return; // TODO@weird94: maybe we should throw an error here and do some cleaning work?

const { unitId, subUnitId } = value;
updateSheet(unitId, subUnitId);
})
);

//#endregion

// #region zoom
Expand Down
6 changes: 3 additions & 3 deletions packages/sheets-filter/src/controllers/sheets-filter.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export class SheetsFilterController extends Disposable {
const filterColumn = filterModel.getAllFilterColumns();
const effected = filterColumn.filter((column) => column[0] >= anchor);
if (effected.length !== 0) {
const { newRange, oldRange } = this.moveCriteria(unitId, subUnitId, effected, count);
const { newRange, oldRange } = this._moveCriteria(unitId, subUnitId, effected, count);
redos.push(...newRange.redos, ...oldRange.redos);
undos.push(...newRange.undos, ...oldRange.undos);
}
Expand Down Expand Up @@ -281,7 +281,7 @@ export class SheetsFilterController extends Disposable {

let newRangeCriteria: { undos: IMutationInfo[]; redos: IMutationInfo[] } = { undos: [], redos: [] };
if (shifted.length > 0) {
const { oldRange, newRange } = this.moveCriteria(unitId, subUnitId, shifted, -removeCount);
const { oldRange, newRange } = this._moveCriteria(unitId, subUnitId, shifted, -removeCount);
newRangeCriteria = newRange;
redos.push(...oldRange.redos);
undos.unshift(...oldRange.undos);
Expand Down Expand Up @@ -693,7 +693,7 @@ export class SheetsFilterController extends Disposable {
}));
}

private moveCriteria(unitId: string, subUnitId: string, target: [number, FilterColumn][], step: number) {
private _moveCriteria(unitId: string, subUnitId: string, target: [number, FilterColumn][], step: number) {
const defaultSetCriteriaMutationParams: ISetSheetsFilterCriteriaMutationParams = {
unitId,
subUnitId,
Expand Down
3 changes: 2 additions & 1 deletion packages/sheets-filter/src/models/filter-model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@

import type { CellValue, ICellData, IRange, Nullable, Worksheet } from '@univerjs/core';
import type { Observable } from 'rxjs';
import type { IAutoFilter, ICustomFilter, ICustomFilters, IFilterColumn, IFilters } from './types';
import { CellValueType, Disposable, extractPureTextFromCell, mergeSets, Rectangle, Tools } from '@univerjs/core';
import { BehaviorSubject } from 'rxjs';
import { ensureNumeric, getCustomFilterFn, isNumericFilterFn, notEquals } from './custom-filters';
import { CustomFilterOperator, type IAutoFilter, type ICustomFilter, type ICustomFilters, type IFilterColumn, type IFilters } from './types';
import { CustomFilterOperator } from './types';

const EMPTY = () => new Set<number>();

Expand Down
123 changes: 56 additions & 67 deletions packages/sheets-ui/src/controllers/auto-fill.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import {
IUniverInstanceService,
ObjectMatrix,
Rectangle,
toDisposable,
Tools,
UniverInstanceType,
} from '@univerjs/core';
Expand Down Expand Up @@ -113,7 +112,7 @@ export class AutoFillController extends Disposable {

private _init() {
this._initDefaultHook();
this._onSelectionControlFillChanged();
this._initSelectionControlFillChanged();
this._initQuitListener();
this._initSkeletonChange();
}
Expand Down Expand Up @@ -179,11 +178,10 @@ export class AutoFillController extends Disposable {
this._autoFillService.setShowMenu(false);
}

// eslint-disable-next-line max-lines-per-function
private _onSelectionControlFillChanged() {
private _initSelectionControlFillChanged() {
const disposableCollection = new DisposableCollection();
const addListener = (disposableCollection: DisposableCollection) => {
// Each range change requires re-listening
const updateListener = () => {
// Each range change requires re-listening.
disposableCollection.dispose();

const currentRenderer = this._renderManagerService.getCurrentTypeOfRenderer(UniverInstanceType.UNIVER_SHEET);
Expand All @@ -192,78 +190,69 @@ export class AutoFillController extends Disposable {
const selectionRenderService = currentRenderer.with(ISheetSelectionRenderService);
const selectionControls = selectionRenderService.getSelectionControls();
selectionControls.forEach((controlSelection) => {
disposableCollection.add(
toDisposable(
controlSelection.selectionFilled$.subscribe((filled) => {
if (
filled == null ||
disposableCollection.add(controlSelection.selectionFilled$.subscribe((filled) => {
if (
filled == null ||
filled.startColumn === -1 ||
filled.startRow === -1 ||
filled.endColumn === -1 ||
filled.endRow === -1
) {
return;
}
const source: IRange = {
startColumn: controlSelection.model.startColumn,
endColumn: controlSelection.model.endColumn,
startRow: controlSelection.model.startRow,
endRow: controlSelection.model.endRow,
};
const selection: IRange = {
startColumn: filled.startColumn,
endColumn: filled.endColumn,
startRow: filled.startRow,
endRow: filled.endRow,
};

this._commandService.executeCommand(AutoFillCommand.id, { sourceRange: source, targetRange: selection });
})
)
);
) {
return;
}
const source: IRange = {
startColumn: controlSelection.model.startColumn,
endColumn: controlSelection.model.endColumn,
startRow: controlSelection.model.startRow,
endRow: controlSelection.model.endRow,
};
const selection: IRange = {
startColumn: filled.startColumn,
endColumn: filled.endColumn,
startRow: filled.startRow,
endRow: filled.endRow,
};

this._commandService.executeCommand(AutoFillCommand.id, { sourceRange: source, targetRange: selection });
}));

// double click to fill range, range length will align to left or right column.
// fill results will be as same as drag operation
disposableCollection.add(
toDisposable(
controlSelection.fillControl.onDblclick$.subscribeEvent(() => {
const source = {
startColumn: controlSelection.model.startColumn,
endColumn: controlSelection.model.endColumn,
startRow: controlSelection.model.startRow,
endRow: controlSelection.model.endRow,
};
this._handleDbClickFill(source);
})
)
);

disposableCollection.add(
toDisposable(
controlSelection.fillControl.onPointerDown$.subscribeEvent(() => {
const visibleState = this._editorBridgeService.isVisible();
if (visibleState.visible) {
this._editorBridgeService.changeVisible({
visible: false,
eventType: DeviceInputEventType.PointerDown,
unitId: currentRenderer.unitId,
});
}
})
)
);
disposableCollection.add(controlSelection.fillControl.onDblclick$.subscribeEvent(() => {
const source = {
startColumn: controlSelection.model.startColumn,
endColumn: controlSelection.model.endColumn,
startRow: controlSelection.model.startRow,
endRow: controlSelection.model.endRow,
};
this._handleDbClickFill(source);
}));

disposableCollection.add(controlSelection.fillControl.onPointerDown$.subscribeEvent(() => {
const visibleState = this._editorBridgeService.isVisible();
if (visibleState.visible) {
this._editorBridgeService.changeVisible({
visible: false,
eventType: DeviceInputEventType.PointerDown,
unitId: currentRenderer.unitId,
});
}
}));
});
};

addListener(disposableCollection);
updateListener();

this.disposeWithMe(
this._commandService.onCommandExecuted((command: ICommandInfo) => {
if (command.id === SetSelectionsOperation.id) {
addListener(disposableCollection);
}
})
);
// Should subscribe current current renderer change as well.
// TODO@yuhongz: this seems not ideal. This should be an `IRenderModule` for running with multiple renderers?
this.disposeWithMe(this._commandService.onCommandExecuted((command: ICommandInfo) => {
if (command.id === SetSelectionsOperation.id) {
updateListener();
}
}));

this.disposeWithMe(this._univerInstanceService.getCurrentTypeOfUnit$(UniverInstanceType.UNIVER_SHEET)
.subscribe(() => updateListener()));
}

private _handleDbClickFill(source: IRange) {
Expand Down
3 changes: 2 additions & 1 deletion packages/sheets-ui/src/controllers/clipboard/clipboard.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export class SheetClipboardController extends RxDisposable {
super();
this._init();
this._initCommandListener();
this._initUIComponents();
// this._initUIComponents();
this._pasteWithDoc();
}

Expand Down Expand Up @@ -913,6 +913,7 @@ export class SheetClipboardController extends RxDisposable {
if (sheetsUIConfig?.clipboardConfig?.hidePasteOptions) {
return;
}

this.disposeWithMe(
this._uiPartsService.registerComponent(BuiltInUIPart.CONTENT, () => connectInjector(ClipboardPopupMenu, this._injector))
);
Expand Down
7 changes: 4 additions & 3 deletions packages/sheets-ui/src/controllers/permission/sheet-permission-render.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import type { IRenderContext, IRenderModule, Spreadsheet } from '@univerjs/engine-render';
import type { MenuConfig } from '@univerjs/ui';
import type { IUniverSheetsUIConfig } from '../config.schema';
import { connectInjector, Disposable, IConfigService, Inject, Injector, IPermissionService, IUniverInstanceService } from '@univerjs/core';
import { IRenderManagerService } from '@univerjs/engine-render';
import { CheckMarkSingle, DeleteSingle, LockSingle, ProtectSingle, WriteSingle } from '@univerjs/icons';
Expand All @@ -30,7 +31,7 @@ import { UNIVER_SHEET_PERMISSION_ALERT_DIALOG } from '../../views/permission/err
import { RANGE_PROTECTION_CAN_NOT_VIEW_RENDER_EXTENSION_KEY, RANGE_PROTECTION_CAN_VIEW_RENDER_EXTENSION_KEY, RangeProtectionCanNotViewRenderExtension, RangeProtectionCanViewRenderExtension } from '../../views/permission/extensions/range-protection.render';
import { worksheetProtectionKey, WorksheetProtectionRenderExtension } from '../../views/permission/extensions/worksheet-permission.render';
import { PermissionDetailUserPart } from '../../views/permission/panel-detail/PermissionDetailUserPart';
import { type IUniverSheetsUIConfig, SHEETS_UI_PLUGIN_CONFIG_KEY } from '../config.schema';
import { SHEETS_UI_PLUGIN_CONFIG_KEY } from '../config.schema';

export interface IUniverSheetsPermissionMenuConfig {
menu: MenuConfig;
Expand Down Expand Up @@ -99,14 +100,14 @@ export class SheetPermissionRenderController extends Disposable implements IRend
this._initRender();
this._initSkeleton();

this._rangeProtectionRuleModel.ruleChange$.subscribe((info) => {
this.disposeWithMe(this._rangeProtectionRuleModel.ruleChange$.subscribe((info) => {
if ((info.oldRule?.id && this._rangeProtectionCanViewRenderExtension.renderCache.has(info.oldRule.id)) || this._rangeProtectionCanViewRenderExtension.renderCache.has(info.rule.id)) {
this._rangeProtectionCanViewRenderExtension.clearCache();
}
if ((info.oldRule?.id && this._rangeProtectionCanNotViewRenderExtension.renderCache.has(info.oldRule.id)) || this._rangeProtectionCanNotViewRenderExtension.renderCache.has(info.rule.id)) {
this._rangeProtectionCanNotViewRenderExtension.clearCache();
}
});
}));
}

private _initRender(): void {
Expand Down
1 change: 1 addition & 0 deletions packages/sheets-ui/src/services/sheet-skeleton-manager.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export class SheetSkeletonManagerService extends Disposable implements IRenderMo
this._currentSkeletonBefore$.complete();
this._currentSkeleton$.complete();
this._sheetSkeletonParamStore = new Map();
this._sheetSkService.deleteSkeleton(this._context.unitId, this._sheetId);
});

this._initRemoveSheet();
Expand Down
3 changes: 1 addition & 2 deletions packages/sheets-ui/src/views/clipboard/ClipboardPopupMenu.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { DropdownOverlay, DropdownProvider, DropdownTrigger } from '@univerjs/de
import { convertTransformToOffsetX, convertTransformToOffsetY, IRenderManagerService } from '@univerjs/engine-render';
import { CheckMarkSingle, MoreDownSingle, PasteSpecial } from '@univerjs/icons';
import clsx from 'clsx';
import React, { useCallback, useEffect, useState } from 'react';
import { useCallback, useEffect, useState } from 'react';
import { SheetOptionalPasteCommand } from '../../commands/commands/clipboard.command';
import { SetScrollOperation } from '../../commands/operations/scroll.operation';
import { useActiveWorkbook, useSheetSkeleton } from '../../components/hook';
Expand Down Expand Up @@ -113,7 +113,6 @@ export const ClipboardPopupMenu = () => {
const clipboardService = useDependency(ISheetClipboardService);
const showMenu = useObservable(clipboardService.showMenu$, true);
const pasteOptionsCache = useObservable(clipboardService.pasteOptionsCache$, null);
const renderManagerService = useDependency(IRenderManagerService);
const localeService = useDependency(LocaleService);
const commandService = useDependency(ICommandService);

Expand Down
Loading

0 comments on commit 121c8e3

Please sign in to comment.