Bridouille / android-beacon-scanner

A simple android iBeacon, AltBeacon, Eddystone and RuuviTag beacon scanner
https://play.google.com/store/apps/details?id=com.bridou_n.beaconscanner
330 stars 108 forks source link

A few more suggestions #1

Closed doubleo2 closed 7 years ago

doubleo2 commented 7 years ago

Yesterday, when I suggested creating a PerActivity component and continuing to scan when the device rotates, I didn't think about how those two changes would affect each other. Originally, I thought BeaconManager could live in the AppComponent and the activity would just resubscribe to the event bus. However, I didn't realize BeaconConsumer has to be a Context. Your solution is much simpler.

Again, the app is still simple enough (and nothing keeps a reference to an activity) so all the dagger modules could easily be in the AppComponent, but your PerActivity component is exactly how I would have created it.

Let me know if you have any questions about my changes

Bridouille commented 7 years ago

Thanks for theses suggestions, I didn't think of the .idea folder, and the contextModule is definitely a great idea!

I actually find the idea of using a BehaviorSubject for the bluetooth state changes very smart since we get the first state initializing it. I think I'm newer than you to RxJava so I didn't even know about the BehaviorSubject class.

There is one thing that I'm not 100% sure, does Dagger knows that it's the constructor in BluetoothManager that is supposed to replace the @Provides annotation in a module ? Otherwise I don't know how it knows how to construct my BluetoothManager, I thought the @Inject annotation was only to tell Dagger to inject a dependency and not to declare a method that can provides some!

doubleo2 commented 7 years ago

Use @Inject to annotate the constructor that Dagger should use to create instances of a class (source)

It's a little magical, but Dagger is smart enough to know that if you @Inject a constructor that it can use the constructor to create instances of that class