Skip to content

Commit

Permalink
fix: fix memory leak on dispose sheet unit (#1900)
Browse files Browse the repository at this point in the history 
* fix: fix memory leak on dispose sheet unit

* chore: comment testing code

* fix: fix lint errors

* fix: fix menu error
  • Loading branch information
@wzhudev
wzhudev authored Apr 14, 2024
1 parent 496dcb8 commit 4a5eca1
Show file tree
Hide file tree
Showing 36 changed files with 499 additions and 470 deletions.
5 changes: 5 additions & 0 deletions examples/src/sheets/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ univer.registerPlugin(UniverSheetsFindReplacePlugin);
// create univer sheet instance
univer.createUniverSheet(DEFAULT_WORKBOOK_DATA_DEMO);

// Uncomment the following lines to test if the document is disposed correctly without memory leaks.
// setTimeout(() => {
// univer.__getInjector().get(IUniverInstanceService).disposeDocument(DEFAULT_WORKBOOK_DATA_DEMO.id);
// }, 5000);

// sheet condition formatting
univer.registerPlugin(UniverSheetsConditionalFormattingUIPlugin);

Expand Down
41 changes: 23 additions & 18 deletions packages/core/src/services/instance/instance.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,16 @@ export interface IUniverInstanceService {

export const IUniverInstanceService = createIdentifier<IUniverInstanceService>('univer.current');
export class UniverInstanceService extends Disposable implements IUniverInstanceService {
private _focused: DocumentDataModel | Workbook | SlideDataModel | null = null;
private readonly _focused$ = new BehaviorSubject<Nullable<string>>(null);
readonly focused$ = this._focused$.asObservable();
get focused(): Nullable<Workbook | DocumentDataModel | SlideDataModel> {
const id = this._focused$.getValue();
if (!id) {
return null;
}

return this.getUniverSheetInstance(id) || this.getUniverDocInstance(id) || this.getUniverSlideInstance(id);
}

private readonly _currentSheet$ = new BehaviorSubject<Nullable<Workbook>>(null);
readonly currentSheet$ = this._currentSheet$.asObservable();
Expand Down Expand Up @@ -229,11 +236,12 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
this._currentDoc$.next(this.getUniverDocInstance(id) || null);
}

getCurrentUniverSheetInstance(): Workbook {
getCurrentUniverSheetInstance(): Nullable<Workbook> {
const sheet = this._currentSheet$.getValue();
if (!sheet) {
throw new Error('No current sheet!');
return null;
}

return sheet;
}

Expand All @@ -256,29 +264,25 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
}

focusUniverInstance(id: string | null): void {
if (id) {
this._focused =
this.getUniverSheetInstance(id) ||
this.getUniverDocInstance(id) ||
this.getUniverSlideInstance(id) ||
null;
}

this._focused$.next(id);

[FOCUSING_DOC, FOCUSING_SHEET, FOCUSING_SLIDE].forEach((k) => this._contextService.setContextValue(k, false));

if (this._focused instanceof Workbook) {
if (this.focused instanceof Workbook) {
this._contextService.setContextValue(FOCUSING_DOC, false);
this._contextService.setContextValue(FOCUSING_SHEET, true);
} else if (this._focused instanceof DocumentDataModel) {
this._contextService.setContextValue(FOCUSING_SLIDE, false);
} else if (this.focused instanceof DocumentDataModel) {
this._contextService.setContextValue(FOCUSING_DOC, true);
} else if (this._focused instanceof SlideDataModel) {
this._contextService.setContextValue(FOCUSING_SHEET, false);
this._contextService.setContextValue(FOCUSING_SLIDE, false);
} else if (this.focused instanceof SlideDataModel) {
this._contextService.setContextValue(FOCUSING_DOC, false);
this._contextService.setContextValue(FOCUSING_SHEET, false);
this._contextService.setContextValue(FOCUSING_SLIDE, true);
}
}

getFocusedUniverInstance(): Nullable<Workbook | DocumentDataModel | SlideDataModel> {
return this._focused;
return this.focused;
}

getDocumentType(unitId: string): UniverInstanceType {
Expand Down Expand Up @@ -312,7 +316,8 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
const index = this._sheets.indexOf(sheet);
this._sheets.splice(index, 1);
this._sheetDisposed$.next(sheet);
// this.focusUniverInstance(null);
this.setCurrentUniverSheetInstance('');
this.focusUniverInstance(null);
return true;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/engine-render/src/render-manager.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export interface IRenderManagerService {
createRenderWithParent(unitId: string, parentUnitId: string): IRender;
createRender(unitId: string): IRender;
addItem(unitId: string, item: IRender): void;
removeItem(unitId: string): void;
removeRender(unitId: string): void;
setCurrent(unitId: string): void;
getRenderById(unitId: string): Nullable<IRender>;
getRenderAll(): Map<string, IRender>;
Expand Down Expand Up @@ -163,7 +163,7 @@ export class RenderManagerService implements IRenderManagerService {
this._renderMap.set(unitId, item);
}

removeItem(unitId: string) {
removeRender(unitId: string) {
const item = this._renderMap.get(unitId);
if (item != null) {
this._disposeItem(item);
Expand Down
2 changes: 1 addition & 1 deletion packages/rpc/src/services/remote-instance/remote-instance.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export interface IRemoteSyncMutationOptions extends IExecutionOptions {
fromSync?: boolean;
}

export const RemoteSyncServiceName = 'univer.remote-sync-service';
export const RemoteSyncServiceName = 'rpc.remote-sync.service';
/**
* This service is provided by the primary Univer.
*
Expand Down
15 changes: 12 additions & 3 deletions packages/sheets-conditional-formatting-ui/src/controllers/cf.render.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import { ConditionalFormattingIcon, ConditionalFormattingRuleModel, ConditionalF

import type { IConditionalFormattingCellData } from '@univerjs/sheets-conditional-formatting';

/**
* @todo RenderUnit
*/
@OnLifecycle(LifecycleStages.Rendered, RenderController)
export class RenderController extends Disposable {
constructor(@Inject(SheetInterceptorService) private _sheetInterceptorService: SheetInterceptorService,
Expand Down Expand Up @@ -72,22 +75,28 @@ export class RenderController extends Disposable {

// After the conditional formatting is marked dirty to drive a rendering, to trigger the window within the conditional formatting recalculation
this.disposeWithMe(this._conditionalFormattingViewModel.markDirty$.pipe(bufferTime(16), filter((v) => {
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance()!;
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return false;

const worksheet = workbook.getActiveSheet();
return v.filter((item) => item.unitId === workbook.getUnitId() && item.subUnitId === worksheet.getSheetId()).length > 0;
})).subscribe(markDirtySkeleton));

// Sort and delete does not mark dirty.
this.disposeWithMe(this._conditionalFormattingRuleModel.$ruleChange.pipe(bufferTime(16), filter((v) => {
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance()!;
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return false;

const worksheet = workbook.getActiveSheet();
return v.filter((item) => ['sort', 'delete'].includes(item.type) && item.unitId === workbook.getUnitId() && item.subUnitId === worksheet.getSheetId()).length > 0;
})).subscribe(markDirtySkeleton));

// Once the calculation is complete, a view update is triggered
// This rendering does not trigger conditional formatting recalculation,because the rule is not mark dirty
this.disposeWithMe(this._conditionalFormattingService.ruleComputeStatus$.pipe(bufferTime(16), filter((v) => {
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance()!;
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return false;

const worksheet = workbook.getActiveSheet();
return v.filter((item) => item.unitId === workbook.getUnitId() && item.subUnitId === worksheet.getSheetId()).length > 0;
})).subscribe(markDirtySkeleton));
Expand Down
154 changes: 77 additions & 77 deletions packages/sheets-conditional-formatting-ui/src/menu/manage-rule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,102 +27,59 @@ import { AddConditionalRuleMutation, ConditionalFormattingRuleModel, DeleteCondi

import { CF_MENU_OPERATION, OpenConditionalFormattingOperator } from '../commands/operations/open-conditional-formatting-panel';

const commandList = [SetWorksheetActiveOperation.id, AddConditionalRuleMutation.id, SetConditionalRuleMutation.id, DeleteConditionalRuleMutation.id, MoveConditionalRuleMutation.id];

export const FactoryManageConditionalFormattingRule = (componentManager: ComponentManager) => {
const key = 'conditional-formatting-menu-icon';
componentManager.register(key, Conditions);
return (_accessor: IAccessor) => {
const localeService = _accessor.get(LocaleService);
const selectionManagerService = _accessor.get(SelectionManagerService);
const commandService = _accessor.get(ICommandService);
const univerInstanceService = _accessor.get(IUniverInstanceService);
const conditionalFormattingRuleModel = _accessor.get(ConditionalFormattingRuleModel);

const commandList = [SetWorksheetActiveOperation.id, AddConditionalRuleMutation.id, SetConditionalRuleMutation.id, DeleteConditionalRuleMutation.id, MoveConditionalRuleMutation.id];

const clearRangeEnable$ = new Observable<boolean>((subscriber) =>
merge(
selectionManagerService.selectionMoveEnd$,
new Observable<null>((commandSubscribe) => {
const disposable = commandService.onCommandExecuted((commandInfo) => {
const { id, params } = commandInfo;
const unitId = univerInstanceService.getCurrentUniverSheetInstance()!.getUnitId();
if (commandList.includes(id) && (params as { unitId: string }).unitId === unitId) {
commandSubscribe.next(null);
}
});
return () => disposable.dispose();
})
).pipe(debounceTime(16)).subscribe(() => {
const ranges = selectionManagerService.getSelections()?.map((selection) => selection.range) || [];
const unitId = univerInstanceService.getCurrentUniverSheetInstance()!.getUnitId();
const subUnitId = univerInstanceService.getCurrentUniverSheetInstance()!.getActiveSheet().getSheetId();
const allRule = conditionalFormattingRuleModel.getSubunitRules(unitId, subUnitId) || [];
const ruleList = allRule.filter((rule) => rule.ranges.some((ruleRange) => ranges.some((range) => Rectangle.intersects(range, ruleRange))));
subscriber.next(!!ruleList.length);
return (accessor: IAccessor) => {
const localeService = accessor.get(LocaleService);
const selectionManagerService = accessor.get(SelectionManagerService);
const commandService = accessor.get(ICommandService);
const univerInstanceService = accessor.get(IUniverInstanceService);
const conditionalFormattingRuleModel = accessor.get(ConditionalFormattingRuleModel);
const clearRangeEnable$ = new Observable<boolean>((subscriber) => merge(
selectionManagerService.selectionMoveEnd$,
new Observable<null>((commandSubscribe) => {
const disposable = commandService.onCommandExecuted((commandInfo) => {
const { id, params } = commandInfo;
const unitId = univerInstanceService.getCurrentUniverSheetInstance()?.getUnitId();
if (commandList.includes(id) && (params as { unitId: string }).unitId === unitId) {
commandSubscribe.next(null);
}
});
return () => disposable.dispose();
})
);
).pipe(debounceTime(16)).subscribe(() => {
const ranges = selectionManagerService.getSelections()?.map((selection) => selection.range) || [];
const workbook = univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return;
const allRule = conditionalFormattingRuleModel.getSubunitRules(workbook.getUnitId(), workbook.getActiveSheet().getSheetId()) || [];
const ruleList = allRule.filter((rule) => rule.ranges.some((ruleRange) => ranges.some((range) => Rectangle.intersects(range, ruleRange))));
subscriber.next(!!ruleList.length);
}));
const clearSheetEnable$ = new Observable<boolean>((subscriber) =>
merge(
selectionManagerService.selectionMoveEnd$,
new Observable<null>((commandSubscribe) => {
const disposable = commandService.onCommandExecuted((commandInfo) => {
const { id, params } = commandInfo;
const unitId = univerInstanceService.getCurrentUniverSheetInstance()!.getUnitId();
const unitId = univerInstanceService.getCurrentUniverSheetInstance()?.getUnitId();
if (commandList.includes(id) && (params as { unitId: string }).unitId === unitId) {
commandSubscribe.next(null);
}
});
return () => disposable.dispose();
})
).pipe(debounceTime(16)).subscribe(() => {
const unitId = univerInstanceService.getCurrentUniverSheetInstance()!.getUnitId();
const subUnitId = univerInstanceService.getCurrentUniverSheetInstance()!.getActiveSheet().getSheetId();
const allRule = conditionalFormattingRuleModel.getSubunitRules(unitId, subUnitId) || [];
const workbook = univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return;
const allRule = conditionalFormattingRuleModel.getSubunitRules(workbook.getUnitId(), workbook.getActiveSheet().getSheetId()) || [];
subscriber.next(!!allRule.length);
})
);
const selections$ = new Observable((subscriber) => {
const commonSelections = [
{
label: localeService.t('sheet.cf.ruleType.highlightCell'),
value: CF_MENU_OPERATION.highlightCell,
},
{
label: localeService.t('sheet.cf.panel.rankAndAverage'),
value: CF_MENU_OPERATION.rank,
},
{
label: localeService.t('sheet.cf.ruleType.formula'),
value: CF_MENU_OPERATION.formula,
},
{
label: localeService.t('sheet.cf.ruleType.colorScale'),
value: CF_MENU_OPERATION.colorScale,
},
{
label: localeService.t('sheet.cf.ruleType.dataBar'),
value: CF_MENU_OPERATION.dataBar,
}, {
label: localeService.t('sheet.cf.ruleType.iconSet'),
value: CF_MENU_OPERATION.icon,
},
{
label: localeService.t('sheet.cf.menu.manageConditionalFormatting'),
value: CF_MENU_OPERATION.viewRule,
}, {
label: localeService.t('sheet.cf.menu.createConditionalFormatting'),
value: CF_MENU_OPERATION.createRule,
},
{
label: localeService.t('sheet.cf.menu.clearRangeRules'),
value: CF_MENU_OPERATION.clearRangeRules,
disabled: !clearRangeEnable$,
},
{
label: localeService.t('sheet.cf.menu.clearWorkSheetRules'),
value: CF_MENU_OPERATION.clearWorkSheetRules,
},
];
const commonSelections = getCommonSelection(localeService);
clearRangeEnable$.subscribe((v) => {
const item = commonSelections.find((item) => item.value === CF_MENU_OPERATION.clearRangeRules);
if (item) {
Expand All @@ -139,7 +96,6 @@ export const FactoryManageConditionalFormattingRule = (componentManager: Compone
});
subscriber.next(commonSelections);
});

return {
id: OpenConditionalFormattingOperator.id,
type: MenuItemType.SELECTOR,
Expand All @@ -148,7 +104,51 @@ export const FactoryManageConditionalFormattingRule = (componentManager: Compone
icon: key,
tooltip: localeService.t('sheet.cf.title'),
selections: selections$,
hidden$: getMenuHiddenObservable(_accessor, UniverInstanceType.SHEET),
hidden$: getMenuHiddenObservable(accessor, UniverInstanceType.SHEET),
} as IMenuSelectorItem;
};
};

function getCommonSelection(localeService: LocaleService) {
return [
{
label: localeService.t('sheet.cf.ruleType.highlightCell'),
value: CF_MENU_OPERATION.highlightCell,
},
{
label: localeService.t('sheet.cf.panel.rankAndAverage'),
value: CF_MENU_OPERATION.rank,
},
{
label: localeService.t('sheet.cf.ruleType.formula'),
value: CF_MENU_OPERATION.formula,
},
{
label: localeService.t('sheet.cf.ruleType.colorScale'),
value: CF_MENU_OPERATION.colorScale,
},
{
label: localeService.t('sheet.cf.ruleType.dataBar'),
value: CF_MENU_OPERATION.dataBar,
}, {
label: localeService.t('sheet.cf.ruleType.iconSet'),
value: CF_MENU_OPERATION.icon,
},
{
label: localeService.t('sheet.cf.menu.manageConditionalFormatting'),
value: CF_MENU_OPERATION.viewRule,
}, {
label: localeService.t('sheet.cf.menu.createConditionalFormatting'),
value: CF_MENU_OPERATION.createRule,
},
{
label: localeService.t('sheet.cf.menu.clearRangeRules'),
value: CF_MENU_OPERATION.clearRangeRules,
disabled: false,
},
{
label: localeService.t('sheet.cf.menu.clearWorkSheetRules'),
value: CF_MENU_OPERATION.clearWorkSheetRules,
},
];
}
7 changes: 6 additions & 1 deletion packages/sheets-data-validation/src/controllers/dv-render.controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ const INVALID_MARK = {
},
};

/**
* @todo RenderUnit
*/
@OnLifecycle(LifecycleStages.Rendered, DataValidationRenderController)
export class DataValidationRenderController extends RxDisposable {
constructor(
Expand Down Expand Up @@ -169,7 +172,9 @@ export class DataValidationRenderController extends RxDisposable {

private _initSkeletonChange() {
const markSkeletonDirty = () => {
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance()!;
const workbook = this._univerInstanceService.getCurrentUniverSheetInstance();
if (!workbook) return;

const unitId = workbook.getUnitId();
const subUnitId = workbook.getActiveSheet().getSheetId();
const skeleton = this._sheetSkeletonManagerService.getOrCreateSkeleton({ unitId, sheetId: subUnitId });
Expand Down
Loading

0 comments on commit 4a5eca1

Please sign in to comment.