refactor: replace snackbars with persistent info popups#315
Conversation
ui.showSnackbar has a few issues that make it unreliable for surfacing errors and notifications: - Limited lifetime: a snackbar launched while the window is hidden is often never shown if more than a few seconds pass before the window becomes visible. - Only the last of several snackbars launched in a row is displayed. - Toolbox keeps the coroutine suspended for the snackbar's lifetime and cancels (rather than resumes) it on dismissal, propagating the cancel to the caller and skipping any follow-up code such as resetting the busy state. Switch to ui.showInfoPopup, which is backed by a persistent dialog state: it is still rendered once the window becomes visible, resumes normally on dismissal, and is serialized via a mutex so popups are shown one after another instead of overwriting each other. Since popups render after the window becomes visible, the ErrorReporter buffer-and-flush abstraction is no longer needed. Remove it along with the parallel errorBuffer in CoderRemoteProvider and the visibilityState plumbing threaded through the setup wizard steps; errors are now shown directly where they occur.
| ) | ||
| break | ||
| } | ||
| context.logger.error(ex, "workspace polling error encountered") |
There was a problem hiding this comment.
nit: it may be worth distinguishing this error log from the one above it, unless they suggest the same issue and remedy
| /** | ||
| * Serializes popups so they are shown one after another. [ToolboxUi.showInfoPopup] is | ||
| * backed by a single dialog slot, so launching two popups concurrently would cause the | ||
| * second to replace the first before the user ever sees it. | ||
| */ | ||
| private val popupMutex = Mutex() |
There was a problem hiding this comment.
this is my lack of Kotlin knowledge showing, but isn't a mutex just going to prevent two popups at once? does this actually queue them up to be shown one after another as the comment suggests?
There was a problem hiding this comment.
So the initial issue was that two showSnackbar from the same coroutine ended up showing only one of them (usually the last one). showInfoPopup however do not have this issue, however if you have two coroutines (running in parallel either in a thread pool under the same dispatcher or threads from different dispatchers ) launching the showInfoPopup then you'd get a similar behavior, only one pop-up. To be honest I might have been overly defensive because up until now we always launch from coroutines running on the same dispatcher (the default one), so we don't need the mutex right now. Let me revert this part back
4 participants
ui.showSnackbar has a few issues that make it unreliable for surfacing errors and notifications:
Switch to ui.showInfoPopup, which is backed by a persistent dialog state: it is still rendered once the window becomes visible, resumes normally on dismissal, and is serialized via a mutex so popups are shown one after another instead of overwriting each other.
Since popups render after the window becomes visible, the ErrorReporter buffer-and-flush abstraction is no longer needed. Remove it along with the parallel errorBuffer in CoderRemoteProvider and the visibilityState plumbing threaded through the setup wizard steps; errors are now shown directly where they occur.