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

MutableCollectionProperty #2485

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@pepibumur

pepibumur commented Oct 19, 2015

What?

I thought it would be useful having a custom property in ReactiveCocoa for collections that notified about changes in the collection with more details (elements updated, removed, inserted, ...). That way if you use the property to represent for example a TableViewCollection you can update on the view only the elements that have actually changed.

I originally implemented the feature on this repository https://github.com/gitdoapp/RAC-MutableCollectionProperty

The component is fully tested.

@NachoSoto

This comment has been minimized.

Show comment
Hide comment
@NachoSoto

NachoSoto Oct 20, 2015

Member

Thanks for opening a PR!

I don't think this belongs as part of RAC though, I think it makes much more sense as a separate library (which I just starred, it looks awesome!). What do you think?

Member

NachoSoto commented Oct 20, 2015

Thanks for opening a PR!

I don't think this belongs as part of RAC though, I think it makes much more sense as a separate library (which I just starred, it looks awesome!). What do you think?

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Oct 20, 2015

Member

Agreed, this should remain separate from RAC.

Also, this is more complicated than necessary. I've written some collection change notification systems that looked similar. It took me forever to realize that insert and remove is all you need with element and index. Everything else (move, replace, etc.) is a composite and should be defined in terms of them.

Often the reason for extra events is batching and animating. I'd recommend keeping these concerns separate and limiting yourself to this.

enum Event<T> {
    case Insert(T, Int)
    case Remove(T, Int)
    case Composite([Event])
}

The Composite case could even be removed if you buffer before mapping to animations.

Member

neilpa commented Oct 20, 2015

Agreed, this should remain separate from RAC.

Also, this is more complicated than necessary. I've written some collection change notification systems that looked similar. It took me forever to realize that insert and remove is all you need with element and index. Everything else (move, replace, etc.) is a composite and should be defined in terms of them.

Often the reason for extra events is batching and animating. I'd recommend keeping these concerns separate and limiting yourself to this.

enum Event<T> {
    case Insert(T, Int)
    case Remove(T, Int)
    case Composite([Event])
}

The Composite case could even be removed if you buffer before mapping to animations.

@pepibumur

This comment has been minimized.

Show comment
Hide comment
@pepibumur

pepibumur Oct 20, 2015

Hey @NachoSoto and @neilpa , I can keep it in that library. I wasn't sure if it could fit with the core library components. Also thanks for the tips @neilpa , I'll apply your suggestions. I was actually thinking about the same but ended up adding events which aren't needed at all.

Thanks you both, closing this PR then.

pepibumur commented Oct 20, 2015

Hey @NachoSoto and @neilpa , I can keep it in that library. I wasn't sure if it could fit with the core library components. Also thanks for the tips @neilpa , I'll apply your suggestions. I was actually thinking about the same but ended up adding events which aren't needed at all.

Thanks you both, closing this PR then.

@pepibumur pepibumur closed this Oct 20, 2015

@pepibumur pepibumur deleted the feature/mutableCollectionProperty branch Oct 20, 2015

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Oct 20, 2015

Member

@pepibumur Awesome! I also opened an issue in that project in case you have other questions for me about this stuff. I've given it quite a bit of thought in past projects.

Member

neilpa commented Oct 20, 2015

@pepibumur Awesome! I also opened an issue in that project in case you have other questions for me about this stuff. I've given it quite a bit of thought in past projects.

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