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

2.0.0 forces rx/observables #28

Closed
scriptacus opened this issue Jul 15, 2015 · 7 comments
Closed

2.0.0 forces rx/observables #28

scriptacus opened this issue Jul 15, 2015 · 7 comments

Comments

@scriptacus
Copy link

The only real Presenter class in 2.0.0 is now RxPresenter. This seems kind of silly. Can we get a basic presenter that at least has "getView()" something similar?

@konmik
Copy link
Owner

konmik commented Jul 15, 2015

Hi,

getView() looks like a workaround to me, its elimination was a matter of time.

Do you have a use case for it?

@scriptacus
Copy link
Author

My presenter calls methods on the associated view, but I'm not using Rx. Ususally just "getView().setFoo( true );" I'm sure I can do the same with the observable returned by RxPresenter.view(), but I'm not using that functionality, so it seems odd to need it. For now I've just defined my own view in my presenter class and am setting it during onTakeView.

@konmik
Copy link
Owner

konmik commented Jul 15, 2015

Sounds reasonable.

I was planning to completely drop non-rx part of the library.
Why are you avoiding rx?
For me the perfect presenter looks like a some kind of rx subject, that receives views+requests and emits data.

@scriptacus
Copy link
Author

I'm mostly avoiding it because:

  1. It's ugly, and retrolambda (which cleans it up a bit) seems like such a hack that I'm hesitant to use it.
  2. I'm still learning Java/Android, and Rx is like an entire meta-language on top of that.
  3. It's confusing as all hell, even after reading a bunch of tutorials.

My understanding, regarding 1), is that this:

getView().setFoo( true );

... will become something like this:

        view().subscribe( new Action1<SomeActivity>() {
            @Override
            public void call( SomeActivity someActivity ) {
                someActivity.setFoo( true );
            }
        } );

That's just ugly, and non-necessary.

@konmik
Copy link
Owner

konmik commented Jul 15, 2015

Ok, I've got your point. I do not use retrolambda as well. :)

Do you know that calling getView() everywhere will sooner or later lead to NPE?

You don't need getView during onTakeView, but after onTakeView the view can be dropped at any moment, leaving you with NPE.

Rx guarantee that the callback is called only when getView() is not null. If you're not using rx you will need to make all that callbacks yourself and duplicate rx functionality on your own.

@scriptacus
Copy link
Author

Regarding NPE, I generally do null checks. If they get too cumbersome, I just wrap the whole thing in an update function that does an early out if getView() is null.

Btw, it looks like RxPresenter calls view.onNext(null); during onDropView. Doesn't that mean a null check within the observable will be necessary?

Regarding Rx in general; I find it difficult to wrap my head around how to handle everything. As an example, one case where I'm using it is: I have a RecyclerView with N objects in it. When you click one, it calls all the way back to the presenter, passing the ID of the clicked object. I needed to debounce those clicks before taking action, and after struggling a bit I ended up with an entire list of PublishSubjects, that mirrors the list of objects, to handle the clicks because I couldn't figure out how to get the debouce to work on a per-ID basis with just a single publish subject or observable.

@konmik
Copy link
Owner

konmik commented Jul 15, 2015

Yes, view() requires to make null checks. This observable was first designed to be used by deliverXXX methods internally. Nulls are needed to prevent memory leaks inside of combineLatest, but now I think that I need to split view() into takes() and drops() somehow.

I will keep getView(). I may probably subclass Presenter to make OldSchoolPresenterWithGetView or something. :D

Thanks for the feedback.

konmik added a commit that referenced this issue Jul 15, 2015
@konmik konmik closed this as completed Jul 15, 2015
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

No branches or pull requests

2 participants