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

Add LightCycles.bind() to the built-in dispatchers section readme #80

Closed

Conversation

NikoYuwono
Copy link
Contributor

Fix #36

@glung So we agree to use solution number one (for anyone else, please see the discussion in the issue)

  • Keep the auto-binding for Activity and Fragment
  • Add rule for when the LightCycles.bind() need to be called in Dispatchers and add them to Readme (Possibly in Built-in dispatcher section)

So I added the LightCycles.bind() into Built-in Dispatcher section, to make it clearer for the user when to call LightCycles.bind()

Let me know if you have any feedback or different idea! :)

@glung
Copy link
Contributor

glung commented Dec 7, 2016

Add rule for when the LightCycles.bind() need to be called and add them to Readme

I think I misread it. adding the LightCycles.bind call by defaul in built in disptchers ?

That's what I meant about perf when I wrote the following :

I think option 1 is better for the users for the lib and if we have perf issues we will address them later. I am confident.

@NikoYuwono
Copy link
Contributor Author

I think I misread it. adding the LightCycles.bind call by defaul in built in disptchers ?

Sorry if my explanation is ambigous. From what I understand is we need to call LightCycles.bind() if we extends built-in dispatchers (ActivityLightCycleDispatcher, FragmentLightCycleDispatcher, SupportFragmentLightCycleDispatcher) like what we have in current example.

So based on our previous discussion, we agreed that we don't need to implement auto-binding because we think there won't be any significant performance difference with it. So we won't change the current implementation of Dispatchers.

But I also worried about the problem of the user not knowing when to call LightCycles.bind() inside Dispatcher because currently there are no standard when to call it inside Dispatcher.

So I think with this Readme update, I think we can made it clear to user to ask them to call LightCycles.bind() inside onCreate() method

Please let me know if I misunderstand something!

@glung
Copy link
Contributor

glung commented Dec 9, 2016

Let me put it this way, if we add LightCycles.bind() in the dispatcher base classes (like for the activities and fragment), it will bind transparently any dispatcher extending the bae one.

And because we are not expecting a significant performance difference with it, we can just push it.

Here is what I mean : 9d3cd37.

@NikoYuwono
Copy link
Contributor Author

Oh sorry for the misunderstanding! I`ll close this PR then.

@NikoYuwono NikoYuwono closed this Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants