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

Change onWakeUp() to onAttachView(TiView) and onSleep() to onDetachView() #26

Merged
merged 3 commits into from
Oct 21, 2016

Conversation

passsy
Copy link
Contributor

@passsy passsy commented Oct 14, 2016

The new naming is more convenient throughout most android MPV libraries. Switching to Ti should now be simpler. It reflects better that it's only about the view being absent. wakeup and sleep could imply the Presenter is kind of starting/stopping which is not the case.

onAttachView(TiView) has the attached view as first parameter which is @NonNull. No calls to getView() are neccessary inside onAttachView(TiView).
getView() is now @Nullable (and always was null when not between wakeup() and sleep() state) and causes lint to hint a Nullable warning. This warning was missing and caused a lot of NPE for Ti beginners

…ew()

The new naming is more convenient thoughout most android MPV libraries. Switching to Ti should now be simpler. It also reflects better than before that the it's only the view being absent. wakeup and sleep could imply the Presenter is kind of starting/stopping which is not the case.

onAttachView(TiView) has the attached view as first parameter which is @nonnull. No calls to getView() are neccessary inside onAttachView(TiView).
getView() is now @nullable (and always was null when not between wakeup() and sleep() state) and causes lint to hint a Nullable warning. This warning was missing and caused a lot of NPE for Ti beginners
Copy link
Contributor

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

The changes looks great and I approve that.
But don't forget to update the Readme ;)

}

mView = view;
moveToState(State.VIEW_ATTACHED, false);
mCalled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

mCalled isn't a good name.
I think it would be nicer if we have two booleans. mViewAttachedCalled and mViewDetachedCalled.
So we can't override the single boolean anymore...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a general property for all super call checks. Implementation and naming copied from AOSP Actiivty and Fragment https://github.com/android/platform_frameworks_base/blob/4b1a8f46d6ec55796bf77fd8921a5a242a219278/core/java/android/app/Activity.java#L748

@@ -288,25 +323,18 @@ public String toString() {
}

/**
* when calling {@link #wakeUp()} the presenter can start communicating with the {@link
* TiView}.
* The view is now attached and ready to receive events. {@link #getView()} is not guaranteed
Copy link
Contributor

Choose a reason for hiding this comment

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

Better Javadoc:
getView() may be null.
Current javadoc is very confused and have to be read more than once ;)

*/
protected void onDetachView() {
if (mCalled) {
throw new IllegalAccessError("don't call #onSleep() directly, call #detachView()");
Copy link
Contributor

Choose a reason for hiding this comment

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

don`t call #onDetachView () directly...

right?

public void attachWithoutInitialize() throws Exception {

// not calling
// mPresenter.create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed. The method should say what happen here.
Maybe a better name is attachWithoutOnCreate. But I'm also fine with the current name

@StefMa StefMa merged commit 3a61c4d into master Oct 21, 2016
@StefMa StefMa deleted the feature/refactor_presenter_view_methods branch October 21, 2016 14:27
@passsy passsy added this to the 0.8.0 milestone Oct 21, 2016
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