Implement dependency injection using hilt and inject CoroutineDisptacher

[ShridharGoel]

No description provided.

[david-allison]

What does this do to build times?

[ShridharGoel]

What does this do to build times?

I think there's no significant difference in build time, though it needs to be checked in detail.

[ShridharGoel]

Any idea how we can exclude a directory from compilerArgs, specifically -Xlint:deprecation?
Will need to exclude the build folder, since there's no point of checking for deprecation there and it leads to an error showing a deprecation in a hilt generated file.
I tried using exclude '**/AnkiDroid/build/**' but it doesn't work, what can be the correct path to use here?
Suggestions would be appreciated.

[mikehardy]

lint at least offers this:

https://developer.android.com/reference/tools/gradle-api/7.1/com/android/build/api/dsl/LintOptions

     checkGeneratedSources true

Not sure if lint can check for deprecation or not, but if lint can detect deprecation then perhaps the compiler flag could go away and we could rely on lint with this flag.

Otherwise I'm not sure how this could be done. I did a brief search for a repo for hilt and could not find one ? But it strikes me that they should have a version that does not generate deprecated APIs, or if it does it does similar to us and generates a conditional where they use the old or the new depending on runtime SDK and suppresswarnings

[ShridharGoel]

Should be fine after #12431 gets merged.

[lukstbit]

I'm for dependency injection as this would be a step towards some sort of architecture where everything is not tied to everything. However I'm not sure this PR is the way to start with this.

Unless @ShridharGoel has plans for more work on top of this in the real near future, I wouldn't approve this:

• the dependency injection in LoadPronunciationActivity is useless as the dispatchers are not used. There are no tests and writing some would require a refactoring(with the complexity of needing to abstract all http calls) of the entire activity. Even if this were to be done we still wouldn't use the dispatchers there as a ViewModel might be more appropriate(I've done such a refactoring on a local branch)
• following issue one, this will increase build time(kapt is slow and Google is(slowly) replacing it with ksp anyway).
• following issue two, the cost benefit of this is not great as we get one injection which isn't used versus: increased build times, more maintainer work to keep updated the plugins/libraries and more chances for potential issues with build and kapt errors to deal with.

Just my 2:moneybag:

[ShridharGoel]

the dependency injection in LoadPronunciationActivity is useless as the dispatchers are not used

@lukstbit What do you mean by this? Aren't they being used inside the activity?

[ShridharGoel]

The plan here is to use dependency injection at more places in the app and not only limiting it to coroutine dispatchers.

[ShridharGoel]

this will increase build time

We're already using annotationProcessor, so isn't it already factored in?

[ShridharGoel]

as we get one injection

This is only for the initial PR. Further changes would be there in different pull requests.

[david-allison]

@mikehardy auto-service was added in 1a81df5 (#4969)

    compileOnly "com.google.auto.service:auto-service-annotations:1.0.1"
    annotationProcessor "com.google.auto.service:auto-service:1.0.1"

This was for ACRA. Is this still necessary as we've partially moved away from annotation-based ACRA instantiation

EDIT: Our one usage:

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/analytics/AcraAnalyticsInteraction.kt

Lines 27 to 40 in 2f1f789

	/**
	* This ACRA Extension sends an analytics hit during crash handling while ACRA is enabled.
	* Questions answered: "Number of ACRA reports sent", "ACRA vs Analytics count differences"
	* See <a href="https://github.com/ACRA/acra/wiki/Custom-Extensions">Custom Extensions</a>
	*/
	@AutoService(ReportInteraction::class)
	class AcraAnalyticsInteraction : ReportInteraction {
	override fun performInteraction(context: Context, config: CoreConfiguration, reportFile: File): Boolean {
	// Send an analytics exception hit with a UUID to match
	Timber.e("ACRA handling crash, sending analytics exception report")
	UsageAnalytics.sendAnalyticsEvent("ACRA Crash Handler", "UUID " + Installation.id(context))
	return true
	}
	}

EDIT: https://github.com/ACRA/acra/wiki/Custom-Extensions#registering-an-extension - looks like this can be manual. We may want to extract this to another issue if it's a good target for reducing compile time

[mikehardy]

Hmm annotations fir Acra are on the way out - the PR for Acra has the final structure more or less and the work that already went in killed annotations I think. Could be a remove it and see if it works test

[david-allison]

Edited the comment above (sorry), still used in one case

[lukstbit]

What do you mean by this? Aren't they being used inside the activity?

They are just that ideally you'd use this injection in other places as well, most notable being tests for that class(which we don't have).

The plan here is to use dependency injection at more places in the app and not only limiting it to coroutine dispatchers.

This is what I'm concerned about, that we are introducing dependencies/plugins which are not going to really be used in the near future seeing that the bulk work is done in scoped storage, some cleanups and moving to the new backend(at least from the changes I see).

[github-actions]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically