search

airbnb / infinity

UITableViews for the web (DEPRECATED)
http://airbnb.io/infinity/
Other
2.8k stars 281 forks source link

Added support for prepending items #16

Closed saiwong closed 11 years ago

saiwong commented 11 years ago

Added support for prepending items to the ListView.

reissbaker commented 11 years ago

Unfortunately, this won't work as intended given multiple prepends, or prepends after appends -- the wrong positioning coordinates will get cached.

updateCoordsFor caches the positioning of the ListItem, given a y-offset from the parent ListView. If all ListItems are appended to the end, the y-offset is just the height. In a prepend, the y-offset is zero. In this code, though, the height is still being passed as the y-offset.

That said, even if that were fixed, this would introduce some regression -- pages could be removed from the screen too early, because the first page might only have a single (potentially small) ListItem in it. Ideally a repartition call would be made after each prepend, although the performance implications of that aren't super great.

I'm conflicted about this. On the one hand, the current implementation of ListViews is naive and will be scrapped for a balanced binary tree, where all of this will be easy and have no negative performance implications. On the other hand, this is a useful feature, even if it's not possible to use it in a performant way given the current underlying implementation.

Oh well. Make it work with repartitioning and I'll merge it in. It may not live long, but it'll be useful in the meantime.

saiwong commented 11 years ago

This is actually not the case. When the prepend happens, it is always the first of the list, so the coordinates are calculated from that. The coordinates of all subsequent pages are pushed down from there. For appends, it is always appended to the last item and therefore the coordinates are correct always!

We have a working version of this that will be pushed out to live tomorrow morning EST. You can check it out at:

http://live.huffingtonpost.com

reissbaker commented 11 years ago

Assume a two prepends happen in a row, though. Let's imagine it's in an empty ListView for simplicity:

  1. The first prepend runs, and calls updateCoords on the prepended ListItem. It passes the ListItem in as the listItem arg, and the ListView's height as the yOffset arg. Since the height is zero, the yOffset is zero, the ListItem will set its top to that. So far, everything works as expected.
  2. The second prepend runs, and calls updateCoords on the prepended ListItem. It passes the ListItem in as the listItem arg, and the ListView's height as the yOffset arg. Since the height is non-zero, the yOffset is non-zero, and the ListItem will set its top (incorrectly) to be non-zero. Prepended items should always have a top of zero, at least immediately after being prepended, since their offset relative to the parent is zero by definition.

Sometimes this will appear to work, because it's all tricky edge cases and interactions between separate objects. But I don't see how the top of a prepended ListItem could actually be set correctly if the ListView has items in it, given the ListView's height as the y-offset. And if the top is wrong on a ListItem, sometimes the top will be wrong on a Page as well, since Pages take their top values from their first ListItem.

saiwong commented 11 years ago

Committed fixes to yOffset issue when prepending items. Thanks for the help! The prepend now correctly adjusts the top/bottom values of all Pages and ListItems. Repartitioning now works correctly! Yeah!

reissbaker commented 11 years ago

Wooooo! I'll merge in shortly, gotta update versioning.

vishalsood commented 11 years ago

@reissbaker @saiwong I need the prepend logic too but don't want to manually edit the files. Any chance you can get this merged anytime soon?

saiwong commented 11 years ago

@vishalsood You can always just fork + branch and merge this into your branch! No manual editing and you can even do everything via the Github web interface. Go Github!

0xgeert commented 11 years ago

Please ignore the above reference, can't seem to delete it.

Just encountered a problem with this prepend-pull, in which multiple prepended items receive the same top-properties. I'll update when I know more.

erictheise commented 11 years ago

@reissbaker, @saiwong, like @vishalsood I'm wondering what are the blockers for this pull request?

reissbaker commented 11 years ago

Merged in.