Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

LayoutAdapter with automatic animations #141

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rep2
Copy link
Contributor

@Rep2 Rep2 commented May 16, 2017

I found this code quite useful in my project, but it might not be something you would find is in scope of LayoutKit. If so fell free to close this PR.

Currently ReloadableViewLayoutAdapter uses layoutProvider: @escaping (Void) -> T to create new layouts every time reload is called.

Instead of recreating all layouts, it is possible to use BatchUpdates to create new Layouts and Section only when needed. This improves performance and simplifies layout creation. Also, if BatchUpdates is used to animate content change, but all of the layouts are recreated, only animated cells will have newly created layouts which can lead to weird behaviour.

Furthermore, it is possible to create BatchUpdates from model being presented. Using this two functionalities it is possible to simply present model and have animations calculated and performed automatically.

For example, all of the animations bellow are done by presenting model using LayoutAdapterWithAutomaticBatchUpdates without any custom calculations.

animationsexample

This is done by calling

public func reload(
    viewModel: [[Any]], 
    elementCompareCallback: @escaping (Any, Any) -> Bool = { _ in return false }, 
    layoutProvider: @escaping (IndexPath) -> Layout, 
    animated: Bool = true, 
    completion: (() -> Void)? = nil)

on LayoutAdapterWithAutomaticBatchUpdates. Method calculates BatchUpdates based on model change, uses BatchUpdates to calculate layouts change and presents them using ReloadableViewLayoutAdapter.reload.

@Rep2 Rep2 closed this May 16, 2017
@Rep2 Rep2 changed the title LayoutAdapter with cached layouts LayoutAdapter with automatic animations May 16, 2017
@Rep2 Rep2 reopened this May 16, 2017
@Rep2 Rep2 closed this May 16, 2017
@Rep2 Rep2 reopened this May 16, 2017
@staguer
Copy link
Contributor

staguer commented May 26, 2017

Hi @Rep2 , this looks very useful and interesting! Thank you for submitting this PR. I'm still reading through it and thinking about it and I'll have more questions later.

(Meanwhile I just have one minor comment that class LayoutAdapterCacheHandler is repeated in two files in this PR :-) )

Copy link
Collaborator

@nicksnyder nicksnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR. What you are trying to accomplish (intelligently reusing layouts, and automatically calculating BatchUpdates) are would be nice features to have.

Just glancing though the PR, it looks like the code is structured in a way such that it works outside of LayoutKit (i.e. using extensions) but that isn't necessarily the way that we would want the code structured if we were to put it in to LayoutKit. All of the code would need documentation as well.

More generally, this code is complex enough that we need to fully understand how it works and think through all the cases to make sure that it is correct. My guess is that a mergeable version of this PR will look significantly different than what it is right now.

@staguer I will leave it up to you whether you want to shepherd this PR or think about solving this problem from first principles (if it is worth solving at all). If it were me, I would probably do the latter with this PR as inspiration.

Removes element at index.
Action that would cause an error are ignored.
*/
mutating func remove(safeAt index: Index) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am suspicious of code that needs to use these safe methods. Using methods like this may mask sloppy management of indexes or mask bugs.

@@ -0,0 +1,57 @@
extension BatchUpdates {
static func calculate(old: [[Any]], new: [[Any]], elementCompareCallback: (Any, Any) -> Bool) -> BatchUpdates {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calculate is not a great name for method, old and new are not great names for variables, and any type should be avoided.

@@ -0,0 +1,57 @@
extension BatchUpdates {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be a separate extension on BatchUpdates


remainingNewValues[newValue.offset].alreadyFound = true

continue outer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use constructs like this, it makes the code hard to read.

@Rep2
Copy link
Contributor Author

Rep2 commented May 31, 2017

Thanks for the response. I agree with and understand your position. I have no problem if you modify and/or use this code outside of this PR.

If you will proceed with this idea feel free to add me to the future PRs. I will respond to the comments as soon as I find the time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants