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

Support blueprint update animations in coordinator updates #274

Merged
merged 4 commits into from
Mar 10, 2021

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Mar 2, 2021

This PR updates the somewhat experimental ItemContentCoordinator APIs to properly support animations for Blueprint-based cells. It has a few changes:

  1. Share the ViewAnimation type across both scrolling and item content updates, by renaming ScrollAnimation to ViewAnimation.
  2. Remove the View/view from the ItemContentCoordinator – exposing this makes no sense for blueprint cells, and also, all updates to the item content should be done through apply updates, not through manually poking the view.
  3. Update the machinery used to propagate animations from coordinators to actually properly wrap updates in an animation block.
  4. Add a demo for showing how to do an "on tap" animation when a row is tapped.

@kyleve kyleve force-pushed the kve/coordinator-ontap-action branch 2 times, most recently from e53faa7 to ea3cfe2 Compare March 3, 2021 01:18
@kyleve kyleve force-pushed the kve/coordinator-ontap-action branch from ea3cfe2 to eda97ec Compare March 3, 2021 01:26
@kyleve kyleve changed the title [WIP] Support blueprint update animations in coordinator updates Support blueprint update animations in coordinator updates Mar 3, 2021
@kyleve kyleve requested a review from kylebshr March 3, 2021 01:29
@kyleve kyleve marked this pull request as ready for review March 3, 2021 01:29
…-action

* origin/main:
  [SwipeActions] Limit overscrolling to the right (#270)
Copy link
Collaborator

@kylebshr kylebshr left a comment

Choose a reason for hiding this comment

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

I think this looks good other than a timer/race condition question

{
self.willDisplay_calls.append(view)
self.willDisplay_calls.append(())
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 really understand what these arrays are doing, what's the purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for tracking the number of calls to the func during testing, so I can make sure the number of calls is right. It could also just be a counter since the type here is Void, but I kept it like this since thats what the rest of the file does.

ListableUI/Sources/ViewAnimation.swift Outdated Show resolved Hide resolved
ListableUI/Sources/ViewAnimation.swift Show resolved Hide resolved
var new = self.currentProvider()

update(&new)

self.update(animated: animated, new)
if delay > 0 {
Timer.scheduledTimer(withTimeInterval: delay, repeats: false) { _ in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this timer need to be retained? I can never remember. Separately, does it need to be canceled if another update happened? e.g., what if update A happened with a 1s delay, then B with 0.3s, would be occur and then get reverted back to A?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't need to be retained, it gets retained by the Runloop itself, and then fires and then is deallocated.

I didn't figure any queueing / collapsing of info was useful here, since it's a lot of extra behaviour, and might not be expected.

Good point about the disparate delays – I guess I should update the backing value immediately; OR alternatively, not pull the current value until the block executes (that actually seems better?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going to go with the latter:

    public func update(
        animation: ViewAnimation = .default,
        after delay: TimeInterval = 0,
        update : @escaping (inout Item<Content>) -> ()
    ) {
        if delay > 0 {
            Timer.scheduledTimer(withTimeInterval: delay, repeats: false) { _ in
                var new = self.currentProvider()
                update(&new)
                
                self.updateCallback(new, animation)
            }
        } else {
            var new = self.currentProvider()
            update(&new)
            
            self.updateCallback(new, animation)
        }
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@kyleve kyleve force-pushed the kve/coordinator-ontap-action branch from 17e3b5a to 5aa80f6 Compare March 10, 2021 01:53
@kyleve kyleve enabled auto-merge March 10, 2021 02:05
@kyleve kyleve merged commit 540bf48 into main Mar 10, 2021
@kyleve kyleve deleted the kve/coordinator-ontap-action branch March 10, 2021 02:28
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