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

Add helper contentObservable for contentObserver #145

Closed
wants to merge 1 commit into from

Conversation

HTChang
Copy link

@HTChang HTChang commented Mar 10, 2015

Add an helper function

ContentObservable.fromContentObserver(contentResolver, watchUri, schedulerHandler)

that makes the ContentObserver as an Observable

@HTChang HTChang force-pushed the contentObserver branch 2 times, most recently from f45cc93 to fc6bb1b Compare March 10, 2015 03:26
static Observable<Uri> fromContentObserver(final ContentResolver contentResolver,
final Uri uri, final Handler schedulerHandler) {
if (SDK_INT < 16) {
return Observable.error(new UnsupportedOperationException());
Copy link
Contributor

Choose a reason for hiding this comment

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

Just throw

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reminder.

@JakeWharton
Copy link
Contributor

This would need a test. Not sure how to do it with Robolectric, but with an integration test you can extend ProviderTestCase2.

*/
static Observable<Uri> fromContentObserver(final ContentResolver contentResolver,
final Uri uri, final Handler schedulerHandler) {
if (SDK_INT < 16) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use Build.VERSION_CODES.JELLY_BEAN instead of 16?

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw this too, but doesn't this project build with 4.0 because it's not an Android module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Meh, you're right. That's annoying. I guess this could be reversed to SDK_INT > VERSION_CODES.ICE_CREAM_SANDWICH_MR1, not sure if that feels weird though.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a 4.1.1.4 in Maven Central that can be used to get JELLY_BEAN

@ronshapiro
Copy link
Contributor

ShadowContentObserver#dispatchChange might be a good place to look for tests.

@JakeWharton
Copy link
Contributor

Yeah, but Robolectric :(

@wuman
Copy link
Contributor

wuman commented Mar 11, 2015

An improved version here wuman@c85d2ff. Didn't want to send a separate pull request because it's largely based on this PR.

Improvements include:

  • Removed the need to check for api levels. For api levels below 16, the base Uri will be emitted.
  • Added a unit test.
  • Made the scheduler handler optional.

@HTChang Feel free to incorporate my changes as you see fit.

@HTChang
Copy link
Author

HTChang commented Mar 11, 2015

@wuman thanks, already modified my code per your suggestion!

@artem-zinnatullin
Copy link
Contributor

Damn, I just needed this too and implemented it when I saw this PR..

But, I have an addition:

ContentResolver#registerContentObserver() has parameter notifyForDescendents, which is just true in this PR, I think it should be decision of the user

From Javadoc:

@param notifyForDescendents If true changes to URIs beginning with uri
will also cause notifications to be sent. If false only changes to the exact URI
specified by uri will cause notifications to be sent. If true, any URI values
at or below the specified URI will also trigger a match.

@JakeWharton
Copy link
Contributor

This has been removed as part of #172 for a future release. Consider building on SqlBrite's ability to query a content resolver (not that I'm biased...).

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.

5 participants