Fixes #4294: After deleting a profile deleted successfully message should be displayed

[Akshatkamboj14]

Explanation

Fixes #4294

Essential Checklist

• The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• Any changes to scripts/assets files have their rationale included in the PR explanation.
• The PR follows the style guide.
• The PR does not contain any unnecessary code changes from Android Studio (reference).
• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
• The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

• Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
• For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
• Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
• Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

snackbar.webm

• Screenshots of tests passing on my local
  
  

[Akshatkamboj14]

Hey @BenHenning, Can you take a first pass at this and let me know if there are corrections to be made? after that i will proceed to add tests for this. Thanks.

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[BenHenning]

Thanks @Akshatkamboj14. Sorry for the review delay here.

I think from a UX perspective that we actually want to route back and show the snackbar simultaneously (otherwise, the user could still interact with the profile after it's been deleted which could result in problems & user confusion).

I think this will require introducing some sort of general-purpose SnackbarManager class which can show snackbars irrespective of the current activity showing. E.g. something with an API like:

class SnackbarManager @Inject constructor(private val snackbarController: SnackbarController) {
  // Use SnackbarController for displaying the current snackbar, hiding it after the snackbar hides, or showing a new one.

  fun showSnackbar(@StringRes messageStringId: Int, duration: SnackbarDuration) {
    // Enqueue snackbar using SnackbarController...
  }

  fun enableShowingSnackbars(activity: AppCompatActivity) {
    // Called in an activity's onCreate to start listening for snackbar state.
    snackbarController.getCurrentSnackbar().toLiveData().observe(activity) { result ->
      when (result) {
        is AsyncResult.Success -> when (val request = result.value) {
          is SnackbarController.SnackbarRequest.ShowSnackbar -> showSnackbar(activity.view, request)
          SnackbarController.SnackbarRequest.ShowNothing -> {} // Do nothing.
        }
        // Other cases...
      }
    }
  }

  private fun showSnackbar(activityView: View?, showRequest: SnackbarController.SnackbarRequest.ShowSnackbar) {
    if (activityView == null) {
      // ...log an error if activityView is null and ignore the snackbar request (can't be shown--no activity UI).
      return
    }
    // ...compute length based on showRequest.duration and messageString based on showRequest.messageStringId -- the latter should be done using appResourceHandler (so that the string is in the correct language).
    Snackbar.make(activityView, messageString, length)
    // ...show the snackbar & add a callback for when it dismisses--at that point SnackbarController.dismissCurrentSnackbar() should be called.
  }

  // Abstract domain layer.
  enum class SnackbarDuration {
    SHORT,
    LONG
  }
}

with a controller to store the actual snackbar state:

@Singleton
class SnackbarController @Inject constructor() {
  // The class should have a queue of snackbars.

  fun getCurrentSnackbar(): DataProvider<SnackbarRequest>

  fun enqueueSnackbar(request: SnackbarRequest.ShowSnackbar)

  fun dismissCurrentSnackbar()

  sealed class SnackbarRequest {
    data class ShowSnackbar(@StringRes val messageStringId: Int, val duration: SnackbarDuration): SnackbarRequest()

    object ShowNothing: SnackbarRequest()
  }

  // Abstraction for Snackbar's length constants.
  enum class SnackbarDuration {
    SHORT,
    LONG
  }
}

[Akshatkamboj14]

Hey @BenHenning Sorry for late reply as i have exams i was unable to update on this?.. I have a question can you explain a bit more on this I think from a UX perspective that we actually want to route back and show the snackbar simultaneously (otherwise, the user could still interact with the profile after it's been deleted which could result in problems & user confusion).
I am not getting like in which order to show to show the snackbar?

[BenHenning]

Hey @BenHenning Sorry for late reply as i have exams i was unable to update on this?.. I have a question can you explain a bit more on this I think from a UX perspective that we actually want to route back and show the snackbar simultaneously (otherwise, the user could still interact with the profile after it's been deleted which could result in problems & user confusion). I am not getting like in which order to show to show the snackbar?

From the video you have in the PR description, the current behavior seems to be that app won't navigate away from the "profile edit screen" until after the snackbar's "ok" button is shown (allowing the user to still interact with parts of the UI). We should be navigating the user back to the profile list and then showing the snackbar after deletion confirmation. Does that help clarify?

[Akshatkamboj14]

Hey @BenHenning Sorry for late reply as i have exams i was unable to update on this?.. I have a question can you explain a bit more on this I think from a UX perspective that we actually want to route back and show the snackbar simultaneously (otherwise, the user could still interact with the profile after it's been deleted which could result in problems & user confusion). I am not getting like in which order to show to show the snackbar?

From the video you have in the PR description, the current behavior seems to be that app won't navigate away from the "profile edit screen" until after the snackbar's "ok" button is shown (allowing the user to still interact with parts of the UI). We should be navigating the user back to the profile list and then showing the snackbar after deletion confirmation. Does that help clarify?

Ya sure @BenHenning Thanks for the Clarification.

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[Akshatkamboj14]

Hey @BenHenning, I have pushed some sample code(not completed)
according to this gist also implemented the state as discussed in yesterday's meeting. PTAL.

[oppiabot]

Unassigning @Akshatkamboj14 since a re-review was requested. @Akshatkamboj14, please make sure you have addressed all review comments. Thanks!

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[BenHenning]

Thanks @Akshatkamboj14. Which parts do you need clarification on?

I took a pass on the manager & controller and left a few follow-up comments. I think a good next step would be to focus on getting tests working for the controller to verify that it works in all the cases expected, then do the same with the manager (since the manager depends on the controller). Once both those test suites are done and showing that the respective classes are working, you should be able to hook into the manager and manually test that it's showing correctly (otherwise the manager/controller implementations & tests will need revision).

[BenHenning]

You need to call asDeffered() here, not cast it, e.g. showFuture.asDeferred().

[Akshatkamboj14]

@BenHenning I am trying on this suggestion, but this is not working can you clarify more on this?

[BenHenning]

You'll need to import it, e.g. import kotlinx.coroutines.asDeferred.

[Akshatkamboj14]

@BenHenning Already tried, not working

[BenHenning]

This shouldn't need to be exposed since the queue can be observed via getCurrentSnackbarState.

[Akshatkamboj14]

Okay got it.

[BenHenning]

Here we need for onShow to finish before we can actually change the snackbar to be shown. We also need to observe onDismiss so that we know when to remove the snackbar from the queue. Something like:

CoroutineScope(backgroundDispatcher).launch {
  onShow.await()
  // TODO: Move to helper.
  val queueFrontWhenShown = snackbarQueue.peek()
  check(queueFrontWhenShown is CurrentSnackbarState.WaitingToShow)
  check(queueFrontWhenShown.snackbarId == snackbarId)
  snackbarQueue.removeFirst()
  snackbarQueue.pushFirst(CurrentSnackbarState.Showing(queueFrontWhenShown.request, snackbarId))
  asyncDataSubscriptionManager.notifyChange(GET_CURRENT_SNACKBAR_REQUEST_PROVIDER_ID)
  onDismiss.await()
  // TODO: Move to helper.
  val queueFrontWhenDismissed = snackbarQueue.peek()
  check(queueFrontWhenDismissed is CurrentSnackbarState.Showing)
  check(queueFrontWhenDismissed.snackbarId == snackbarId)
  snackbarQueue.removeFirst()
  asyncDataSubscriptionManager.notifyChange(GET_CURRENT_SNACKBAR_REQUEST_PROVIDER_ID)
}.invokeOnCompletion {
  // Log error if it != null.
}

[Akshatkamboj14]

@BenHenning when on this line snackbarQueue.pushFirst(CurrentSnackbarState.Showing(queueFrontWhenShown.request, snackbarId))
actually, this is not compatible as snackbarRequestQueue is of type ShowSnackbarRequest and we can't add CurrentSnackbarState, can you PTAL?

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

[oppiabot]

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!