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.

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

Iteration on caption #3816

Merged
merged 6 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,14 @@ fun TimelineItemVideoView(
onContentLayoutChange: (ContentAvoidingLayoutData) -> Unit,
modifier: Modifier = Modifier,
) {
val description = stringResource(CommonStrings.common_image)
val description = stringResource(CommonStrings.common_video)
Column(
modifier = modifier.semantics { contentDescription = description }
) {
val containerModifier = if (content.showCaption) {
Modifier
.padding(top = 6.dp)
.clip(RoundedCornerShape(6.dp))
.padding(top = 6.dp)
.clip(RoundedCornerShape(6.dp))
} else {
Modifier
}
Expand All @@ -93,8 +93,8 @@ fun TimelineItemVideoView(
var isLoaded by remember { mutableStateOf(false) }
AsyncImage(
modifier = Modifier
.fillMaxWidth()
.then(if (isLoaded) Modifier.background(Color.White) else Modifier),
.fillMaxWidth()
.then(if (isLoaded) Modifier.background(Color.White) else Modifier),
model = MediaRequestData(
source = content.thumbnailSource,
kind = MediaRequestData.Kind.File(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ class DefaultPinnedMessagesBannerFormatter @Inject constructor(
messageType.bestDescription.prefixWith(CommonStrings.common_audio)
}
is VoiceMessageType -> {
messageType.bestDescription.prefixWith(CommonStrings.common_voice_message)
// In this case, do not use bestDescription, because the filename is useless, only use the caption if available.
messageType.caption?.prefixWith(sp.getString(CommonStrings.common_voice_message))
?: sp.getString(CommonStrings.common_voice_message)
}
is OtherMessageType -> {
messageType.body
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,25 +110,27 @@ class DefaultRoomLastMessageFormatter @Inject constructor(
messageType.toPlainText(permalinkParser)
}
is VideoMessageType -> {
sp.getString(CommonStrings.common_video)
messageType.bestDescription.prefixWith(sp.getString(CommonStrings.common_video))
}
is ImageMessageType -> {
sp.getString(CommonStrings.common_image)
messageType.bestDescription.prefixWith(sp.getString(CommonStrings.common_image))
}
is StickerMessageType -> {
sp.getString(CommonStrings.common_sticker)
messageType.bestDescription.prefixWith(sp.getString(CommonStrings.common_sticker))
}
is LocationMessageType -> {
sp.getString(CommonStrings.common_shared_location)
}
is FileMessageType -> {
sp.getString(CommonStrings.common_file)
messageType.bestDescription.prefixWith(sp.getString(CommonStrings.common_file))
}
is AudioMessageType -> {
sp.getString(CommonStrings.common_audio)
messageType.bestDescription.prefixWith(sp.getString(CommonStrings.common_audio))
}
is VoiceMessageType -> {
sp.getString(CommonStrings.common_voice_message)
// In this case, do not use bestDescription, because the filename is useless, only use the caption if available.
messageType.caption?.prefixWith(sp.getString(CommonStrings.common_voice_message))
?: sp.getString(CommonStrings.common_voice_message)
}
is OtherMessageType -> {
messageType.body
Expand All @@ -140,7 +142,7 @@ class DefaultRoomLastMessageFormatter @Inject constructor(
return message.prefixIfNeeded(senderDisambiguatedDisplayName, isDmRoom, isOutgoing)
}

private fun String.prefixIfNeeded(
private fun CharSequence.prefixIfNeeded(
senderDisambiguatedDisplayName: String,
isDmRoom: Boolean,
isOutgoing: Boolean,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,11 @@ class DefaultPinnedMessagesBannerFormatterTest {
val expectedResult = when (type) {
is VideoMessageType,
is AudioMessageType,
is VoiceMessageType,
is ImageMessageType,
is StickerMessageType,
is FileMessageType,
is LocationMessageType -> AnnotatedString::class.java
is VoiceMessageType,
is EmoteMessageType,
is TextMessageType,
is NoticeMessageType,
Expand All @@ -176,7 +176,7 @@ class DefaultPinnedMessagesBannerFormatterTest {
val expectedResult = when (type) {
is VideoMessageType -> "Video: Shared body"
is AudioMessageType -> "Audio: Shared body"
is VoiceMessageType -> "Voice message: Shared body"
is VoiceMessageType -> "Voice message"
is ImageMessageType -> "Image: Shared body"
is StickerMessageType -> "Sticker: Shared body"
is FileMessageType -> "File: Shared body"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,32 +208,51 @@ class DefaultRoomLastMessageFormatterTest {

// Verify results of DM mode
for ((type, result) in resultsInDm) {
val string = result.toString()
val expectedResult = when (type) {
is VideoMessageType -> "Video"
is AudioMessageType -> "Audio"
is VideoMessageType -> "Video: Shared body"
is AudioMessageType -> "Audio: Shared body"
is VoiceMessageType -> "Voice message"
is ImageMessageType -> "Image"
is StickerMessageType -> "Sticker"
is FileMessageType -> "File"
is ImageMessageType -> "Image: Shared body"
is StickerMessageType -> "Sticker: Shared body"
is FileMessageType -> "File: Shared body"
is LocationMessageType -> "Shared location"
is EmoteMessageType -> "* $senderName ${type.body}"
is TextMessageType,
is NoticeMessageType,
is OtherMessageType -> body
}
assertWithMessage("$type was not properly handled for DM").that(result).isEqualTo(expectedResult)
val shouldCreateAnnotatedString = when (type) {
is VideoMessageType -> true
is AudioMessageType -> true
is VoiceMessageType -> false
is ImageMessageType -> true
is StickerMessageType -> true
is FileMessageType -> true
is LocationMessageType -> false
is EmoteMessageType -> false
is TextMessageType -> false
is NoticeMessageType -> false
is OtherMessageType -> false
}
if (shouldCreateAnnotatedString) {
assertWithMessage("$type doesn't produce an AnnotatedString")
.that(result)
.isInstanceOf(AnnotatedString::class.java)
}
assertWithMessage("$type was not properly handled for DM").that(string).isEqualTo(expectedResult)
}

// Verify results of Room mode
for ((type, result) in resultsInRoom) {
val string = result.toString()
val expectedResult = when (type) {
is VideoMessageType -> "$expectedPrefix: Video"
is AudioMessageType -> "$expectedPrefix: Audio"
is VideoMessageType -> "$expectedPrefix: Video: Shared body"
is AudioMessageType -> "$expectedPrefix: Audio: Shared body"
is VoiceMessageType -> "$expectedPrefix: Voice message"
is ImageMessageType -> "$expectedPrefix: Image"
is StickerMessageType -> "$expectedPrefix: Sticker"
is FileMessageType -> "$expectedPrefix: File"
is ImageMessageType -> "$expectedPrefix: Image: Shared body"
is StickerMessageType -> "$expectedPrefix: Sticker: Shared body"
is FileMessageType -> "$expectedPrefix: File: Shared body"
is LocationMessageType -> "$expectedPrefix: Shared location"
is TextMessageType,
is NoticeMessageType,
Expand All @@ -249,7 +268,8 @@ class DefaultRoomLastMessageFormatterTest {
is FileMessageType -> true
is LocationMessageType -> false
is EmoteMessageType -> false
is TextMessageType, is NoticeMessageType -> true
is TextMessageType -> true
is NoticeMessageType -> true
is OtherMessageType -> true
}
if (shouldCreateAnnotatedString) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
senderDisambiguatedDisplayName = senderDisambiguatedDisplayName,
body = messageBody,
imageUriString = content.fetchImageIfPresent(client)?.toString(),
imageMimeType = content.getImageMimetype(),
roomName = roomDisplayName,
roomIsDm = isDm,
roomAvatarPath = roomAvatarUrl,
Expand Down Expand Up @@ -316,6 +317,17 @@
}
.getOrNull()
}

private suspend fun NotificationContent.MessageLike.RoomMessage.getImageMimetype(): String? {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be reused in the previous function (fetchImageIfPresent)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, maybe. Also I do not see a better way to avoid the code duplication here. I do not like using Pair.

if (appPreferencesStore.doesHideImagesAndVideosFlow().first()) {
return null

Check warning on line 323 in libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolver.kt

View check run for this annotation

Codecov / codecov/patch

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolver.kt#L323

Added line #L323 was not covered by tests
}
return when (val messageType = messageType) {
is ImageMessageType -> messageType.info?.mimetype
is VideoMessageType -> null // Use the thumbnail here?
else -> null
}
}
}

@Suppress("LongParameterList")
Expand All @@ -333,6 +345,7 @@
// We cannot use Uri? type here, as that could trigger a
// NotSerializableException when persisting this to storage
imageUriString: String? = null,
imageMimeType: String? = null,
threadId: ThreadId? = null,
roomName: String? = null,
roomIsDm: Boolean = false,
Expand All @@ -358,6 +371,7 @@
senderDisambiguatedDisplayName = senderDisambiguatedDisplayName,
body = body,
imageUriString = imageUriString,
imageMimeType = imageMimeType,
threadId = threadId,
roomName = roomName,
roomIsDm = roomIsDm,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ class NotificationBroadcastReceiverHandler @Inject constructor(
?: stringProvider.getString(R.string.notification_sender_me),
body = message,
imageUriString = null,
imageMimeType = null,
threadId = threadId,
roomName = room.displayName,
roomIsDm = room.isDm,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,22 @@ class DefaultNotificationCreator @Inject constructor(
senderPerson
).also { message ->
event.imageUri?.let {
message.setData("image/", it)
message.setData(event.imageMimeType ?: "image/", it)
}
message.extras.putString(MESSAGE_EVENT_ID, event.eventId.value)
}
addMessage(message)

// Add additional message for captions
if (event.imageUri != null && event.body != null) {
addMessage(
MessagingStyle.Message(
event.body,
event.timestamp,
senderPerson,
)
)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ data class NotifiableMessageEvent(
val body: String?,
// We cannot use Uri? type here, as that could trigger a
// NotSerializableException when persisting this to storage
val imageUriString: String?,
private val imageUriString: String?,
Comment on lines 32 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for this PR, but since we're no longer persisting these (I think) we might be able to use Uri again.

Copy link
Member Author

Choose a reason for hiding this comment

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

A yes, true!

val imageMimeType: String?,
val threadId: ThreadId?,
val roomName: String?,
val roomIsDm: Boolean = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ class DefaultNotifiableEventResolverTest {
senderDisambiguatedDisplayName = A_USER_NAME_2,
body = "Call in progress (unsupported)",
imageUriString = null,
imageMimeType = null,
threadId = null,
roomName = A_ROOM_NAME,
roomAvatarPath = null,
Expand Down Expand Up @@ -669,6 +670,7 @@ class DefaultNotifiableEventResolverTest {
canBeReplaced = false,
isRedacted = false,
imageUriString = null,
imageMimeType = null,
type = EventType.CALL_NOTIFY,
)
)
Expand Down Expand Up @@ -704,6 +706,7 @@ class DefaultNotifiableEventResolverTest {
canBeReplaced = false,
isRedacted = false,
imageUriString = null,
imageMimeType = null,
type = EventType.CALL_NOTIFY,
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ fun aNotifiableMessageEvent(
canBeReplaced = false,
isRedacted = isRedacted,
imageUriString = null,
imageMimeType = null,
roomAvatarPath = null,
senderAvatarPath = null,
soundName = null,
Expand Down
Loading