From cd3b8f357199fcc47ef90111e244a7ccce010cd0 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 8 Feb 2023 13:42:35 +1100 Subject: [PATCH 1/2] Fixed a few issues related to the GroupAvatarDownloadJob Added logic to avoid scheduling a GroupAvatarDownloadJob if there is already an existing one with the same parameters Added logic to avoid trying to download an old avatar for a group if there is a new one Added logic to prevent an old GroupAvatarDownloadJob from overriding a valid avatar with an old one --- .../securesms/database/SessionJobDatabase.kt | 4 +-- .../securesms/database/Storage.kt | 4 +-- .../libsession/database/StorageProtocol.kt | 2 +- .../messaging/jobs/BackgroundGroupAddJob.kt | 4 +-- .../messaging/jobs/GroupAvatarDownloadJob.kt | 29 +++++++++++++++++-- .../pollers/OpenGroupPoller.kt | 8 +++-- 6 files changed, 38 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt index 4425e3d85d..66497d9da8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt @@ -83,11 +83,11 @@ class SessionJobDatabase(context: Context, helper: SQLCipherOpenHelper) : Databa } } - fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? { + fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? { val database = databaseHelper.readableDatabase return database.getAll(sessionJobTable, "$jobType = ?", arrayOf(GroupAvatarDownloadJob.KEY)) { jobFromCursor(it) as GroupAvatarDownloadJob? - }.filterNotNull().find { it.server == server && it.room == room } + }.filterNotNull().find { it.server == server && it.room == room && (imageId == null || it.imageId == imageId) } } fun cancelPendingMessageSendJobs(threadID: Long) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt index f7e2d9d06b..33aad17d1d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt @@ -226,8 +226,8 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context, return DatabaseComponent.get(context).sessionJobDatabase().getMessageReceiveJob(messageReceiveJobID) } - override fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? { - return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room) + override fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? { + return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room, imageId) } override fun resumeMessageSendJobIfNeeded(messageSendJobID: String) { diff --git a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt index c80ef24642..ee124aa0e1 100644 --- a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt +++ b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt @@ -49,7 +49,7 @@ interface StorageProtocol { fun getAttachmentUploadJob(attachmentID: Long): AttachmentUploadJob? fun getMessageSendJob(messageSendJobID: String): MessageSendJob? fun getMessageReceiveJob(messageReceiveJobID: String): Job? - fun getGroupAvatarDownloadJob(server: String, room: String): Job? + fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): Job? fun resumeMessageSendJobIfNeeded(messageSendJobID: String) fun isJobCanceled(job: Job): Boolean diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt index ef67408fb3..5154101328 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt @@ -43,8 +43,8 @@ class BackgroundGroupAddJob(val joinUrl: String): Job { storage.setOpenGroupPublicKey(openGroup.server, openGroup.serverPublicKey) val info = storage.addOpenGroup(openGroup.joinUrl()) val imageId = info?.imageId - if (imageId != null) { - JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.room, openGroup.server)) + if (imageId != null && storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId) == null) { + JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId)) } Log.d(KEY, "onOpenGroupAdded(${openGroup.server})") storage.onOpenGroupAdded(openGroup.server) diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt index 6429d760ae..e176046361 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt @@ -5,7 +5,7 @@ import org.session.libsession.messaging.open_groups.OpenGroupApi import org.session.libsession.messaging.utilities.Data import org.session.libsession.utilities.GroupUtil -class GroupAvatarDownloadJob(val room: String, val server: String) : Job { +class GroupAvatarDownloadJob(val server: String, val room: String, val imageId: String?) : Job { override var delegate: JobDelegate? = null override var id: String? = null @@ -13,10 +13,30 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { override val maxFailureCount: Int = 10 override fun execute(dispatcherName: String) { + if (imageId == null) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob now requires imageId")) + return + } + val storage = MessagingModuleConfiguration.shared.storage - val imageId = storage.getOpenGroup(room, server)?.imageId ?: return + val storedImageId = storage.getOpenGroup(room, server)?.imageId + + if (storedImageId == null || storedImageId != imageId) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId does not match the OpenGroup")) + return + } + try { val bytes = OpenGroupApi.downloadOpenGroupProfilePicture(server, room, imageId).get() + + // Once the download is complete the imageId might no longer match, so we need to fetch it again just in case + val postDownloadStoredImageId = storage.getOpenGroup(room, server)?.imageId + + if (postDownloadStoredImageId == null || postDownloadStoredImageId != imageId) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId no longer matches the OpenGroup")) + return + } + val groupId = GroupUtil.getEncodedOpenGroupID("$server.$room".toByteArray()) storage.updateProfilePicture(groupId, bytes) storage.updateTimestampUpdated(groupId, System.currentTimeMillis()) @@ -30,6 +50,7 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { return Data.Builder() .putString(ROOM, room) .putString(SERVER, server) + .putString(IMAGE_ID, imageId) .build() } @@ -40,6 +61,7 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { private const val ROOM = "room" private const val SERVER = "server" + private const val IMAGE_ID = "imageId" } class Factory : Job.Factory { @@ -47,7 +69,8 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { override fun create(data: Data): GroupAvatarDownloadJob { return GroupAvatarDownloadJob( data.getString(ROOM), - data.getString(SERVER) + data.getString(SERVER), + if (data.hasString(IMAGE_ID)) { data.getString(IMAGE_ID) } else { null } ) } } diff --git a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt index 353b4e1bf5..4f270206be 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt @@ -165,14 +165,16 @@ class OpenGroupPoller(private val server: String, private val executorService: S pollInfo.details.imageId != null && ( pollInfo.details.imageId != existingOpenGroup.imageId || !storage.hasDownloadedProfilePicture(dbGroupId) - ) + ) && + storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, pollInfo.details.imageId) == null ) || ( pollInfo.details == null && existingOpenGroup.imageId != null && - !storage.hasDownloadedProfilePicture(dbGroupId) + !storage.hasDownloadedProfilePicture(dbGroupId) && + storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, existingOpenGroup.imageId) == null ) ) { - JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server)) + JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server, existingOpenGroup.imageId)) } } From 9fd68d27f83e0a8daa98c7e0c29d7d4d7f50276f Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 10 Feb 2023 10:05:28 +1100 Subject: [PATCH 2/2] Fixed a few issues with the GroupAvatarDownloadJob Added a method to remove the Group avatar Fixed an issue where the recipient diffing on the HomeActivity wasn't working properly Fixed an issue where the GroupAvatarDownloadJob could be scheduled even when there was already one scheduled --- .../securesms/database/GroupDatabase.java | 19 +++++++++++++++++++ .../securesms/database/Storage.kt | 4 ++++ .../database/model/ThreadRecord.java | 6 ++++++ .../securesms/home/HomeDiffUtil.kt | 7 +++++-- .../libsession/database/StorageProtocol.kt | 1 + .../messaging/jobs/GroupAvatarDownloadJob.kt | 2 +- .../messaging/open_groups/OpenGroup.kt | 2 +- .../pollers/OpenGroupPoller.kt | 10 +++++++++- .../database/LokiOpenGroupDatabaseProtocol.kt | 1 + 9 files changed, 47 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java index 584bf3a71a..79adead57e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java @@ -318,6 +318,25 @@ public class GroupDatabase extends Database implements LokiOpenGroupDatabaseProt notifyConversationListListeners(); } + @Override + public void removeProfilePicture(String groupID) { + databaseHelper.getWritableDatabase() + .execSQL("UPDATE " + TABLE_NAME + + " SET " + AVATAR + " = NULL, " + + AVATAR_ID + " = NULL, " + + AVATAR_KEY + " = NULL, " + + AVATAR_CONTENT_TYPE + " = NULL, " + + AVATAR_RELAY + " = NULL, " + + AVATAR_DIGEST + " = NULL, " + + AVATAR_URL + " = NULL" + + " WHERE " + + GROUP_ID + " = ?", + new String[] {groupID}); + + Recipient.applyCached(Address.fromSerialized(groupID), recipient -> recipient.setGroupAvatarId(null)); + notifyConversationListListeners(); + } + public boolean hasDownloadedProfilePicture(String groupId) { try (Cursor cursor = databaseHelper.getReadableDatabase().query(TABLE_NAME, new String[]{AVATAR}, GROUP_ID + " = ?", new String[] {groupId}, diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt index 33aad17d1d..076fa57afc 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt @@ -324,6 +324,10 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context, DatabaseComponent.get(context).groupDatabase().updateProfilePicture(groupID, newValue) } + override fun removeProfilePicture(groupID: String) { + DatabaseComponent.get(context).groupDatabase().removeProfilePicture(groupID) + } + override fun hasDownloadedProfilePicture(groupID: String): Boolean { return DatabaseComponent.get(context).groupDatabase().hasDownloadedProfilePicture(groupID) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java b/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java index dfc4c1bc84..f3e72a8747 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/model/ThreadRecord.java @@ -51,6 +51,7 @@ public class ThreadRecord extends DisplayRecord { private final long expiresIn; private final long lastSeen; private final boolean pinned; + private final int initialRecipientHash; public ThreadRecord(@NonNull String body, @Nullable Uri snippetUri, @NonNull Recipient recipient, long date, long count, int unreadCount, @@ -68,6 +69,7 @@ public class ThreadRecord extends DisplayRecord { this.expiresIn = expiresIn; this.lastSeen = lastSeen; this.pinned = pinned; + this.initialRecipientHash = recipient.hashCode(); } public @Nullable Uri getSnippetUri() { @@ -176,4 +178,8 @@ public class ThreadRecord extends DisplayRecord { public boolean isPinned() { return pinned; } + + public int getInitialRecipientHash() { + return initialRecipientHash; + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt b/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt index 1baec20854..b883709c0a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt @@ -28,8 +28,11 @@ class HomeDiffUtil( if (isSameItem) { isSameItem = (oldItem.unreadCount == newItem.unreadCount) } if (isSameItem) { isSameItem = (oldItem.isPinned == newItem.isPinned) } - // Note: For some reason the 'hashCode' value can change after initialisation so we can't cache it - if (isSameItem) { isSameItem = (oldItem.recipient.hashCode() == newItem.recipient.hashCode()) } + // The recipient is passed as a reference and changes to recipients update the reference so we + // need to cache the hashCode for the recipient and use that for diffing - unfortunately + // recipient data is also loaded asyncronously which means every thread will refresh at least + // once when the initial recipient data is loaded + if (isSameItem) { isSameItem = (oldItem.initialRecipientHash == newItem.initialRecipientHash) } // Note: Two instances of 'SpannableString' may not equate even though their content matches if (isSameItem) { isSameItem = (oldItem.getDisplayBody(context).toString() == newItem.getDisplayBody(context).toString()) } diff --git a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt index ee124aa0e1..d75e209d11 100644 --- a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt +++ b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt @@ -80,6 +80,7 @@ interface StorageProtocol { // Open Group Metadata fun updateTitle(groupID: String, newValue: String) fun updateProfilePicture(groupID: String, newValue: ByteArray) + fun removeProfilePicture(groupID: String) fun hasDownloadedProfilePicture(groupID: String): Boolean fun setUserCount(room: String, server: String, newValue: Int) diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt index e176046361..5861229e52 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt @@ -68,8 +68,8 @@ class GroupAvatarDownloadJob(val server: String, val room: String, val imageId: override fun create(data: Data): GroupAvatarDownloadJob { return GroupAvatarDownloadJob( - data.getString(ROOM), data.getString(SERVER), + data.getString(ROOM), if (data.hasString(IMAGE_ID)) { data.getString(IMAGE_ID) } else { null } ) } diff --git a/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt b/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt index 6a2a771e08..80a9a1e501 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroup.kt @@ -36,7 +36,7 @@ data class OpenGroup( val server = json.get("server").asText().lowercase(Locale.US) val displayName = json.get("displayName").asText() val publicKey = json.get("publicKey").asText() - val imageId = json.get("imageId")?.asText() + val imageId = if (json.hasNonNull("imageId")) { json.get("imageId")?.asText() } else { null } val canWrite = json.get("canWrite")?.asText()?.toBoolean() ?: true val infoUpdates = json.get("infoUpdates")?.asText()?.toIntOrNull() ?: 0 OpenGroup(server = server, room = room, name = displayName, publicKey = publicKey, imageId = imageId, canWrite = canWrite, infoUpdates = infoUpdates) diff --git a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt index 4f270206be..562ddda699 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt @@ -159,6 +159,7 @@ class OpenGroupPoller(private val server: String, private val executorService: S }) } + // Update the group avatar if ( ( pollInfo.details != null && @@ -174,7 +175,14 @@ class OpenGroupPoller(private val server: String, private val executorService: S storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, existingOpenGroup.imageId) == null ) ) { - JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server, existingOpenGroup.imageId)) + JobQueue.shared.add(GroupAvatarDownloadJob(server, roomToken, existingOpenGroup.imageId)) + } + else if ( + pollInfo.details != null && + pollInfo.details.imageId == null && + existingOpenGroup.imageId != null + ) { + storage.removeProfilePicture(dbGroupId) } } diff --git a/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt b/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt index 28bcecdbd9..6ccb1ab61a 100644 --- a/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt +++ b/libsignal/src/main/java/org/session/libsignal/database/LokiOpenGroupDatabaseProtocol.kt @@ -4,4 +4,5 @@ interface LokiOpenGroupDatabaseProtocol { fun updateTitle(groupID: String, newValue: String) fun updateProfilePicture(groupID: String, newValue: ByteArray) + fun removeProfilePicture(groupID: String) }