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

First attempt at adding Account Observable #103

Closed
wants to merge 2 commits into from

Conversation

Alexander--
Copy link

This is a rough draft, produced from code, I have been using for a while already.

Note, that unlike most other operators in the library OnSubscribeAddAccount is left public to allow people to better adjust it for various needs - the code is already too sophisticated and does too much to try and stuff everything in bunch of static methods.

Before finishing this, I would like to know answers to following questions:

  1. How do I test it? Robolectric 2.3 does not have proper shadow for AccountManager, so should I just write my own? By the way, are there plans to update to Robolectric 2.4, perhaps by employing SDK manager plugin to fetch dependencies? I am aware of #1369, but at current rate it is not going resolve itself.
  2. What are project guidelines (ok, what should those be) regarding use of annotations? Complex AccountManager methods need @Nullable annotation rather badly, but which one to choose? Android ones? IntelliJ ones (used so far, because they are easiest to get). Perhaps, something more complex (Checker Framework)?

@JakeWharton
Copy link
Contributor

I think this belongs in the framework module, not the core.

By the way, are there plans to update to Robolectric 2.4, perhaps by employing SDK manager plugin to fetch dependencies?

These have nothing to do with each other.

What are project guidelines (ok, what should those be) regarding use of annotations?

The Android ones.

@Alexander--
Copy link
Author

I think this belongs in the framework module

What is the reason behind such decision? From my viewpoint, android.account is no different from other core Android APIs, at least no less than Support Library.

Also, How do I add Android annotations to rxandroid build structure?

@JakeWharton
Copy link
Contributor

It's an integration with a framework library not adapting RxJava to
platform conventions and APIs.
On Nov 28, 2014 7:17 PM, "Alexander--" [email protected] wrote:

I think this belongs in the framework module

What is the reason behind such decision? From my viewpoint,
android.account is no different from other core Android APIs, at least no
less than Support Library.


Reply to this email directly or view it on GitHub
#103 (comment).

@omo
Copy link
Contributor

omo commented Nov 29, 2014

At the same time, the API surface is not opinionated at all, that is (IMO) the criteria of what should go to the framework module. Things like RxActivity subclass are obviously opinionated and should go rxandroid-framework, but this one doesn't seem so.

I'm not sure if we should focus on such level of minimalism at this point. Rx-ification of Android frameworks is what RxAndroid is for and this one fits in that category.

As RxAndroid grows, the distinction between core and non-core might become clearer. We can split non-core part out at that point. I feel it's a bit too hasty to decide it now though.

@JakeWharton
Copy link
Contributor

Contributions to this library are guilty until proven innocent. Every single API addition should be justified and widely applicable. Not forcing minimalism at this point is going to turn the library into a mess that cannot be versioned or at all evolved without constant breaking API changes or mass amounts of pain. This is further exacerbated by the fact that there is no single person at the helm and no one is being paid to actually work on this.

@@ -0,0 +1,42 @@
package rx.android.account;
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs license header

@Alexander-- Alexander-- force-pushed the account-observable branch 2 times, most recently from 0a091d5 to c171bf7 Compare December 2, 2014 03:33
public void run() {
subscribe(subscriber);
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand why this is necessary. It looks like you're re-implementing OperatorSubscribeOn functionality? What makes this different from saying o.lift(new OnSubscribeAddAccount(..)).subscribeOn(handlerThreadScheduler)?

Copy link
Author

Choose a reason for hiding this comment

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

It looks like you're re-implementing OperatorSubscribeOn functionality?

I am not. This operator does it for the same reason throttle accepts a Scheduler parameter - because Account creation can only happen on a Handler thread.

@Alexander-- Alexander-- force-pushed the account-observable branch 2 times, most recently from 0140278 to c5e43b9 Compare December 4, 2014 01:38
@Alexander--
Copy link
Author

@mttkay, thanks for pointing out issues with imports - I have noticed, that those weren't consistent across tests, so I have fixed them in other placers as well.

If there are other issues, preventing this from being merged, please let me know.

@ronshapiro
Copy link
Contributor

I'm not convinced this solves an issue or pain-point for anyone else. I can't imagine wanting to modify a stream of Accounts in a functional paradigm, doing this on multiple threads, or composing it with another stream of objects. Can anyone explain what makes this a particular "Rx" solution to a use case?

Speaking of, I think that we should mandate that every addition is accompanied by a couple different use cases. There needs to be a separation between "I've used this in an Rx fashion in my app" and "Many people will use this how I have."

@Alexander--
Copy link
Author

Can anyone explain what makes this a particular "Rx" solution to a use case?

Android accounts can be managed externally (e.g. removed from System Settings app by user). Because of this every use of Account object must be guarded by check for account existence and, if necessary, call to account creation routine (later being asynchronous). After an Account is created, the application is notified by callback.

TL; DR: every call to Account-dependant method in application requires an asynchronous callback. Is it clear enough now?

@JakeWharton
Copy link
Contributor

His question is more about as to whether or not this would ever actually be used in a functional pipeline. The asynchronous nature is a given, and it's fine to use RxJava for that and I don't think anyone would object to using it for this. The concern is whether this is a high-utility API compared to the others provided in the core.

@Alexander--
Copy link
Author

Just because RxJava is good at dealing with infinite streams, does not mean it is only for long functional pipelines. For example, most users won't ever need to throttle streams of SMS arrival Broadcast - they will receive a single one and unsubscribe. Personally I do my best to combine multiple async events together, so most of my server-facing code starts from Observable<Account>. Sometimes I just combine Observable<Account> and Observable<NerworkInfo> together or even simply subscribe to Observable<Account> before letting a user into some screen. But that's me. Android Account Framework can be used for variety of reasons: to run energy-saving Sync Adapters, to insert stuff into system Content Providers, to access existing Account-based facilities etc., so other people may find their own use cases for the operator in question.

@JakeWharton
Copy link
Contributor

Neither long nor infinite was mentioned, and we all understand the value in what account management provides. I was referring to the compositional nature so your two examples are those which we were after. Even more of these and more concrete would be great.

Again, the concern is of the concrete use cases—not hypothetical ones people may find later. We should be starting with as few pieces of extremely high-value APIs in a 1.0. Adding is easy, removing is hard or impossible. His concern (and really all of ours) is simply determining whether this is on the side of high-value or not.

This library lacks not only a clear voice, but any distinction of what should or should not be included. To make it worse, we now have two modules between which the boundary is fuzzy. How do we determine what makes the cut as widely applicable and worth the cost of supporting the code? On what side does this fall?

@mttkay
Copy link
Collaborator

mttkay commented Dec 4, 2014

How would you guys feel about using the rxandroid-framework module as a sort of incubator for potentially useful library features? We could let things mature in the framework first, gauge its impact and utility, and if people find that something is useful enough for it to be available outside the framework we could promote it into the library.

At least that way we do not lose potentially useful work? It would mean of course that the framework need to take a more liberal approach to adding features.

@JakeWharton
Copy link
Contributor

I was going to propose that (along with a rename to convey), but I worried people would get too sentimental of the contents. I'm actually fine with it so long as we aggressively message the intent. And it has a different name like rxandroid-incubator or something...

@mttkay
Copy link
Collaborator

mttkay commented Dec 4, 2014

👍

@Alexander--
Copy link
Author

This library lacks not only a clear voice, but any distinction of what should or should not be included.

To be honest, I kind of realize what are you coming at. android.accounts API is handy, but many apps don't need to batch network operations or track their users and, as such, won't ever need such Observable (unlike currently included things, which are fairly general-purpose). But I don't think, that this code should go into rxandroid-framework (because they have to be separated based on different reasons). It also doesn't really deserve a separate project in it's current state. How about adding such stuff to the main codebase, but putting in some kind of transitional package (something like rx.android.misc)? If some of it gains enough size/momentum to warrant a separate project, just move it to fitting package in project of it's own.

@mttkay
Copy link
Collaborator

mttkay commented Dec 4, 2014

I don't think the idea was to suggest a different project, just a different module that is deployed as a separate artifact. That's easy to accomplish with Gradle and maintains tight dev-time integration with the main library.

You'd have one more dependency in your application project, nothing else would change; it would still be maintained as part of this project. I don't think a separate package in the main library changes anything.

@dlew
Copy link
Collaborator

dlew commented Dec 4, 2014

Generally I like the idea of an incubator to determine what should be in vs. out, but how would we determine if people are actually using different incubator classes? What's the metric?

@ronshapiro
Copy link
Contributor

We could go the Guava route of having @Beta annotations, but I'm not sure anyone really pays attention to those.

Another options is to build multiple artifacts and link them all in the README. It would be a little more cumbersome and less discoverable in an IDE if you haven't included the other jars, but it would mean we could cut something off more easily if we decided it was no longer needed. There's nothing in some of the View related utilities that this depends on, so we could break it up into rxandroid and rxandroid-views and rxandroid-accounts etc. I'm not sure it's a good idea, but figured I'd throw it out.

@Alexander-- Alexander-- changed the title [questions] First attempt at adding Account Observable First attempt at adding Account Observable Dec 7, 2014
@JakeWharton
Copy link
Contributor

As part of #172 support for components like this have been removed. There's far too much breadth in the use case of these components to warrant inclusion in a core module like RxAndroid. This is something that would be better served as a standalone, focused library.

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.

7 participants