Merge pull request #1141 from vector-im/bugfix/fre/improve_timestamp_position

Improve timestamp rendering for poll event content

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt

@@ -16,6 +16,7 @@
 
 package io.element.android.features.messages.impl.timeline.components
 
+import android.annotation.SuppressLint
 import androidx.compose.foundation.Canvas
 import androidx.compose.foundation.background
 import androidx.compose.foundation.clickable
@@ -68,9 +69,11 @@ import io.element.android.features.messages.impl.timeline.model.TimelineItemGrou
 import io.element.android.features.messages.impl.timeline.model.bubble.BubbleState
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemImageContent
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemLocationContent
+import io.element.android.features.messages.impl.timeline.model.event.TimelineItemPollContent
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemTextContent
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemVideoContent
 import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemImageContent
+import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemPollContent
 import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemTextContent
 import io.element.android.libraries.designsystem.components.EqualWidthColumn
 import io.element.android.libraries.designsystem.components.avatar.Avatar
@@ -357,11 +360,15 @@ private fun MessageEventBubbleContent(
     onMessageLongClick: () -> Unit,
     inReplyToClick: () -> Unit,
     onTimestampClicked: () -> Unit,
-    modifier: Modifier = Modifier
+    @SuppressLint("ModifierParameter") bubbleModifier: Modifier = Modifier, // need to rename this modifier to distinguish it from the following ones
 ) {
-    val isMediaItem = event.content is TimelineItemImageContent
+    val timestampPosition = when (event.content) {
-        || event.content is TimelineItemVideoContent
+        is TimelineItemImageContent,
-        || event.content is TimelineItemLocationContent
+        is TimelineItemVideoContent,
+        is TimelineItemLocationContent -> TimestampPosition.Overlay
+        is TimelineItemPollContent -> TimestampPosition.Below
+        else -> TimestampPosition.Default
+    }
     val replyToDetails = event.inReplyTo as? InReplyTo.Ready
 
     // Long clicks are not not automatically propagated from a `clickable`
@@ -384,96 +391,97 @@ private fun MessageEventBubbleContent(
 
     @Composable
     fun ContentAndTimestampView(
-        overlayTimestamp: Boolean,
+        timestampPosition: TimestampPosition,
         modifier: Modifier = Modifier,
         contentModifier: Modifier = Modifier,
         timestampModifier: Modifier = Modifier,
     ) {
-        if (overlayTimestamp) {
+        when (timestampPosition) {
-            Box(modifier) {
+            TimestampPosition.Overlay ->
-                ContentView(modifier = contentModifier)
+                Box(modifier) {
-                TimelineEventTimestampView(
+                    ContentView(modifier = contentModifier)
-                    event = event,
+                    TimelineEventTimestampView(
-                    onClick = onTimestampClicked,
+                        event = event,
-                    onLongClick = ::onTimestampLongClick,
+                        onClick = onTimestampClicked,
-                    modifier = timestampModifier
+                        onLongClick = ::onTimestampLongClick,
-                        .padding(horizontal = 4.dp, vertical = 4.dp) // Outer padding
+                        modifier = timestampModifier
-                        .background(ElementTheme.colors.bgSubtleSecondary, RoundedCornerShape(10.0.dp))
+                            .padding(horizontal = 4.dp, vertical = 4.dp) // Outer padding
-                        .align(Alignment.BottomEnd)
+                            .background(ElementTheme.colors.bgSubtleSecondary, RoundedCornerShape(10.0.dp))
-                        .padding(horizontal = 4.dp, vertical = 2.dp) // Inner padding
+                            .align(Alignment.BottomEnd)
-                )
+                            .padding(horizontal = 4.dp, vertical = 2.dp) // Inner padding
-            }
+                    )
-        } else {
+                }
-            Box(modifier) {
+            TimestampPosition.Aligned ->
-                ContentView(modifier = contentModifier)
+                Box(modifier) {
-                TimelineEventTimestampView(
+                    ContentView(modifier = contentModifier)
-                    event = event,
+                    TimelineEventTimestampView(
-                    onClick = onTimestampClicked,
+                        event = event,
-                    onLongClick = ::onTimestampLongClick,
+                        onClick = onTimestampClicked,
-                    modifier = timestampModifier
+                        onLongClick = ::onTimestampLongClick,
-                        .align(Alignment.BottomEnd)
+                        modifier = timestampModifier
-                        .padding(horizontal = 8.dp, vertical = 4.dp)
+                            .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. */
+    /** Groups the different components in a Column with some space between them. */
-    @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. */
     @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) {
+            when {
-                val senderName = inReplyToDetails.senderDisplayName ?: inReplyToDetails.senderId.value
+                inReplyToDetails != null -> {
-                val attachmentThumbnailInfo = attachmentThumbnailInfoForInReplyTo(inReplyToDetails)
+                    val senderName = inReplyToDetails.senderDisplayName ?: inReplyToDetails.senderId.value
-                val text = textForInReplyTo(inReplyToDetails)
+                    val attachmentThumbnailInfo = attachmentThumbnailInfoForInReplyTo(inReplyToDetails)
-                ReplyToContent(
+                    val text = textForInReplyTo(inReplyToDetails)
-                    senderName = senderName,
+                    ReplyToContent(
-                    text = text,
+                        senderName = senderName,
-                    attachmentThumbnailInfo = attachmentThumbnailInfo,
+                        text = text,
-                    modifier = Modifier
+                        attachmentThumbnailInfo = attachmentThumbnailInfo,
-                        .padding(top = 8.dp, start = 8.dp, end = 8.dp)
+                        modifier = Modifier
-                        .clip(RoundedCornerShape(6.dp))
+                            .padding(top = 8.dp, start = 8.dp, end = 8.dp)
-                        .clickable(enabled = true, onClick = inReplyToClick),
+                            .clip(RoundedCornerShape(6.dp))
-                )
+                            .clickable(enabled = true, onClick = inReplyToClick),
-            }
+                    )
-            val modifierWithPadding = if (isMediaItem) {
+                    if (timestampPosition == TimestampPosition.Overlay) {
-                Modifier.padding(start = 8.dp, end = 8.dp, bottom = 8.dp)
+                        modifierWithPadding = Modifier.padding(start = 8.dp, end = 8.dp, bottom = 8.dp)
-            } else {
+                        contentModifier = Modifier.clip(RoundedCornerShape(12.dp))
-                Modifier
+                    } else {
-            }
+                        contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 0.dp, bottom = 8.dp)
-
+                    }
-            val contentModifier = if (isMediaItem) {
+                }
-                Modifier.clip(RoundedCornerShape(12.dp))
+                timestampPosition != TimestampPosition.Overlay -> {
-            } else {
+                    contentModifier = Modifier.padding(start = 12.dp, end = 12.dp, top = 8.dp, bottom = 8.dp)
-                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)
                 }
             }
 
             ContentAndTimestampView(
-                overlayTimestamp = isMediaItem,
+                timestampPosition = timestampPosition,
                 contentModifier = contentModifier,
                 modifier = modifierWithPadding,
             )
         }
     }
 
-    if (isMediaItem && replyToDetails == null) {
+    CommonLayout(inReplyToDetails = replyToDetails, modifier = bubbleModifier)
-        SimpleMediaItemLayout()
-    } else {
-        CommonLayout(inReplyToDetails = replyToDetails, modifier = modifier)
-    }
 }
 
 @Composable
@@ -810,3 +818,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 = {},
+    )
+}

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/TimestampPosition.kt

@@ -0,0 +1,41 @@
+/*
+ * 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 overlay the timeline event content (eg. image).
+     */
+    Overlay,
+
+    /**
+     * 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;
+
+    companion object {
+        /**
+         * Default timestamp position for timeline event contents.
+         */
+        val Default: TimestampPosition = Aligned
+    }
+}

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