Make MeViewControllers observe changes to account - #4660
Merged
Conversation
Otherwise, it wouldn't emit a value until some account property changed
We're already observing changes to the data model, no need for a second refresh
Instead of using a class property, make the observables instance properties so we can inject the context of TestContextManager.
While writing unit tests, the previous code was failing. I'm still not sure why, but it seemed more adequate to use the given objectID, since it's a map function.
koke
changed the title
This avoids some problems testing account change observers, since dispatch_async doesn't play nice with XCTest asynchronous helpers
Rely only on RxSwift instead of notifications, as they are an implementation detail that might change. Since the problem with notifications is gone, moved the tests back to AccountServiceTests.
This was referenced Jan 15, 2016
Contributor
|
|
koke
added a commit
that referenced
this pull request
Jan 22, 2016
…es-to-profile Make MeViewControllers observe changes to account
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fix introduces RxSwift 2.1 and a new way to observe changes to the default account.
I think this could be made more generic allowing us to subscribe to changes to any specific managed object, but observing the default account was good enough for now.
Known Issues
The observables only support subscribing on the main thread. I've done some testing with
subscribeOn, and values would sometimes be emitted on the right queue, but others would be forced on the main thread. I believe this is because the account changed notification is posted on the main thread, and RxCocoa's rx_notification, doesn't handle the thread switch well, but I'd have to dig more to figure our exactly what's going on.Fixes #4416 and sets the scene for #4613
Needs Review: @sendhil