Conversation
…app. Leaving show method only where duration was Snackbar.LENGTH_INDEFINITE
Generated by 🚫 dangerJS |
|
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
Closing in favor of #10856 |
Partially Fixes #10771
This is a draft, uploaded to make easier to elaborate/evaluate on the approach to solve the issue. Some notes/doubts:
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.txtif necessary.