Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: revert of #3670 (WPB-14433) 🍒 #3702

Closed
wants to merge 2 commits into from
Closed

fix: revert of #3670 (WPB-14433) 🍒 #3702

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b1a346
Commit with unresolved merge conflicts outside of submodules
yamilmedina Dec 3, 2024
9194df9
Merge branch 'develop' into fix/assets-issue-cherry-pick
yamilmedina Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/src/main/kotlin/com/wire/android/mapper/ContactMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.wire.android.ui.home.conversationslist.model.Membership
import com.wire.android.ui.home.newconversation.model.Contact
import com.wire.android.ui.userprofile.common.UsernameMapper
import com.wire.android.util.EMPTY
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.publicuser.model.UserSearchDetails
import com.wire.kalium.logic.data.service.ServiceDetails
import com.wire.kalium.logic.data.user.ConnectionState
Expand All @@ -35,6 +36,7 @@ import javax.inject.Inject
class ContactMapper
@Inject constructor(
private val userTypeMapper: UserTypeMapper,
private val wireSessionImageLoader: WireSessionImageLoader,
) {

fun fromOtherUser(otherUser: OtherUser): Contact {
Expand All @@ -46,7 +48,7 @@ class ContactMapper
handle = handle.orEmpty(),
label = UsernameMapper.fromOtherUser(otherUser),
avatarData = UserAvatarData(
asset = previewPicture?.let { ImageAsset.UserAvatarAsset(it) },
asset = previewPicture?.let { ImageAsset.UserAvatarAsset(wireSessionImageLoader, it) },
connectionState = connectionStatus,
nameBasedAvatar = NameBasedAvatar(fullName = name, accentColor = otherUser.accentId)
),
Expand All @@ -65,7 +67,7 @@ class ContactMapper
handle = String.EMPTY,
label = String.EMPTY,
avatarData = UserAvatarData(
asset = previewAssetId?.let { ImageAsset.UserAvatarAsset(it) },
asset = previewAssetId?.let { ImageAsset.UserAvatarAsset(wireSessionImageLoader, it) },
membership = Membership.Service
),
membership = Membership.Service,
Expand All @@ -83,7 +85,7 @@ class ContactMapper
handle = handle.orEmpty(),
label = mapUserHandle(user),
avatarData = UserAvatarData(
asset = previewAssetId?.let { ImageAsset.UserAvatarAsset(it) },
asset = previewAssetId?.let { ImageAsset.UserAvatarAsset(wireSessionImageLoader, it) },
nameBasedAvatar = NameBasedAvatar(fullName = name, accentColor = -1)
),
membership = userTypeMapper.toMembership(type),
Expand Down
6 changes: 4 additions & 2 deletions app/src/main/kotlin/com/wire/android/mapper/ConversationMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import com.wire.android.ui.home.conversationslist.model.BlockState
import com.wire.android.ui.home.conversationslist.model.ConversationInfo
import com.wire.android.ui.home.conversationslist.model.ConversationItem
import com.wire.android.ui.home.conversationslist.showLegalHoldIndicator
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.conversation.ConversationDetails.Connection
import com.wire.kalium.logic.data.conversation.ConversationDetails.Group
import com.wire.kalium.logic.data.conversation.ConversationDetails.OneOne
Expand All @@ -40,6 +41,7 @@ import com.wire.kalium.logic.data.user.UserAvailabilityStatus

@Suppress("LongMethod")
fun ConversationDetailsWithEvents.toConversationItem(
wireSessionImageLoader: WireSessionImageLoader,
userTypeMapper: UserTypeMapper,
searchQuery: String,
selfUserTeamId: TeamId?
Expand Down Expand Up @@ -72,7 +74,7 @@ fun ConversationDetailsWithEvents.toConversationItem(
is OneOne -> {
ConversationItem.PrivateConversation(
userAvatarData = UserAvatarData(
asset = conversationDetails.otherUser.previewPicture?.let { UserAvatarAsset(it) },
asset = conversationDetails.otherUser.previewPicture?.let { UserAvatarAsset(wireSessionImageLoader, it) },
availabilityStatus = conversationDetails.otherUser.availabilityStatus,
connectionState = conversationDetails.otherUser.connectionStatus,
nameBasedAvatar = NameBasedAvatar(conversationDetails.otherUser.name, conversationDetails.otherUser.accentId)
Expand Down Expand Up @@ -109,7 +111,7 @@ fun ConversationDetailsWithEvents.toConversationItem(
is Connection -> {
ConversationItem.ConnectionConversation(
userAvatarData = UserAvatarData(
asset = conversationDetails.otherUser?.previewPicture?.let { UserAvatarAsset(it) },
asset = conversationDetails.otherUser?.previewPicture?.let { UserAvatarAsset(wireSessionImageLoader, it) },
availabilityStatus = conversationDetails.otherUser?.availabilityStatus ?: UserAvailabilityStatus.NONE,
nameBasedAvatar = NameBasedAvatar(conversationDetails.otherUser?.name, conversationDetails.otherUser?.accentId ?: -1)
),
Expand Down
5 changes: 4 additions & 1 deletion app/src/main/kotlin/com/wire/android/mapper/MessageMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import com.wire.android.ui.home.conversationslist.model.Membership
import com.wire.android.ui.theme.Accent
import com.wire.android.util.time.ISOFormatter
import com.wire.android.util.ui.UIText
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.message.DeliveryStatus
import com.wire.kalium.logic.data.message.Message
import com.wire.kalium.logic.data.message.MessageContent
Expand All @@ -51,6 +52,8 @@ class MessageMapper @Inject constructor(
private val userTypeMapper: UserTypeMapper,
private val messageContentMapper: MessageContentMapper,
private val isoFormatter: ISOFormatter,
// TODO(qol): a message mapper should not depend on a UI related component
private val wireSessionImageLoader: WireSessionImageLoader
) {

fun memberIdList(messages: List<Message>): List<UserId> = messages.flatMap { message ->
Expand Down Expand Up @@ -197,7 +200,7 @@ class MessageMapper @Inject constructor(
}

private fun getUserAvatarData(sender: User?) = UserAvatarData(
asset = sender?.previewAsset(),
asset = sender?.previewAsset(wireSessionImageLoader),
availabilityStatus = sender?.availabilityStatus ?: UserAvailabilityStatus.NONE,
membership = sender?.userType?.let { userTypeMapper.toMembership(it) } ?: Membership.None,
connectionState = getConnectionState(sender),
Expand Down
16 changes: 16 additions & 0 deletions app/src/main/kotlin/com/wire/android/mapper/OtherAccountMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package com.wire.android.mapper

import com.wire.android.ui.home.conversations.avatar
import com.wire.android.ui.userprofile.self.model.OtherAccount
<<<<<<< HEAD
import com.wire.kalium.logic.data.user.SelfUser
import javax.inject.Inject

Expand All @@ -29,5 +30,20 @@ class OtherAccountMapper @Inject constructor() {
fullName = selfUser.name ?: "",
avatarData = selfUser.avatar(selfUser.connectionStatus),
handle = selfUser.handle
=======
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.team.Team
import com.wire.kalium.logic.data.user.SelfUser
import javax.inject.Inject

class OtherAccountMapper @Inject constructor(
private val wireSessionImageLoader: WireSessionImageLoader
) {
fun toOtherAccount(selfUser: SelfUser, team: Team?): OtherAccount = OtherAccount(
id = selfUser.id,
fullName = selfUser.name ?: "",
avatarData = selfUser.avatar(wireSessionImageLoader, selfUser.connectionStatus),
teamName = team?.name
>>>>>>> dda09e7ca (fix: revert of #3670 (WPB-14433) (#3700))
)
}
4 changes: 4 additions & 0 deletions app/src/main/kotlin/com/wire/android/mapper/RegularMessageContentMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.wire.android.ui.home.conversations.model.UIMessageContent
import com.wire.android.ui.home.conversations.model.UIQuotedMessage
import com.wire.android.util.time.ISOFormatter
import com.wire.android.util.ui.UIText
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.asset.AttachmentType
import com.wire.kalium.logic.data.asset.isDisplayableImageMimeType
import com.wire.kalium.logic.data.id.ConversationId
Expand All @@ -49,6 +50,7 @@ import javax.inject.Inject
@Suppress("TooManyFunctions")
class RegularMessageMapper @Inject constructor(
private val messageResourceProvider: MessageResourceProvider,
private val wireSessionImageLoader: WireSessionImageLoader,
private val isoFormatter: ISOFormatter,
) {

Expand Down Expand Up @@ -205,6 +207,7 @@ class RegularMessageMapper @Inject constructor(
is MessageContent.QuotedMessageDetails.Asset -> when (AttachmentType.fromMimeTypeString(quotedContent.assetMimeType)) {
AttachmentType.IMAGE -> UIQuotedMessage.UIQuotedData.DisplayableImage(
ImageAsset.PrivateAsset(
wireSessionImageLoader,
conversationId,
it.messageId,
it.isQuotingSelfUser
Expand Down Expand Up @@ -246,6 +249,7 @@ class RegularMessageMapper @Inject constructor(
UIMessageContent.ImageMessage(
assetId = AssetId(remoteData.assetId, remoteData.assetDomain.orEmpty()),
asset = ImageAsset.PrivateAsset(
wireSessionImageLoader,
message.conversationId,
message.id,
sender is SelfUser
Expand Down
6 changes: 3 additions & 3 deletions app/src/main/kotlin/com/wire/android/mapper/SystemMessageContentMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ class SystemMessageContentMapper @Inject constructor(

private fun mapTeamMemberRemovedMessage(
content: MessageContent.TeamMemberRemoved
): UIMessageContent.SystemMessage = UIMessageContent.SystemMessage.TeamMemberRemoved_Legacy(content.userName)
): UIMessageContent.SystemMessage = UIMessageContent.SystemMessage.TeamMemberRemoved_Legacy(content)

private fun mapConversationRenamedMessage(
senderUserId: UserId,
Expand All @@ -197,7 +197,7 @@ class SystemMessageContentMapper @Inject constructor(
user = sender,
type = SelfNameType.ResourceTitleCase
)
return UIMessageContent.SystemMessage.RenamedConversation(authorName, content.conversationName)
return UIMessageContent.SystemMessage.RenamedConversation(authorName, content)
}

fun mapMemberChangeMessage(
Expand Down Expand Up @@ -271,7 +271,7 @@ class SystemMessageContentMapper @Inject constructor(
UIMessageContent.SystemMessage.HistoryLost

private fun mapMLSWrongEpochWarning(): UIMessageContent.SystemMessage =
UIMessageContent.SystemMessage.MLSWrongEpochWarning
UIMessageContent.SystemMessage.MLSWrongEpochWarning()

private fun mapConversationHistoryListProtocolChanged(): UIMessageContent.SystemMessage =
UIMessageContent.SystemMessage.HistoryLostProtocolChanged
Expand Down
4 changes: 3 additions & 1 deletion app/src/main/kotlin/com/wire/android/mapper/UICallParticipantMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ package com.wire.android.mapper

import com.wire.android.model.ImageAsset
import com.wire.android.ui.calling.model.UICallParticipant
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.call.Participant
import javax.inject.Inject

class UICallParticipantMapper @Inject constructor(
private val wireSessionImageLoader: WireSessionImageLoader,
private val userTypeMapper: UserTypeMapper,
) {
fun toUICallParticipant(participant: Participant) = UICallParticipant(
Expand All @@ -34,7 +36,7 @@ class UICallParticipantMapper @Inject constructor(
isSpeaking = participant.isSpeaking,
isCameraOn = participant.isCameraOn,
isSharingScreen = participant.isSharingScreen,
avatar = participant.avatarAssetId?.let { ImageAsset.UserAvatarAsset(it) },
avatar = participant.avatarAssetId?.let { ImageAsset.UserAvatarAsset(wireSessionImageLoader, it) },
membership = userTypeMapper.toMembership(participant.userType),
hasEstablishedAudio = participant.hasEstablishedAudio,
accentId = participant.accentId
Expand Down
10 changes: 6 additions & 4 deletions app/src/main/kotlin/com/wire/android/mapper/UIParticipantMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package com.wire.android.mapper
import com.wire.android.ui.home.conversations.avatar
import com.wire.android.ui.home.conversations.details.participants.model.UIParticipant
import com.wire.android.ui.home.conversations.previewAsset
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.message.UserSummary
import com.wire.kalium.logic.data.message.reaction.MessageReaction
import com.wire.kalium.logic.data.message.receipt.DetailedReceipt
Expand All @@ -32,6 +33,7 @@ import javax.inject.Inject

class UIParticipantMapper @Inject constructor(
private val userTypeMapper: UserTypeMapper,
private val wireSessionImageLoader: WireSessionImageLoader
) {
fun toUIParticipant(user: User, isMLSVerified: Boolean = false): UIParticipant = with(user) {
val (userType, connectionState, unavailable) = when (this) {
Expand All @@ -43,7 +45,7 @@ class UIParticipantMapper @Inject constructor(
id = id,
name = name.orEmpty(),
handle = handle.orEmpty(),
avatarData = avatar(connectionState),
avatarData = avatar(wireSessionImageLoader, connectionState),
isSelf = user is SelfUser,
isService = userType == UserType.SERVICE,
membership = userTypeMapper.toMembership(userType),
Expand All @@ -65,7 +67,7 @@ class UIParticipantMapper @Inject constructor(
id = userSummary.userId,
name = userSummary.userName.orEmpty(),
handle = userSummary.userHandle.orEmpty(),
avatarData = userSummary.previewAsset(),
avatarData = userSummary.previewAsset(wireSessionImageLoader),
membership = userTypeMapper.toMembership(userSummary.userType),
unavailable = !userSummary.isUserDeleted && userSummary.userName.orEmpty().isEmpty(),
isDeleted = userSummary.isUserDeleted,
Expand All @@ -81,7 +83,7 @@ class UIParticipantMapper @Inject constructor(
id = userSummary.userId,
name = userSummary.userName.orEmpty(),
handle = userSummary.userHandle.orEmpty(),
avatarData = userSummary.previewAsset(),
avatarData = userSummary.previewAsset(wireSessionImageLoader),
membership = userTypeMapper.toMembership(userSummary.userType),
unavailable = !userSummary.isUserDeleted && userSummary.userName.orEmpty().isEmpty(),
isDeleted = userSummary.isUserDeleted,
Expand All @@ -98,7 +100,7 @@ class UIParticipantMapper @Inject constructor(
id = userSummary.userId,
name = userSummary.userName.orEmpty(),
handle = userSummary.userHandle.orEmpty(),
avatarData = previewAsset(),
avatarData = previewAsset(wireSessionImageLoader),
membership = userTypeMapper.toMembership(userSummary.userType),
unavailable = !userSummary.isUserDeleted && userSummary.userName.orEmpty().isEmpty(),
isDeleted = userSummary.isUserDeleted,
Expand Down
56 changes: 15 additions & 41 deletions app/src/main/kotlin/com/wire/android/model/ImageAsset.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,47 +18,35 @@

package com.wire.android.model

import androidx.appcompat.app.AppCompatActivity
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Stable
import androidx.compose.ui.platform.LocalInspectionMode
import androidx.compose.ui.res.painterResource
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
import com.wire.android.R
import com.wire.android.ui.LocalActivity
import com.wire.android.util.ui.WireSessionImageLoader
import com.wire.kalium.logic.data.id.ConversationId
import com.wire.kalium.logic.data.id.QualifiedIdMapper
import com.wire.kalium.logic.data.user.UserAssetId
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.serialization.KSerializer
import kotlinx.serialization.Serializable
import kotlinx.serialization.descriptors.PrimitiveKind
import kotlinx.serialization.descriptors.PrimitiveSerialDescriptor
import kotlinx.serialization.encoding.Decoder
import kotlinx.serialization.encoding.Encoder
import okio.Path
import okio.Path.Companion.toPath
import javax.inject.Inject

@Stable
@Serializable
sealed class ImageAsset {

/**
* Represents an image asset that is stored locally on the device, and it isn't necessarily bounded to any specific conversation or
* message, i.e. some preview images that the user selected from local device gallery.
*/
@Stable
@Serializable
data class Local(
val dataPath: @Serializable(with = PathAsStringSerializer::class) Path,
val dataPath: Path,
val idKey: String
) : ImageAsset()

<<<<<<< HEAD
sealed class Remote : ImageAsset() {
=======
sealed class Remote(private val imageLoader: WireSessionImageLoader) : ImageAsset() {
>>>>>>> dda09e7ca (fix: revert of #3670 (WPB-14433) (#3700))

/**
* Value that uniquely identifies this Asset,
Expand All @@ -72,53 +60,39 @@ sealed class ImageAsset {
withCrossfadeAnimation: Boolean = false
) = when {
LocalInspectionMode.current -> painterResource(id = R.drawable.ic_welcome_1)
else -> {
hiltViewModel<RemoteAssetImageViewModel>(
// limit the scope of the ViewModel to the current activity so that there's one image loader instance for the Activity
viewModelStoreOwner = checkNotNull(LocalActivity.current as? AppCompatActivity ?: LocalViewModelStoreOwner.current) {
"No ViewModelStoreOwner was provided via LocalViewModelStoreOwner"
},
key = "remote_asset_image_loader"
).imageLoader.paint(asset = this, fallbackData = fallbackData, withCrossfadeAnimation = withCrossfadeAnimation)
Comment on lines -77 to -82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but now after understanding the issues with images I think that this could be the problem, so just removing passing viewmodelStoreOwner and a key should make it so that this view model and image loader is created per particular screen and not whole activity which means it's created again when account is switched 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yamilmedina I just checked that and images are loading fine for both logged in accounts from different federated environments after I removed just these two parameters viewModelStoreOwner and key, so it looks like this:

else -> hiltViewModel<RemoteAssetImageViewModel>().imageLoader
    .paint(asset = this, fallbackData = fallbackData, withCrossfadeAnimation = withCrossfadeAnimation)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3712
here's the alternative fix 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix

}
else -> imageLoader.paint(asset = this, fallbackData = fallbackData, withCrossfadeAnimation = withCrossfadeAnimation)
}
}

@Stable
@Serializable
data class UserAvatarAsset(
private val imageLoader: WireSessionImageLoader,
val userAssetId: UserAssetId
) : Remote() {
) : Remote(imageLoader) {
override val uniqueKey: String
get() = userAssetId.toString()
}

@Stable
@Serializable
data class PrivateAsset(
private val imageLoader: WireSessionImageLoader,
val conversationId: ConversationId,
val messageId: String,
val isSelfAsset: Boolean,
val isEphemeral: Boolean = false
) : Remote() {
) : Remote(imageLoader) {
override fun toString(): String = "$conversationId:$messageId:$isSelfAsset:$isEphemeral"
override val uniqueKey: String
get() = toString()
}
}

fun String.parseIntoPrivateImageAsset(qualifiedIdMapper: QualifiedIdMapper): ImageAsset.PrivateAsset {
fun String.parseIntoPrivateImageAsset(
imageLoader: WireSessionImageLoader,
qualifiedIdMapper: QualifiedIdMapper,
): ImageAsset.PrivateAsset {
val (conversationIdString, messageId, isSelfAsset, isEphemeral) = split(":")
val conversationIdParam = qualifiedIdMapper.fromStringToQualifiedID(conversationIdString)

return ImageAsset.PrivateAsset(conversationIdParam, messageId, isSelfAsset.toBoolean(), isEphemeral.toBoolean())
return ImageAsset.PrivateAsset(imageLoader, conversationIdParam, messageId, isSelfAsset.toBoolean(), isEphemeral.toBoolean())
}

object PathAsStringSerializer : KSerializer<Path> {
override val descriptor = PrimitiveSerialDescriptor("Path", PrimitiveKind.STRING)
override fun serialize(encoder: Encoder, value: Path) = encoder.encodeString(value.toString())
override fun deserialize(decoder: Decoder): Path = decoder.decodeString().toPath(normalize = true)
}

@HiltViewModel
class RemoteAssetImageViewModel @Inject constructor(val imageLoader: WireSessionImageLoader) : ViewModel()
Loading