Attachment upload fixes #2342
Attachment upload fixes #2342
Conversation
zsmb13 force-pushed the feature/attachment-upload-fixes branch from c15248c to 7374829 Compare 
zsmb13 force-pushed the feature/attachment-upload-fixes branch from 2e2eee9 to fc1cc3d Compare 
| /** | ||
| * A number of writes to ignore (not issue progress updates for). | ||
| * | ||
| * This accounts for the [HttpLoggingInterceptor] and [CurlInterceptor] configured in | ||
| * [BaseChatModule] that will trigger writing the body into their logs. | ||
| * | ||
| * We don't want to issue progress updates for these writes, only the real write that's | ||
| * going out to the network that happens after these. | ||
| */ | ||
| private const val PROGRESS_UPDATES_TO_SKIP = 2 |
There was a problem hiding this comment.
I don't like this much, but there's no apparent other way to achieve this. Logging the body with these two interceptors triggers the same writes, and we somehow have to ignore those when reporting our progress.
This is also the best wisdom that the internet had on this https://stackoverflow.com/a/44125313
A real solution would be intercepting the call with an OkHttp network interceptor, but we can't pass in individual callbacks per different Retrofit upload calls with that approach (at least not easily).
| } | ||
| | ||
| public fun isComplete(): Flow<Boolean> = isComplete | ||
| public fun isComplete(): StateFlow<Boolean> = isComplete |
There was a problem hiding this comment.
Easier to observe from Jetpack Compose this way
There was a problem hiding this comment.
This class will be removed after ProgressTrackerFactory refactor. 😉
| targetValue: String, | ||
| ) { | ||
| if (!isComplete) { | ||
| val nominalProgress = MediaStringUtil.convertFileSizeByteCount((progress * progressCorrection).toLong()) |
There was a problem hiding this comment.
This was simply an incorrect calculation, getting us stuck permanently at 0 for this value on the UI
| uploaded += read.toLong() | ||
| sink.write(buffer, 0, read) | ||
| uploaded += read.toLong() | ||
| withCallback { onProgress((100 * uploaded / total)) } |
There was a problem hiding this comment.
We still have an issue here which is that we're just writing into a buffer, and showing the write progress of that operation on the UI. This means we get to things like 10 MB / 10 MB rather quickly but then keep spinning because the network upload is actually still happening (as far as I understand).
There are mentions of this issue in a bunch of places, but I didn't find a good solution so far:
| val uploadStates: List<StateFlow<Boolean>> = attachments | ||
| .mapNotNull { it.uploadId } | ||
| .map { uploadId -> ProgressTrackerFactory.getOrCreate(uploadId).isComplete() } | ||
| val uploadedCount: Int by combine(uploadStates) { values -> values.count { it } }.collectAsState(initial = 0) |
There was a problem hiding this comment.
I'd maybe just rename the it to isUploaded for clarity, the rest looks good!
There was a problem hiding this comment.
I wanted to take this suggestion but values.count { isUploaded -> isUploaded } is just so ehh
🎯 Goal
Make attachment uploads show progress nicer in both SDKs.
🛠 Implementation details
See commit messages for small, step-by-step changes.
🎨 UI Changes
Note: the Compose SDK's "after" recording doesn't include the fix for attachment counts in the footer yet.
2021-09-15.11-23-10.mp4
2021-09-15.11-25-30.mp4
2021-09-15.11-22-09.mp4
2021-09-15.11-24-17.mp4
🧪 Testing
Build the sample apps, upload various (especially large-ish) files.
☑️ Checklist
developbranchNew code is covered by unit testsAffected documentation updated (docusaurus, tutorial, CMS (task created))