From 0764bc41aa0d28e2a6dcbf6d169d3f8ea092a627 Mon Sep 17 00:00:00 2001 From: qsm-odoo Date: Thu, 4 May 2017 16:56:57 +0200 Subject: [PATCH] [FIX] web: list rendering was not properly updated after discard Before this commit, when hitting the "Discard" button in an editable list view, the line switched to readonly mode but with the undiscarded data (even though the data was properly discarded and not saved). Fortunately, the fix is in fact making the save/discard/update system more consistent. Here are some reasons why: - the `_confirmSave` method is now used after a save or a discard (while it was before only called on readonly views where a change was made by a special field widget). - Abandonning a record (discarding a new unsaved one), is now the responsability of `discardChanges` (and not `_setMode`). - The renderer `updateState` method is used when its state is updated. - ... Note that setting the renderer mode is now taking care of deferreds properly (the widgets deferred are now considered when switching a row to readonly/edit mode). --- .../static/src/js/fields/relational_fields.js | 9 +- .../static/src/js/views/abstract_renderer.js | 7 +- .../src/js/views/basic/basic_controller.js | 27 ++-- .../src/js/views/form/form_controller.js | 8 +- .../src/js/views/list/list_controller.js | 16 ++- .../js/views/list/list_editable_renderer.js | 127 ++++++++---------- .../static/src/js/views/list/list_renderer.js | 4 +- addons/web/static/tests/views/list_tests.js | 29 ++++ 8 files changed, 129 insertions(+), 98 deletions(-) diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js index 135d21e5adf..51986224c78 100644 --- a/addons/web/static/src/js/fields/relational_fields.js +++ b/addons/web/static/src/js/fields/relational_fields.js @@ -638,7 +638,7 @@ var FieldX2Many = AbstractField.extend({ return this._super(); } if (this.renderer) { - this.renderer.updateState(this.value); + this.renderer.updateState(this.value, {}); this.pager.updateState({ size: this.value.count }); return $.when(); } @@ -746,8 +746,8 @@ var FieldX2Many = AbstractField.extend({ onFailure: def.reject.bind(def), }); } else { - self.renderer.setRowMode(recordID, 'readonly'); - def.resolve(); + self.renderer.setRowMode(recordID, 'readonly') + .done(def.resolve.bind(def)); } }); return def; @@ -791,7 +791,8 @@ var FieldX2Many = AbstractField.extend({ */ _onEditLine: function (ev) { ev.stopPropagation(); - this.renderer.setRowMode(ev.data.recordID, 'edit'); + this.renderer.setRowMode(ev.data.recordID, 'edit') + .done(ev.data.onSuccess); }, /** * Updates the given record with the changes. diff --git a/addons/web/static/src/js/views/abstract_renderer.js b/addons/web/static/src/js/views/abstract_renderer.js index 70ad5350670..7d10f05eab8 100644 --- a/addons/web/static/src/js/views/abstract_renderer.js +++ b/addons/web/static/src/js/views/abstract_renderer.js @@ -65,15 +65,18 @@ return Widget.extend({ setLocalState: function (localState) { }, /** - * update the state of the view. It always retrigger a full rerender. + * Updates the state of the view. It retriggers a full rerender, unless told + * otherwise (for optimization for example). * * @param {any} state * @param {Object} params + * @param {boolean} [params.noRender=false] + * if true, the method only updates the state without rerendering * @returns {Deferred} */ updateState: function (state, params) { this.state = state; - return this._render(); + return params.noRender ? $.when() : this._render(); }, //-------------------------------------------------------------------------- diff --git a/addons/web/static/src/js/views/basic/basic_controller.js b/addons/web/static/src/js/views/basic/basic_controller.js index b7cd40d9e7c..f1e71bcfd5a 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -118,13 +118,18 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { var self = this; recordID = recordID || this.handle; return this.canBeDiscarded(recordID).then(function (needDiscard) { - if (needDiscard) { // Just some optimization - self.model.discardChanges(recordID); - } if (options && options.readonlyIfRealDiscard && !needDiscard) { return; } - self._setMode('readonly', recordID); + + if (needDiscard) { // Just some optimization + self.model.discardChanges(recordID); + } + if (self.model.isNew(recordID)) { + self._abandonRecord(recordID); + return; + } + return self._confirmSave(recordID); }); }, /** @@ -386,7 +391,7 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { if (options.stayInEdit) { return def; } else { - return def.then(this._setMode.bind(this, 'readonly', recordID)); + return def.then(this._confirmSave.bind(this, recordID)); } }, /** @@ -397,17 +402,13 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { * @private * @param {string} mode - 'readonly' or 'edit' * @param {string} [recordID] + * @returns {Deferred} */ _setMode: function (mode, recordID) { - recordID = recordID || this.handle; - // If trying to make a temporary record readonly, discard the record - if (mode === 'readonly' && this.model.isNew(recordID)) { - this._abandonRecord(recordID); - return; - } - if (recordID === this.handle) { - this.update({mode: mode}, {reload: false}); + if ((recordID || this.handle) === this.handle) { + return this.update({mode: mode}, {reload: false}); } + return $.when(); }, /** * Helper method, to get the current environment variables from the model diff --git a/addons/web/static/src/js/views/form/form_controller.js b/addons/web/static/src/js/views/form/form_controller.js index f15bdfa5382..51357ce967e 100644 --- a/addons/web/static/src/js/views/form/form_controller.js +++ b/addons/web/static/src/js/views/form/form_controller.js @@ -67,7 +67,7 @@ var FormController = BasicController.extend({ }).then(function (handle) { self.handle = handle; self._updateEnv(); - self._setMode('edit'); + return self._setMode('edit'); }); }, /** @@ -183,7 +183,11 @@ var FormController = BasicController.extend({ */ _confirmSave: function (id) { if (id === this.handle) { - return this.reload(); + if (this.mode === 'readonly') { + return this.reload(); + } else { + return this._setMode('readonly'); + } } else { // a subrecord changed, so update the corresponding relational field // i.e. the one whose value is a record with the given id or a list diff --git a/addons/web/static/src/js/views/list/list_controller.js b/addons/web/static/src/js/views/list/list_controller.js index 241aeb8a104..b08291854d3 100644 --- a/addons/web/static/src/js/views/list/list_controller.js +++ b/addons/web/static/src/js/views/list/list_controller.js @@ -200,7 +200,7 @@ var ListController = BasicController.extend({ this.model.addDefaultRecord(this.handle, {position: this.editable}).then(function (recordID) { self._toggleNoContentHelper(false); var state = self.model.get(self.handle); - self.renderer.updateState(state); + self.renderer.updateState(state, {}); self.renderer.editRecord(recordID); }); }, @@ -231,13 +231,14 @@ var ListController = BasicController.extend({ */ _confirmSave: function (id) { var state = this.model.get(this.handle); - return this.renderer.confirmSave(state, id); + return this.renderer.updateState(state, {noRender: true}) + .then(this._setMode.bind(this, 'readonly', id)); }, /** * @override * @private */ - _getSidebarEnv: function() { + _getSidebarEnv: function () { var env = this._super.apply(this, arguments); var record = this.model.get(this.handle); return _.extend(env, {domain: record.getDomain()}); @@ -249,12 +250,14 @@ var ListController = BasicController.extend({ * @private * @param {string} mode * @param {string} [recordID] - default to main recordID + * @returns {Deferred} */ _setMode: function (mode, recordID) { - this._super.apply(this, arguments); if ((recordID || this.handle) !== this.handle) { - this.renderer.setRowMode(recordID, mode); this._updateButtons(mode); + return this.renderer.setRowMode(recordID, mode); + } else { + return this._super.apply(this, arguments); } }, /** @@ -356,7 +359,8 @@ var ListController = BasicController.extend({ */ _onEditLine: function (ev) { ev.stopPropagation(); - this._setMode('edit', ev.data.recordID); + this._setMode('edit', ev.data.recordID) + .done(ev.data.onSuccess); }, /** * Opens the Export Dialog 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 901e178eba6..65614e04983 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 @@ -85,20 +85,6 @@ ListRenderer.include({ } return this._super(recordID); }, - /** - * This method is called by the controller when the user has clicked 'save', - * and the model as actually saved the current changes. We then need to - * set the correct values for each cell. The difference with confirmChange - * is that the edited line is now in readonly mode, so we can just set its - * new value. - * - * @param {Object} state the new full state for the list - * @param {string} savedRecordID the id for the saved record - */ - confirmSave: function (state, savedRecordID) { - this.state = state; - this.setRowMode(savedRecordID, 'readonly'); - }, /** * Edit a given record in the list * @@ -143,6 +129,7 @@ ListRenderer.include({ * * @param {string} recordID * @param {string} mode + * @returns {Deferred} */ setRowMode: function (recordID, mode) { var self = this; @@ -155,40 +142,67 @@ ListRenderer.include({ this.currentRow = editMode ? rowIndex : null; var $row = this.$('.o_data_row:nth(' + rowIndex + ')'); + var $tds = $row.children('.o_data_cell'); + var oldWidgets = _.clone(this.allFieldWidgets[record.id]); + // When switching to edit mode, force the dimensions of all cells to + // their current value so that they won't change if their content + // changes, to prevent the view from flickering. if (editMode) { - // Instantiate column widgets and destroy potential readonly ones - var oldWidgets = _.clone(this.allFieldWidgets[record.id]); - var $tds = $row.children('.o_data_cell'); - // Force the size of each cell to its current value so that it - // won't change if its content changes, to prevent the view from - // flickering $tds.each(function () { var $td = $(this); $td.css({width: $td.outerWidth()}); }); - _.each(this.columns, function (node, colIndex) { - var $td = $tds.eq(colIndex); - var $newTd = self._renderBodyCell(record, node, colIndex, { - renderInvisible: true, - renderWidgets: true, - }); - - // TODO this is ugly... - if ($td.hasClass('o_list_button')) { - self._unregisterModifiersElement(node, record, $td.children()); - } - - $td.empty().append($newTd.contents()); - }); - _.each(oldWidgets, this._destroyFieldWidget.bind(this, record)); - } else { - for (var colIndex = 0; colIndex < this.columns.length; colIndex++) { - this._setCellValue(rowIndex, colIndex); - } } - $row.toggleClass('o_selected_row', editMode); // Toggle here so that style is applied at the end + // Prepare options for cell rendering (this depends on the mode) + var options = { + 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'; + } + + // Switch each cell to the new mode; note: the '_renderBodyCell' + // function might fill the 'this.defs' variables with multiple deferred + // so we create the array and delete it after the rendering. + var defs = []; + this.defs = defs; + _.each(this.columns, function (node, colIndex) { + var $td = $tds.eq(colIndex); + var $newTd = self._renderBodyCell(record, node, colIndex, options); + + // Widgets are unregistered of modifiers data when they are + // destroyed. This is not the case for simple buttons so we have to + // do it here. + if ($td.hasClass('o_list_button')) { + self._unregisterModifiersElement(node, record, $td.children()); + } + + // For edit mode we only replace the content of the cell with its + // new content (invisible fields, editable fields, ...). + // For readonly mode, we replace the whole cell so that the + // dimensions of the cell are not forced anymore. + if (editMode) { + $td.empty().append($newTd.contents()); + } else { + self._unregisterModifiersElement(node, record, $td); + $td.replaceWith($newTd); + } + }); + delete this.defs; + + // Destroy old field widgets + _.each(oldWidgets, this._destroyFieldWidget.bind(this, record)); + + // Toggle selected class here so that style is applied at the end + $row.toggleClass('o_selected_row', editMode); + + return $.when.apply($, defs); }, //-------------------------------------------------------------------------- @@ -360,6 +374,7 @@ ListRenderer.include({ * Activates the row at the given row index. * * @param {integer} rowIndex + * @returns {Deferred} */ _selectRow: function (rowIndex) { // Do nothing if already selected @@ -372,40 +387,14 @@ ListRenderer.include({ return this._unselectRow().then(function () { // Notify the controller we want to make a record editable var record = self.state.data[rowIndex]; + var def = $.Deferred(); self.trigger_up('edit_line', { recordID: record.id, + onSuccess: def.resolve.bind(def), }); + return def; }); }, - /** - * Set the value of a cell. This method can be called when the value of a - * cell was modified, for example after an onchange. - * - * @param {integer} rowIndex - * @param {integer} colIndex - */ - _setCellValue: function (rowIndex, colIndex) { - var record = this.state.data[rowIndex]; - var node = this.columns[colIndex]; - - var $oldTd = this.$('.o_data_row:nth(' + rowIndex + ') > .o_data_cell:nth(' + colIndex + ')'); - var $newTd = this._renderBodyCell(record, node, colIndex, {mode: 'readonly'}); - this._unregisterModifiersElement(node, record, $oldTd); - $oldTd.replaceWith($newTd); - - // Destroy old cell field widget if any - // TODO this is very inefficient O(n^3) instead of O(n) on row update - // because this method is called for each cell (O(n)), each time we have - // to find the associated record widget (O(n)) and once found, the - // search is performed again by the _destroyFieldWidget function - if (node.tag === 'field') { - var recordWidgets = this.allFieldWidgets[record.id]; - var w = _.findWhere(recordWidgets, {name: node.attrs.name}); // - if (w) { - this._destroyFieldWidget(record, w); - } - } - }, /** * This method is called whenever we click/move outside of a row that was * in edit mode. This is the moment we save all accumulated changes on that 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 3cda7e9f809..8437881a771 100644 --- a/addons/web/static/src/js/views/list/list_renderer.js +++ b/addons/web/static/src/js/views/list/list_renderer.js @@ -268,9 +268,7 @@ var ListRenderer = BasicRenderer.extend({ return $td.append(this._renderButton(record, node)); } if (node.attrs.widget || (options && options.renderWidgets)) { - this.defs = []; // TODO maybe wait for those somewhere ? var widget = this._renderFieldWidget(node, record, _.pick(options, 'mode')); - delete this.defs; return $td.append(widget.$el); } var name = node.attrs.name; @@ -534,9 +532,11 @@ var ListRenderer = BasicRenderer.extend({ */ _renderRow: function (record) { var self = this; + this.defs = []; // TODO maybe wait for those somewhere ? var $cells = _.map(this.columns, function (node, index) { return self._renderBodyCell(record, node, index, {mode: 'readonly'}); }); + delete this.defs; var className = 'o_data_row'; var decorations = this._computeDecorationClassNames(record); if (decorations.length) { diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index 764bd2017b4..79666c949c3 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -2036,6 +2036,35 @@ QUnit.module('Views', { list.destroy(); }); + + QUnit.test('discarding changes in a row properly updates the rendering', function (assert) { + assert.expect(3); + + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: + '' + + '' + + '', + }); + + assert.strictEqual(list.$('.o_data_cell:first').text(), "yop", + "first cell should contain 'yop'"); + + list.$('.o_data_cell:first').click(); + list.$('input[name="foo"]').val("hello").trigger('input'); + list.$buttons.find('.o_list_button_discard').click(); + assert.strictEqual($('.modal:visible').length, 1, + "a modal to ask for discard should be visible"); + + $('.modal:visible .btn-primary').click(); + assert.strictEqual(list.$('.o_data_cell:first').text(), "yop", + "first cell should still contain 'yop'"); + + list.destroy(); + }); }); });