From c75680c527844b3b984fa6dfbe04b612528cd9fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 5 May 2017 06:50:59 +0200 Subject: [REF] web: optimize basic model (improve perfs for most views) The 'get' method in the basic model is called very often (too often, but that is another issue). For a large list with many columns, it can be quickly be called more than 10k times. In that function, we had to protect our integrity by performing a deep clone with $.extend, so noone else can be allowed to change something in our internal structure. This is good in theory, but not so good in practice, as $.extend is a really expensive operation. Also, we try to have the smallest external API by deleting useless keys. But instead of keeping a blacklist of private keys, it is simpler to have a whitelist of public keys. So, what we do is to explicitely construct the object returned by key, since we know the types of every values involved. This is more explicit, and at the same time, much faster. Also, we propagate the options for elements of type list. This is why I had to change the isFieldSet method. In a benchmark I just ran, loading and rendering a large list (200 rows, 7 columns) went from about 2.7s to 0.7s. We still do way too much work, but at least it is much faster... --- .../web/static/src/js/views/basic/basic_model.js | 146 +++++++++++++-------- 1 file changed, 88 insertions(+), 58 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index 17cb4d277c2..abb7242661e 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -289,35 +289,27 @@ var BasicModel = AbstractModel.extend({ return null; } - var record = this.localData[id]; + var element = this.localData[id]; if (options.env) { var env = { - ids: record.res_ids ? record.res_ids.slice(0) : [], + ids: element.res_ids ? element.res_ids.slice(0) : [], }; - if (record.type === 'record') { - env.currentId = this.isNew(record.id) ? undefined : record.res_id; + if (element.type === 'record') { + env.currentId = this.isNew(element.id) ? undefined : element.res_id; } return env; } - // do not copy fields: this has a really big performance cost, for views - // with many records and lots of fields (for ex, kanban view contacts) - var fields = record.fields; - delete record.fields; - var element = $.extend(true, {}, record); - record.fields = fields; - element.fields = fields; - var field, relDataPoint; if (element.type === 'record') { - // apply changes - _.extend(element.data, element._changes); - for (var fieldName in element.data) { - field = element.fields[fieldName]; - if (element.data[fieldName] === null) { - element.data[fieldName] = false; + var data = _.extend({}, element.data, element._changes); + var relDataPoint; + for (var fieldName in data) { + var field = element.fields[fieldName]; + if (data[fieldName] === null) { + data[fieldName] = false; } if (!field) { continue; @@ -326,72 +318,110 @@ var BasicModel = AbstractModel.extend({ if (field.type === 'float' || field.type === 'integer' || field.type === 'monetary') { - element.data[fieldName] = element.data[fieldName] || 0; + data[fieldName] = data[fieldName] || 0; } } // get relational datapoint if (field.type === 'many2one') { if (options.raw) { - relDataPoint = this.localData[element.data[fieldName]]; - element.data[fieldName] = relDataPoint ? relDataPoint.res_id : false; + relDataPoint = this.localData[data[fieldName]]; + data[fieldName] = relDataPoint ? relDataPoint.res_id : false; } else { - element.data[fieldName] = this.get(element.data[fieldName]) || false; + data[fieldName] = this.get(data[fieldName]) || false; } } if (field.type === 'one2many' || field.type === 'many2many') { if (options.raw) { - relDataPoint = this.localData[element.data[fieldName]]; - var ids = _.map(relDataPoint.data, function (id) { + relDataPoint = this.localData[data[fieldName]]; + var relData = relDataPoint._changes || relDataPoint.data; + var ids = _.map(relData, function (id) { return self.localData[id].res_id; }); - element.data[fieldName] = ids; + data[fieldName] = ids; } else { - element.data[fieldName] = this.get(element.data[fieldName]) || []; + data[fieldName] = this.get(data[fieldName]) || []; } } } + var record = { + context: _.extend({}, element.context), + count: element.count, + data: data, + domain: element.domain.slice(0), + fields: element.fields, + fieldsInfo: element.fieldsInfo, + getContext: element.getContext, + getDomain: element.getDomain, + getEvalContext: element.getEvalContext, + getFieldNames: element.getFieldNames, + id: element.id, + limit: element.limit, + model: element.model, + offset: element.offset, + res_ids: element.res_ids.slice(0), + specialData: _.extend({}, element.specialData), + type: 'record', + viewType: element.viewType, + }; - // this is not strictly necessary, but it hides some implementation - // details, and can easily be removed if needed. - delete element.orderedBy; - delete element.aggregateValues; - delete element.groupedBy; - if (this.isNew(element.id)) { - delete element.res_id; + if (!this.isNew(element.id)) { + record.res_id = element.res_id; } var evalContext; - Object.defineProperty(element, 'evalContext', { + Object.defineProperty(record, 'evalContext', { get: function () { - evalContext = evalContext || self._getEvalContext(record); + evalContext = evalContext || self._getEvalContext(element); return evalContext; }, }); + return record; } - if (element.type === 'list') { - // apply changes if any - if (element._changes) { - element.data = element._changes; - element.count = element._changes.length; - element.res_ids = _.map(element._changes, function (elemID) { - return self.localData[elemID].res_id; - }); - } - // get relational datapoint - element.data = _.map(element.data, function (elemID) { - return self.get(elemID); + + // here, type === 'list' + var listData, listCount, resIDs; + if (element._changes) { + listData = element._changes; + listCount = listData.length; + resIDs = _.map(listData, function (elemID) { + return self.localData[elemID].res_id; }); + } else { + listData = element.data; + listCount = element.count; + resIDs = element.res_ids; } - - delete element._cache; - delete element._changes; - delete element._forceM2MLink; - delete element.static; - delete element.parentID; - delete element.relation_field; - delete element.rawContext; - - return element; + listData = _.map(listData, function (elemID) { + return self.get(elemID, options); + }); + var list = { + aggregateValues: _.extend({}, element.aggregateValues), + context: _.extend({}, element.context), + count: listCount, + data: listData, + domain: element.domain.slice(0), + fields: element.fields, + getContext: element.getContext, + getDomain: element.getDomain, + getEvalContext: element.getEvalContext, + getFieldNames: element.getFieldNames, + groupedBy: element.groupedBy, + id: element.id, + isOpen: element.isOpen, + limit: element.limit, + model: element.model, + offset: element.offset, + orderedBy: element.orderedBy, + res_id: element.res_id, + res_ids: resIDs, + type: 'list', + value: element.value, + viewType: element.viewType, + }; + if (element.fieldsInfo) { + list.fieldsInfo = element.fieldsInfo; + } + return list; }, /** * Returns true if a record is dirty. A record is considered dirty if it has @@ -2038,7 +2068,7 @@ var BasicModel = AbstractModel.extend({ return true; case 'one2many': case 'many2many': - return value.count > 0; + return value.length > 0; default: return value !== false; } -- cgit v1.2.3 From 1c5ea99b7b82c2b6f28ad1a3dbffc1300fda6466 Mon Sep 17 00:00:00 2001 From: Christophe Matthieu Date: Thu, 4 May 2017 13:27:31 +0200 Subject: [FIX] web: display a translation alert when saving the form view In the form view, when the user saves a record (with a multilang instance), an alert is displayed to display all modified translatable fields. --- .../web/static/src/js/views/basic/basic_model.js | 34 ++++++++++++++-------- 1 file changed, 22 insertions(+), 12 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index abb7242661e..ca042d63d69 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -708,6 +708,7 @@ var BasicModel = AbstractModel.extend({ * be 'locally' saved: its changes written in a _savePoint key that can * be restored later by call discardChanges with option rollback to true * @returns {Deferred} + * Resolved with the list of field names (whose value has been modified) */ save: function (record_id, options) { var self = this; @@ -723,7 +724,6 @@ var BasicModel = AbstractModel.extend({ rec._savePoint = _.extend({}, newValue); } }); - return $.when(); } var shouldReload = 'reload' in options ? options.reload : true; var method = self.isNew(record_id) ? 'create' : 'write'; @@ -742,10 +742,17 @@ var BasicModel = AbstractModel.extend({ }); } + var def = $.Deferred(); + var changedFields = Object.keys(changes); + + if (options.savePoint) { + return def.resolve(changedFields); + } + // in the case of a write, only perform the RPC if there are changes to save - if (method === 'create' || Object.keys(changes).length) { + if (method === 'create' || changedFields.length) { var args = method === 'write' ? [[record.data.id], changes] : [changes]; - return self._rpc({ + self._rpc({ model: record.model, method: method, args: args, @@ -759,23 +766,26 @@ var BasicModel = AbstractModel.extend({ record.count++; } - // Update the data directly or reload them - var def; - if (shouldReload) { - def = self._fetchRecord(record); - } else { - _.extend(record.data, record._changes); - } + var _changes = record._changes; // Erase changes as they have been applied record._changes = {}; record._isDirty = false; - return def; + // Update the data directly or reload them + if (shouldReload) { + self._fetchRecord(record).then(function (record) { + def.resolve(changedFields); + }); + } else { + _.extend(record.data, _changes); + def.resolve(changedFields); + } }); } else { - return $.when(record_id); + def.resolve(changedFields); } + return def; }); }, /** -- cgit v1.2.3 From a0217f4475b02660fc687d154f1287029ed0e23d Mon Sep 17 00:00:00 2001 From: Martin Geubelle Date: Fri, 12 May 2017 13:22:01 +0200 Subject: [FIX] web: BasicModel: update parent data after record deletion After the record deletion, the parent should be updated to take the deletion into account. This was causing an issue in the Kanban view: after a column deletion, it appeared again if you created a new one. --- .../web/static/src/js/views/basic/basic_model.js | 23 ++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index ca042d63d69..f6c4d6ea23b 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -180,15 +180,16 @@ var BasicModel = AbstractModel.extend({ }); }, /** - * Delete a list of records, then, if a parentID is given, reload the - * parent. + * Delete a list of records, then, if the records have a parent, reload it. * * @todo we should remove the deleted records from the localData - * @todo why can't we infer modelName? + * @todo why can't we infer modelName? Because of grouped datapoint + * --> res_id doesn't correspond to the model and we don't have the + * information about the related model * * @param {string[]} recordIds list of local resources ids. They should all - * be of type 'record', and of the same model - * @param {string} modelName + * be of type 'record', be of the same model and have the same parent. + * @param {string} modelName mode name used to unlink the records * @returns {Deferred} */ deleteRecords: function (recordIds, modelName) { @@ -202,9 +203,15 @@ var BasicModel = AbstractModel.extend({ }) .then(function () { _.each(records, function (record) { - record.res_ids.splice(record.offset, 1); - record.res_id = record.res_ids[record.offset]; - record.count--; + var parent = record.parentID && self.localData[record.parentID]; + if (parent && parent.type === 'list') { + parent.data = _.without(parent.data, record.id); + delete self.localData[record.id]; + } else { + record.res_ids.splice(record.offset, 1); + record.res_id = record.res_ids[record.offset]; + record.count--; + } }); }); }, -- cgit v1.2.3 From 55de60db6e2fb7851526c258f1801f7d321974b7 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Mon, 15 May 2017 10:19:44 +0200 Subject: [FIX] web: BasicModel: missing fields in data for x2m The 'data' attribute of exported datapoints (using 'get') must contain a value for each field in the view, and the default value is false when those fields are unset. The code manipulating those datapoints supposes that this is the case and it makes it easier and clean. However, it wasn't the case for x2manys in new records for which the default value contains CREATE commands with missing fields: in that case, those missing fields weren't added in data. This caused a crash in the Employee form view, in edit, if the user clicked on 'Create and Edit' of the 'Working hours' field, because some code in field_utils assumes that if the value of a date field isn't false, it means that it is a moment instance (but in that particular case it was undefined). --- addons/web/static/src/js/views/basic/basic_model.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index f6c4d6ea23b..5e5b36472f5 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -2314,13 +2314,13 @@ var BasicModel = AbstractModel.extend({ }); x2manyList._changes = x2manyList._changes || []; x2manyList._changes.push(r.id); - r._changes = value[2]; // this is necessary so the fields are initialized - for (var fieldName in value[2]) { + _.each(r.getFieldNames(), function (fieldName) { r.data[fieldName] = null; - } + }); + r._changes = _.defaults(value[2], r.data); for (var name in r._changes) { var isFieldInView = name in r.fields; if (isFieldInView && r.fields[name].type === 'many2one') { -- cgit v1.2.3