From 23b8573ad47e9715fed1257a1e2bbfa5e8eadef1 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Wed, 23 Aug 2023 17:46:55 +0200 Subject: [PATCH 1/6] Improve timestamp rendering for poll event content --- ...melineItemEventForTimestampViewProvider.kt | 1 + .../components/TimelineItemEventRow.kt | 167 ++++++++++-------- .../timeline/components/TimestampPosition.kt | 34 ++++ 3 files changed, 132 insertions(+), 70 deletions(-) create mode 100644 features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt index 7697ccf4a4..c52e49c512 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt @@ -35,5 +35,6 @@ class TimelineItemEventForTimestampViewProvider : PreviewParameterProvider Unit, modifier: Modifier = Modifier ) { - val isMediaItem = event.content is TimelineItemImageContent - || event.content is TimelineItemVideoContent - || event.content is TimelineItemLocationContent + val timestampPosition = when (event.content) { + is TimelineItemImageContent, + is TimelineItemVideoContent, + is TimelineItemLocationContent -> TimestampPosition.Above + is TimelineItemPollContent -> TimestampPosition.Below + else -> TimestampPosition.Aligned + } val replyToDetails = event.inReplyTo as? InReplyTo.Ready // Long clicks are not not automatically propagated from a `clickable` @@ -384,96 +390,97 @@ private fun MessageEventBubbleContent( @Composable fun ContentAndTimestampView( - overlayTimestamp: Boolean, + timestampPosition: TimestampPosition, modifier: Modifier = Modifier, contentModifier: Modifier = Modifier, timestampModifier: Modifier = Modifier, ) { - if (overlayTimestamp) { - Box(modifier) { - ContentView(modifier = contentModifier) - TimelineEventTimestampView( - event = event, - onClick = onTimestampClicked, - onLongClick = ::onTimestampLongClick, - modifier = timestampModifier - .padding(horizontal = 4.dp, vertical = 4.dp) // Outer padding - .background(ElementTheme.colors.bgSubtleSecondary, RoundedCornerShape(10.0.dp)) - .align(Alignment.BottomEnd) - .padding(horizontal = 4.dp, vertical = 2.dp) // Inner padding - ) - } - } else { - Box(modifier) { - ContentView(modifier = contentModifier) - TimelineEventTimestampView( - event = event, - onClick = onTimestampClicked, - onLongClick = ::onTimestampLongClick, - modifier = timestampModifier - .align(Alignment.BottomEnd) - .padding(horizontal = 8.dp, vertical = 4.dp) - ) - } + when (timestampPosition) { + TimestampPosition.Above -> + Box(modifier) { + ContentView(modifier = contentModifier) + TimelineEventTimestampView( + event = event, + onClick = onTimestampClicked, + onLongClick = ::onTimestampLongClick, + modifier = timestampModifier + .padding(horizontal = 4.dp, vertical = 4.dp) // Outer padding + .background(ElementTheme.colors.bgSubtleSecondary, RoundedCornerShape(10.0.dp)) + .align(Alignment.BottomEnd) + .padding(horizontal = 4.dp, vertical = 2.dp) // Inner padding + ) + } + TimestampPosition.Aligned -> + Box(modifier) { + ContentView(modifier = contentModifier) + TimelineEventTimestampView( + event = event, + onClick = onTimestampClicked, + onLongClick = ::onTimestampLongClick, + modifier = timestampModifier + .align(Alignment.BottomEnd) + .padding(horizontal = 8.dp, vertical = 4.dp) + ) + } + TimestampPosition.Below -> + Column(modifier) { + ContentView(modifier = contentModifier) + TimelineEventTimestampView( + event = event, + onClick = onTimestampClicked, + onLongClick = ::onTimestampLongClick, + modifier = timestampModifier + .align(Alignment.End) + .padding(horizontal = 8.dp, vertical = 4.dp) + ) + } } } - /** Used only for media items, with no reply to metadata. It displays the contents with no paddings. */ - @Composable - fun SimpleMediaItemLayout(modifier: Modifier = Modifier) { - ContentAndTimestampView(overlayTimestamp = true, modifier = modifier) - } - - /** Used for every other type of message, groups the different components in a Column with some space between them. */ + /** Groups the different components in a Column with some space between them. */ @Composable fun CommonLayout( inReplyToDetails: InReplyTo.Ready?, modifier: Modifier = Modifier ) { + var modifierWithPadding: Modifier = Modifier + var contentModifier: Modifier = Modifier EqualWidthColumn(modifier = modifier, spacing = 8.dp) { - if (inReplyToDetails != null) { - val senderName = inReplyToDetails.senderDisplayName ?: inReplyToDetails.senderId.value - val attachmentThumbnailInfo = attachmentThumbnailInfoForInReplyTo(inReplyToDetails) - val text = textForInReplyTo(inReplyToDetails) - ReplyToContent( - senderName = senderName, - text = text, - attachmentThumbnailInfo = attachmentThumbnailInfo, - modifier = Modifier - .padding(top = 8.dp, start = 8.dp, end = 8.dp) - .clip(RoundedCornerShape(6.dp)) - .clickable(enabled = true, onClick = inReplyToClick), - ) - } - val modifierWithPadding = if (isMediaItem) { - Modifier.padding(start = 8.dp, end = 8.dp, bottom = 8.dp) - } else { - Modifier - } - - val contentModifier = if (isMediaItem) { - Modifier.clip(RoundedCornerShape(12.dp)) - } else { - if (inReplyToDetails != null) { - Modifier.padding(start = 12.dp, end = 12.dp, top = 0.dp, bottom = 8.dp) - } else { - Modifier.padding(start = 12.dp, end = 12.dp, top = 8.dp, bottom = 8.dp) + when { + inReplyToDetails != null -> { + val senderName = inReplyToDetails.senderDisplayName ?: inReplyToDetails.senderId.value + val attachmentThumbnailInfo = attachmentThumbnailInfoForInReplyTo(inReplyToDetails) + val text = textForInReplyTo(inReplyToDetails) + ReplyToContent( + senderName = senderName, + text = text, + attachmentThumbnailInfo = attachmentThumbnailInfo, + modifier = Modifier + .padding(top = 8.dp, start = 8.dp, end = 8.dp) + .clip(RoundedCornerShape(6.dp)) + .clickable(enabled = true, onClick = inReplyToClick), + ) + if (timestampPosition == TimestampPosition.Above) { + modifierWithPadding = Modifier.padding(start = 8.dp, end = 8.dp, bottom = 8.dp) + contentModifier = Modifier.clip(RoundedCornerShape(12.dp)) + } else { + contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 0.dp, bottom = 8.dp) + } + } + timestampPosition != TimestampPosition.Above -> { + contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 8.dp, bottom = 8.dp) } } ContentAndTimestampView( - overlayTimestamp = isMediaItem, + timestampPosition = timestampPosition, contentModifier = contentModifier, modifier = modifierWithPadding, ) } } - if (isMediaItem && replyToDetails == null) { - SimpleMediaItemLayout() - } else { - CommonLayout(inReplyToDetails = replyToDetails, modifier = modifier) - } + CommonLayout(inReplyToDetails = replyToDetails, modifier = modifier) } @Composable @@ -810,3 +817,23 @@ internal fun TimelineItemEventRowLongSenderNamePreview() = ElementPreviewLight { onTimestampClicked = {}, ) } + +// Note: no need for light/dark variant for this preview, we only look at the timestamp position +@Preview +@Composable +internal fun TimelineItemEventTimestampBelowPreview() = ElementPreviewLight { + TimelineItemEventRow( + event = aTimelineItemEvent(content = aTimelineItemPollContent()), + isHighlighted = false, + canReply = true, + onClick = {}, + onLongClick = {}, + onUserDataClick = {}, + inReplyToClick = {}, + onReactionClick = { _, _ -> }, + onReactionLongClick = { _, _ -> }, + onMoreReactionsClick = {}, + onSwipeToReply = {}, + onTimestampClicked = {}, + ) +} diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt new file mode 100644 index 0000000000..cb1a201f81 --- /dev/null +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.features.messages.impl.timeline.components + +enum class TimestampPosition { + /** + * Timestamp should be rendered above the timeline event content (eg. image). + */ + Above, + + /** + * Timestamp should be aligned with the timeline event content if this is possible (eg. text). + */ + Aligned, + + /** + * Timestamp should always be rendered below the timeline event content (eg. poll). + */ + Below, +} From 3c8cf0ef675706ee094d1d573a1e0ab093942e40 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Thu, 24 Aug 2023 11:38:14 +0200 Subject: [PATCH 2/6] Add default timestamp position --- .../impl/timeline/components/TimelineItemEventRow.kt | 2 +- .../impl/timeline/components/TimestampPosition.kt | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt index 2794c2b898..0fd05e4717 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt @@ -366,7 +366,7 @@ private fun MessageEventBubbleContent( is TimelineItemVideoContent, is TimelineItemLocationContent -> TimestampPosition.Above is TimelineItemPollContent -> TimestampPosition.Below - else -> TimestampPosition.Aligned + else -> TimestampPosition.Default } val replyToDetails = event.inReplyTo as? InReplyTo.Ready diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt index cb1a201f81..aaae767dbc 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt @@ -30,5 +30,12 @@ enum class TimestampPosition { /** * Timestamp should always be rendered below the timeline event content (eg. poll). */ - Below, + Below; + + companion object { + /** + * Default timestamp position for timeline event contents. + */ + val Default: TimestampPosition = Aligned + } } From 8aebb940060ebd8e2829b6cfa81aa1aea26b8814 Mon Sep 17 00:00:00 2001 From: ElementBot Date: Thu, 24 Aug 2023 10:13:05 +0000 Subject: [PATCH 3/6] Update screenshots --- ..._TimelineItemEventTimestampBelow_0_null,NEXUS_5,1.0,en].png | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.timeline.components_null_TimelineItemEventTimestampBelow_0_null,NEXUS_5,1.0,en].png diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.timeline.components_null_TimelineItemEventTimestampBelow_0_null,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.timeline.components_null_TimelineItemEventTimestampBelow_0_null,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..3db42b6e0c --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.timeline.components_null_TimelineItemEventTimestampBelow_0_null,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:5af81e5f2081b949673c5f08282b5165eb7df4bb970c1db485376c86543e1a96 +size 57313 From 875c38a1916e6a8a90f041c553febd3c62f21dfe Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 25 Aug 2023 09:30:05 +0200 Subject: [PATCH 4/6] Rename TimestampPosition.Above to Overlay --- .../impl/timeline/components/TimelineItemEventRow.kt | 8 ++++---- .../impl/timeline/components/TimestampPosition.kt | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt index 0fd05e4717..90e286030d 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt @@ -364,7 +364,7 @@ private fun MessageEventBubbleContent( val timestampPosition = when (event.content) { is TimelineItemImageContent, is TimelineItemVideoContent, - is TimelineItemLocationContent -> TimestampPosition.Above + is TimelineItemLocationContent -> TimestampPosition.Overlay is TimelineItemPollContent -> TimestampPosition.Below else -> TimestampPosition.Default } @@ -396,7 +396,7 @@ private fun MessageEventBubbleContent( timestampModifier: Modifier = Modifier, ) { when (timestampPosition) { - TimestampPosition.Above -> + TimestampPosition.Overlay -> Box(modifier) { ContentView(modifier = contentModifier) TimelineEventTimestampView( @@ -460,14 +460,14 @@ private fun MessageEventBubbleContent( .clip(RoundedCornerShape(6.dp)) .clickable(enabled = true, onClick = inReplyToClick), ) - if (timestampPosition == TimestampPosition.Above) { + if (timestampPosition == TimestampPosition.Overlay) { modifierWithPadding = Modifier.padding(start = 8.dp, end = 8.dp, bottom = 8.dp) contentModifier = Modifier.clip(RoundedCornerShape(12.dp)) } else { contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 0.dp, bottom = 8.dp) } } - timestampPosition != TimestampPosition.Above -> { + timestampPosition != TimestampPosition.Overlay -> { contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 8.dp, bottom = 8.dp) } } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt index aaae767dbc..7063e07635 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt @@ -18,9 +18,9 @@ package io.element.android.features.messages.impl.timeline.components enum class TimestampPosition { /** - * Timestamp should be rendered above the timeline event content (eg. image). + * Timestamp should overlay the timeline event content (eg. image). */ - Above, + Overlay, /** * Timestamp should be aligned with the timeline event content if this is possible (eg. text). From 4eb49383e1b0be9cb1bfb19e2edb4f0a2ced7a1e Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 25 Aug 2023 09:36:21 +0200 Subject: [PATCH 5/6] Rename modifier --- .../impl/timeline/components/TimelineItemEventRow.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt index 90e286030d..449b6ee104 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt @@ -351,6 +351,7 @@ private fun MessageSenderInformation( } } +@Suppress("") @Composable private fun MessageEventBubbleContent( event: TimelineItem.Event, @@ -359,7 +360,7 @@ private fun MessageEventBubbleContent( onMessageLongClick: () -> Unit, inReplyToClick: () -> Unit, onTimestampClicked: () -> Unit, - modifier: Modifier = Modifier + bubbleModifier: Modifier = Modifier ) { val timestampPosition = when (event.content) { is TimelineItemImageContent, @@ -480,7 +481,7 @@ private fun MessageEventBubbleContent( } } - CommonLayout(inReplyToDetails = replyToDetails, modifier = modifier) + CommonLayout(inReplyToDetails = replyToDetails, modifier = bubbleModifier) } @Composable From a77571a85bc5dc01b3c61ac6b70cb9ad1ace7951 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Fri, 25 Aug 2023 10:12:21 +0200 Subject: [PATCH 6/6] cleanup --- .../components/TimelineItemEventForTimestampViewProvider.kt | 1 - .../messages/impl/timeline/components/TimelineItemEventRow.kt | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt index c52e49c512..7697ccf4a4 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventForTimestampViewProvider.kt @@ -35,6 +35,5 @@ class TimelineItemEventForTimestampViewProvider : PreviewParameterProvider Unit, inReplyToClick: () -> Unit, onTimestampClicked: () -> Unit, - bubbleModifier: Modifier = Modifier + @SuppressLint("ModifierParameter") bubbleModifier: Modifier = Modifier, // need to rename this modifier to distinguish it from the following ones ) { val timestampPosition = when (event.content) { is TimelineItemImageContent,