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

Add completion handlers to the extensions #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ellneal
Copy link

@ellneal ellneal commented Jul 14, 2020

Checklist

Description

Adds completion handlers to the UIKit/AppKit extensions.

This is a duplicate of #107 but I've taken a different approach to how the completion handlers are implemented (e.g. using a CATransaction for the non-closure based APIs).

Motivation and Context

I'm using DifferenceKit to create an API compatible version of the NSDiffableDataSource family of classes that run on iOS 12 and earlier, which requires completion handlers.

Impact on Existing Code

None. This is API compatible as the completion handlers are optional and default to nil.

@fruitcoder
Copy link

👏 can we merge this?

@ellneal
Copy link
Author

ellneal commented Oct 6, 2020

@fruitcoder I actually need to fix it, the completion handlers don't work.

@fruitcoder
Copy link

Hm :/ the code makes sense to me, do you have an idea why it's not working?

@ellneal
Copy link
Author

ellneal commented Oct 6, 2020

Calling group.notify before the first group.enter causes the notify block to be called immediately. I need to put that in a defer block.

@ellneal ellneal marked this pull request as draft October 6, 2020 13:09
@ellneal ellneal marked this pull request as ready for review December 3, 2020 15:10
@crsantos
Copy link

Had a chance to test the UICollectionView extension here and it works fine, and it finally solves the problem of not having a completion on every changeset applied. I ran into some issues due to this before.

This enables a more controlled way of notifying that all changesets were applied.

Q: I would only ask if adding a default parameter like queue = .main would be a good option.
Allowing consumers to inject the queue in which the completion should be called.

@ra1028 if you have the chance of looking into this PR 🙏

@ellneal
Copy link
Author

ellneal commented May 17, 2021

Q: I would only ask if adding a default parameter like queue = .main would be a good option.
Allowing consumers to inject the queue in which the completion should be called.

The reason I didn't do this is that these functions should never be called from anything other than the main thread (since they're calling UIKit functions), so made sense to call the completion block on the same thread.

@crsantos
Copy link

Totally agree @ellneal 👌

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

Successfully merging this pull request may close these issues.

3 participants