Possible bug: VerticalDelegate.controlsPerPage sizeProp scope

I've ran into what seems like a bug:

enyo.DataList.delegates.vertical contains the following line of code:
perPage   = list.controlsPerPage = Math.ceil(((fn(list) * multi) / childSize) + 1);
Note the fn variable, it references the width or height methods on the delegate, but it gets executed globally. However, the methods it references seem to think they are executed within the delegate's scope.

I'm running into this error when I'm initializing a new DataList and refreshing a pre-existing collection at the roughly the same time. Both these processes are asynchronous and they don't always generate errors, which makes it a bit difficult to track down what's going on so exactly.

I'm not sure if this isn't partly my own fault, but when I do run into an error, it's because the width method gets executed within the global scope.

Comments

  • I'm using enyo 2.4 myself, but I just checked in the git repo and the same error seems present in the current master branch as well.
  • edited September 2014
    After some further digging, the plot seems to thicken... This scenario takes place under the following conditions:

    I start out with an existing collection, with existing records. There is no DataList yet.

    Next, I execute a fetchAndReplace on the collection and instantiate a new DataList instance. Some bindings attach the collection instance to the new DataList.

    The DataList gets instantiated, but rendering is deferred.

    While rendering is still deferred, the collection has succesfully fetched new data and it's old models get removed.

    The DataList.modelsRemoved method gets triggered and it's super method DataRepeater.modelsRemoved tries to deselect existing items. These items, however, do not exist yet because rendering of the list was deferred and has not happened yet.

    DataRepeater tries to find the (non-existent) list items that correspond with the deleted records by their index. During this search VerticalDelegate tries to calculate the amount of items contained within a single page, which triggers the call to VerticalDelegate.controlsPerPage.

    I have now patched the bad function call mentioned in the original post as follows:
    perPage   = list.controlsPerPage = Math.ceil(((fn.call(this, list) * multi) / childSize) + 1);
    This however, leads to another problem:
    Because the DataList has not rendered yet, list.boundsCache is undefined. VerticalDelegate.width/VerticalDelegate.height do not update list.boundsCache either, because list._updateBounds is still undefined as well. Furthermore, even if it would try to update the boundsCache, the bounds would still be undefined.

    To get around this, I have now also patched VerticalDelegate.width as follows:

    width: function (list) {
    if (list._updateBounds || !list.boundsCache) { this.updateBounds(list); }
    return list.boundsCache ? list.boundsCache.width : undefined;
    },
    Now it returns undefined if the list has no bounds. This in turn makes VerticalDelegate.controlsPerPage return 1 item per page, which is the same as the fallback value in VerticalDelegate.pageForIndex.

    I hope this provides some more insight into the problem, the use-case that triggers it and possible ways to solve it.
  • Created an issue and pull request for the scoping bug:

    Issue: https://enyojs.atlassian.net/browse/ENYO-4076
    Pull request: https://github.com/enyojs/enyo/pull/867
  • Created an issue and pull request for accessing undefined bounds as well:

    Issue: https://enyojs.atlassian.net/browse/ENYO-4077
    Pull request: https://github.com/enyojs/enyo/pull/868
  • By the way, this is my first time making a pull request to Enyo. Is there an easy way to link a pull-request to an issue? I now simply included a link to the pull-request in the comments, but if there is a better way I'd be happy to adopt it.
  • Hi @ruben_vreeken‌, thanks for the pull requests! We're happy to review / integrate them - could you add the developer sign-off (see the "Contribution guidelines" section: http://enyojs.com/community/contribute/). We need to update our public JIRA to have a code review field as that is where you'd link the PR, ideally. In the meantime, placing the link in a comment works.
  • Aaron, thanks for the heads-up. I completely missed the request to do so on github. I just added the signoff.
  • Not a problem, thanks for the update! And again, thanks for your contributions, we really appreciate it.
Sign In or Register to comment.