Skip to content

Commit

Permalink
Merge pull request #36 from martinRenou/avoid_making_copies
Browse files Browse the repository at this point in the history 
Avoid making copies
  • Loading branch information
@martinRenou
martinRenou committed Feb 20, 2019
2 parents ce4a3a1 + 0558164 commit 4c1a118
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 88 deletions.
92 changes: 34 additions & 58 deletions js/src/sheet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,11 @@ let SheetModel = widgets.DOMWidgetModel.extend({
}, this);
},
cells_to_grid: function() {
let data = cloneDeep(this.get('data'));
let data = this.get('data');
each(this.get('cells'), (cell) => {
this._cell_data_to_grid(cell, data)
})
this.set('data', data);
this.save_changes()
});
this.set_data(data);
},
_cell_data_to_grid: function(cell, data) {
let value = cell.get('value');
Expand Down Expand Up @@ -184,7 +183,7 @@ let SheetModel = widgets.DOMWidgetModel.extend({
},
update_data_grid: function() {
// create a row x column array of arrays filled with null
let data = cloneDeep(this.get('data')); // clone, otherwise backbone/underscore won't notice the change
let data = this.get('data');
let rows = this.get('rows');
let columns = this.get('columns');

Expand Down Expand Up @@ -212,8 +211,17 @@ let SheetModel = widgets.DOMWidgetModel.extend({
}
data[i] = row;
}
this.set('data', data);
this.save_changes();
this.set_data(data);
},
set_data: function(new_value) {
this.set('data', new_value);

// Workaround for:
// - sending the changes to Python
// - triggering the change event
// This workaround is faster than making a deep copy of data
this.sync("update", this);
this.trigger("change:data");
}
}, {
serializers: extend({
Expand Down Expand Up @@ -250,55 +258,35 @@ Handsontable.renderers.registerRenderer('styled', function customRenderer(hotIns
});
});

let testing = false;
let setTesting = function() {
testing = true;
};

let SheetView = widgets.DOMWidgetView.extend({
render: function() {
/*
We debounce rendering of the table, since rendering can take quite some time, however that
makes unittesting difficult, since the results don't happend instanely. Maybe a better solution
is to use a deferred object.
*/
if(testing) {
this.throttled_on_data_change = this._real_on_data_change;
this.throttled_render = this._real_table_render;

} else {
this.throttled_on_data_change = debounce(() => this._real_on_data_change(), 100);
this.throttled_render = debounce(() => this._real_table_render(), 100);
}
//
//this.listenTo(this.model, 'change:data', this.on_data_change)
this.displayed.then(() => {
this._build_table().then(hot => {
this._build_table().then((hot) => {
this.hot = hot;
Handsontable.hooks.add('afterChange', () => this._on_change(), this.hot);
Handsontable.hooks.add('afterRemoveCol', () => this._on_change_grid(), this.hot);
Handsontable.hooks.add('afterRemoveRow', () => this._on_change_grid(), this.hot);
this.model.on('change:data', this.on_data_change, this);
this.model.on('change:column_headers change:row_headers', this._update_hot_settings, this);
this.model.on('change:stretch_headers change:column_width', this._update_hot_settings, this);
});
});
this.model.on('change:data', this.on_data_change, this);
this.model.on('change:column_headers change:row_headers', this._update_hot_settings, this);
this.model.on('change:stretch_headers change:column_width', this._update_hot_settings, this);
},
processPhosphorMessage: function(msg) {
SheetView.__super__.processPhosphorMessage.apply(this, arguments);
switch (msg.type) {
case 'resize':
case 'after-show':
this.throttled_render();
this.table_render();
break;
}
},
_build_table(options) {
return Promise.resolve(new Handsontable(this.el, extend({}, options, {
_build_table() {
return Promise.resolve(new Handsontable(this.el, extend({
data: this._get_cell_data(),
rowHeaders: true,
colHeaders: true,
cells: (...args) => this._cell(...args)
cells: (...args) => this._cell(...args),
afterChange: (changes, source) => { this._on_change(changes, source); },
afterRemoveCol: (changes, source) => { this._on_change_grid(changes, source); },
afterRemoveRow: (changes, source) => { this._on_change_grid(changes, source); }
}, this._hot_settings())));
},
_update_hot_settings: function() {
Expand Down Expand Up @@ -338,9 +326,9 @@ let SheetView = widgets.DOMWidgetView.extend({
this.model.save_changes();
},
_on_change: function(changes, source) {
//*
if(source == 'loadData')
return; // ignore loadData
if(this.hot === undefined || source == 'loadData') {
return;
}
if(source == 'alter') {
let data = this.hot.getSourceDataArray();
this.model.set({'rows': data.length, 'columns': data[0].length});
Expand All @@ -352,42 +340,32 @@ let SheetView = widgets.DOMWidgetView.extend({
//this.hot.validateCells(_.bind(function(valid){
// console.log('valid?', valid)
// if(valid) {
let data = cloneDeep(this.model.get('data'));
let data = this.model.get('data');
let value_data = this.hot.getSourceDataArray();
put_values2d(data, value_data);
this.model.set('data', cloneDeep(data));
this.model.save_changes();
this.model.set_data(data);
// }
//}, this))
/**/
},
on_data_change: function() {
this.throttled_on_data_change();
//this._real_on_data_change()
},
_real_on_data_change: function() {
let data = extract2d(this.model.get('data'), 'value');
let rows = data.length;
let cols = data[0].length;
let changed = false;
let rows_previous = this.hot.countRows();
let cols_previous = this.hot.countCols();
//*
if(rows > rows_previous) {
this.hot.alter('insert_row', rows-1, rows-rows_previous);
changed = true;
}
if(rows < this.hot.countRows()) {
this.hot.alter('remove_row', rows-1, rows_previous-rows);
changed = true;
}
if(cols > cols_previous) {
this.hot.alter('insert_col', cols-1, cols-cols_previous);
changed = true;
}
if(cols < cols_previous) {
this.hot.alter('remove_col', cols-1, cols_previous-cols);
changed = true;
}/**/

this.hot.loadData(data);
Expand All @@ -398,16 +376,15 @@ let SheetView = widgets.DOMWidgetView.extend({
colHeaders: this.model.get('column_headers'),
rowHeaders: this.model.get('row_headers')
});
this.throttled_render();
//this.hot.render()
this.table_render();
},
set_cell: function(row, column, value) {
this.hot.setDataAtCell(row, column, value);
},
get_cell: function(row, column) {
return this.hot.getDataAtCell(row, column);
},
_real_table_render: function() {
table_render: function() {
this.hot.render();
}
});
Expand All @@ -418,6 +395,5 @@ export {
SheetModel,
SheetView,
CellRangeModel,
Handsontable,
setTesting
Handsontable
};
2 changes: 0 additions & 2 deletions js/src/test/test_renderer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import { extend } from 'lodash';
// @ts-ignore
import {Handsontable} from 'handsontable';

ipysheet.setTesting()

describe('renderer', function() {
beforeEach(async function() {
this.manager = new DummyManager({ipysheet: extend(ipysheet, ipysheet_renderer)});
Expand Down
53 changes: 25 additions & 28 deletions js/src/test/test_sheet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,26 @@ import * as ipysheet from '../sheet';
import {DummyManager} from './dummy-manager';
import { expect } from 'chai';

ipysheet.setTesting()
var make_view = async function() {
const options = { model: this.sheet };
const view = await this.manager.create_view(this.sheet); //new ipysheet.SheetView(options);
await this.manager.display_view(undefined, view);
return view;
}

var data_cloner = function() {
var data = this.sheet.get('data')
return JSON.parse(JSON.stringify(data))
}

var wait_validate = async function(view) {
return new Promise(function(resolve, reject) {
view.hot.validateCells(function(valid) {
//console.log('waited for validate,', valid)
resolve(valid)
})
})
}

describe('sheet', function() {
beforeEach(async function() {
Expand Down Expand Up @@ -64,48 +83,26 @@ describe('sheet', function() {
expect(this.sheet.get('data')[0]).to.have.lengthOf(5);
expect(this.sheet.get('data')[1]).to.have.lengthOf(5);
})
var make_view = async function() {
const options = { model: this.sheet };
const view = await this.manager.create_view(this.sheet); //new ipysheet.SheetView(options);
await this.manager.display_view(undefined, view);
return view;
}
var data_cloner = function() {
var data = this.sheet.get('data')
return JSON.parse(JSON.stringify(data))
}
var wait_validate = async function(view) {
return new Promise(function(resolve, reject) {
view.hot.validateCells(function(valid) {
//console.log('waited for validate,', valid)
resolve(valid)
})
})
}
it('model reflecting view', async function() {
var view = await make_view.call(this)
view.set_cell(1,2, 123)
await wait_validate(view)
expect(this.sheet.get('data')[1][2].value, 'cell changes should be reflected in model').to.equal(123);
/**/
})
it('view reflecting model', async function() {
var view = await make_view.call(this)
var data = this.sheet.get('data')
console.log(data_clone)
var data_clone = data_cloner.call(this)
data_clone[1][2].value = 123
expect(data[1][2].value, 'cloned data check').to.not.equal(123);
this.sheet.set('data', data_clone)
data[1][2].value = 123
this.sheet.set_data(data)
expect(view.get_cell(1,2), 'model change should be reflected in view').to.equal(123);
})
it('view reflecting different view', async function() {
var view1 = await make_view.call(this)
var view2 = await make_view.call(this)
view1.set_cell(1,2, 123)
view1.set_cell(1, 2, 123)
var bla = await wait_validate(view1)
expect(view1.get_cell(1,2), 'model change should be reflected in view').to.equal(123);
expect(view1.get_cell(1,2), 'cell changes in one view should be reflected in a related view'). to.equal(view2.get_cell(1,2));
expect(view1.get_cell(1, 2), 'model change should be reflected in view').to.equal(123);
expect(view1.get_cell(1, 2), 'cell changes in one view should be reflected in a related view'). to.equal(view2.get_cell(1,2));
})
// we don't validate at the moment
it.skip('invalid sheet should not propagate to model', async function() {
Expand Down

0 comments on commit 4c1a118

Please sign in to comment.