feat: add use case to download assets by its asset key/id (AR-1094) #243
Conversation
…uploading an asset
|
@typfel @gongracr I made the changes here after our conversation, if you see specifically the last commit (eee3ed0) you'll see that now the sync it's happening in a one shot execution 🙌 My only concern it is with the slow sync (now it's actually slower), since the first time we have to download every contact avatar asset, it's a cost we have to assume with this approach. Anyway, after that first sync we don't have a performance degradation from what I could test, since the assets are already present and no download it's made after that. |
But why we are downloading the pictures during the slow sync? Why don't we download the profile pictures the first time we want display them in the UI. |
| assetRepository.saveUserPictureAsset(mapAssetsForUsersToBePersisted(usersToBePersisted)) | ||
| // not sure about this, because on first sync it's taking ages =/ | ||
| // options, doing it in 2 steps like before and downloading on demand or doing it somehow in a non.blocking fashion | ||
| assetRepository.downloadUsersPictureAssets(mapAssetsForUsersToBePersisted(usersToBePersisted)) |
There was a problem hiding this comment.
Yes, we shouldn't do this here but on demand.
There was a problem hiding this comment.
You already have everything for this though?
GetPublicAssetUseCase(user. previewPicture)
There was a problem hiding this comment.
I see an issue with this tough, because we have a FOREIGN KEY constraint on Users Table pointing to the Assets table. So the first time we need to persist in the assets table first, then the user (this was the reason of having the empty asset insert 😓 )
The option to achieve the on demand execution, that I also agree, will be removing the constraint and have the relation only at the Kotlin Model level 🤔 Would that make sense ?
There was a problem hiding this comment.
Yes, I think it makes sense to remove the foreign key constraint in this case since we want to fetch it lazily.
There was a problem hiding this comment.
Applied the changes, now we are not using anymore the FK constraint, and we are able to do the sync on demand later via usecase
| import com.wire.kalium.logic.functional.suspending | ||
|
|
||
| interface GetPublicAssetUseCase { | ||
| suspend operator fun invoke(assetKey: String): Either<CoreFailure, ByteArray> |
There was a problem hiding this comment.
Can't we have assetKey be of type AssetId or UserAssetId?
There was a problem hiding this comment.
Thanks, changes applied, completely agree that using this parameter suits it better
logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/asset/GetPublicAssetUseCase.kt
Outdated
Show resolved
Hide resolved
logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/UploadUserAvatarUseCase.kt
Outdated
Show resolved
Hide resolved
|
🚀 |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
https://wearezeta.atlassian.net/browse/AR-1094
This change includes the following aspects of downloading a public asset (user's avatar)
Dependencies (Optional)
This PR needs to be merged after
#241
Needs releases with:
Attachments (Optional)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.