[SES-1966] Attachment batch download and tidy-up #1507

simophin · 2024-05-30T11:46:56Z

Description

This PR reworks some aspect of the attachment download:

A new AttachmentDownloadHelper is created to batch the download request.
The batch download requests will be checked preliminarily, before the request is transformed into a AttachmentDownloadJob

There's one more fix on the ThreadUtils where the ExecutorService seems to misbehave. Changed over to use the Kotlin Coroutine's IO dispatcher.

AL-Session

Great work - some optional comments for you to address as you see fit. Approved!

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

simophin · 2024-06-03T04:51:46Z

@bemusementpark Wanna have a look again? I've extracted the said Flow operator to Flow.timedBuffer.

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

app/src/test/java/org/thoughtcrime/securesms/util/FlowUtilsKtTest.kt

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

…ownload-task # Conflicts: # app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationAdapter.kt # app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationViewModel.kt

…ownload-task

bemusementpark

a few ideas, mostly the naming, maybe it's more of a JobScheduler, or a Debouncer, than a Helper, which suggests more benign impedance matching/courtesy functions, rather than actual business logic.

bemusementpark · 2024-06-24T23:40:31Z

app/src/test/java/org/thoughtcrime/securesms/util/FlowUtilsKtTest.kt

+        val maxItems = 5
+
+        // When
+        val result = flow.timedBuffer(timeoutMillis, maxItems).toCollection(mutableListOf())


Suggested change

val result = flow.timedBuffer(timeoutMillis, maxItems).toCollection(mutableListOf())

val result = flow.timedBuffer(timeoutMillis, maxItems).toList()

Can we use built-ins? result doesn't need to be a MutableList. Don't unnecessarily expose.

Yep! ChatGPT really needs to up its game!

app/src/test/java/org/thoughtcrime/securesms/util/FlowUtilsKtTest.kt

bemusementpark · 2024-06-28T00:20:02Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+                    val pendingAttachmentIDs = storage
+                        .getAllPendingJobs(AttachmentDownloadJob.KEY, AttachmentUploadJob.KEY)
+                        .values
+                        .mapNotNullTo(hashSetOf()) {


It'd be more readable to just mapNotNull, and toSet, if you want performance, then convert to sequence above

Done. Let's go for readability

bemusementpark · 2024-06-28T00:28:50Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+                    // Before handling out attachment to the download task, we need to
+                    // check the requisite for that attachment. This check is very likely to be
+                    // performed again in the download task, but adding stuff into job system
+                    // is expensive so we need to avoid spawning new task whenever we can.


This is a detailed low-level comment, but there's no comment on this class or entire init block, so the intention of this big chain is kind of unclear. Consider adding higher level comments and/or breaking this out into a well-named function .

init { initAttachmentProcessing() // <- insert better name here }

bemusementpark · 2024-06-28T00:37:15Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+            downloadRequests
+                .receiveAsFlow()
+                .timedBuffer(BUFFER_TIMEOUT_MILLS, BUFFER_MAX_ITEMS)
+                .map { attachments ->


Maybe some fun deduplicate(attachment) could replace this block with .map(::deduplicate) and make this chain a lot more manageable/readable.

bemusementpark · 2024-06-28T02:13:31Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+        }
+
+        val threadRecipient = storage.getRecipientForThread(message.threadId)
+        if (threadRecipient == null) {


do we have to retrieve a thread AND a Contact before we can return a boolean ||?

Is it not valid if we have a thread, but no contact?

I guess not, have updated to reflect this

The logic was inserted here trying to replicate the same in AttachmentDownloadJob.execute. I guess it's better to extract the logic from that function and use it here.

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

bemusementpark · 2024-06-28T02:25:27Z

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

+
+            if (buffer.size < maxItems) {
+                // If the buffer is not full, wait until the time limit is reached
+                delay((System.currentTimeMillis() + timeoutMillis - bufferBeganAt).coerceAtLeast(0L))


Suggested change

delay((System.currentTimeMillis() + timeoutMillis - bufferBeganAt).coerceAtLeast(0L))

delay(System.currentTimeMillis() + timeoutMillis - bufferBeganAt))

Don't need it to be non-negative, <= 0 returns immediately.

A negative number doesn't make sense for delay. If I am the implementator of delay, I would throw an exception. Were you looking at the implementation of the delay itself? I'd prefer not to rely on the leniency of such implementation details.

I did look at the source, haha, yeah, but it's also documented:

delay
suspend fun delay(timeMillis: Long)(source)
Delays coroutine for at least the given time without blocking a thread and resumes it after a specified time. If the given timeMillis is non-positive, this function returns immediately.

simophin

@bemusementpark I have addressed some of your concerns and also reverted some changes to make my intent clearer.

simophin · 2024-06-28T03:48:26Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+        if (message.isOutgoing) return true
+
+        val sender = message.individualRecipient.address.serialize()
+        val contact = storage.getContactWithSessionID(sender)


I think this is just introducing unnecessary mental load. It takes no time for the original code to read but it'd probably take me more than 1 sec to read with this style. Also I feel really awkward to have inter-inline function returns in Kotlin, it seems like after-thought after introducing these inline functions that could change the main syntax flow.

simophin · 2024-06-28T03:55:39Z

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

+
+            if (buffer.size < maxItems) {
+                // If the buffer is not full, wait until the time limit is reached
+                delay((System.currentTimeMillis() + timeoutMillis - bufferBeganAt).coerceAtLeast(0L))


A negative number doesn't make sense for delay. If I am the implementator of delay, I would throw an exception. Were you looking at the implementation of the delay itself? I'd prefer not to rely on the leniency of such implementation details.

simophin · 2024-06-28T03:57:43Z

app/src/test/java/org/thoughtcrime/securesms/util/FlowUtilsKtTest.kt

+        val maxItems = 5
+
+        // When
+        val result = flow.timedBuffer(timeoutMillis, maxItems).toCollection(mutableListOf())


Yep! ChatGPT really needs to up its game!

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

simophin · 2024-06-28T04:10:30Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+        }
+
+        val threadRecipient = storage.getRecipientForThread(message.threadId)
+        if (threadRecipient == null) {


I guess not, have updated to reflect this

simophin · 2024-06-28T04:26:19Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+                    // Before handling out attachment to the download task, we need to
+                    // check the requisite for that attachment. This check is very likely to be
+                    // performed again in the download task, but adding stuff into job system
+                    // is expensive so we need to avoid spawning new task whenever we can.


simophin · 2024-06-28T04:27:39Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+        }
+
+        val threadRecipient = storage.getRecipientForThread(message.threadId)
+        if (threadRecipient == null) {


The logic was inserted here trying to replicate the same in AttachmentDownloadJob.execute. I guess it's better to extract the logic from that function and use it here.

simophin · 2024-06-28T05:40:58Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+            downloadRequests
+                .receiveAsFlow()
+                .timedBuffer(BUFFER_TIMEOUT_MILLS, BUFFER_MAX_ITEMS)
+                .map { attachments ->


simophin · 2024-06-28T05:41:09Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+                    val pendingAttachmentIDs = storage
+                        .getAllPendingJobs(AttachmentDownloadJob.KEY, AttachmentUploadJob.KEY)
+                        .values
+                        .mapNotNullTo(hashSetOf()) {


Done. Let's go for readability

bemusementpark

I'm liking the KDoc and structure here! maybe a tiny bit more Boolean simplification could help, otherwise LGTM!

bemusementpark · 2024-06-28T11:45:05Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHandler.kt

+ * requests will go through different level of checking before they are queued for download.
+ *
+ * To use this handler, call [onAttachmentDownloadRequest] with the attachment that needs to be
+ * downloaded. The call to [onAttachmentDownloadRequest] is cheap and can be called multiple times.


bemusementpark · 2024-06-28T11:45:58Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHandler.kt

+                    jobQueue.add(
+                        AttachmentDownloadJob(
+                            attachmentID = attachment.attachmentId.rowId,
+                            databaseMessageID = attachment.mmsId


this is cool, now this is the first interesting part you see, which is clearly adding jobs, so kinda self-documenting IMHO.

bemusementpark · 2024-06-28T11:57:58Z

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt

+ * time [timeoutMillis] limit.
+ */
+fun <T> Flow<T>.timedBuffer(timeoutMillis: Long, maxItems: Int): Flow<List<T>> {
+    return channelFlow {


= could save some brackets and indentation.

bemusementpark · 2024-06-28T12:06:17Z

libsession/src/main/java/org/session/libsession/messaging/jobs/AttachmentDownloadJob.kt

+            if (threadRecipient == null || sender == null || (contact == null && !selfSend)) {
+                return false
+            }
+            if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && storage.getUserPublicKey() != sender) {


Would it simpler, like this?

Suggested change

if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && storage.getUserPublicKey() != sender) {

if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && !selfSend) {

or even:

Suggested change

if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && storage.getUserPublicKey() != sender) {

return threadRecipient.isGroupRecipient || contact?.isTrusted == true || selfSend

actually, that is a big mental unlock for me understanding this easily without squinting.

if ( its a group or its a trusted contact, or its from us ) then its good 👌

tbh I have no idea what that check actually check. This whole check is existing code and I figure I'd reuse the same

bemusementpark · 2024-06-28T12:12:47Z

libsession/src/main/java/org/session/libsession/messaging/jobs/AttachmentDownloadJob.kt

+            }
+
+            val contact = sender?.let { storage.getContactWithSessionID(it) }
+            if (threadRecipient == null || sender == null || (contact == null && !selfSend)) {


If they're in a group are they definitely a contact? this part requires that we have a contact for them, even if they're in a group. Is that necessary? (I don't know the full semantic of a Contact)

I think the last of this is partially built into the next if statement.

Suggested change

if (threadRecipient == null || sender == null || (contact == null && !selfSend)) {

if (threadRecipient == null || sender == null)) {

but then you can probably inline the threadRecipient and sender :

val threadRecipient = storage.getRecipientForThread(threadID) ?: return false

etc.

I think it's one of those painful "what do you mean by contact" problem that we have on Android.
From doing some quick tests we were able to see a photo in a group from someone who isn't an actual contact. But this db call probably means something different by "contact", especially with the isTrusted property in there.

bemusementpark · 2024-06-28T12:27:05Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt

+        if (message.isOutgoing) return true
+
+        val sender = message.individualRecipient.address.serialize()
+        val contact = storage.getContactWithSessionID(sender)


I'm not a fan of the ?: run { ... } syntax myself, but once the logs are gone and it's a 1-liner it's nicer ?: return false

I thought these would maintain the same functionality just reduce the repetitive references to each val.

@you

* fix: Authenticate all Open Group API calls * Use unblinded authentication when we have `capabilities` data for the open group server we are sending the request to but don't have the `blind` capability * Use blinded authentication when we haven't gotten any `capabilities` for an open group server, or if we have `capabilities` and the server has the `blind` capability * Hide send button when message contains only whitespace * Fix bug displaying user ID when quoting own message * Fix notification update for incoming unsend request * Improve check if author is own user when quoting messages * Fixed video call auto rotate, when auto rotate is disabled * refactor: simplify comparison * Stop playing message if deleted * Accidental change * Accidental change * Comments * Feedback * Comments * Import * Fix delete message for everyone doesn't stop the audio playing * Correct the usage of flowOn * Import * Optimise XML * Remove unused file * Remove view pools * Remove the use of executor in ThreadUtils * Using trim and empty to capture semantic concept of nothing being in there * Remove config checks (PR 1294) Refactor: remove checks for whether new config is enabled throughout config factory generation. First commit from PR 1294. * [SES-2162] - Remove wrapping of config message (#1517) * Remove wrapping of config message * Addresses feedback * Merged in ThreadUtils fix * JDK installation * Revert JDK change --------- Co-authored-by: fanchao <[email protected]> * Update libsession * [SES-337] Add rounded corners to thumbnail in QuoteView (#1285) * Add rounded corners to thumbnail in QuoteView * Simplify ThumbnailView * Cleanup ThumbnailView * Removed include custom attributes The custom attributes are not passed to the view. I added the radius programatically instead. * Clipping whole thumbnail view instead of just the image requests --------- Co-authored-by: AL-Session <[email protected]> Co-authored-by: ThomasSession <[email protected]> * Highlight @you mentions (#985) * Highlight @you mentions * fix: resolve merge conflicts * Setting the proper design rules for mentions * New RoundedBackgroundSpan, applied to "you" mentions The rounded background highlighter can take padding, so there is no need to add those extra spaces at the start and end. * Better mention highlight logic Some mention highlight should only format the text and not apply any styling. Also making sure we cater for all cases properly * Updated the text color logic based on design rules * Fine tuning the color rules * Removing usage of Resources.getSystem() Only making the db call if there actually is a mention * Moving color definition outside the loop to avoid repetitions --------- Co-authored-by: charles <[email protected]> Co-authored-by: 0x330a <[email protected]> Co-authored-by: ThomasSession <[email protected]> * [SES-2018] Refactor mention (#1510) * Refactor mention * Fixes robolectric test problem * Fixes tests * Naming and comments * Naming * Dispatcher --------- Co-authored-by: fanchao <[email protected]> * [SES-1966] Attachment batch download and tidy-up (#1507) * Attachment batch download * Addressed feedback and test issues * Feedback fixes * timedWindow for flow * Feedback * Dispatchers * Remove `flowOn` * New implementation of timedBuffer * Organise import * Feedback * Fix test * Tidied up logic around `eligibleForDownload` * Updated comment --------- Co-authored-by: fanchao <[email protected]> * Fix issue with span being the full length (#1528) * Proper display of unresolved names in mentions (#1530) * Fix issue with span being the full length * Making sure a mention with a username without a resolved name still displayed with the appropriate style with the truncated is * Testnet build (#1532) Co-authored-by: fanchao <[email protected]> * Allow "public.loki.foundation" to be accessed by http (#1534) Co-authored-by: fanchao <[email protected]> * Bumping the version code and name * Reverting temporary change --------- Co-authored-by: charles <[email protected]> Co-authored-by: andrew <[email protected]> Co-authored-by: aaronkerckhoff <[email protected]> Co-authored-by: Rugved Darwhekar <[email protected]> Co-authored-by: 0x330a <[email protected]> Co-authored-by: fanchao <[email protected]> Co-authored-by: Fanchao Liu <[email protected]> Co-authored-by: AL-Session <[email protected]> Co-authored-by: ceokot <[email protected]>

Attachment batch download

abf7e0a

AL-Session approved these changes May 30, 2024

View reviewed changes

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt Outdated Show resolved Hide resolved

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt Outdated Show resolved Hide resolved

Addressed feedback and test issues

ceff51b

bemusementpark reviewed May 31, 2024

View reviewed changes

fanchao added 2 commits June 3, 2024 09:55

Feedback fixes

6f8f11e

timedWindow for flow

b4448c6

bemusementpark suggested changes Jun 4, 2024

View reviewed changes

fanchao added 2 commits June 4, 2024 21:36

Feedback

a50d026

Dispatchers

a8f8f06

ThomasSession approved these changes Jun 20, 2024

View reviewed changes

bemusementpark reviewed Jun 21, 2024

View reviewed changes

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/AttachmentDownloadHelper.kt Outdated Show resolved Hide resolved

app/src/main/java/org/thoughtcrime/securesms/util/FlowUtils.kt Outdated Show resolved Hide resolved

fanchao added 5 commits June 21, 2024 11:07

Remove flowOn

c33870c

New implementation of timedBuffer

1785d4b

Merge remote-tracking branch 'upstream/dev' into ses-1966-untrusted-d…

dc80f15

…ownload-task # Conflicts: # app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationAdapter.kt # app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationViewModel.kt

Organise import

cd91a08

Merge remote-tracking branch 'upstream/dev' into ses-1966-untrusted-d…

33b18bd

…ownload-task

bemusementpark reviewed Jun 28, 2024

View reviewed changes

fanchao added 2 commits June 28, 2024 15:43

Feedback

64eecc0

Fix test

8be6a5f

simophin commented Jun 28, 2024

View reviewed changes

bemusementpark approved these changes Jun 28, 2024

View reviewed changes

fanchao added 2 commits July 1, 2024 16:49

Tidied up logic around eligibleForDownload

367a48f

Updated comment

04a6571

ThomasSession merged commit 0da949c into oxen-io:dev Jul 1, 2024
1 check was pending

	val result = flow.timedBuffer(timeoutMillis, maxItems).toCollection(mutableListOf())
	val result = flow.timedBuffer(timeoutMillis, maxItems).toList()

	delay((System.currentTimeMillis() + timeoutMillis - bufferBeganAt).coerceAtLeast(0L))
	delay(System.currentTimeMillis() + timeoutMillis - bufferBeganAt))

	if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && storage.getUserPublicKey() != sender) {
	if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && !selfSend) {

	if (!threadRecipient.isGroupRecipient && contact?.isTrusted != true && storage.getUserPublicKey() != sender) {
	return threadRecipient.isGroupRecipient \|\| contact?.isTrusted == true \|\| selfSend

	if (threadRecipient == null \|\| sender == null \|\| (contact == null && !selfSend)) {
	if (threadRecipient == null \|\| sender == null)) {

[SES-1966] Attachment batch download and tidy-up #1507

[SES-1966] Attachment batch download and tidy-up #1507

Conversation

simophin commented May 30, 2024

Description

AL-Session left a comment

Choose a reason for hiding this comment

simophin commented Jun 3, 2024

bemusementpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

simophin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark Jun 28, 2024 •

edited

Loading