From dbdd742d2046f208c0057fdc60b359e5c88814ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 15 Dec 2017 10:06:03 +0100 Subject: [PATCH] [FIX] web: better mode handling, prevent useless work in some cases Before this commit, the way mode was handled was not optimal. The end result was correct (the widget were properly created/displayed in edit/readonly as required), but useless work was done. In particular, if we have list/x2manys with widgets and modifiers, it could happen that widgets were created in edit mode, then destroyed and recreated in readonly mode. This could happen in some important views, for example, any view with a field many2many_tags with a readonly modifiers (pretty much all views handling taxes) In this commit, we add a 'editable' attribute to list renderer. This is necessary, because the list renderer is not actually editable when it is in a x2many in a form view which is in readonly mode. Also, we need to manage the distinction between the fact that a list view is editable, and the fact that some widgets are created in edit and other in readonly mode, depending on modifiers, and on which line is being edited. We also add a benchmark (courtesy of CHM) to be able to measure the performance impact of edit/readonly/onchange in a larger form view. Before this commit, running this form benchmark on my machine gave a number of 0.4 op/sec, and after, it increases to 1.26 op/sec, so a pretty large improvement. --- .../static/src/js/fields/relational_fields.js | 2 +- .../src/js/views/basic/basic_renderer.js | 47 +++++---- .../js/views/list/list_editable_renderer.js | 11 +-- .../static/src/js/views/list/list_renderer.js | 3 +- .../web/static/src/js/views/list/list_view.js | 2 +- .../tests/fields/relational_fields_tests.js | 42 ++++++++ .../web/static/tests/views/form_benchmarks.js | 95 +++++++++++++++++++ addons/web/static/tests/views/list_tests.js | 6 +- addons/web/views/webclient_templates.xml | 1 + 9 files changed, 174 insertions(+), 35 deletions(-) create mode 100644 addons/web/static/tests/views/form_benchmarks.js diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js index 9cdadb1051a..73e474b0342 100644 --- a/addons/web/static/src/js/fields/relational_fields.js +++ b/addons/web/static/src/js/fields/relational_fields.js @@ -839,7 +839,7 @@ var FieldX2Many = AbstractField.extend({ this.currentColInvisibleFields = this._evalColumnInvisibleFields(); this.renderer = new ListRenderer(this, this.value, { arch: arch, - mode: this.mode, + editable: this.mode === 'edit' && arch.attrs.editable, addCreateLine: !this.isReadonly && this.activeActions.create, addTrashIcon: !this.isReadonly && this.activeActions.delete, viewType: viewType, diff --git a/addons/web/static/src/js/views/basic/basic_renderer.js b/addons/web/static/src/js/views/basic/basic_renderer.js index 21fabbc2274..64b90c33022 100644 --- a/addons/web/static/src/js/views/basic/basic_renderer.js +++ b/addons/web/static/src/js/views/basic/basic_renderer.js @@ -251,13 +251,12 @@ var BasicRenderer = AbstractRenderer.extend({ function _apply(element) { // If the view is in edit mode and that a widget have to switch // its "readonly" state, we have to re-render it completely - if ('readonly' in modifiers - && self.mode === "edit" - && element.widget - && (element.widget.mode === 'readonly') !== modifiers.readonly) - { - self._rerenderFieldWidget(element.widget, record); - return; // Rerendering already applied the modifiers, no need to go further + if ('readonly' in modifiers && element.widget) { + var mode = modifiers.readonly ? 'readonly' : modifiersData.baseMode; + if (mode !== element.widget.mode) { + self._rerenderFieldWidget(element.widget, record, mode); + return; // Rerendering already applied the modifiers, no need to go further + } } // Toggle modifiers CSS classes if necessary @@ -429,6 +428,12 @@ var BasicRenderer = AbstractRenderer.extend({ this.allModifiersData.push(modifiersData); } } + // we register here the base mode of the node. This is a field widget + // specific settings which represents the generic mode for the widget, + // regardless of its modifiers. The interesting case is the list view: + // all widgets are supposed to be in the baseMode 'readonly', except the + // ones that are in the line that is currently being edited. + modifiersData.baseMode = (options && options.mode) || this.mode; // Evaluate if necessary if (!modifiersData.evaluatedModifiers[record.id]) { @@ -454,7 +459,7 @@ var BasicRenderer = AbstractRenderer.extend({ } modifiersData.elementsByRecord[record.id].push(newElement); - this._applyModifiers(modifiersData, record, newElement); + this._applyModifiers(modifiersData, record, newElement, options); } return modifiersData.evaluatedModifiers[record.id]; @@ -490,22 +495,20 @@ var BasicRenderer = AbstractRenderer.extend({ * @param {Object} node * @param {Object} record * @param {Object} [options] - * @param {Object} [modifiersOptions] * @returns {AbstractField} */ - _renderFieldWidget: function (node, record, options, modifiersOptions) { + _renderFieldWidget: function (node, record, options) { var fieldName = node.attrs.name; - // Register the node-associated modifiers - var modifiers = this._registerModifiers(node, record); - + var mode = options && options.mode || this.mode; + var modifiers = this._registerModifiers(node, record, null, options); // Initialize and register the widget // Readonly status is known as the modifiers have just been registered var Widget = record.fieldsInfo[this.viewType][fieldName].Widget; - var widget = new Widget(this, fieldName, record, _.extend({ - mode: modifiers.readonly ? 'readonly' : this.mode, + var widget = new Widget(this, fieldName, record, { + mode: modifiers.readonly ? 'readonly' : mode, viewType: this.viewType, - }, options || {})); + }); // Register the widget so that it can easily be found again if (this.allFieldWidgets[record.id] === undefined) { @@ -526,15 +529,16 @@ var BasicRenderer = AbstractRenderer.extend({ // associated to new widget) var self = this; def.then(function () { - self._registerModifiers(node, record, widget, _.extend({ + self._registerModifiers(node, record, widget, { callback: function (element, modifiers, record) { element.$el.toggleClass('o_field_empty', !!( record.data.id - && (modifiers.readonly || self.mode === 'readonly') + && (modifiers.readonly || mode === 'readonly') && !element.widget.isSet() )); }, - }, modifiersOptions || {})); + mode: mode + }); self._postProcessField(widget, node); }); @@ -602,11 +606,12 @@ var BasicRenderer = AbstractRenderer.extend({ * @private * @param {Widget} widget * @param {Object} record + * @param {string} mode either 'readonly' or 'edit' * @returns {AbstractField} */ - _rerenderFieldWidget: function (widget, record) { + _rerenderFieldWidget: function (widget, record, mode) { // Render the new field widget - var newWidget = this._renderFieldWidget(widget.__node, record); + var newWidget = this._renderFieldWidget(widget.__node, record, {mode: mode}); widget.$el.replaceWith(newWidget.$el); // Destroy the old widget and position the new one at the old one's diff --git a/addons/web/static/src/js/views/list/list_editable_renderer.js b/addons/web/static/src/js/views/list/list_editable_renderer.js index c1ef23b6b5e..12a942fd6ec 100644 --- a/addons/web/static/src/js/views/list/list_editable_renderer.js +++ b/addons/web/static/src/js/views/list/list_editable_renderer.js @@ -54,7 +54,7 @@ ListRenderer.include({ * @returns {Deferred} */ start: function () { - if (this.mode === 'edit') { + if (this._isEditable()) { this.$el.css({height: '100%'}); core.bus.on('click', this, this._onWindowClicked.bind(this)); } @@ -295,12 +295,7 @@ ListRenderer.include({ renderInvisible: editMode, renderWidgets: editMode, }; - if (!editMode) { - // Force 'readonly' mode for widgets in readonly rows as - // otherwise they default to the view mode which is 'edit' for - // an editable list view - options.mode = 'readonly'; - } + options.mode = editMode ? 'edit' : 'readonly'; // Switch each cell to the new mode; note: the '_renderBodyCell' // function might fill the 'this.defs' variables with multiple deferred @@ -417,7 +412,7 @@ ListRenderer.include({ * @returns {boolean} */ _isEditable: function () { - return this.mode === 'edit' && !this.state.groupedBy.length && this.arch.attrs.editable; + return !this.state.groupedBy.length && this.editable; }, /** * Move the cursor on the end of the previous line, if possible. diff --git a/addons/web/static/src/js/views/list/list_renderer.js b/addons/web/static/src/js/views/list/list_renderer.js index b7ca5cc118b..670931044c0 100644 --- a/addons/web/static/src/js/views/list/list_renderer.js +++ b/addons/web/static/src/js/views/list/list_renderer.js @@ -60,6 +60,7 @@ var ListRenderer = BasicRenderer.extend({ this.hasSelectors = params.hasSelectors; this.selection = []; this.pagers = []; // instantiated pagers (only for grouped lists) + this.editable = params.editable; }, //-------------------------------------------------------------------------- @@ -261,7 +262,7 @@ var ListRenderer = BasicRenderer.extend({ // We register modifiers on the element so that it gets the correct // modifiers classes (for styling) - var modifiers = this._registerModifiers(node, record, $td); + var modifiers = this._registerModifiers(node, record, $td, _.pick(options, 'mode')); // If the invisible modifiers is true, the element is left empty. // Indeed, if the modifiers was to change the whole cell would be // rerendered anyway. diff --git a/addons/web/static/src/js/views/list/list_view.js b/addons/web/static/src/js/views/list/list_view.js index b2295affb98..8a989dad35e 100644 --- a/addons/web/static/src/js/views/list/list_view.js +++ b/addons/web/static/src/js/views/list/list_view.js @@ -48,7 +48,7 @@ var ListView = BasicView.extend({ this.rendererParams.arch = arch; this.rendererParams.hasSelectors = 'hasSelectors' in params ? params.hasSelectors : true; - this.rendererParams.mode = mode; + this.rendererParams.editable = params.readonly ? false : arch.attrs.editable; this.loadParams.limit = this.loadParams.limit || 80; this.loadParams.type = 'list'; diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js index fb10cf6d784..a9d879196b3 100644 --- a/addons/web/static/tests/fields/relational_fields_tests.js +++ b/addons/web/static/tests/fields/relational_fields_tests.js @@ -4164,6 +4164,48 @@ QUnit.module('relational_fields', { testUtils.unpatch(AbstractField); }); + QUnit.test('editable one2many with sub widgets are rendered in readonly', function (assert) { + assert.expect(2); + + var editableWidgets = 0; + testUtils.patch(AbstractField, { + init: function () { + this._super.apply(this, arguments); + if (this.mode === 'edit') { + editableWidgets++; + } + }, + }); + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch:'
' + + '' + + '' + + '' + + '' + + '' + + '' + + '
', + res_id: 1, + viewOptions: { + mode: 'edit', + }, + }); + + assert.strictEqual(editableWidgets, 1, + "o2m is only widget in edit mode"); + form.$('tbody td.o_field_x2many_list_row_add a').click(); + + assert.strictEqual(editableWidgets, 3, + "3 widgets currently in edit mode"); + + form.destroy(); + testUtils.unpatch(AbstractField); + }); + QUnit.test('one2many editable list with onchange keeps the order', function (assert) { assert.expect(2); diff --git a/addons/web/static/tests/views/form_benchmarks.js b/addons/web/static/tests/views/form_benchmarks.js new file mode 100644 index 00000000000..d664859ba2f --- /dev/null +++ b/addons/web/static/tests/views/form_benchmarks.js @@ -0,0 +1,95 @@ +odoo.define('web.list_benchmarks', function (require) { +"use strict"; + +var FormView = require('web.FormView'); +var testUtils = require('web.test_utils'); + +var createView = testUtils.createView; + +QUnit.module('Form View', { + beforeEach: function () { + this.data = { + foo: { + fields: { + many2many: { string: "bar", type: "many2many", relation: 'bar'}, + }, + records: [ + { id: 1, many2many: []}, + ], + onchanges: {} + }, + bar: { + fields: { + char: {string: "char", type: "char"}, + many2many: { string: "pokemon", type: "many2many", relation: 'pokemon'}, + }, + records: [], + onchanges: {} + }, + pokemon: { + fields: { + name: {string: "Name", type: "char"}, + }, + records: [], + onchanges: {} + }, + }; + this.arch = null; + this.run = function (assert, done, cb) { + var data = this.data; + var arch = this.arch; + var viewOptions = this.viewOptions; + new Benchmark.Suite({}) + .add('form', function () { + var list = createView({ + View: FormView, + model: 'foo', + data: data, + arch: arch, + res_id: 1, + }); + if (cb) { + cb(list); + } + list.destroy(); + }) + .on('cycle', function(event) { + assert.ok(true, String(event.target)); + }) + .on('complete', done) + .run({ 'async': true }); + }; + } +}, function () { + QUnit.test('x2many with 250 rows, 2 fields (with many2many_tags, and modifiers), onchanges, and edition', function (assert) { + var done = assert.async(); + assert.expect(1); + + this.data.foo.onchanges.many2many = function (obj) { + obj.many2many = [5].concat(obj.many2many); + }; + for (var i = 2; i < 500; i) { + this.data.bar.records.push({ + id: i, + char: "automated data", + }); + this.data.foo.records[0].many2many.push(i); + } + this.arch = + '
' + '' + '' + '' + '' + '' + '' + '
'; + this.run(assert, done, function (form) { + form.$buttons.find('.o_form_button_edit').click(); + form.$('.o_data_cell:first').click(); + form.$('input:first').val("tralala").trigger('input'); + }); + }); +}); + +}); \ No newline at end of file diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index 44c76d41c34..2993e393a4f 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -474,13 +474,13 @@ QUnit.module('Views', { var n = 0; testUtils.intercept(list, "field_changed", function () { - n = 1; + n += 1; }); $td.click(); $td.find('input').val('abc').trigger('input'); - assert.strictEqual(n, 1, "field_changed should not have been triggered"); - list.$('td:not(.o_list_record_selector)').eq(2).click(); assert.strictEqual(n, 1, "field_changed should have been triggered"); + list.$('td:not(.o_list_record_selector)').eq(2).click(); + assert.strictEqual(n, 1, "field_changed should not have been triggered"); list.destroy(); }); diff --git a/addons/web/views/webclient_templates.xml b/addons/web/views/webclient_templates.xml index 5117d1e633c..5fa62beb761 100644 --- a/addons/web/views/webclient_templates.xml +++ b/addons/web/views/webclient_templates.xml @@ -586,6 +586,7 @@ +