fix: stabilize message media loading [WPB-18914]#4687
fix: stabilize message media loading [WPB-18914]#4687
Conversation
|
| audioMessagePlayer.preloadAudioMessage(args.conversationId, args.messageId) | ||
| try { | ||
| // calls preload to initially fetch the audio asset data to be ready and schedule waves mask generation if needed | ||
| audioMessagePlayer.preloadAudioMessage(args.conversationId, args.messageId) |
There was a problem hiding this comment.
ConversationAudioMessagePlayer is a singleton, wouldn't be better to have this logic of checking and not preloading the same audio message again inside this singleton player instead of having this not so pretty approach here with keeping a map in companion object?
| .firstOrNull { it != null } // wait for the first non-null value | ||
| .let { state = state.copy(wavesMask = it) } | ||
| ?.let { | ||
| cachedWavesMasks[args.key] = it |
There was a problem hiding this comment.
Waves masks are being generated only once and then are stored in the DB, so there isn't much overhead here. fetchWavesMask waits until the waves mask is generated, or just takes the value from DB if it's already there.
I don't understand this idea of having cachedWavesMasks - fetchWavesMask method is still executed, even if we initially already have a wave mask in AudioMessageState, so there's no advantage.
| hiltViewModelScoped<AssetLocalPathViewModelImpl, AssetLocalPathViewModel, AssetLocalPathArgs>( | ||
| AssetLocalPathArgs(message.conversationId, message.header.messageId) | ||
| ) | ||
| var rememberedAssetDataPath by rememberSaveable(message.header.messageId) { |
There was a problem hiding this comment.
I'm not sure if I get that but isn't it redundant?
I mean, viewModel.localAssetPath is already a composable state, and it's stored in a ViewModel so it survives when the screen recomposes, why then do we need also rememberSaveable variable here?
| } | ||
|
|
||
| private companion object { | ||
| val cachedLocalAssetPaths = ConcurrentHashMap<String, String>() |
There was a problem hiding this comment.
I'm still not convinced whether it's a good approach. I think I would rather prefer having some in-memory cache in kalium, like we do with FlowCache for instance, because this approach honestly feels like an anti-pattern, it breaks the separation of concerns, mixes logic layer with storage and makes it so that the data doesn't flow the way it should so it can also make it harder to test.
| if (configuration is SwipeableMessageConfiguration.NotSwipeable) { | ||
| content() | ||
| return | ||
| } | ||
|
|
||
| SwipeableBox( |
There was a problem hiding this comment.
I fixed some excessive recompositions some time ago: #4398
This change will actually introduce a regression and make it so that these unwanted recompositions start happening again, because when the message's state changes, then message.isSwipeable changes and depending on that we will again either wrap the content in SwipeableMessageBox or not which causes the whole message’s content to recompose.
2 participants
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
Causes (Optional)
MessageDetailsView, so writes to theAssettable could invalidate the message list while the conversation was loading.Solutions
MessageDetailsViewfrom theAssettable and adding a forward DB migration.