Skip to content

Create a Sequencing snackbar feature#10820

Closed
develric wants to merge 3 commits intodevelopfrom
issue/10771-failed-upload-snackbar-shown-after-succesful-upload
Closed

Create a Sequencing snackbar feature#10820
develric wants to merge 3 commits intodevelopfrom
issue/10771-failed-upload-snackbar-shown-after-succesful-upload

Conversation

@develric
Copy link
Copy Markdown
Contributor

Partially Fixes #10771

This is a draft, uploaded to make easier to elaborate/evaluate on the approach to solve the issue. Some notes/doubts:

  • coroutine improvements in SnackbarSequencer: for example job cancellation?
  • modifications/improvements on estimateNextAvailableTimeSlot algorithm
  • Snackbar.LENGTH_INDEFINITE: for now those snackbar not added to the sequencer; decide if trigger an exception in case they are accidentally added or silently overwrite with some default
  • ...

To test:

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 19, 2019

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 19, 2019

You can test the changes on this Pull Request by downloading the APK here.

private const val LONG_DURATION_MS = 2750

@Singleton
class SnackbarSequencer @Inject constructor() : Snackbar.Callback(), CoroutineScope {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this issue @develric! ;) I realize it's still just a draft PR so I haven't reviewed the code line by line, I just looked at the overall approach.
My first thought when we discussed this issue was having a queue of messages waiting to be shown (with a limited capacity) and adding onDismiss callback in which we'd check if the queue is empty or if we should show a SnackBar. I'm not sure if it would work or which approach is better, I just wanted to share my thoughts so we can both think about it. Wdyt?
One thing I realized while reviewing this code is that we need to be extra careful not to leak any activities/views. WPSnackbar holds a reference to the view == activity. If we decide to go ahead with either of the approaches, we should make sure we keep just a WeakReference to the context (making sure the optional action callbacks don't leak the context might be even more tricky). Wdyt?

Copy link
Copy Markdown
Contributor Author

@develric develric Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @malinajirka and thanks for keeping an eye on this (much appreciated 🙇‍♂️). I try to give some more detail below hoping it helps on further discussion/elaboration. Let me know wdyt 😊.

My first thought when we discussed this issue was having a queue of messages...

This was the first approach I tried actually. What I got tough is that at least on the emulator with my mac a bit loaded I suspect android missed some callbacks event and I ended up having messages in queue that would stay there until some (if any) next snackbar. So in case we decide to go with this approach I'm not sure it's enough to rely on the callback as unique mechanism to poll from the queue 🤷‍♂️.
The intention of what I tried to implement was to introduce some scheduled delay in between the snackbars show and to saturate it just to not make the snackbar show up too far from the timestamp when it was generated (basic attempt in the estimateNextAvailableTimeSlot that for sure we can improve on). In this way we should not be worried on callbacks to be skipped even if of course it can be less precise on snackbar duration and can basically fall in something similar to the snackbar overlapping we are having now in cases where we have many snachbars (due to delay saturation in estimateNextAvailableTimeSlot). Maybe one possibility in those cases can be to "emulate" the queue with loss you were suggesting skipping the snackbar show (but I wonder if it can make more sense to keep the snackbar with a small timing sequencing even if not lasting for all the desired snackbar duration?).

we need to be extra careful not to leak any activities/views.

Absolute good point! Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! ❤️

This was the first approach I tried actually. What I got tough is that at least on the emulator with my mac a bit loaded I suspect android missed some callbacks event and I ended up having messages in queue that would stay there until some (if any) next snackbar. So in case we decide to go with this approach I'm not sure it's enough to rely on the callback as unique mechanism to poll from the queue 🤷‍♂️.

ohh, I wouldn't expect it to miss the callbacks 🤔. Perhaps it misses them when the activity isn't resumed or something like that 😞 🤷‍♂ . In that case your approach seems reasonable:).

You mentioned you have concerns about changing the behavior in the entire app. I can't say I'm feeling differently. One thought which just crossed my mind would be handle the specific scenario we are having troubles with manually - show a snackbar when an upload fails and somehow replace it with a snackbar saying something like "1 Upload succeeded and 1 upload failed`. We already do a similar thing in Notifications, they have unique ids though 🤷‍♂. => I'm not saying we should do it, again just something we might want to think about.

@develric develric changed the title Issue/10771 failed upload snackbar shown after successful upload Create a Sequencing snackbar feature Nov 20, 2019
@develric
Copy link
Copy Markdown
Contributor Author

Closing in favor of #10856

@develric develric closed this Nov 26, 2019
@jkmassel jkmassel deleted the issue/10771-failed-upload-snackbar-shown-after-succesful-upload branch October 17, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed upload Snackbar shown after successful upload

2 participants