Opened 12 years ago

Closed 11 years ago

#6389 closed defect (fixed)

[patch][cla] _removeItems() in DojoData needs to check for existence of data[i]

Reported by: guest Owned by: benschell
Priority: high Milestone: 1.2
Component: DojoX Grid Version: 1.0
Keywords: 1.0.2 release Cc: sschaer@…
Blocked By: Blocking:

Description (last modified by benschell)

Upon deleting an item from a large store with identity (eg ItemFileWriteStore) the _rowIdenties are rebuilt. In case a user has scrolled through the grid fast enough, the data[] array can contain empty entries. Example: Grid with 20 rowsPerPage, 40 keepRows, 100 rows in total. User scrolls to the end fast, then the data[] contains entries for the first page data[0 ]- data[19 ] and the last page data[80 ] - data[99 ].

Rebuilding of the _rowIdenties has to check whether the data[i] entry exists, otherwise an error data[i].__dojo_data_item does not exist occurs.

File: dojox/grid/_data/model.js Class: dojox.grid.data.DojoData?

_removeItems: function(inRowIndexes /*array*/){
    // ...
    // Rebuild _rowIdentities
    this._rowIdentities = {};
    for (var i = 0; i < this.data.length; i++){
      // data[i] can be undefined ! need to add check
      if(this.data[i])
        this._setRowId(this.data[i].____dojo_data_item, 0, i);
    }
}

Attachments (2)

6389.patch (8.2 KB) - added by benschell 11 years ago.
6389-2.patch (8.1 KB) - added by benschell 11 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 12 years ago by Adam Peller

Description: modified (diff)
Owner: changed from sorvell to benschell

comment:2 Changed 12 years ago by benschell

Description: modified (diff)
Status: newassigned

Changed 11 years ago by benschell

Attachment: 6389.patch added

comment:3 Changed 11 years ago by benschell

Patch uploaded. Corrects the behavior and includes a new test case to facilitate testing larger result sets with DojoData?.

comment:4 Changed 11 years ago by benschell

Summary: _removeItems() in DojoData needs to check for existence of data[i][patch][cla] _removeItems() in DojoData needs to check for existence of data[i]

comment:5 Changed 11 years ago by Adam Peller

Milestone: 1.2

Ben, I'd be happy to commit the patch, but I need instructions on how to reproduce the bug with this test. Also, can you use dojo.forEach() in your patch? (the index should be available as the second arg)

comment:6 Changed 11 years ago by Adam Peller

p.s. the test doesn't load. It says it can't find DojoData?

comment:7 Changed 11 years ago by benschell

Adam,

I just uploaded another patch offering. I didn't change the for loop in the initial patch, I think I just cleaned up some of the spacing. This second patch has two differences from the first: a) changed the for(var i=0... to dojo.forEach(... as requested, and b) removed a comment in the test file.

I'm not sure why the test file wouldn't be loading. I applied the patch to a fresh checkout and it works fine for me, but I could be doing something wrong. The test file is essentially a direct copy of test_dojo_data_edit.html, with a change to which json file it is loading. Maybe I have a bug somewhere else?

As for how to reproduce the original bug, take the given test case (once it works). Grab the scroll handle on the top grid and scroll to the bottom as fast as you can, pause to let the data load, then scroll back to the top as quickly as possible. The goal is to have data loaded at the top and at the bottom, but some rows in between that are not loaded (this isn't as hard as it sounds). Next, select a row near the top (say, 3, for instance), then click the "Remove" button at the top. Without the patch, an error is thrown, as the row identities for those un-loaded rows in the middle of the grid are trying to be set. With the patch, those un-loaded rows are ignored, since they haven't been loaded from the store.

Changed 11 years ago by benschell

Attachment: 6389-2.patch added

comment:8 Changed 11 years ago by Adam Peller

Resolution: fixed
Status: assignedclosed

(In [13392]) Check array elements in _removeItems. Fixes #6389. !strict Thanks, Ben.

Note: See TracTickets for help on using tickets.