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
A: 3 - Created the App, BaseActivity, BaseFragment, and custom PerActivity, PerFragment, and PerChildFragment Scopes. Closes #3 #20
Conversation
…PerFragment, and PerChildFragment Scopes. Closes #3
|
||
/** | ||
* The Android {@link Application}. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation:
* <b>DEPENDENCY INJECTION</b>
* We could extend {@link dagger.android.DaggerApplication} so we can get the boilerplate
* dagger code for free. However, we want to avoid inheritance (if possible and it is in this case)
* so that we have to option to inherit from something else later on if needed
* (e.g. if we need to override MultidexApplication).
protected FragmentManager childFragmentManager; | ||
|
||
@Inject | ||
DispatchingAndroidInjector<Fragment> fragmentInjector; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be renamed to childFragmentInjector
.
return fragmentInjector; | ||
} | ||
|
||
protected final void addFragment(int containerViewId, Fragment fragment) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can annotate it with (@idres id containerViewId,...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Thanks.
Fixed: 88f9f5b
I also added a quick tip about the support annotations in the article.
|
||
@Binds | ||
@PerActivity | ||
abstract Context activityContext(Activity activity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scoping the Context activityContext(Activity activity)
with @PerActivity
is not necessary since the Activity
instance will always be unique (new instances of it will not be created even without any scope). In general, providing Application
, Activity
, Fragment
, Service
, etc does not require scoped annotations since they are the components being injected and their instance is unique.
The same thing applies to static FragmentManager activityFragmentManager(Activity activity)
. The Activity
instance is unique so the FragmentManager
it returns always come from the same Activity. Thus, the @PerActivity
is not necessary here as the scope is implicitly per activity (literally).
However, using scope annotations in these cases makes the module easier to read. We wouldn’t have to look at what is being provided in order to understand its scope. I choose readability here over (negligible) “performance/optimization”.
|
||
@Inject | ||
@Named(BaseActivityModule.ACTIVITY_FRAGMENT_MANAGER) | ||
protected FragmentManager fragmentManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Why is FragmentManager injected into BaseActivity? Why not just use getFragmentManager() method?" See #52
public abstract class BaseFragment extends Fragment implements HasFragmentInjector { | ||
|
||
@Inject | ||
protected Context activityContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Why is activity Context injected into BaseFragment? Why not just use getContext() method?" See #52
// Note that this should not be used within a child fragment. | ||
@Inject | ||
@Named(BaseFragmentModule.CHILD_FRAGMENT_MANAGER) | ||
protected FragmentManager childFragmentManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Why is child FragmentManager injected into BaseFragment? Why not just use getChildFragmentManager() method?" See #52
@Named(CHILD_FRAGMENT_MANAGER) | ||
@PerFragment | ||
static FragmentManager childFragmentManager(@Named(FRAGMENT) Fragment fragment) { | ||
return fragment.getChildFragmentManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fragment.getChildFragmentManager()
is available since API 17. This is one of the reasons why the non-support setup in the master branch uses minSdkVersion
of 17 and not lower.
For support for API levels below 17 down to 14, take a look at the master-support branch.
public abstract class BaseActivity extends Activity implements HasFragmentInjector { | ||
|
||
@Inject | ||
@Named(BaseActivityModule.ACTIVITY_FRAGMENT_MANAGER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use @Named
vs @Qualifier
?
Short answer is preference and lack of better naming ideas.
Instead of using @Named
, we can also use a custom Qualifier
in order to distinguish between the FragmentManager
type from the activity versus the fragment.
E.G.
@Qualifier
@Retention(RetentionPolicy.RUNTIME)
public @interface ActivityFragmentManager {
}
@Qualifier
@Retention(RetentionPolicy.RUNTIME)
public @interface FragmentFragmentManager {
}
@Module
public abstract class BaseActivityModule {
@Provides
@ActivityFragmentManager
@PerActivity
static FragmentManager activityFragmentManager(Activity activity) {
return activity.getFragmentManager();
}
}
@Module
public abstract class BaseFragmentModule {
@Provides
@FragmentFragmentManager
@PerFragment
static FragmentManager childFragmentManager(@Named(FRAGMENT) Fragment fragment) {
return fragment.getChildFragmentManager();
}
}
public abstract class BaseActivity extends Activity implements HasFragmentInjector {
@Inject
@ActivityFragmentManager
protected FragmentManager fragmentManager;
...
}
public abstract class BaseFragment extends Fragment implements HasFragmentInjector {
@Inject
@FragmentFragmentManager
protected FragmentManager fragmentManager;
...
}
I'm not a fan of the name FragmentFragmentManager
. It looks weird (though doesn't sound as weird). I can't think of a more appropriate name... Besides, using @Named
is common practice. If @Qualifier
s were used every time multiple instances of the same type had to be provided in the same graph, then we would end up with large quantities of qualifiers (not that its bad or anything). It certainly has its advantages over using @Named
, which on a side note is just another custom qualifier;
@Qualifier
@Documented
@Retention(RUNTIME)
public @interface Named {
String value() default "";
}
Why use @Qualifier
over @Named
?
There are 2 concrete advantages of using @Qualifier
over @Named
, though be it a bit "elitist".
- Solid
@interface
references instead of possibly brittle Strings. @Named
Strings may "leak" a reference to the module / component via an import.
We could easily mitigate these @Named
issues though;
-
Using
@Named
requires the use of Strings, which can be misspelled. So a module may provide a "peach" but a variable may be injected with a "peech". This will never happen when using@Qualifier
. However, a simple way to fix this flaw is to simply define the String (i.e. "peach") as a static final field and use that instead of typing the string over and over. -
We mostly come to the immediate conclusion that we should place the static final String in the
@Module
that uses/provides it. At a glance, there seems to be no issues with this approach. However, if we really think hard and adopt a super strict policy, we'll notice that this will lead to a violation of a core principle of dependency injection.That principle is that a class should not know anything about how it is injected or anything related to the injection process. Take the setup shown in this repository,
import com.vestrel00.daggerbutterknifemvp.ui.common.view.BaseFragmentModule; @PerFragment public final class PerFragmentUtil { @Inject PerFragmentUtil(@Named(BaseFragmentModule.FRAGMENT) Fragment fragment) { } }
Note that
PerFragmentUitl
appears in the next PR.Notice the import for
com.vestrel00.daggerbutterknifemvp.ui.common.view.BaseFragmentModule;
, which is required to access theFRAGMENT
String used for@Named
. In turn,PerFragmentUtil
now references theBaseFragmentModule
, which it shouldn't. Classes being injected should have no visibility of@Module
s and@Component
s. This is clearly stated here: https://google.github.io/dagger/android.html#daggerandroid. It is one of the main motivations of why the dagger.android extension was created.In practice, this violation is bad because not
PerFragmentUtil
can only be created / provided whenBaseFragmentModule
is a part of the injection graph. What if we want to use a different module other thanBaseFragmentModule
? We can't. Well we can as long as the String values are the same but still doesn't look right when we look at the code. We won't really encounter any issues in this particular example (BaseActivityModule
, after all, is a module that is included in all of our modules so it is guaranteed to be in the injection graph), this was just an illustration of why this practice is bad.A simple solution is to move the static final String
FRAGMENT
outside of theBaseActivityModule
into a separate class. This way, theBaseActivityModule
need not be referenced. Then any@Module
can be used in the injection graph to provide the@Named(FRAGMENT
). The caveat to this solution is that a separate class would be required. Furthermore, the static final String seems like it should belong toBaseActivityModule
.
Conclusion
This has been a long "elitist" rant. I recognize that these are really nit-picky details and won't really pose any issues during development. Therefore, we ignore these "issues" in this repo as they are far from pragmatic. We want to be pragmatic programmers, not elitists / over-engineerists!
* <b>VIEW BINDING</b> | ||
* This fragment handles view bind and unbinding. | ||
*/ | ||
public abstract class BaseFragment extends Fragment implements HasFragmentInjector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest branches of this repo have BaseFragment
subclassing DialogFragment
instead of Fragment
in order to flawlessly support DialogFragment
injection. See this PR for more details.
Created the App, BaseActivity, BaseFragment, and custom PerActivity, PerFragment, and PerChildFragment Scopes.