search

SitePen / dgrid

A lightweight, mobile-ready, data-driven, modular grid widget designed for use with dstore
http://dgrid.io/
Other
628 stars 298 forks source link

Editor clobbers _alwaysOnWidgetColumns when mixed-in with ColumnSet #1412

Closed scotth7447 closed 4 years ago

scotth7447 commented 6 years ago

Editor._configColumns() sets this._alwaysOnWidgetColumns = [], and then pushes columns onto that array in _configureEditorColumn() for always-on edit columns.

ColumnSet.configStructure() calls this._configColumns() multiple times, once per item in this.columnSets.

When you have a grid with both ColumnSet and Editor mixedin, Editor clobbers the this._alwaysOnWidgetColumns array for the first (n-1) column sets. This means that when removing a row in removeRow(), it is not aware of the editor widgets in the first (n-1) column sets and does not destroy them. It properly disposes of the editor widgets for the cells in the last column set only. This results in a memory/widget leak in the registry.

I'm currently using dgrid 1.1.0, but I checked the latest source on github and it doesn't look like this has been noticed/fixed yet. I searched before submitting this issue and didn't see it mentioned.

edhager commented 6 years ago

I am working on a fix in this branch: https://github.com/edhager/dgrid/tree/editor-columnset-clobber-fix-1412

So far the unit tests pass. I'm going to write some more unit tests that combine Editor and ColumnSet and looks for memory leaks.

If you try out my branch, let me know if it works for you or not.

scotth7447 commented 6 years ago

Thanks for fixing this! However, I have two questions:

  1. Doesn't this only work if you declare the grid with declare([Grid, ColumnSet, Editor])? I think it will fail with declare([Grid, Editor, ColumnSet]) because ColumnSet.configStructure() calls Editor._configColumns() before it calls this.inherited(arguments). Would doing so violate a standard practice for dgrid mixin order?

  2. Does a similar fix need to be made for Tree._configColumns(), which also resets an instance property: this._expanded = {} - in the latest code, by calling this._resetExpanded()? I have never used Tree, so I do not know if it is compatible with ColumnSet.