Skip to content

Rewrite delegates to take perform update closures #14

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

Closed
wants to merge 1 commit into from
Closed

Conversation

JosephDuffy
Copy link
Member

@JosephDuffy JosephDuffy commented Sep 29, 2020

This is a major API change so is in a v2 branch. If we agree on a direction we could start adding PRs in to this branch.

This change would enable multiple changes to be applied at once, e.g. by requiring balanced calls to mappingWillBeginUpdating and mappingDidEndUpdating and only performing the updates when an equal number of mappingDidEndUpdating calls have been made. This could be a small performance improvement but would also improve animations.

By adding the performUpdate closure the changes to the model layer can be applied only when the consumer is expecting them, e.g. for UICollectionView this would be inside performBatchUpdates and would fix the existing crashes that occur when performing a change when there are pending layout changes.

Some tests and types have been commented out since this is more of a starting point for a discussion around the API than it is a solid implementation.

This is the closest I can see the API being (although it's still a breaking change) but maybe a bigger change would be better.

The biggest PITA is that the sections need to keep track of "current" data (e.g. what's being displayed by a collection view) and the "pending" data (e.g. what's about to be applied). I don't think this adds any memory overhead but it does add much more to the knowledge required when implementing a section.

This is a major API change so is in a v2 branch.

This change would enable multiple changes to be applied at once, e.g. by requiring balanced calls to `mappingWillBeginUpdating` and `mappingDidEndUpdating` and only performing the updates when an equal number of `mappingDidEndUpdating` calls have been made. This could be a small performance improvement but would also improve animations.

By adding the `performUpdate` closure the changes to the model layer can be applied only when the consumer is expecting them, e.g. for `UICollectionView` this would be inside `performBatchUpdates` and would fix the existing crashes that occur when performing a change when there are pending layout changes.

Some tests and types have been commented out since this is more of a starting point for a discussion around the API than it is a solid implementation.

This is the closest I can see the API being (although it's still a breaking change) but maybe a bigger change would be better.
@JosephDuffy JosephDuffy requested a review from shaps80 September 29, 2020 15:49
@shaps80
Copy link
Collaborator

shaps80 commented Sep 30, 2020

Yeah I generally agree on this approach. I've been thinking the same, in fact its pretty much how the new diffable datasources work. I was thinking we could take a page out of that handbook and implement something similar. Although ideally I'd like to explore options that allow us to use iOS 13+ based APIs (even lower level stuff like diffable collection which is perfect for a single section), then fallback to another solution (even injected perhaps?) on older platforms.

From my own explorations I think what's key is the following:

  • Section's should be Identifiable
  • Element's should be Identifiable
  • Sections should hold onto a current snapshot that can be used for diffing during an update

EDIT:

I also think if we're truly considering a breaking change for a 2.0 (which I agree we must) then we should reconsider the whole model, rather than patching.

@JosephDuffy
Copy link
Member Author

There are a lot of nice iOS 13+ APIs so we should for sure be aiming to support them. The app I'm using this in requires iOS 11 so I'm going to need support for at least iOS 11. The API could be the same as the types iOS 13 provides for easy conformance though.

Identifiable is iOS 13 but I think we'd be baking the requirement for an identifier in to the protocols for sections etc. We could add an extension that uses the id from Identifiable to make implementation easier though.

An identifier will sort out moves/deletes/inserts, but knowing when the content has changed would be required for reloads/updates.

I did wonder if there's a way to have this not be a breaking change. Making the performUpdate optional could allow for a transitional period that could fix the crashes for consumers that update their sections but I don't think would break API, then we can figure out a better overall API for the larger (v2) change.

I'm not sure what the best way to discuss/track v2 changes is? GitHub has the projects feature but I've never actually used it as a collaboration tool.

@shaps80
Copy link
Collaborator

shaps80 commented Oct 6, 2020

There are a lot of nice iOS 13+ APIs so we should for sure be aiming to support them. The app I'm using this in requires iOS 11 so I'm going to need support for at least iOS 11. The API could be the same as the types iOS 13 provides for easy
conformance though.
👍

Identifiable is iOS 13 but I think we'd be baking the requirement for an identifier in to the protocols for sections etc. We could add an extension that uses the id from Identifiable to make implementation easier though.
👍

An identifier will sort out moves/deletes/inserts, but knowing when the content has changed would be required for reloads/updates.
👍 I implemented a diffable DS the other day and wondered about this myself. I think the intent is to remove reloads from the collection/table view (which as we know is generally shit anyway) and leave that to the developer. How we decide to implement it is the question from my POV but I agree we need to handle this.

I did wonder if there's a way to have this not be a breaking change. Making the performUpdate optional could allow for a transitional period that could fix the crashes for consumers that update their sections but I don't think would break API, then we can figure out a better overall API for the larger (v2) change.
👍 I like this a lot as I would like more time to discuss the v2 Roadmap.

I'm not sure what the best way to discuss/track v2 changes is? GitHub has the projects feature but I've never actually used it as a collaboration tool.
Yeah I've used it to track tasks for a version and it works well to keep aligned but discussing the roadmap and purpose etc would probably be better if we do it on a call (especially since its really just the 2 of us for the moment)

@shaps80
Copy link
Collaborator

shaps80 commented Oct 6, 2020

Quick question @JosephDuffy – could we just add default implementations for perform handlers? Not sure its a good idea, just asking in case that's an alternative non-breaking change?

@JosephDuffy
Copy link
Member Author

Quick question @JosephDuffy – could we just add default implementations for perform handlers? Not sure its a good idea, just asking in case that's an alternative non-breaking change?

That should be possible yes. I've talked about this a little in my v2 roadmap proposal in the main repo's wiki. As mentioned #16 (comment) I will try and keep the discussion in that repo.

@JosephDuffy
Copy link
Member Author

Closing this for now in favour of composed-swift/ComposedUI#15, which accomplishes the batch updates at a higher level.

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

Successfully merging this pull request may close these issues.

2 participants