syncthing-android icon indicating copy to clipboard operation
syncthing-android copied to clipboard

Published 20 hours ago • verified publisher icon syncthing

Refactor to use Dagger2 Hilt

Open adamszewe opened this issue 1 year ago • 2 comments

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

adamszewe avatar Feb 04 '24 21:02 adamszewe

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 avatar Feb 06 '24 11:02 imsodin

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:

  • keep this PR as draft, in case any one else wanted to attempt the same, to keep the conversation open
  • simplify the current DI in the app in future PRs (there are a couple of classes that don't need injection at all)
  • try again in few weeks when this change would be easier to do and would result in a much smaller PR that is easier to review

adamszewe avatar Feb 06 '24 18:02 adamszewe