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

Iteration on caption #3816

Merged
merged 6 commits into from
Nov 6, 2024
Merged

Iteration on caption #3816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd481d3
Add a message in the notification for the caption. Fixes #2602
bmarty Nov 4, 2024
64b189c
Notification with image: provide the mimetype if available.
bmarty Nov 4, 2024
c04bd64
Fix wrong description.
bmarty Nov 4, 2024
f00478d
Format file...
bmarty Nov 4, 2024
461c314
Improve last message formatter: add caption (or filename)
bmarty Nov 4, 2024
f2c63a5
Do not render filename of voice message.
bmarty Nov 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
14 changes: 14 additions & 0 deletions ...in/io/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ class DefaultNotifiableEventResolver @Inject constructor(
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 @@ class DefaultNotifiableEventResolver @Inject constructor(
}
.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
}
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 @@ internal fun buildNotifiableMessageEvent(
// 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 @@ internal fun buildNotifiableMessageEvent(
senderDisambiguatedDisplayName = senderDisambiguatedDisplayName,
body = body,
imageUriString = imageUriString,
imageMimeType = imageMimeType,
threadId = threadId,
roomName = roomName,
roomIsDm = roomIsDm,
Expand Down
1 change: 1 addition & 0 deletions ...element/android/libraries/push/impl/notifications/NotificationBroadcastReceiverHandler.kt
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion ...lin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ 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)
}
Expand Down
3 changes: 2 additions & 1 deletion ...tlin/io/element/android/libraries/push/impl/notifications/model/NotifiableMessageEvent.kt
View file
Open in desktop
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
3 changes: 3 additions & 0 deletions ...o/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolverTest.kt
View file
Open in desktop
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
1 change: 1 addition & 0 deletions ...n/io/element/android/libraries/push/impl/notifications/fixtures/NotifiableEventFixture.kt
View file
Open in desktop
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