search

syncthing / syncthing-android

Wrapper of syncthing for Android.
https://syncthing.net/
Mozilla Public License 2.0
3.16k stars 362 forks source link

Refactor to use Dagger2 Hilt #2053

Open adamszewe opened 5 months ago

adamszewe commented 5 months ago

WIP Refactoring the app to use Dagger2 Hilt 🚀 https://dagger.dev/hilt/

imsodin commented 5 months ago

Please don't invest any more time in this for the moment!

This is a big change, so lots of things to go wrong, and I don't have much time and inclination to review it. What's the motivation for this change? It looks like it replaces manual-ish dependency injection with something more annotation based.

Edit: Dang I forgot to state an important bit: I recognise you already contributed quite a few things - thanks a lot for that! That's basically why I am asking and even considering this.

adamszewe commented 5 months ago

Please don't invest any more time in this for the moment!

This is a big change, so lots of things to go wrong, and I don't have much time and inclination to review it. What's the motivation for this change? It looks like it replaces manual-ish dependency injection with something more annotation based.

Edit: Dang I forgot to state an important bit: I recognise you already contributed quite a few things - thanks a lot for that! That's basically why I am asking and even considering this.

👋 @imsodin Yes, the main purpose of this change would be to:

  1. get rid of the dagger component and old way of injecting classes, which requires the injected class to request injection explicitly
  2. make it easier to provide and inject dependencies in general

It's a big change, maybe too big for now. Thanks for the early feedback 😃 So, the good news is that the refactor, as it is now, works as expected. I tested manually all the screens I found and didn't find any issues. This is because there are not many classes being injected and there is only a single dagger component in the app. But I agree it would be a bad idea to merge it as it is, as I may not be aware of some edge cases and would avoid breaking the app for the users.

I'd do the following then: