Instagram / IGListKit

A data-driven UICollectionView framework for building fast and flexible lists.
https://instagram.github.io/IGListKit/
MIT License
12.88k stars 1.54k forks source link

Add option to maintain scroll position when performing updates. #242

Closed jessesquires closed 4 years ago

jessesquires commented 7 years ago

Internally, we discussed adding this to the API, as it's likely a common use case.

We've built this for IG Direct, where we keep the scroll position while loading previous messages.

Proposed addition:

// IGListAdapter

- (void)performUpdatesAnimated:(BOOL)animated 
           fixedScrollPosition:(BOOL)fixedScrollPosition
                    completion:(nullable IGListUpdaterCompletion)completion;

I'm leaning toward adding this method, not replacing:

- (void)performUpdatesAnimated:(BOOL)animated completion:(nullable IGListUpdaterCompletion)completion;

The existing performUpdatesAnimated: would call the new one, passing NO for fixedScrollPosition:

Thoughts?

jessesquires commented 7 years ago

Alternative method spelling:

?

rnystrom commented 7 years ago

@jessesquires Ya I agree w/ adding a new method. It'll make upgrading a lot smoother for a method that is going to be used a lot.

I think I like "maintainScrollPosition".

Also I don't think we can do animated:YES maintainScrollPosition:YES and do an animated update w/ fixed scroll position. Maybe we have performUpdatesMaintainingScrollPositionWithCompletion:? Or something like that.

Side note, I have a feeling this is how animateWithDuration:animations: became animateWithDuration:delay:usingSpringWithDamping:initialSpringVelocity:options:animations:completion: πŸ˜‚

jessesquires commented 7 years ago

@rnystrom lol πŸ˜† yup.

+1 for maintainScrollPosition

Also I don't think we can do animated:YES maintainScrollPosition:YES and do an animated update w/ fixed scroll position.

Derp. You're right. In Direct, we pass animated:NO.

Now it definitely makes sense add this as a new method. πŸ’―

rnystrom commented 7 years ago

@jessesquires Check out internal comments on diff, I'm fine punting on this feature for now, we can pick it into a minor release. Lmk what you think.

jessesquires commented 7 years ago

Yep was thinking the same πŸ‘

michalkalinowski- commented 7 years ago

Any tips on implementing this? I need to maintain scroll position in my app when items are added on top of the list. I've seen old issue on @jessesquires repo: https://github.com/jessesquires/JSQMessagesViewController/issues/441 and I'm trying to implement something similar in listAdapterUpdaterWillPerformBatchUpdatesWith collectionView + listAdapterUpdaterDidPerformBatchUpdatesWith collectionView delegate methods, but I'm still getting visual glitches.

The only different solution I see is to modify layout itself to insert cells at negative y values but that seems like a lot of work.

If it's something that can be implemented in IGListkit I'm more than happy to take this task, just need some tips on how to code that.

rnystrom commented 7 years ago

@michalkalinowski- we have this internally at Instagram and are going to work on porting it to be more generic.

The basics of it are:

zhubofei commented 7 years ago

@rnystrom What if the saved topmost item is removed (or moved) after the update? Do we just use the previous content offset as a fallback?

jessesquires commented 7 years ago

This is an additive change. Bumping to 3.1.0 release.

zhubofei commented 7 years ago

@jessesquires @rnystrom You guys want to do this one internally or can I take it? Have some free time recentlyπŸ‘·.

rnystrom commented 7 years ago

@zhubofei We've got this built internally and just need to move it out. We'll release it once we get a chance πŸ˜…

darrarski commented 7 years ago

@rnystrom any news about this feature? If open sourcing the solution is not on your priority list right now, could you just provide some hints so I can try to implement it on my own?

rnystrom commented 7 years ago

@darrarski its on our radar for after 3.0 release. Here's a super short writeup on how to get it working tho:

https://github.com/Instagram/IGListKit/issues/644#issuecomment-294359113

darrarski commented 7 years ago

@rnystrom just FYI, I manage to solve it without saving visible cell. Perhaps it's not ideal solution, but works for my needs.

By default, content offset is measured from the top of the view, and is preserved when updating content. The tick it to preserve content offset measured from the bottom of the view instead.

I ended with creating extension to UIScrollView that calculates and allows to set "reversed" content offset: UIScrollView_ReversedContentOffset.swift

I am using it like this:

let reversedContentOffset = listCollectionView.reversedContentOffset
listAdapter.performUpdates(animated: false) { [weak self] _ in
    self?.listCollectionView.reversedContentOffset = reversedContentOffset
}
pmusolino commented 7 years ago

@darrarski I think that this solution doesn't work properly with cells that have dynamic height

darrarski commented 7 years ago

@Codeido it works pretty well here: https://github.com/darrarski/Messages-iOS As you can see the cells can have various height, depending on contained text. I understand that this isn't a perfect solution for everyone's needs. Perhaps it just doesn't work for your case, but I can't tell without more info about how you are using it.

tunidev commented 7 years ago

What the status of this ?

darrarski commented 7 years ago

@sheinix yes, please check out my comments above and this repo: Messages-iOS. You should be able to get the same behaviour without IGListKit, just check out my implementation.

sheinix commented 7 years ago

@darrarski you havelistAdapter.performUpdates(animated: false) that it isn't on JSQMessagesViewController. Are you suggesting to use self.collectionView.performBatchUpdates ?

darrarski commented 7 years ago

@sheinix it doesn't matter how you update your collection, you can do it using plain UIKit API or use IGListKit or other library to do it for you. The topic is about preserving scroll position when updating the collection which my implementation somehow solves (perhaps not it all possible cases).

yood commented 6 years ago

https://github.com/Instagram/IGListKit/issues/242#issuecomment-266448871 doesn't work for me. The results for performUpdates(animated: false) are rendered for a few frames before the completion block (with the setContentOffset) is called.

It looks like the issue is the CATransaction that wraps non-animated batch updates, which is committed before the completion blocks are called. When I committed the CATransaction after the completion blocks instead, my setContentOffset in the completion block was correctly applied before the UICollectionView rendered the updates, resulting in a smooth prepending experience.

https://github.com/Instagram/IGListKit/blob/290d592983713c3ef82eb4950ba773a0059563a2/Source/IGListAdapterUpdater.m#L237-L244

HowardCsie commented 6 years ago

@yood Thanks. from

[CATransaction begin];
[CATransaction setDisableActions:YES];
[collectionView performBatchUpdates:^{
    batchUpdatesBlock(result);
} completion:^(BOOL finished) {
    [CATransaction commit];
    batchUpdatesCompletionBlock(finished);
}];

to

[CATransaction begin];
[CATransaction setDisableActions:YES];
[collectionView performBatchUpdates:^{
    batchUpdatesBlock(result);
} completion:^(BOOL finished) {
    batchUpdatesCompletionBlock(finished);
    [CATransaction commit];
}];

make it more smooth.

hahtml commented 5 years ago

Any progress on this one? Looks like this is a commonly used feature. πŸ˜†

spnkr commented 5 years ago

bump

hydyy commented 5 years ago

Any progress on this one? Looks like this is a commonly used feature.

hydyy commented 5 years ago

@rnystrom

hahtml commented 5 years ago

The only promising way I find to solve the stutter/visual glitch issue is to use a custom UICollectionViewFlowLayout, override prepare func and set offset there.

This is the only solution to maintain scroll position smoothly in our case.

qhhonx commented 5 years ago

The only promising way I find to solve the stutter/visual glitch issue is to use a custom UICollectionViewFlowLayout, override prepare func and set offset there.

This is the only solution to maintain scroll position smoothly in our case.

How about overriding this API which gives custom target contentOffset when performBatchUpdates.

- (CGPoint)targetContentOffsetForProposedContentOffset:(CGPoint)proposedContentOffset NS_AVAILABLE_IOS(7_0); // a layout can return the content offset to be applied during transition or update animations

or but lower iOS9.0 not supported

- (CGPoint)collectionView:(UICollectionView *)collectionView targetContentOffsetForProposedContentOffset:(CGPoint)proposedContentOffset NS_AVAILABLE_IOS(9_0); // customize the content offset to be applied during transition or update animations

However, I doubt that it is not feasible to maintain every performUpdates situation except for loading newer/older data.

In our production use case, the above API overrides can achieve fixedScrollPosition while performUpdates to loading older messages like iMessage App.

sasojadrovski commented 5 years ago

The only promising way I find to solve the stutter/visual glitch issue is to use a custom UICollectionViewFlowLayout, override prepare func and set offset there.

This is the only solution to maintain scroll position smoothly in our case.

@hahtml Could you please provide an example on how to achieve this? It would be greatly appreciated! Thanks! 😊

danqing commented 4 years ago

@hahtml 's approach is the only one that works for me.

I'm not sure if iOS or IGListKit has any change in the last 2 years that breaks other solutions, but I've tried almost everything in this thread and all others gave me glitch. @yood 's approach may work, but I'm hesitant to change that order because I'm using the callback in other places too and I don't want to cause any additional trouble.

import UIKit

class ChatMessageLayout: UICollectionViewFlowLayout {

  var maintainOffset = false {
    didSet {
      self.currentHeight = self.collectionView?.contentSize.height
    }
  }
  private var currentHeight: CGFloat!

  override func prepare() {
    if self.maintainOffset, let cv = self.collectionView {
      cv.contentOffset = CGPoint(x: cv.contentOffset.x, y: cv.contentOffset.y + collectionViewContentSize.height - self.currentHeight)
      self.maintainOffset = false
    }
  }

}

Usage:

self.layout.maintainOffset = true
self.adapter.performUpdates(animated: false)
lorixx commented 4 years ago

Yeah, maintaining the scroll position should not be IGListKit's job, in IG we also use the UICollectionViewLayout's API to maintain the currentOffset or maintaining the same distance from the VERY bottom.

There are two way to maintain scroll position:

I would suggest you do it:

  1. Save the scroll position state right before calling -performUpdatesAnimate:completion:
  2. Restore the scroll position in the finalizeCollectionViewUpdates of your customized UICollectionViewFlowLayout
lsdimagine commented 4 years ago

@hahtml 's approach is the only one that works for me.

I'm not sure if iOS or IGListKit has any change in the last 2 years that breaks other solutions, but I've tried almost everything in this thread and all others gave me glitch. @yood 's approach may work, but I'm hesitant to change that order because I'm using the callback in other places too and I don't want to cause any additional trouble.

import UIKit

class ChatMessageLayout: UICollectionViewFlowLayout {

  var maintainOffset = false {
    didSet {
      self.currentHeight = self.collectionView?.contentSize.height
    }
  }
  private var currentHeight: CGFloat!

  override func prepare() {
    if self.maintainOffset, let cv = self.collectionView {
      cv.contentOffset = CGPoint(x: cv.contentOffset.x, y: cv.contentOffset.y + collectionViewContentSize.height - self.currentHeight)
      self.maintainOffset = false
    }
  }

}

Usage:

self.layout.maintainOffset = true
self.adapter.performUpdates(animated: false)

hmm it doesn't work for dynamic cells. The collectionViewContentSize.height is not accurate in prepare

mohanbright commented 4 years ago
let reversedContentOffset = listCollectionView.reversedContentOffset
listAdapter.performUpdates(animated: false) { [weak self] _ in
    self?.listCollectionView.reversedContentOffset = reversedContentOffset
}

Didn't worked for cells with dynamic height

rcaos commented 3 years ago

This works for me. Thanks!

@hahtml 's approach is the only one that works for me.

I'm not sure if iOS or IGListKit has any change in the last 2 years that breaks other solutions, but I've tried almost everything in this thread and all others gave me glitch. @yood 's approach may work, but I'm hesitant to change that order because I'm using the callback in other places too and I don't want to cause any additional trouble.

import UIKit

class ChatMessageLayout: UICollectionViewFlowLayout {

  var maintainOffset = false {
    didSet {
      self.currentHeight = self.collectionView?.contentSize.height
    }
  }
  private var currentHeight: CGFloat!

  override func prepare() {
    if self.maintainOffset, let cv = self.collectionView {
      cv.contentOffset = CGPoint(x: cv.contentOffset.x, y: cv.contentOffset.y + collectionViewContentSize.height - self.currentHeight)
      self.maintainOffset = false
    }
  }

}

Usage:

self.layout.maintainOffset = true
self.adapter.performUpdates(animated: false)
aimalygin commented 3 years ago

Maybe someone will be useful. I solve this issue in StableCollectionViewLayout. It is based on the UICollectionViewLayout approach using prepare(forCollectionViewUpdates:) and targetContentOffset(forProposedContentOffset:) methods.