Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OndemandGrid not removing all items from trackable store #1305

Closed
wildcatcs opened this issue Aug 26, 2016 · 5 comments · Fixed by #1471
Closed

OndemandGrid not removing all items from trackable store #1305

wildcatcs opened this issue Aug 26, 2016 · 5 comments · Fixed by #1471
Assignees

Comments

@wildcatcs
Copy link

When an OnDemandGrid is given a trackable store with a moderate number of items, when all of the items are removed and a new set added not all of the original rows are deleted. This only happens if one of the columns is sorted.

Code simply removes all items from a trackable gird, than adds a new 200 items to the collection each time the refresh button is pressed. It works fine unless the grid is sorted on the Integer column. Then the grid's sort gets messed up and items that were removed from the collection are not removed from the grid. Changing the sort cleans up the bad rows.

Realize this would probably be a case for just setting a new collection, but this problem also happens if you are removing a large portion of the overall set of data.

Sample code attached (warning not pretty, just hacked up performance.html)
performance.txt

@msssk
Copy link
Contributor

msssk commented Aug 30, 2016

Can you provide some more details on the bug you are observing?

  1. What actions are you performing?
  2. What are you expecting to happen?
  3. What are you observing happening?

I don't see any problem that matches what you are describing, but I am looking into this issue:

  1. Load the test page
  2. Click the "integer" column header to sort (ascending)
  3. Click the "refresh" button
    1. Result: table rows are rendered
  4. Click the "integer" column header to sort (descending)
  5. Click the "refresh" button
    1. Expected: the table rows should render
    2. Result: the table has no rows rendered

@wildcatcs
Copy link
Author

  1. Downloaded performance.txt renamed it and dropped into test folder of latest dgrid (replacing existing file)
  2. Open the file via double clicking on it
  3. Press the refresh button a few times fairly rapidly (everything works fine)
  4. Sort the grid by the integer column and scroll through the list (should be sorted nicely)
  5. press refresh again
  6. The sort order is not the same as before the refesh was pressed even though the column is still sorted.
  7. Press refresh again
  8. Scroll through the list, you will start to see duplicate integers showing up even though they have all been removed
  9. Resort the grid and everything will be correct again, until refresh is pressed.

This is on Chrome (52) safari and I do believe we've seen it on firefox but haven't personally tried it.

Let me know if there is anything I can help you with.

@msssk
Copy link
Contributor

msssk commented Aug 31, 2016

@wildcatcs Following your instructions in Chrome 52 I don't see any issues. The ids will change each time you press refresh, but I'm seeing consistently correct sorting and no duplicate values in the integer column.

@wildcatcs
Copy link
Author

I've played some more with the example and realized I wasn't testing with the version I thought I was. I was using version 0.4.0 of dgrid with dstore 1.0.3. I upgraded to dgrid 1.1 and the behavior is definitely different but not bug free.

I can sort ascending and press refresh tell my heart is content and all is well (the original issue). However if I sort descending and press refresh the grid goes blank. There are no exceptions. Pressing to change the sort direction causes the grid to repopulate.

Attaching some screen shots:
Loaded the page, pressed refresh , sorted descending (screen shot1), pressed refresh (screen shot2), resorted (screen shot3)
screen shot1
screen shot2
screen shot3

Again, sorry about my confusion on the version.

@msssk
Copy link
Contributor

msssk commented May 27, 2020

When the "integer" column is sorted ascending, the test page's "Refresh" button removes rows from top to bottom. When the "integer" column is sorted descending the rows are removed from bottom to top. OnDemandList#_processScroll handles row removal differently depending on whether or not the rows are visible, and in the case of bottom to top removal a number of non-visible rows are removed. This is the reason that "sort ascending -> refresh" works while "sort descending -> refresh" fails. If you reverse the order of the col.remove loop in the go function invoked by the "Refresh" button then you see the opposite: "sort ascending -> refresh" fails while "sort descending -> refresh" succeeds.

When rows.max is set to less than 0 this check always fails, preventing addition of new items to the store from inserting new rows.

msssk added a commit to msssk/dgrid that referenced this issue May 27, 2020
If the rows of a grid are removed from bottom to top then each row removal
will decrement rows.max, including the final row removal which will set
rows.max to -1. This breaks the logic in _StoreMixin's 'add, update' event
handler preventing insertion of new rows when items are added to the store.

Fixes SitePen#1305
msssk added a commit that referenced this issue Jun 9, 2020
If the rows of a grid are removed from bottom to top then each row removal
will decrement rows.max, including the final row removal which will set
rows.max to -1. This breaks the logic in _StoreMixin's 'add, update' event
handler preventing insertion of new rows when items are added to the store.

Fixes #1305
msssk added a commit that referenced this issue Jun 22, 2020
If the rows of a grid are removed from bottom to top then each row removal
will decrement rows.max, including the final row removal which will set
rows.max to -1. This breaks the logic in _StoreMixin's 'add, update' event
handler preventing insertion of new rows when items are added to the store.

Fixes #1305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants