DroidKaigi / conference-app-2024

The Official Conference App for DroidKaigi 2024
Apache License 2.0
457 stars 201 forks source link

Spamming the favorite button in session details produces an undesirable queueing of messages #760

Closed mannodermaus closed 2 months ago

mannodermaus commented 2 months ago

While working on https://github.com/DroidKaigi/conference-app-2024/issues/756, I noticed that clicking the ❤️ button on a session repeatedly will cause an odd UX behavior: If the session is not favorited, then it will be favorited and a snackbar message appears. If the button is pressed again quickly (while that snackbar message is still being shown), the "unfavorite" action will not be executed until it has disappeared. If I go nuts on the button and click it a lot, this leads to weird and glitchy UI:

https://github.com/user-attachments/assets/d75fd1ac-d075-4494-a391-a1ec20b4a337

I have several ideas for fixing this:

1) In timetableItemDetailPresenter, don't wait for the result of userMessageStateHolder.showMessage() and instead launch it in a separate coroutine. I'm not the biggest fan of this, as you could probably still queue up a lot of async coroutines this way and create overlapping snackbars 2) Extend the API of UserMessageStateHolder to allow for premature cancellation of enqueued messages. This is probably much more involved, but I could see a method like cancelMessage(String) being useful...

@takahirom What do you think?

takahirom commented 2 months ago

Thank you for your opinion. I don't think the current one is a good approach. We used to have a ViewModel like this, where each event had its own job:

class ViewModel {
  fun onXxx() {
    viewModelScope.launch {
      ...
    }
  }
}

What we currently have is this, where we have only one job for all events:

    SafeLaunchedEffect(Unit) {
        events.collect { event ->
            when (event) {
                is Bookmark -> {

I believe it would be better to have a separate job for each event:

    SafeLaunchedEffect(Unit) {
        events.collect { event ->
            launch { // LaunchEffect has its own CoroutineScope, so we can launch coroutines here.
              when (event) {
                  is Bookmark -> {

Additionally, I think we could create a utility method for this use case:

EventEffect(events) { event ->
}

If we want to do something with a Snackbar, we can launch an additional coroutine job.

@mannodermaus What do you think about this?

takahirom commented 2 months ago

I'll work on this