Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Use addAll instead of our own fori #64

Merged
merged 1 commit into from
Jan 24, 2017
Merged

Use addAll instead of our own fori #64

merged 1 commit into from
Jan 24, 2017

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Jan 24, 2017

This will fix #63

@StefMa StefMa added the rx label Jan 24, 2017
@StefMa
Copy link
Contributor Author

StefMa commented Jan 24, 2017 •
edited
Loading

CompositeSubscription and CompositeDisposable have a different behavior in addAll.
See this issue on RxJava.

I've already replace our own fori with addAll, even in rx2.
Should we "revert" it and use our previous implementation?! 🤔

@passsy
Copy link
Contributor

passsy commented Jan 24, 2017

I prefer addAll because CompositeSubscription and CompositeDisposable handle concurrency better than our fori solution.

The difference between rx1 and rx2 is interesting but not a big deal. In the end every Disposable will be freed once dispose() gets called in TiPresenter#onDestroy()

passsy
passsy approved these changes Jan 24, 2017
View reviewed changes
Copy link
Contributor

@passsy passsy left a comment

Choose a reason for hiding this comment

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

feel free to merge

@StefMa StefMa merged commit 53579df into GCX-HCI:master Jan 24, 2017
@StefMa StefMa mentioned this pull request Jan 24, 2017
Closed
@passsy passsy deleted the feature/use_addall_instead_of_fori branch January 24, 2017 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@passsy passsy passsy approved these changes

Assignees
No one assigned
Labels
rx
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User addAll instead of using an fori
2 participants
@StefMa @passsy