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

TLIndexPathUpdates should optionally suppress scrolling when inserting rows #12

Open
getaaron opened this issue Jan 7, 2014 · 14 comments

Comments

@getaaron
Copy link

getaaron commented Jan 7, 2014

If the user is scrolled down 20 messages on a table, and a new message comes in, calling [TLIndexPathUpdates -performBatchUpdatesOnTableView: withRowAnimation:] will insert rows at the top, pushing the other cells down. It can be confusing for a user to have the table scroll all of a sudden, so an option should be provided to suppress this behavior during row insertion.

This Stack Overflow answer describes one method for inserting rows at the top, and manually adjusting the table's contentOffset so the user never notices anything.

@wtmoose
Copy link
Member

wtmoose commented Jan 7, 2014

Thanks for the suggestion. I'm pretty slammed with work this month, so you might consider applying the workaround you linked to yourself if you're in a hurry. Maybe you could use the workaround instead of calling [TLIndexPathUpdates -performBatchUpdatesOnTableView: withRowAnimation:] when you detect that the first cell is above the fold?

@getaaron
Copy link
Author

getaaron commented Jan 7, 2014

Sure. Would you be interested in a pull request that added this functionality?

@wtmoose
Copy link
Member

wtmoose commented Jan 7, 2014

Sure. But I've been meaning to add an expanded API for performBatchUpdatesOnTableView that takes an option argument and a completion block. So this is what I'm thinking unless you have a better idea:

NS_ENUM(NSInteger, TLTableViewBatchUpdateOption) {
    TLTableViewBatchUpdateOptionNone = 0,
    TLTableViewBatchUpdateOptionMinimizeScroll = 1 << 0,
};

- (void)performBatchUpdatesOnTableView:(UITableView *)tableView withRowAnimation:(UITableViewRowAnimation)animation option:(TLTableViewBatchUpdateOption)option completion:(void(^)(BOOL finished))completion;

I'd think the minimize scroll option would for now simply kick over to the above technique anytime it can be determined that the table's visible state can be preserved as-is.

@getaaron
Copy link
Author

getaaron commented Jan 7, 2014

OK, I can include that. In which cases should BOOL finished == NO?

@wtmoose
Copy link
Member

wtmoose commented Jan 7, 2014

None for now I think. I mainly included it to be consistent with the API on UICollectionView

- (void)performBatchUpdates:(void (^)(void))updates completion:(void (^)(BOOL finished))completion

and with the UIView block-based animation APIs. I can note that it currently always returns TRUE in the API doc.

@getaaron
Copy link
Author

getaaron commented Jan 8, 2014

I'm unable to get the UITableView to report any contentSize changes from anywhere within the performBatchUpdatesOnTableView:withRowAnimation: method while I'm inserting rows. I've tried a few approaches, like:

NSLog(@"  Content Size before layout: %@", NSStringFromCGSize(tableView.contentSize));
[tableView layoutSubviews];
NSLog(@"  Content Size after layout: %@", NSStringFromCGSize(tableView.contentSize));

and

NSLog(@"  Content Size before layout: %@", NSStringFromCGSize(tableView.contentSize));
[tableView setNeedsLayout];  // tried both with and without this
[tableView layoutIfNeeded];
NSLog(@"  Content Size after layout: %@", NSStringFromCGSize(tableView.contentSize));

But no matter what, contentSize stays the same until the next run loop.

Do you have any ideas?


If we can't get that to work, here are a few alternatives:

  1. iterate through tableView:heightForRowAtIndexPath: for every option (probably nonperformant)
  2. add an API to TLIndexPathDataModel to cache precalculated row heights, and require it for the TLTableViewBatchUpdateOptionMinimizeScroll option. This approach has the downside of making the API a little more complex, but could improve table scrolling performance for some users.

A possible interface for alternative 2 could be:

- (void)setRowHeight:(NSNumber *)rowHeight forItem:(id)item;
- (void)setRowHeights:(NSArray *)rowHeights forItem:(NSArray *)items;
- (CGFloat)heightForItem:(id)item;
- (CGFloat)heightForItemAtIndexPath:(NSIndexPath)indexPath;

@wtmoose
Copy link
Member

wtmoose commented Jan 8, 2014

Before answering your question, I spent a couple hours last night on a more generic approach that would work more generically with batch updates without requiring the use of reloadData. The idea was to adjust the contentOffset at each animation frame using CADisplayLink. You can try the proof-of-concept here. Tapping any cell inserts a row at the top. It doesn't work consistently, though. Sometimes there is small visible motion. Interested if you have any ideas for fixing this.

Regarding your question, why are you wanting the table view to report contentSize changes? The solutions I looked at in your link all attempt to predict the future contentSize by calculating the heights of the inserted rows and adding that to the current contentSize. And this is essentially what is done in UITableViewController+ScrollOptimizer (in the Extensions folder).

@getaaron
Copy link
Author

getaaron commented Jan 8, 2014

Since the table view stores its content size, my plan was:

CGSize oldContentSize = tableView.contentSize;
// do stuff…
CGFloat heightOffset = tableView.contentSize.height - oldContentSize.height;
// add heightOffset to tableView.contentOffset.y

We could take a similar approach, getting the height for each item by calling heightForRowAtIndexPath: on inserted items. However, you wouldn't be able to subtract the height of deleted items this way, since those are gone from the tableview's model. Also, if we detect that all affected cells are offscreen we could wrap the whole thing in [UIView +setAnimationsEnabled:] (disable animation, perform batch updates, modify contentOffset, then enable).

The CADisplayLink approach is interesting - it doesn't require any height information. It's definitely jerky, though, and I don't think we can make it less jerky since we're fighting against the animation.

I hadn't seen UITableViewController+ScrollOptimizer. Sorry about missing that!

@wtmoose
Copy link
Member

wtmoose commented Jan 8, 2014

Yeah, it's too bad about the CADisplayLink issue. I think that approach would be simpler and more broadly applicable if it worked.

Here are a few thoughts that might help:

  1. We could simplify the problem by reducing the scope of the effect. For example, only apply the effect when the update object only contains inserts.
  2. Otherwise, you can get the heights of deleted cells before starting the batch update.
  3. I'm not keen on the idea of caching heights in the data model or anywhere for that matter until there is an observed performance issue. My gut is that calling heightForRowAtIndexPath on the deltas is relatively cheap.

Sorry I don't have more time to dig into this.

@getaaron
Copy link
Author

getaaron commented Jan 8, 2014

That's OK, I'll work in it more. I think the tableview might cache the heights by itself, so I might be able to get the heights of the deleted rows by calling rectForRowAtIndexPath:. I'll post here again when I have more.

@stefreak
Copy link

I noticed that this works for UITableView:

override func controller(controller: TLIndexPathController, didUpdateDataModel updates: TLIndexPathUpdates) {

    UIView.performWithoutAnimation {
        if self.isViewLoaded() && self.view.window != nil {
            self.tableView.beginUpdates()
            updates.performBatchUpdatesOnTableView(self.tableView, withRowAnimation: .None)
            self.tableView.contentOffset = /* your magic here */
            self.tableView.endUpdates()
        } else {
            self.tableView.reloadData()
        }
    }
}

I am fixing the contentOffset, by looking for a referenceItem that is present in the old and the new TLIndexPathDataModel and also is currently visible, determining the relative position from the top of the UITableView to the referenceItem and then basically scrolling to the new item after performBatchUpdates.

It only works with UIView.performWithoutAnimation which does not affect the row animations (row animation fade in etc. seems to work just fine).

Should I create a demo project?

@getaaron
Copy link
Author

@stefreak I've long since abandoned the project I was working on in 2014, so I would love to look at a sample project.

@stefreak
Copy link

stefreak commented Apr 22, 2016

@getaaron KeepScrollPositionDemo.zip

Somehow the row animation doesn't work in the demo, but I'm pretty sure it did work in another project.

I am not sure if this approach could/should be ported to TLTableViewController TLCollectionViewController though....

I am also not sure if this only works because of the beginUpdates(); beginUpdates(); endUpdates(); endUpdates(); (fist time in the demo and second time in TLIndexPathUpdates) or if it would also work with only beginUpdates(); endUpdates()

@stefreak
Copy link

The row animation seems to work when the row is updated in my other project, so yeah it only works correctly without any animations. but it's still better than reloadData because the cells that did not change are preserved

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

No branches or pull requests

3 participants