Fix broadcast game screen blinking when a new move is received #1313
Conversation
The bug was due to the following reason. When a move is received by the BroadcastRoundController, its state changes. During the next frame the BroadcastRoundGameProvider state changes to AsyncLoading(value: <oldValue>) causing the broadcast game screen to switch to the loading state instead of using the oldValue.
disable action buttons until loading is complete
| final broadcastState = ref.watch(broadcastGameControllerProvider(roundId, gameId)); | ||
|
|
||
| switch (broadcastState) { | ||
| case AsyncValue(value: final broadcastState?, hasValue: true): |
There was a problem hiding this comment.
Why the hasValue: true here? It doesn't seem necessary and this usage cannot be found in riverpod documentation (nor in any other part of the app).
There was a problem hiding this comment.
It is not yet in documentation but it has the same effect has having skipError: true and skipLoadingOnReload: true when using when. This is what allow us to avoid the screen blinking by avoiding the loading screen.
| final isLocalEvaluationEnabled = broadcastState.isLocalEvaluationEnabled; | ||
| final currentNode = broadcastState.currentNode; | ||
|
|
||
| return Shimmer( |
There was a problem hiding this comment.
I assume the Shimmer here is because we cannot guarantee the round is loaded.
Better put a comment above to clarify that for future readers. This situation where we load two separate controller is not seen anywhere else in the app.
Ideally they should be merged into one to remedy this situation. Creating another provider just for this purpose is OK and it is even recommended by riverpod to combine providers.
|
|
||
| switch (game) { | ||
| case AsyncValue(value: final game?, hasValue: true): | ||
| final broadcastGameState = |
There was a problem hiding this comment.
Another example when reading this code feels weird. First there is game async value which is pattern matched, and then we require the value of broadcastGameState.
Right now I understand the logic since I'm reviewing. But think of future readers (including ourselves of course, in a few months when we will have forgotten any of this). This code is rather confusing.
So again: a new provider must be made that combine both states. It will greatly simplify the logic and make things much less confusing.
There was a problem hiding this comment.
This pull request did not introduce two separate providers. Its goal is to simply fix the blinking screen.
I can combine both providers in a new pull request but I think we should discuss how that should be done because it is not obvious to me.
veloce
changed the title
No description provided.