15

I'm working with MVVM, and I have made different implementations of it, but one thing that is still making me doubt is how do I get data from a Repository (Firebase) from my ViewModel without attaching any lifecycle to the ViewModel.

I have implemented observeForever() from the ViewModel, but I don't think that is a good idea because I think I should communicate from my repository to my ViewModel either with a callback or a Transformation.

I leave here an example where I fetch a device from Firebase and update my UI, if we can see here, I'm observing the data coming from the repo from the UI, but from the ViewModel I'm also observing data from the repo, and here is where I really doubt if I'm using the right approach, since I don't know if observeForever() will be cleared on onCleared() if my view is destroyed, so it won't keep the observer alive if the view dies.

UI

override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        button.setOnClickListener {
            val deviceId = editText.text.toString().trim()
            observeData(deviceId)
        }
    }

    fun observeData(deviceId:String){
        viewModel.fetchDeviceData(deviceId).observe(this, Observer {
            textView.text = "Tipo: ${it.devType}"
        })

ViewModel

class MainViewmodel: ViewModel() {

    private val repo = Repo()
    fun fetchDeviceData(deviceId:String):LiveData<Device>{
        val mutableData = MutableLiveData<Device>()
        repo.getDeviceData(deviceId).observeForever {
            mutableData.value = it
        }

        return mutableData
    }
}

Repository

class Repo {

    private val db = FirebaseDatabase.getInstance().reference
    fun getDeviceData(deviceId:String):LiveData<Device>{
        val mutableData = MutableLiveData<Device>()
        db.child(deviceId).child("config/device").addListenerForSingleValueEvent(object: ValueEventListener{

            override fun onDataChange(dataSnapshot: DataSnapshot) {
                    val device = dataSnapshot.getValue(Device::class.java)
                    mutableData.value = device
            }

            override fun onCancelled(dataError: DatabaseError) {
                Log.e("Error","handle error callback")
            }
        })

        return mutableData
    }
}

This example just shows how to fetch the device from Firebase, it works, but from my ViewModel, it keeps making me think that observeForever() is not what I'm looking for to communicate data between the repository to the ViewModel.

I have seen Transformations, but I, in this case, I just need to deliver the entire Device object to my UI, so I don't need to transform the Object I'm retrieving to another Object

What should be here the right approach to communicate the repository and the ViewModel properly?

Gastón Saillén
  • 12,319
  • 5
  • 67
  • 77
  • Why do you need Repository layer? Doesn't ViewModel + LiveData provide all of that. You can even use `viewModelScope.launch` for structured concurrency. – IgorGanapolsky Dec 01 '19 at 21:00
  • 1
    I use to have different modules to structure my app, since the repository should communicate to a data layer, I did this example more shrinked in order to not write more code, but what I was looking for was how to interact with other layers without observing forever from the viewmodel – Gastón Saillén Dec 02 '19 at 00:33
  • @IgorGanapolsky It's the idea of MVVM – Master Zzzing Dec 12 '19 at 15:41
  • @MasterZzzingKhmer_Cambodia MVVM doesn't necessitate Repository layer. – IgorGanapolsky Dec 12 '19 at 15:57
  • 4
    @IgorGanapolsky yes, it is not necessary but it's there to keep the code clean. it's a single responsibility principle. – Master Zzzing Dec 12 '19 at 16:02

3 Answers3

21

is observeForever lifecycle aware?

No, that's why it's called observeForever.

I have implemented observeForever() from the ViewModel, but I don't think that is a good idea

No, it's not, you should be using Transformations.switchMap {.

since I don't know if observeForever() will be cleared on onCleared() if my view is destroyed, so it won't keep the observer alive if the view dies.

Well if you're not clearing it in onCleared() using removeObserver(observer), then it won't clear itself, because it observes forever.

here is where I really doubt if I'm using the right approach,

No, you can do much better than this following a reactive approach.

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    setContentView(R.layout.activity_main)

    button.setOnClickListener {
        val deviceId = editText.text.toString().trim()
        viewModel.onSelectedDeviceChanged(deviceId)
    }

    viewModel.selectedDevice.observe(this, Observer { device ->
        textView.text = "Tipo: ${device.devType}"
    })
}

And

class MainViewModel(
    private val savedStateHandle: SavedStateHandle,
): ViewModel() {
    private val repo = Repo() // TODO: move to Constructor Argument with ViewModelProvider.Factory

    private val selectedDeviceId: MutableLiveData<String> = savedStateHandle.getLiveData<String>("selectedDeviceId")

    fun onSelectedDeviceChanged(deviceId: String) {
        selectedDeviceId.value = deviceId
    }

    val selectedDevice = Transformations.switchMap(selectedDeviceId) { deviceId ->
        repo.getDeviceData(deviceId)
    }
}

And

class Repo {
    private val db = FirebaseDatabase.getInstance().reference // TODO: move to constructor arg? Probably

    fun getDeviceData(deviceId:String) : LiveData<Device> {
        return object: MutableLiveData<Device>() {
            private val mutableLiveData = this

            private var query: Query? = null
            private val listener: ValueEventListener = object: ValueEventListener {
                override fun onDataChange(dataSnapshot: DataSnapshot) {
                    val device = dataSnapshot.getValue(Device::class.java)
                    mutableLiveData.value = device
                }

                override fun onCancelled(dataError: DatabaseError) {
                    Log.e("Error","handle error callback")
                }
            }

            override fun onActive() {
                query?.removeEventListener(listener)
                val query = db.child(deviceId).child("config/device")
                this.query = query
                query.addValueEventListener(listener)
            }
    
            override fun onInactive() {
                query?.removeEventListener(listener)
                query = null
            }
        }
    }
}

This way, you can observe for changes made in Firebase (and therefore be notified of future changes made to your values) using LiveData, rather than only execute a single fetch and then not be aware of changes made elsewhere to the same data.

EpicPandaForce
  • 79,669
  • 27
  • 256
  • 428
  • 2
    I have used the debugger to understand better how this works, basically when we push data inside the transformation with the selectedDeviceId and if there is at leats one observer to selectedDevice , it will trigger the transformation, and since the transformation returns a new MutableLiveData object as a Device , everytime we trigger a new operation with a new deviceId , it will pass to onInactive, detach the current observer and start the new one with the new value without keeping the reference of the first observer – Gastón Saillén Nov 29 '19 at 22:13
  • I have a question, If there is an observer for the plug and unplug of the observer of the source without an external callback (LifeCycle.State), then what detaches the observer of the observer... of the observer (I believe there is a fourth observer because there are 2 per callback)? – Delark Oct 21 '20 at 02:07
  • What if I want to proceed the data in the VM? If I have a couple of actions and not a single data involved in the process? – Psijic Nov 08 '20 at 17:02
  • Then use `switchMap` – EpicPandaForce Nov 09 '20 at 00:40
4

To use ObserveForever, you need to remove the observer inside onClear in the ViewModel.

In this case, I would suggest to use Transformation even though you just need a direct mapping without any processing of the data, which is actually the same as what you are doing with the observer for observerForever.

Yang Liu
  • 1,300
  • 10
  • 14
  • About the observeForever() lifecycle, is it attached to the view lifecycle or it has its own inside the viewmodel? do you have any example ? – Gastón Saillén Nov 29 '19 at 15:43
  • 1
    Note that when using observerForever, there is no lifecycleowner attached. observeForever(Observer) is considered as always active and thus will be always notified about modifications. For those observers, you should manually call removeObserver. More to read here https://developer.android.com/reference/android/arch/lifecycle/LiveData – Yang Liu Nov 29 '19 at 15:56
  • One more question, is this seen as an antipattern ? or it is ok to use it this way ? – Gastón Saillén Nov 29 '19 at 16:53
  • You’re welcome. I won’t say this is an antipattern. In this case, it happened to be straightforward passing of data (even though this can be quite common in a big project). In fact, using observerForever can be more error prone since you may forget to remove the observer. If you want to use LiveData between repo and viewmodel, Transformations can be a nice tool to use. – Yang Liu Nov 29 '19 at 17:00
3

observeForever() is not Lifecycle aware and will continue to run until removeObserver() is called. In your ViewModel do this instead,

class MainViewmodel: ViewModel() {

    private val repo = Repo()
    private var deviceData : LiveData<Device>? = null
    fun fetchDeviceData(deviceId:String):LiveData<Device>{
        deviceData = repo.getDeviceData(deviceId)
        return deviceData!!
    }
}
Networks
  • 2,144
  • 1
  • 10
  • 17
  • If I know correctly, the ViewModel is supposed to keep the cached value, rather than directly return a new instance of the LiveData. Otherwise, ViewModel does not cache data across configuration change. – EpicPandaForce Nov 29 '19 at 18:23
  • This solution doesn't allow to proceed the data in VM. – Psijic Nov 08 '20 at 16:32