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 fromSharedPreferencesChanges Observable #25

Merged
merged 2 commits into from
Oct 15, 2014

Conversation

Yarikx
Copy link
Contributor

@Yarikx Yarikx commented Oct 10, 2014

This is wrapper around SharedPreferences.OnSharedPreferenceChangeListener
It's pretty simple and straightforward, but is useful to observe SharedPreferences changes.

this useful to listen for SharedPreferences changes
@nsk-mironov
Copy link
Contributor

I usually use OnSharedPreferenceChangeListener to observe only one particular key. Maybe we should also provide an overload that observes the specified key and emits its value? Something like:

Observable<T> fromSharedPreferencesChanges(SharedPreferences preferences, String key, Function2<SharedPreferences, String, T> extractor);

Observable<Integer> observable = fromSharedPreferencesChanges(preferences, "key_to_observer", new Function2<SharedPreferences, String, Integer> () {
    @Override
    public Integer call(SharedPreferences preferences, String key) {
      return preferences.getInt(key, 0);
    }
};

Anyway, LGTM.

@Yarikx
Copy link
Contributor Author

Yarikx commented Oct 10, 2014

@mironov-nsk I was thinking about it too. Actually I also use modified version of this code that listen only for particular key.
But such approach can be simply achieved via .filter operator.
So it's better to ship more general sollution that can be adapted to particular case.
But anyway we can add one more method to AndroidObservables that will combine this operator with .filter

@Override
public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
if(!subscriber.isUnsubscribed())
subscriber.onNext(key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a style remark: the indentation is off, and we should always use curly braces even for one liners. It makes it more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, is this check necessary? RxJava makes a best effort at unsubscribing, e.g. when the sequence completes or fails. In general it shouldn't be necessary to check a subscription before emitting a notification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About subscriber.isUnsubscribed() check:
I was just following instructions from https://github.com/ReactiveX/RxJava/wiki/Implementing-Your-Own-Operators#other-considerations.

Sometimes I'm not sure should I check for it or not.

@mttkay
Copy link
Collaborator

mttkay commented Oct 13, 2014

👍 except for the remark above

Let me know if or how you want to address it, then I'll land it

@Yarikx
Copy link
Contributor Author

Yarikx commented Oct 13, 2014

And one more point I want to discuss:
Should it be named as 'fromSharedPreferencesChanges' ?
I assume that number of operators will increase, so we need to make convention about names and location of new operators.
Is static method of AndroidObservables is good place for it?
Is methods with prefix 'from...' is good names for operators?

@mttkay
Copy link
Collaborator

mttkay commented Oct 15, 2014

Good questions. I think it's something we have to find out with time. That's exactly why I want the project to remain in pre-release status for a while, since I anticipate there to be breaking API changes still

I'll merge this, but maybe you can open a question issue where we can gather feedback around this, and if necessary, rename things in one fell swoop.

mttkay added a commit that referenced this pull request Oct 15, 2014
Add fromSharedPreferencesChanges Observable
@mttkay mttkay merged commit 2bb5640 into ReactiveX:0.x Oct 15, 2014
@Yarikx Yarikx deleted the shared-preferences-observable branch October 15, 2014 08:06
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.

3 participants