icerockdev / moko-mvvm

Model-View-ViewModel architecture components for mobile (android & ios) Kotlin Multiplatform development
https://moko.icerock.dev/
Apache License 2.0
1k stars 95 forks source link

How to unsubscribe from live data in RecyclerView adapters? #30

Closed mpivchev closed 4 years ago

mpivchev commented 4 years ago

I am trying to update a progress bar on the first item of a RecyclerView using live data, however it doesn't seem to unsubscribe properly so other items also get included into it. Any way to make it unsubscribe properly?

   if (position == 0) {
            ExoPlayerManager.currentPlaytime.addObserver {
                b.progress.max = ExoPlayerManager.exoPlayer.duration.toInt()
                b.progress.progress = it.toInt()
            }
        } else {
            ExoPlayerManager.currentPlaytime.removeObserver { }
            b.icon.setImageDrawable(ContextCompat.getDrawable(b.root.context, R.drawable.ic_music_note))
            b.root.alpha = 0.5f
            b.progress.progress = 0
            b.progress.max = 0
        }

The live data resides here:

object ExoPlayerManager {
    lateinit var exoPlayer: SimpleExoPlayer
    private lateinit var context: Context
    private val handler = Handler()
    val currentPlaytime: MutableLiveData<Long> = MutableLiveData(initialValue = 0)
    //....

The first item updates properly:

image

... but so does the last item:

image

Alex009 commented 4 years ago

hi! you are using lambda incorrectly:

ExoPlayerManager.currentPlaytime.addObserver {
    ...
}

is equal:

val firstLambda = {
    ...
}
ExoPlayerManager.currentPlaytime.addObserver(firstLambda)

and:

ExoPlayerManager.currentPlaytime.removeObserver { }

equal to:

val secondLambda = { }
ExoPlayerManager.currentPlaytime.removeObserver(secondLambda)

i.e. you remove another observer, not those what was added before. so to fix code you should save lamda in class property and add/remove it in correct time:

val progressObserver = {
                b.progress.max = ExoPlayerManager.exoPlayer.duration.toInt()
                b.progress.progress = it.toInt()
            }

...

if (position == 0) {
            ExoPlayerManager.currentPlaytime.addObserver(progressObserver)
        } else {
            ExoPlayerManager.currentPlaytime.removeObserver(progressObserver)
            b.icon.setImageDrawable(ContextCompat.getDrawable(b.root.context, R.drawable.ic_music_note))
            b.root.alpha = 0.5f
            b.progress.progress = 0
            b.progress.max = 0
        }
mpivchev commented 4 years ago

Hi Alex, sadly with those changes the problem still occurs.

This is how it looks like now:

 val progressObserver : (progress: Long) -> Unit = {
            b.progress.max = ExoPlayerManager.exoPlayer.duration.toInt()
            b.progress.progress = it.toInt()
        }

        if (position == 0) {
            ExoPlayerManager.currentPlaytime.addObserver (progressObserver)
        } else {
            ExoPlayerManager.currentPlaytime.removeObserver(progressObserver)
            b.icon.setImageDrawable(ContextCompat.getDrawable(b.root.context, R.drawable.ic_music_note))
            b.root.alpha = 0.5f
            b.progress.progress = 0
            b.progress.max = 0
        }
Alex009 commented 4 years ago

you store progressObserver as ViewHolder property or it just local variable? it should be class property. if it local variable you got just same code what was on start.

class MyViewHolder {
    val progressObserver : (progress: Long) -> Unit = {
            b.progress.max = ExoPlayerManager.exoPlayer.duration.toInt()
            b.progress.progress = it.toInt()
        }

    fun bind(position: Int) {
        if (position == 0) {
            ExoPlayerManager.currentPlaytime.addObserver (progressObserver)
        } else {
            ExoPlayerManager.currentPlaytime.removeObserver(progressObserver)
            b.icon.setImageDrawable(ContextCompat.getDrawable(b.root.context, R.drawable.ic_music_note))
            b.root.alpha = 0.5f
            b.progress.progress = 0
            b.progress.max = 0
        }
    }
}

you should use in addObserver and removeObserver one object