etiennelenhart / Eiffel

Redux-inspired Android architecture library leveraging Architecture Components and Kotlin Coroutines
MIT License
211 stars 14 forks source link

State not updating with new but similar ViewEvent #90

Closed jordond closed 5 years ago

jordond commented 5 years ago

I've run into an issue, and was curious about your thoughts on it.

What's happening is I have a button that dispatches an action, which updates the state with a ViewEvent for navigating to the next screen. Now if the user hits the back button, and tries to click the button again. The state won't update because it is blocked by the equality test. Because theViewEvents are technically the same (even though the previous one was handled).

My use-case is for creating Navigation commands, but I have written a simple test that showcases a more basic example:

data class TestViewEvent(val value: Int) : ViewEvent()

data class ViewEventState(val viewEvent: TestViewEvent? = null) : State

data class DoTestEvent(val data: Int) : Action

val viewEventUpdate = update<ViewEventState, DoTestEvent> { copy(viewEvent = TestViewEvent(it.data)) }

@Test
fun `GIVEN EiffelViewModel subclass WHEN handled 'ViewEvent' dispatched multiple times THEN state is updated`() {
    @UseExperimental(ExperimentalCoroutinesApi::class)
    val viewModel =
        object : EiffelViewModel<ViewEventState, DoTestEvent>(ViewEventState(), Dispatchers.Unconfined) {
            override val update = viewEventUpdate
            override val interceptionDispatcher = Dispatchers.Unconfined
        }

    var actual = 0
    viewModel.state.observeForever { state ->
        state.viewEvent?.peek {
            actual++
            true
        }
    }

    // Succeeds
    viewModel.dispatch(DoTestEvent(42))
    viewModel.dispatch(DoTestEvent(24))
    assertEquals(2, actual) // actual: 2

    actual = 0

    // Fails
    viewModel.dispatch(DoTestEvent(42))
    viewModel.dispatch(DoTestEvent(42))
    assertEquals(2, actual) // actual: 1
}
etiennelenhart commented 5 years ago

Good catch! I actually haven't tested ViewEvent together with EiffelViewModel yet.

I'm currently thinking about making it possible to return null in Update to signal that the State hasn't been modified but this may result in the same state being emitted multiple times if an update is signaled but copy didn't actually change any values. (May happen occasionally, especially with LiveData sources.)

As another solution we maybe could override equals to include the handled property of ViewEvent but I'm not sure if that works.

jordond commented 5 years ago

I tried overriding the equals but it was still failing for me. But I don't know enough about kotlin/java's equality checks to be confident that it doesn't work.

I'll continue brainstorming.

jordond commented 5 years ago

Ah, so If you make the TestViewEvent a regular class instead of a data class, then the equals from ViewEvent will work.

Figured out the solution thanks to the kotlin docs on data classes.

I'll submit a PR soon.