Add fragment delegate #47
Conversation
…ng the fragment code without extending TiFragment or TiDialogFragment
it better belongs to the wiki, the README.md shouldn't be cluttered
It's not possible to pass a single Object into the delegate because in CompositeAndroid the TiViewProvider and the Fragment are different objects. Adding all interfaces one by one helps when testing. There is no god object required which implements all interfaces at once. Logging tag and toString belong in the Fragments itself rather than the delegate. Before it was using the logging tag "TiFragmentDelegate" instead of "SampleFragment" which doesn't help a lot when debugging provideView was moved into the Fragments because this works differently between Compositeandroid and the Ti implementations
| import android.app.Activity; | ||
| import android.support.v4.app.Fragment; | ||
|
|
||
| public interface DelegatedTiFragment { |
There was a problem hiding this comment.
Can't we extend from DelegatedTiActivity? 🤔
There was a problem hiding this comment.
I'd say no, because of the P getRetainedPresenter(); method.
There was a problem hiding this comment.
Sure we could. Or we could rename all activity related callbacks in the DelegatedTiFragment something like: isHostingActivity...
There was a problem hiding this comment.
some methods overlap, some don't. I don't see a reason for inheritance
There was a problem hiding this comment.
some methods overlap
Currently only the P getRetainedPresenter(); don't overlap. The question for me is: Why we haven't it inside the fragment?
I don't see a reason for inheritance
Because a Fragment can only be attached to an Activity. That means a Fragment have always a Activity as "Parent" anywhere...
That's for me a reason enough to use inheritance...
There was a problem hiding this comment.
Fragment and Activity don't share the same interface although many lifecycle methods are equal. Both work differently. Also methods like isActivityChangingConfigurations() are currently not used by the Fragment but for logging and may be removed soon.
P getRetainedPresenter(); is not required for Fragments because the Fragment Object doesn't die and gets retained together with the delegate automatically.
I'm going with the renaming suggested by @rberghegger
| } | ||
| } | ||
|
|
||
| public void onStop() { |
There was a problem hiding this comment.
In our TiActivityDelegate we have called these method with the suffix _afterSuper.
It would be great if you either
- Add this prefix here too.
- Remove the prefix from the
TiActivityDelegate
It is more consistent...
There was a problem hiding this comment.
I've added the suffix
| @@ -316,6 +316,7 @@ public class HelloWorldActivity extends CompositeActivity implements HelloWorldV | |||
| } | |||
| ``` | |||
|
|
|||
|
|
|||
| } | ||
|
|
||
| public void onDestroy() { | ||
| //FIXME handle attach/detach state |
There was a problem hiding this comment.
That's an old comment
There was a problem hiding this comment.
working on that, different PR
| super.onStop(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean postToMessageQueue(final Runnable runnable) { | ||
| return getActivity().getWindow().getDecorView().post(runnable); |
There was a problem hiding this comment.
This crashes if no activity isn't attached. Not sure if this can happen, but it's safer to use a Handler.
There was a problem hiding this comment.
true. It works for us because it's only called in onStart(). But sadly this is public API and should always work. I suggest creating a separate PR for this
|
@vRallev thank you for you contribution! |
Add a fragment delegate and remove duplicated code. This allows reusing the fragment code without extending
TiFragmentorTiDialogFragment.