search

oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

pairWithDelay causing UI rendering bug #34

Closed huan-nguyen closed 6 years ago

huan-nguyen commented 6 years ago

Firstly thank you for this nice sample code.

When playing with the app I found a bug shown in this video: https://youtu.be/RG_m3_jO7SQ

It happens when you check and uncheck the 'complete' box of a task within 2 seconds. Basically what happens is when you check the box, the 'task completed' Snackbar is shown. Then within 2 seconds, you uncheck the box, the 'task completed' is shown again (which does not reflect the current state of the task - activated) and the 'task activated' snackbar is only shown after the second 'task completed' Snackbar is auto-dismissed.

This bug is caused by the way Snackbar show events are currently managed using pairWithDelay which emits a new view state after 2 seconds that clears the property (e.g., taskComplete or taskActivated) which invokes Snackbar.

What happens is:

  1. Current time - check -> taskComplete = true, taskActivated = false -> showMessage for 'task completed' called
  2. Assume 1 second later - uncheck -> taskComplete = true, taskActivated = true -> showMessage for 'task activated' called immediately followed by showMessage for 'task completed'. Thats why you'll see only 'task complete' snackbar
  3. 2 seconds later -> taskComplete = false, taskActivated = true -> showMessage for 'task activated' called
  4. 3 seconds later -> taskComplete = false, taskActivated = true -> Does nothing since the Snackbar's dismissal is handled by Snackbar itself due to Snackbar.LENGTH_LONG is used.

I believe the root cause of this issue is the fact that pairWithDelay with 2 seconds is used. I understand from the conversation in issue #6 that it was an attempt to adopt Hannes's suggestion on SingleLiveEvent alternative. IMHO the key idea from his blog post is to have the presenter/viewmodel responsible for both showing and dismissing Snackbars (by constructing view states). To do that, he set Snackbar time to Snackbar.LENGTH_INDEFINITE.

However, in this app, Snackbar showing time is set to Snackbar.LENGTH_LONG which means the dismissal of Snackbar is handled by the Snackbar itself. This raises a question as to why you need the timer of 2 seconds to update the view state and then that updated state is not used for view rendering (false values of properties like taskComplete or taskActivated are not used in render method). Also Snackbar.LENGTH_LONG is normally not 2 seconds.

IMHO there are 2 approaches to fix this issue:

  1. Remove pairWithDelay. Instead immediately emits a new state with false property.
  2. Follow exactly Hannes' suggestion by having the ViewModel to handle Snackbar's dismissal.

Would be interested hearing your thoughts.

oldergod commented 6 years ago

Thank you for the feedback. I'm really busy now so give me a few weeks to get back to you!

oldergod commented 6 years ago

Remove pairWithDelay. Instead immediately emits a new state with false property.

The problem with that is we would want to show a snackbar on rotation; this above solution doesn't allow that.

I think the problem was that we had different fields rendering the same widget (the snackbar), I changed the code so now they all update the same new field uiNotification. On the delayed event, they would turn that to null only if the existing value match their type. Another idea I had was to have all those snackbar rendered events go through the same stream with a switchMap that would then cancel the to-be-delayed event. But didn't seem good.

The PR is a bit dirty with the formatting but the fix is in there.