Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Bugfix - Lifecycleobserver and RxHandler* #61

Merged
merged 2 commits into from
Jan 24, 2017
Merged

Bugfix - Lifecycleobserver and RxHandler* #61

merged 2 commits into from
Jan 24, 2017

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Jan 23, 2017

Given: : 🌮
Our sample is broken.
It through a IllegalStateException: view subscriptions can't be handled when there is no view.
This cames from RxTiPresenterSubscriptionHandler.manageViewSubscription which is called from the HelloWorldPresenter#onAttachView(view)

But: Why it says that there isn't any view, when onAttachView is called? 🤔

Reson:
After a quick reseach the reason is simple:
We introduced such a exception with #58 .
So this is working like expected.
But: Why is is thrown on onAttachView?
Because of the TiLifecycleObserver. The documentation says:

/**
* gets called when the {@link net.grandcentrix.thirtyinch.TiPresenter.State} changes
*
* @param state the new state of the {@link TiPresenter}
* @param beforeLifecycleEvent {@code true} when called before the {@code on...} lifecycle
* methods, {@code false} when called after
*/

beforeLifecycleEvent is wrong here.
If we take a look into the implementation in TiPresenter#attachView we see the following:

moveToState(State.VIEW_ATTACHED, false);
// Stuff
onAttachView(view);
// More stuff
moveToState(State.VIEW_ATTACHED, true);

The second parameter here indicates if the on* method is already called.
Which is wrong according to the documentation of the TiLifecycleObserver.

The fix: 💚
Because we have already implementations with the TiLifecycleObserver I've just changed the documentation (and a better boolean name 😉)

I've also fixed the implementation of the RxTiPresenter*Handler.. which have thrown the exception...

@passsy passsy merged commit 2d94cc6 into GCX-HCI:master Jan 24, 2017
@passsy passsy deleted the bugfix/lifecycleobserver branch January 24, 2017 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

2 participants