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

Animating cell size change don't-know-how #459

Closed
2 tasks done
iostriz opened this issue Feb 1, 2017 · 13 comments
Closed
2 tasks done

Animating cell size change don't-know-how #459

iostriz opened this issue Feb 1, 2017 · 13 comments
Labels

Comments

@iostriz
Copy link

iostriz commented Feb 1, 2017

Hi IGListGang,

I'm facing one odd problem - but I admit it is the lack of my experience with IGListKit. So please, if you find it interesting enough, answer it, pretty please. I apologise if you had similar question, I glanced all of them without success.

I must have a recipients selector, a horizontal collection view with circles representing contacts and with initials inside. When user clicks on one, it should expand a bit and display some additional information.

Like this:

explain

When user clicks on the cell, it should glide nicely into expanded position and vice versa; preferably with some custom springy-thingy dance. You can take a look into "Classic" approach in provided project, quite straight-forward, less then one screen example...

Then I said, ok let's IGListify this. I have decided to go with single section controller with multiple items inside, because it feels natural to put NSInteger selectedIndex; that will keep information about which model is in selected state, inside section controller.

But, animation was jerky and eye-poking.

Then I have created the same example with one item per section controller (I saw that almost all provided samples in your project use one item per controller), and with some mumbo-jumbo-selectedIndex-delegate-weak-reference, it worked. ...But with the same result...

Here I have provided small project that runs all three examples. Again, please, show me the right path, it feels like it should work, only I don't know where to put this animating thing and how.... Also if you have a better design in mind, I would really appreciate it. Please remove performBitchUpdates if possible. (:-) Hi Ryan!)

RecipientsViewController.zip

br
IGor

New issue checklist

General information

  • IGListKit version: HEAD
  • iOS version(s): latest
  • CocoaPods/Carthage version: latest
  • Xcode version: latest
  • Devices/Simulators affected:
  • Reproducible in the demo project? (Yes/No): Yes
  • Related issues:
@rnystrom
Copy link
Contributor

rnystrom commented Feb 1, 2017

@iostriz thanks for the sample project! TIL that you can animate UICollectionView batch updates w/ the spring animation API. That's pretty cool.

The reason that this doesn't work is b/c IGListKit asynchronously coalesces updates. That is, it collects multiple batch updates during the runloop and unloads them at once. So the call to adapter.performUpdates(animated: true) doesn't actually trigger collection view's batch updates until later (async dispatch calls).

We might be able to do something about this, though. I wonder if we can get the currently stacked animations when batch updates is called, store them somewhere, then hand them to the IGListAdapterUpdater so it can apply them later.

Tbh I have no idea if CATransaction has a way to do this? It might be private-ish (like getting/setting the transaction properties).

Time to dig into some Core Animation...

@iostriz
Copy link
Author

iostriz commented Feb 1, 2017

Hi Ryan, thanks for taking a peek into project.
I would be satisfied with the present situation, only if it does not behave bad in some cases.
E.g. selecting a circle on the left side of the currently selected one works well, but selecting the one on the right side animates poorly.
<sigh!>

@zhubofei
Copy link

zhubofei commented Feb 1, 2017

@iostriz @rnystrom I did some digging on your sample code:

selecting a circle on the left side of the currently selected one works well, but selecting the one on the right side animates poorly.

  • IMHO, the reason why this happens is that instead of updating(reloading) the original cell, IGListCollectionView (actually UICollectionView in general) deletes the old cell and inserts a new one. See here.
  • Moreover, the reason why this code
[UIView animateWithDuration:0.35 delay:0.0 usingSpringWithDamping:0.7 initialSpringVelocity:0.0 options:0 
  animations:^{
    [_collectionView performBatchUpdates:nil completion:nil];
  } completion:nil];

works like magic is that, since the update block is nil, the only thing collectionView does here is invalidating its layout.

So we might be able to bring your springy animation back once we add #360.

@rnystrom
Copy link
Contributor

rnystrom commented Feb 1, 2017

@iostriz ok I did more digging on your setup and got it working a little better, but its not perfect.

A couple recommendations:

  • If the expected behavior is that only a single item can be "expanded" at a time, then you should use a single section controller w/ many items inside it. Why? Because the state is shared among many items (e.g. expanding one collapses another). Sending events back up to the parent just to trickle that state back down breaks the point of keeping section controllers isolated.
  • @zhubofei is right that reloading in batch updates doesn't work b/c we convert them to deletes+inserts, however I see you're using reloadInSectionController:atIndexes: w/out the batch update block, which turns straight into a batch update.

So what I did was update -[RecipientsSectionController didSelectItemAtIndex:] to this:

- (void)didSelectItemAtIndex:(NSInteger)index
{
    NSMutableIndexSet *indexes = [NSMutableIndexSet indexSetWithIndex:_model.selectedIndex];
    [indexes addIndex:index];
    _model.selectedIndex = index;

    [UIView
     animateWithDuration:0.35
     delay:0.0
     usingSpringWithDamping:0.7
     initialSpringVelocity:0.0
     options:0
     animations:^{
         [self.collectionContext reloadInSectionController:self
                                                 atIndexes:indexes];
     }
     completion:nil];
}

Like I said its not perfect, sometimes the animation doesn't spring and just fades, which is boring.


I've honestly never seen the spring animations work like this, so its pretty cool! I wonder if we should add another IGListCollectionContext that calls an empty batch update internally to do things like this? @zhubofei I attempted this w/ invalidating the layout, but it doesn't look like we can animate that.

This made me realize that the reload methods are worth keeping, which is contrary to #392. I think that delete+insert+move have to be done in a batch update, but reloads are valid outside of that scope, especially for stuff like this.

This issue was really valuable!

@zhubofei
Copy link

zhubofei commented Feb 2, 2017

@rnystrom Right, animation over layout invalidation doesn't work (cells spring just fine, but there are some random white views flashing in the background). Don't know why 🤔.

I wonder if we should add another IGListCollectionContext that calls an empty batch update internally to do things like this

This would be great! Actually, I think we can allow sending in an update block as well. So that we can perform some layout-only updates. Something like:

[self.collectionview performBatchUpdates:^{
    [self.collectionview.collectionViewLayout invalidateLayout];
    [self.collectionview setCollectionViewLayout: newLayout animated:YES];
} completion:^(BOOL finished) {
 ...
}];

@iostriz
Copy link
Author

iostriz commented Feb 2, 2017

@rnystrom Glad that I have a "valuable issue"... Maybe you can buy me a sandwich? :-)
Just kidding, this framework is much more valuable to me then this issue is to you.
Looking fwd to the updates on this topic...

@rnystrom
Copy link
Contributor

rnystrom commented Feb 2, 2017

@zhubofei good point. Reminds me that the layout that we're using in #450 caches values, so you can't just change w/e is returned in the size methods, you have to invalidate the layout.

Maybe the layout methods need to be smarter, something like:

- (void)invalidateLayoutForSectionController:(IGListSectionController<IGListSectionType> *)sectionController
  indexes:(NSIndexSet *)indexes
  animated:(BOOL)animated;

And the implementation inside IGListAdapter looks something like:

- (void)invalidateLayoutForSectionController:(IGListSectionController<IGListSectionType> *)sectionController
  indexes:(NSIndexSet *)indexes
  animated:(BOOL)animated {
  NSArray<NSIndexPath *> paths = // create paths from SC + indexes
  UICollectionViewLayoutInvalidationContext *context = // new from layout
  [context invalidateItemsAtIndexPaths:paths];

  void (^updates)() = ^{
    [self.collectionView.collectionViewLayout invalidateLayoutWithContext:context];
  };

  if (animated) {
    [self.collectionView performBatchUpdates:updates completion:nil];
  } else {
    updates();
  }
}

Some thoughts:

  • Idk if we want to add a completion option
  • I tested this w/ @iostriz's example just now and it still does the spring animation. This is probably how we want to do layout invalidation w/ animations.

@iostriz
Copy link
Author

iostriz commented Feb 2, 2017

@rnystrom Glad you find this valuable. Maybe you can buy me a sandwich! :-)
Just kidding, this framework is more valuable to me than this issue is to you.
Looking fwd to the updates on this.

@zhubofei
Copy link

zhubofei commented Feb 3, 2017

@rnystrom

Idk if we want to add a completion option

probably not 😂.

@iostriz
Copy link
Author

iostriz commented Feb 21, 2017

@rnystrom Thanks Ryan for fixing #499. Looking fwd to try it out.

@rnystrom rnystrom reopened this Feb 21, 2017
facebook-github-bot pushed a commit that referenced this issue Feb 21, 2017
Summary:
Adding a new layout-invalidation API, telling the layout object to query and rebuild the layout for all items in the section controller. This works with `UICollectionViewFlowLayout` and should work with other custom layouts (including our own).

Issue fixed: #360, #459

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
Closes #499

Reviewed By: jessesquires

Differential Revision: D4590274

Pulled By: rnystrom

fbshipit-source-id: f87235be4e6c024bf979b831a8938be68895e011
@LivioGama
Copy link

Like mentioned in roberthein/BouncyLayout#6 I have trouble using this new feature with a custom UICollectionViewFlowLayout. When changing a section controller color and height, the color won't change unless I call adapter.reloadObjects([]) and the height won't change : a strange space equal to what the cell should have increased its height of is created like 6 cells more down.

simulator screen shot 13 mai 2017 a 21 19 01
simulator screen shot 13 mai 2017 a 21 19 17

It looks like a cache problem in the layout, but I don't see any cache mecanism in BouncyLayout.

Please find attach my example :
BouncyLayout-master.zip

Maybe I'm doing something wrong. I start to wonder if a "IGListCollectionView**Flow**Layout" won't be necessary in the future.

@LivioGama
Copy link

I progressed a bit, I found a way to do it but it's a bit dirty (see delegate method func didSelect(_ style: CellStyle) and it required that I change BouncyLayout class UIDynamicAnimator variable to open. This animator was the guilty one for caching old values and has to be empty manually before each addition/deletion/update of cells. Any insight regarding your experience guys ?

New code :
BouncyLayout-master.zip

@rnystrom
Copy link
Contributor

@LivioGama do you mind opening a new issue to track this?

@Instagram Instagram locked and limited conversation to collaborators May 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants