jflinter / Dwifft

Swift Diff
MIT License
1.86k stars 147 forks source link

Keep contentOffset when data has been changed #80

Open intoxicated opened 6 years ago

intoxicated commented 6 years ago

I've researched a lot regards to fix contentOffset after table dataSource has been updated. I couldn't find a way to do it with Dwifft. When diffCalculator.row = self.data is invoked with new values (in my case, new values will be added at front of previous data), it changes contentOffset. I attempted to use tableView.setContentOffset(offset, animated: false) after providing new data to diffCalculator which didn't work. Also, when new values (in this case, append to previous data) are added while scrolling, scroll somehow jumps on and off at some weird position. I also noticed, when same values were set to diffCalculator, it treated there were diff. I'd appreciate any advices. Thanks

jflinter commented 6 years ago

Unfortunately I don't think this is an issue with Dwifft, but rather UIKit itself - Dwifft doesn't do anything particularly special w/r/t the way it inserts/deletes rows. A quick Google search suggests that the issue may have to do with dynamcially-sized cells and estimated row heights. There is also the new performBatchUpdates method on UITableView in iOS 11. Would you please try building your app with the performBatchUpdates branch I just pushed (which uses this new API when available) and see if it helps? If not, would you please link to a small project that reproduces the contnetOffset issue you're seeing? Otherwise it'll be pretty impossible for me to help. Thanks!

On Wed, Nov 29, 2017 at 8:54 AM, Wooyoung Chung notifications@github.com wrote:

I've researched a lot regards to fix contentOffset after table dataSource has been updated. I couldn't find a way to do it with Dwifft. When diffCalculator.row = self.data is invoked with new values (in my case, new values will be added at front of previous data), it changes contentOffset. I attempted to use tableView.setContentOffset(offset, animated: false) after providing new data to diffCalculator which didn't work. Also, when new values (in this case, append to previous data) are added while scrolling, scroll somehow jumps on and off at some weird position. I also noticed, when same values were set to diffCalculator, it treated there were diff. I'd appreciate any advices. Thanks

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jflinter/Dwifft/issues/80, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtM5yYMTZX4O3v_7gaZwTT3E39Hmn1Fks5s7Yw1gaJpZM4QvSdB .

larryonoff commented 6 years ago

Just tried performBatchUpdates branch and it didn't help.

larryonoff commented 6 years ago

I think that Dwifft should have method func setSectionedValues(_ values: ..., animated: Bool). This will help people using the library in case of having the same issue. I'm having the same issue, but I want using the library. I can create PR with the proposed functionality.

larryonoff commented 6 years ago

I also think that performBatchUpdates is reasonable to merge into master. It just uses new iOS 11 functionality.

jflinter commented 6 years ago

Can you explain why a setSectionedValues:animated: method would help you here? I am reluctant to introduce a change like that without a really clear understanding of the underlying UIKit issue.

On Tue, Dec 19, 2017 at 2:17 PM larryonoff notifications@github.com wrote:

I also think that performBatchUpdates is reasonable to merge into master. It just uses new iOS 11 functionality.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/jflinter/Dwifft/issues/80#issuecomment-352858941, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtM56R-dITWgAYDL-i6J-4ylaIecjjsks5tCAu1gaJpZM4QvSdB .

larryonoff commented 6 years ago

I completely agree that all changes must be reasonable.

But I think there’re be cases when we don’t need any animations to update table view / collection view, like the current issue or when you need just full table view refresh when you know that completely new data came.

I also want to use sometimes the library as a simple data source without batch updates. But there’s no functionality to disable animations when updating the data.

Sent from my iPhone

On 19 Dec 2017, at 22:44, Jack Flintermann notifications@github.com wrote:

Can you explain why a setSectionedValues:animated: method would help you here? I am reluctant to introduce a change like that without a really clear understanding of the underlying UIKit issue.

On Tue, Dec 19, 2017 at 2:17 PM larryonoff notifications@github.com wrote:

I also think that performBatchUpdates is reasonable to merge into master. It just uses new iOS 11 functionality.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/jflinter/Dwifft/issues/80#issuecomment-352858941, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtM56R-dITWgAYDL-i6J-4ylaIecjjsks5tCAu1gaJpZM4QvSdB .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

larryonoff commented 6 years ago

@intoxicated it looks that solutions is easy, but hacky. Try doing the following after settings sectionedValues tableView.layer.removeAllAnimations()

jflinter commented 6 years ago

@larryonoff - upon further consideration, I don't think your suggestion belongs in Dwifft - it's not, and should not be, responsible for how animations to collection views take place - that's what UICollectionViewLayout, etc is for. As a really simple example, you can get un-animated behavior today by doing something like

UIView.performWithoutAnimation {
  diffCalculator.sectionedValues = ...
}

What this thread needs a cogent explanation of how to preserve a table view's contentOffset without any visual artifacts when inserting rows/sections of variable height. I'm not really interested in pursuing any solutions until that is properly understood.

intoxicated commented 6 years ago

larryonoff's solution doesn't work for me, and moreover, it seems like I lose scroll contentOffset position unexpectedly when updating data while tableview has scrolled.. I'm investigating more on this topic atm tho

danya61 commented 4 years ago

larryonoff's solution doesn't work for me, and moreover, it seems like I lose scroll contentOffset position unexpectedly when updating data while tableview has scrolled.. I'm investigating more on this topic atm tho

Did you find any solution?

danya61 commented 4 years ago

One of the possible solution - perform update in [.default] runloop mode RunLoop.current.perform(inModes: [.default]) { /* Perform update */ } But in this approach your batchUpdates will be async. In my unusual case its not the best solution

larryonoff commented 4 years ago

@danya61 I don't use Dwifft anymore. I switched to DifferenceKit

danya61 commented 4 years ago

@danya61 I don't use Dwifft anymore. I switched to DifferenceKit

Well, I suppose that the issue doesn't associated with the especial library, I've reproduce it even without diff framework . I've got an error when if your content offset < 0 and RunLoop mode is tracking, then after insert section (at least) content offset becomes forced zero

larryonoff commented 4 years ago

@danya61 hmmmm. I don't remember that I've faced with the current issue. But this not 100%

Dreagvor commented 4 years ago

I had the same issue, and resolved with setting collectionView.contentInsetAdjustmentBehavior = .never

aimalygin commented 3 years ago

For UICollectionView you might use StableCollectionViewLayout. This issue is normal behaviour for UITableView/UICollectionView.