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 ControllerFactory. #513

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

AngusMorton
Copy link

@AngusMorton AngusMorton commented Jan 16, 2019

ControllerFactory allows Controllers to be provided dependencies through their constructors after a configuration change. This means field injection is no longer a requirement when injecting dependencies into a Controller.

Usage

Conductor.setControllerFactory(myControllerFactory)

Currently, you have a single ControllerFactory for your entire app. Having a ControllerFactory per Activity/ViewGroup would be better but I don't know enough about Conductors internals to make that change.

Closes #510.

@PaulWoitaschek
Copy link
Collaborator

I would make the Factory an interface and make the default factory final and hide the implementation.
That doesn't force the user to extend a class (composition over inheritance).

Then you could also just do a identity check in ensureRequiredConstructor() and perform an early return without the need for reflection:

if(FACTORY != DefaultControllerFactory.instance() {
  return
}

Another approach (which I prefere) would be to pass the factory to conductor upon initialzation:
Conductor.attachRouter(this, controllerContainer, savedInstanceState, factory)

Then in attachRouter you can just set the factory and don't expose the other implementations.
I don't have currently time to check if this could work.

But I really like the idea to get rid of reflection and allow constructor injection.
However this whole approach is quite limited as dagger has no support for assistend injection. That effectively means that once need a bundle in the constructor you either need to do field injection or workaround it.

@AngusMorton AngusMorton force-pushed the feature/controller-factory branch from 8783a95 to 6d415cf Compare March 5, 2019 11:32
@AngusMorton
Copy link
Author

AngusMorton commented Mar 5, 2019

Thanks for the feedback! I've made the interface change.

If attachRouter accepts a factory, then it follows that you should be able to set a different ControllerFactory per Activity/ViewGroup pair.
I think having a ControllerFactory per Activity/ViewGroup makes sense, but I'm not sure if it'll complicate things too much though.

Dagger doesn't need first-party support for assisted injection, it can be provided through something like AssistedInject or AutoFactory.

@AngusMorton AngusMorton force-pushed the feature/controller-factory branch from 6d415cf to 2fd7b55 Compare March 10, 2019 03:56
ControllerFactory allows Controllers to be provided dependencies through their constructors after a configuration change. This means field injection is no longer a requirement when injecting dependencies into a Controller.
@AngusMorton AngusMorton force-pushed the feature/controller-factory branch from 2fd7b55 to bc4bb1c Compare March 27, 2019 06:20
@@ -92,30 +92,34 @@
private boolean isPerformingExitTransition;
private boolean isContextAvailable;

static volatile ControllerFactory FACTORY = DefaultControllerFactory.INSTANCE;

Choose a reason for hiding this comment

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

  Controller shouldn't know about any factory

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

final class DefaultControllerFactory implements ControllerFactory {

Choose a reason for hiding this comment

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

I think DefaultControllerFactory should implement default logic for creating Controller, but not just be a stub in logic

Copy link
Author

Choose a reason for hiding this comment

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

We could only do that without extra reflection if the first argument for create was a Class.

@AngusMorton
Copy link
Author

@PaulWoitaschek I had a look into adding ControllerFactory as an argument to attachRouter, however, we need some way to retrieve the correct ControllerFactory for the Activity when we are restoring the Backstack. I'm not familiar enough with the internals of Conductor to know what the right approach for this would be so I'm keen to hear any thoughts you have.

Assuming we store the ControllerFactory in the LifecycleHandler, we would need some way for a Router to get it's ControllerFactory, which it would then pass that down through to Backstack.restoreInstanceState -> new RouterTransaction.

@AngusMorton
Copy link
Author

@EricKuck I'm keen to hear your thoughts on this and move this forward

@Miha-x64
Copy link

Miha-x64 commented Oct 8, 2020

One more thing: if Factory instantiates Controllers, we need a way to create transaction without instantiating controllers ourselves. RouterTransaction.with(controllerType: Class<out Controller>)

@AngusMorton
Copy link
Author

One more thing: if Factory instantiates Controllers, we need a way to create transaction without instantiating controllers ourselves. RouterTransaction.with(controllerType: Class<out Controller>)

ControllerFactory would be in your dependency graph so you can inject it and call it yourself!

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

Successfully merging this pull request may close these issues.

Controller Constructor Injection
4 participants