Reverted some View and Widget Observables to use simple objects instead of Event objects

[Alexander--]

Recent changes to UI observables (particularly, 406bf68) added number of POJO event classes as well as some of associated infrastructure. Some of those were really useful, but few others: click, text change and toggle (aka "input") event classes look forced, as if those were added solely to match style of similar methods. Unfortunately, as explained below, they aren't very useful and accomplish nothing, except adding redundant entities and increasing range of possible issues.

Few possible uses for event classes, that come to mind are:

1. Encapsulating state, not stored in event source itself. This is the only sensible use for event classes in Android, which is why OnItemClickEven and OnListViewScrollEvent are legitimately useful and were added for good reason. Clearly not the case for click, text change and CompoundButton toggle events.
2. Storing associated data, for example for doing reduce on it. None of the event classes in question (except OnSubscribeTextViewInput) contain any useful state. Click and compound button switch are self-descriptive. CharSequence, returned by TextView.getText(), can be mutable, and as such, is dangerous to use, unless immediately converted to string (which means, that at least one map call have to be done anyway).
3. Turning emission of same object into emission of multiple distinct objects. RxJava already have timestamp operator for this.
4. Passing those events somewhere else, possibly after serializing them. Doing so with all 3 events in question is pointless and even potentially dangerous. All of them contain little besides strong reference to event source, which makes them potential source of memory leaks, and makes it harder to write Rx plugins, detecting those.

This merge request adds back Observables with tests and corresponding static methods, emitting plain View, TextView and Boolean objects. It also deprecates event-based methods. While we are at it, I have also renamed "input" to "toggle" as original naming choice was just plain bad.

[dlew]

Numbering classes... ick.

[dpsm]

Not sure why the 2 suffix?

[JakeWharton]

Because there's already OnSubscribeTextViewInput. This class is an implementation detail so the ugly "2" can be removed when the former is deleted.

[dlew]

It doesn't make sense to have multiple nearly identical API calls that are subtly different enough to confuse people. I'd rather go one way or the other, but not this both solution (especially since it involves numbering classes).

[hamidp]

We really should strongly define a style/code guide. PRs have started becoming bikeshed style discussions.

cc @mttkay @JakeWharton

[mttkay]

Well, it's not so much bike shedding as it is just that this should be automated. I think keeping a consistent style is hugely important especially with like a dozen PRs coming in every week. I neither have the time nor would I take it if I had it to manually review every code style flaw. That's what tools exist for. I think @JakeWharton already worked on this? I saw a checkstyle template somewhere. It just needs adding to the build.

[hamidp]

@mttkay I did not mean to communicate that this PR is bikeshedding, rather that I've noticed that a lack of style guide can help lead to situations like these.

It is especially easy for stuff like this to happen when a project moves fast and there isn't a central authority on style leading to slow divergence.

[mttkay]

What was everyone's thoughts on this? Should this go in? I don't have a strong opinion on it.

[dlew]

I strongly disagree that we should have two versions of the same thing in the library. I think it's best to be opinionated. Otherwise the library just becomes confusing. I'm fine if we decide the simpler version (shown here) is the right version, but we shouldn't have both.

[hamidp]

I agree with Dan. Either one, but not both.

@JakeWharton you have any thoughts?

[JakeWharton]

No 's' prefix

[JakeWharton]

I think I prefer the more simple ones. There's definitely compelling reasons to just do a breaking API change here.

And yes I need to get checkstyle in here ASAP. I'll do it separately so as to avoid dealing with all the build issues at once.

[Alexander--]

I have addressed all of issues, pointed out by @JakeWharton. Digit suffixes replaced by Old, in case those were to be confusing for someone.

[dlew]

OnSubscribeViewClickOld is almost as bad as OnSubscribeViewClick2. Why don't we just remove all the old code? This method has broken back-compat multiple times already anyways, might as well go for the hat trick.

[Alexander--]

@dlew, well, all my code still uses 4 versions old RxAndroid, because the compatibility gets broken every release :(

[JakeWharton]

That's by design.
On Dec 7, 2014 12:13 AM, "Alexander--" notifications@github.com wrote:

@dlew https://github.com/dlew, well, all my code still uses 4 versions
old RxAndroid, because the compatibility gets broken every release :(

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

[mttkay]

@Alexander-- you're using a pre-release version of a library, that has to be expected. What's the alternative you suggest? We shouldn't hesitate to change this while we're in stage where it's still okay to do exactly that.

RxJava core saw dozens of breaking changes on the road to 1.0. That's I guess that cost we have to pay for using alpha level software, but ultimately, it's healthy for the library.

[Alexander--]

If you are so strongly compelled to remove few backward compatible paths from this PR, do it already (I am, however, still going to include them in future ones).

[dlew]

First of all - we can't edit your PR.

(I am, however, still going to include them in future ones)

I'm trying to make sure I understand this correctly - are you saying that you will keep submitting PRs to re-add any code that we're talking about removing here?

[JakeWharton]

First of all - we can't edit your PR.

You can clone his branch, make the changes, amend to his commit, merge to master, and push to the remote. Then you get the author info but you become the committer. I do this all the time for cleaning up other people's PR as well as squashing. It's super annoying.

[dlew]

@JakeWharton That is convoluted, depressing, and something I hope we can avoid unless necessary.

[Alexander--]

I'm trying to make sure I understand this correctly - are you saying that you will keep submitting PRs to re-add any code that we're talking about removing here?

I don't plan to spam you, unless you ask for it yourself :) I am going to deprecate things (when possible and makes sense) instead of removing them, as I stated in #94.

You can clone his branch, make the changes, amend to his commit, merge to master, and push to the remote.

Alternatively to merge this pull request into master locally you can do

git fetch git://github.com/Alexander--/RxAndroid.git "0.x" && git merge --no-ff --no-commit FETCH_HEAD

perform any changes you want as part of merge commit, and push the changes. IIRC, you can also use

git fetch origin refs/pull/100/head:pr/100 && git merge --no-ff --no-commit FETCH_HEAD

not sure if this was ever documented by Github developers themselves.

[JakeWharton]

Yes that's what I do except on a branch. I have a bash alias that does everything for me.

[hamidp]

This library is still in infancy and in the interest of a clean API in the future we should not be afraid of breaking changes. The least impactful time to remove things is now. The API is in flux and will continue to be in that state for a while. We should remove old code instead of deprecating it.

If you really need backwards compatibility then don't upgrade until you are ready to, or fork. Neither is a good option admittedly.

[JakeWharton]

Per #172 we have removed the view binding observables for the next release.