Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AG-NONE Cleanup destroy #1571

Closed
wants to merge 5 commits into from
Closed

AG-NONE Cleanup destroy #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd2686b
Clean up memory management in ChartContext
olegat May 10, 2024
8ede733
Fix SonarLint errors
olegat May 13, 2024
132151c
CR: Add empty destroy() methods
olegat May 14, 2024
3af3262
Specify that these implement destroy() as part of an interface
olegat May 14, 2024
9c86fc5
nx format
olegat May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 37 additions & 38 deletions packages/ag-charts-community/src/chart/chartContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { ModuleContext } from '../module/moduleContext';
import type { Group } from '../scene/group';
import { Scene } from '../scene/scene';
import { CallbackCache } from '../util/callbackCache';
import { ObjectDestroyer } from '../util/destroy';
import type { Mutex } from '../util/mutex';
import { AnnotationManager } from './annotation/annotationManager';
import type { ChartService } from './chartService';
Expand Down Expand Up @@ -59,6 +60,8 @@ export class ChartContext implements ModuleContext {
tooltipManager: TooltipManager;
zoomManager: ZoomManager;

private readonly owned: ObjectDestroyer;

constructor(
chart: ChartService & { zoomManager: ZoomManager; annotationRoot: Group; keyboard: Keyboard; tooltip: Tooltip },
vars: {
Expand All @@ -78,48 +81,44 @@ export class ChartContext implements ModuleContext {
scene?.setContainer(this.domManager);
this.scene = scene ?? new Scene({ pixelRatio: overrideDevicePixelRatio, domManager: this.domManager });

this.annotationManager = new AnnotationManager(chart.annotationRoot);
this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element);
this.chartEventManager = new ChartEventManager();
this.contextMenuRegistry = new ContextMenuRegistry();
this.cursorManager = new CursorManager(this.domManager);
this.highlightManager = new HighlightManager();
this.interactionManager = new InteractionManager(chart.keyboard, this.domManager);
this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager);
this.focusIndicator = new FocusIndicator(this.domManager);
this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator);
this.toolbarManager = new ToolbarManager();
this.gestureDetector = new GestureDetector(this.domManager);
this.layoutService = new LayoutService();
this.updateService = new UpdateService(updateCallback);
this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator);
this.seriesStateManager = new SeriesStateManager();
this.callbackCache = new CallbackCache();

this.animationManager = new AnimationManager(this.interactionManager, updateMutex);
this.animationManager.skip();
this.animationManager.play();
// Sonar does not like us using assignments in expression, however this is intended.
// We want to use assignments so that the Typescript compiler can check that we are not using an
// uninitialised property, but we also want to guarantee that ObjectDestroyer knows the
// initialisation order so that it can destroy the objects in reverse.
this.owned = new ObjectDestroyer(
this.domManager,
(this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR
(this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element)), // NOSONAR
(this.chartEventManager = new ChartEventManager()), // NOSONAR
(this.contextMenuRegistry = new ContextMenuRegistry()), // NOSONAR
(this.cursorManager = new CursorManager(this.domManager)), // NOSONAR
(this.highlightManager = new HighlightManager()), // NOSONAR
(this.interactionManager = new InteractionManager(chart.keyboard, this.domManager)), // NOSONAR
(this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager)), // NOSONAR
(this.focusIndicator = new FocusIndicator(this.domManager)), // NOSONAR
(this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator)), // NOSONAR
(this.toolbarManager = new ToolbarManager()), // NOSONAR
(this.gestureDetector = new GestureDetector(this.domManager)), // NOSONAR
(this.layoutService = new LayoutService()), // NOSONAR
(this.updateService = new UpdateService(updateCallback)), // NOSONAR
(this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator)), // NOSONAR
(this.seriesStateManager = new SeriesStateManager()), // NOSONAR
(this.callbackCache = new CallbackCache()), // NOSONAR
(this.animationManager = this.createAnimationManager(this.interactionManager, updateMutex)), // NOSONAR
(this.dataService = new DataService<any>(this.animationManager)), // NOSONAR
(this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip)) // NOSONAR
Comment on lines +90 to +109
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These inline assignment and returns are not very readable, can you adjust the pattern here to avoid this and the need for // NOSONAR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is readable.

Sonar argues that assignments in expressions are discouraged is because they can have an unexpected side-effect. For example:

if (isEnabled && x = getValue()) {
  doSomething();
}

In this scenario, it is not clear what the programmer meant:

  • Assign x to a value if isEnabled is true?
  • Do something if isEnabled is true and x is assigned to a true-y value?
  • Or did the programmer mean == / === instead of =?

That can be confusing, because x might be assigned or it might not, and doSomething() might be called or it might not.

However, this argument is not applicable in this case. How can (this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR possibly be misunderstood? There no other logical operators like && and || so there is only 1 possible outcome from this expression.

Also, it is abundantly clear the intent is to assign a value and we didn't intend == or === (because this expression is part of ChartContext's constructor and also why would we want to compare an unassigned value to a newly constructed object?).

The aim objective of ObjectDestroyer is to follow a RAII model to ensure that:

  1. We do not pass uninitialised variables to the constructors
  2. We run the destructors in reverse

The only option to avoid the // NOSONAR could be to use lambdas methods that ObjectDestroyer can call:

  this.owned = new ObjectDestroyer(
    () => this.annotationManager = new AnnotationManager(chart.annotationRoot),
    // ...
);

However, this will violate objective 1). We could end up accidentally creating a circular dependency amongst constructors.

We could also apply the changes that Sonar suggests, which involve putting the assignments as separate expressions

this.annotationManager = new AnnotationManager(chart.annotationRoot);
this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element);
this.chartEventManager = new ChartEventManager();
// ...

this.owned = new ObjectDestroyer(
  this.domManager,
  this.annotationManager,
  this.ariaAnnouncementService,
  this.chartEventManager,
  // ..
);

However, this is not a good suggestion because objective 2) is to ensure that destructors are run in reverse. But with this approach we need to manually ensure the ordering is reversed, which is basically what we're doing now. So we might as well not bother adding ObjectDestroyer if we're going with this approach.

It is a pain to have to write // NOSONAR in each line. I would much prefer multi-line enable/disable options but I can't find something like that. Open to other suggestions to disable the Sonar warnings, but Sonar is wrong in this case.

);
}

this.dataService = new DataService<any>(this.animationManager);
this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip);
private createAnimationManager(interactionManager: InteractionManager, updateMutex: Mutex): AnimationManager {
const animationManager = new AnimationManager(interactionManager, updateMutex);
animationManager.skip();
animationManager.play();
return animationManager;
}

destroy() {
// chart.ts handles the destruction of the scene and zoomManager.
this.tooltipManager.destroy();
this.proxyInteractionService.destroy();
this.regionManager.destroy();
this.focusIndicator.destroy();
this.keyNavManager.destroy();
this.interactionManager.destroy();
this.animationManager.stop();
this.animationManager.destroy();
this.ariaAnnouncementService.destroy();
this.chartEventManager.destroy();
this.highlightManager.destroy();
this.callbackCache.invalidateCache();
this.animationManager.reset();
this.syncManager.destroy();
this.domManager.destroy();
this.owned.destroy();
}
}
6 changes: 5 additions & 1 deletion packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Destroyable } from '../../util/destroy';

export type ContextMenuAction = {
id?: string;
label: string;
Expand All @@ -12,10 +14,12 @@ export type ContextMenuActionParams = {
event: MouseEvent;
};

export class ContextMenuRegistry {
export class ContextMenuRegistry implements Destroyable {
private readonly defaultActions: Array<ContextMenuAction> = [];
private readonly disabledActions: Set<string> = new Set();

destroy() {}

public filterActions(region: string): ContextMenuAction[] {
return this.defaultActions.filter((action) => ['all', region].includes(action.region));
}
Expand Down
5 changes: 4 additions & 1 deletion packages/ag-charts-community/src/chart/interaction/cursorManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { Destroyable } from '../../util/destroy';
import { StateTracker } from '../../util/stateTracker';
import type { DOMManager } from '../dom/domManager';

Expand All @@ -19,11 +20,13 @@ export enum Cursor {
* Manages the cursor styling for an element. Tracks the requested styling from distinct
* dependents and handles conflicting styling requests.
*/
export class CursorManager {
export class CursorManager implements Destroyable {
private readonly stateTracker = new StateTracker('default');

constructor(private readonly domManager: DOMManager) {}

destroy() {}

public updateCursor(callerId: string, style?: string) {
this.stateTracker.set(callerId, style);
this.domManager.updateCursor(this.stateTracker.stateValue()!);
Expand Down
6 changes: 5 additions & 1 deletion packages/ag-charts-community/src/chart/series/seriesStateManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import type { Destroyable } from '../../util/destroy';

export type SeriesGrouping = {
groupIndex: number;
groupCount: number;
stackIndex: number;
stackCount: number;
};

export class SeriesStateManager {
export class SeriesStateManager implements Destroyable {
private readonly groups: {
[type: string]: {
[id: string]: {
Expand All @@ -15,6 +17,8 @@ export class SeriesStateManager {
};
} = {};

destroy() {}

public registerSeries({
id,
seriesGrouping,
Expand Down
5 changes: 4 additions & 1 deletion packages/ag-charts-community/src/util/callbackCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import type { Destroyable } from './destroy';
import { Logger } from './logger';

export class CallbackCache {
export class CallbackCache implements Destroyable {
private cache: WeakMap<Function, Map<string, any>> = new WeakMap();

destroy() {}

call<F extends (...args: any[]) => any>(fn: F, ...params: Parameters<F>): ReturnType<F> | undefined {
let serialisedParams: string;
let paramCache = this.cache.get(fn);
Expand Down
15 changes: 15 additions & 0 deletions packages/ag-charts-community/src/util/destroy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export type Destroyable = { destroy(): void };

export class ObjectDestroyer {
private readonly objs: Destroyable[];
constructor(...objs: Destroyable[]) {
this.objs = [...objs].reverse();
}
destroy() {
this.objs.forEach((o) => {
if ('destroy' in o && typeof o.destroy === 'function') {
o.destroy();
}
});
}
}