[FIX] web: ActionManager: protect set/get scrolltop methods
Here is what could happen before this commit: open a record (let's say project.task) in form view. Then, on the sidebar (actions), choose Merge Selected Tasks), choose a task, and click on Merge. What happens next is a traceback. Here is a more precise description of what happened: - the formview dialog for the merge action closes, which triggers a reload for the main formview - the merge action returned an action description for an act_window - the action manager instantiated a new viewmanager and prepare the form view, which is an async operation (need to load some data to display the form) - the previous view manager (initial one) completes its reload, which makes it rerender the view, which triggers a DOM_updated event - the new view manager see this event, and tries to call getScrollTop on the current view controller, but there are no controller yet, it is not ready. We could argue that some of these events should not happen (for example, if we will open a new view manager action, why do we reload the current view manager?). But solving these issues is a more deep architectural problem (we probably will rewrite the actionmanager/view manager layer at some point). It could also be argued that in any case, the method getScrollTop should not crash (same for setScrollTop). Sadly, these methods are fragile, because some changes in the loading behaviour of a view may break them. So, in any case, protecting them against unexpected traceback is a worthwile investment.
This commit is contained in:
@@ -182,19 +182,35 @@ var ViewManagerAction = WidgetAction.extend({
|
||||
},
|
||||
/**
|
||||
* Sets the scroll position of the widgets's active_view
|
||||
* @param {int} [scrollTop] the number of pixels to scroll
|
||||
* @todo: replace this with a generic get/set local state mechanism.
|
||||
* @see getScrollTop
|
||||
*
|
||||
* @override
|
||||
* @param {integer} [scrollTop] the number of pixels to scroll
|
||||
*/
|
||||
setScrollTop: function(scrollTop) {
|
||||
var viewController = this.widget.active_view.controller;
|
||||
setScrollTop: function (scrollTop) {
|
||||
var activeView = this.widget.active_view;
|
||||
var viewController = activeView && activeView.controller;
|
||||
if (viewController) {
|
||||
viewController.setScrollTop(scrollTop);
|
||||
}
|
||||
},
|
||||
/**
|
||||
* @return {int} the number of pixels the webclient is scrolled when leaving the action
|
||||
* Returns the current scrolling offset for the current action. We have to
|
||||
* ask nicely the question to the active view, because the answer depends
|
||||
* on the view.
|
||||
*
|
||||
* @todo: replace this mechanism with a generic getLocalState and
|
||||
* getLocalState. Scrolling behaviour is only a part of what we might want
|
||||
* to restore.
|
||||
*
|
||||
* @override
|
||||
* @returns {integer} the number of pixels the webclient is currently
|
||||
* scrolled
|
||||
*/
|
||||
getScrollTop: function() {
|
||||
var viewController = this.widget.active_view.controller;
|
||||
getScrollTop: function () {
|
||||
var activeView = this.widget.active_view;
|
||||
var viewController = activeView && activeView.controller;
|
||||
return viewController ? viewController.getScrollTop() : 0;
|
||||
},
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user