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.

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

fix(item-set): don't leak memory when setting groups #316

Merged
merged 2 commits into from
Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/timeline/Timeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,20 @@ export default class Timeline extends Core {
}
}

// This looks weird but it's necessary to prevent memory leaks.
//
// The problem is that the DataView will exist as long as the DataSet it's
// connected to. This will force it to swap the groups DataSet for it's own
// DataSet. In this arrangement it will become unreferenced from the outside
// and garbage collected.
//
// IMPORTANT NOTE: If `this.groupsData` is a DataView was created in this
// method. Even if the original is a DataView already a new one has been
// created and assigned to `this.groupsData`. In case this changes in the
// future it will be necessary to rework this!!!!
if (this.groupsData instanceof DataView) {
this.groupsData.setData(null);
}
this.groupsData = newDataSet;
this.itemSet.setGroups(newDataSet);
}
Expand Down
31 changes: 27 additions & 4 deletions lib/timeline/component/Group.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ class Group {
this.isVisible = null;
this.stackDirty = true; // if true, items will be restacked on next redraw

// This is a stack of functions (`() => void`) that will be executed before
// the instance is disposed off (method `dispose`). Anything that needs to
// be manually disposed off before garbage collection happens (or so that
// garbage collection can happen) should be added to this stack.
this._disposeCallbacks = [];

if (data && data.nestedGroups) {
this.nestedGroups = data.nestedGroups;
if (data.showNested == false) {
Expand Down Expand Up @@ -83,10 +89,14 @@ class Group {
byEnd: []
};
this.checkRangedItems = false; // needed to refresh the ranged items if the window is programatically changed with NO overlap.
const me = this;
this.itemSet.body.emitter.on("checkRangedItems", () => {
me.checkRangedItems = true;
})

const handleCheckRangedItems = () => {
this.checkRangedItems = true;
};
this.itemSet.body.emitter.on("checkRangedItems", handleCheckRangedItems);
this._disposeCallbacks.push(() => {
this.itemSet.body.emitter.off("checkRangedItems", handleCheckRangedItems);
});

this._create();

Expand Down Expand Up @@ -1136,6 +1146,19 @@ class Group {
this._addToSubgroup(item, newSubgroup);
this.orderSubgroups();
}

/**
* Call this method before you lose the last reference to an instance of this.
* It will remove listeners etc.
*/
dispose() {
this.hide();

let disposeCallback;
while ((disposeCallback = this._disposeCallbacks.pop())) {
disposeCallback();
}
}
}

export default Group;
9 changes: 4 additions & 5 deletions lib/timeline/component/ItemSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ class ItemSet extends Component {
if (this.groupsData) {
// remove the group holding all ungrouped items
if (ungrouped) {
ungrouped.hide();
ungrouped.dispose();
delete this.groups[UNGROUPED];

for (itemId in this.items) {
Expand Down Expand Up @@ -1289,13 +1289,12 @@ class ItemSet extends Component {
* @private
*/
_onRemoveGroups(ids) {
const groups = this.groups;
ids.forEach(id => {
const group = groups[id];
const group = this.groups[id];

if (group) {
group.hide();
delete groups[id];
group.dispose();
delete this.groups[id];
}
});

Expand Down