Pinned events : use correct ordering logic

Author: ganfra

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenter.kt

@@ -53,10 +53,10 @@ class PinnedMessagesBannerPresenter @Inject constructor(
     override fun present(): PinnedMessagesBannerState {
         val isFeatureEnabled by featureFlagService.isFeatureEnabledFlow(FeatureFlags.PinnedEvents).collectAsState(initial = false)
         var hasTimelineFailedToLoad by rememberSaveable { mutableStateOf(false) }
-        var currentPinnedMessageIndex by rememberSaveable { mutableIntStateOf(0) }
+        var currentPinnedMessageIndex by rememberSaveable { mutableIntStateOf(-1) }
         val knownPinnedMessagesCount by remember {
             room.roomInfoFlow.map { roomInfo -> roomInfo.pinnedEventIds.size }
-        }.collectAsState(initial = null)
+        }.collectAsState(initial = 0)
 
         var pinnedItems by remember {
             mutableStateOf<ImmutableList<PinnedMessagesBannerItem>>(persistentListOf())
@@ -66,8 +66,8 @@ class PinnedMessagesBannerPresenter @Inject constructor(
             isFeatureEnabled = isFeatureEnabled,
             onItemsChange = { newItems ->
                 val pinnedMessageCount = newItems.size
-                if (currentPinnedMessageIndex >= pinnedMessageCount) {
-                    currentPinnedMessageIndex = 0
+                if (currentPinnedMessageIndex >= pinnedMessageCount || currentPinnedMessageIndex < 0) {
+                    currentPinnedMessageIndex = pinnedMessageCount - 1
                 }
                 pinnedItems = newItems
             },
@@ -102,7 +102,7 @@ class PinnedMessagesBannerPresenter @Inject constructor(
     private fun pinnedMessagesBannerState(
         isFeatureEnabled: Boolean,
         hasTimelineFailed: Boolean,
-        realPinnedMessagesCount: Int?,
+        realPinnedMessagesCount: Int,
         pinnedItems: ImmutableList<PinnedMessagesBannerItem>,
         currentPinnedMessageIndex: Int,
         eventSink: (PinnedMessagesBannerEvents) -> Unit
@@ -111,7 +111,7 @@ class PinnedMessagesBannerPresenter @Inject constructor(
         return when {
             !isFeatureEnabled -> PinnedMessagesBannerState.Hidden
             hasTimelineFailed -> PinnedMessagesBannerState.Hidden
-            realPinnedMessagesCount == null || realPinnedMessagesCount == 0 -> PinnedMessagesBannerState.Hidden
+            realPinnedMessagesCount == 0 -> PinnedMessagesBannerState.Hidden
             currentPinnedMessage == null -> PinnedMessagesBannerState.Loading(realPinnedMessagesCount = realPinnedMessagesCount)
             else -> {
                 PinnedMessagesBannerState.Loaded(

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerState.kt

@@ -26,13 +26,14 @@ import io.element.android.libraries.ui.strings.CommonStrings
 @Immutable
 sealed interface PinnedMessagesBannerState {
     data object Hidden : PinnedMessagesBannerState
-    data class Loading(val realPinnedMessagesCount: Int) : PinnedMessagesBannerState
+    sealed interface Visible : PinnedMessagesBannerState
+    data class Loading(val realPinnedMessagesCount: Int) : Visible
     data class Loaded(
         val currentPinnedMessage: PinnedMessagesBannerItem,
         val currentPinnedMessageIndex: Int,
         val knownPinnedMessagesCount: Int,
         val eventSink: (PinnedMessagesBannerEvents) -> Unit
-    ) : PinnedMessagesBannerState
+    ) : Visible
 
     fun pinnedMessagesCount() = when (this) {
         is Hidden -> 0
@@ -42,7 +43,7 @@ sealed interface PinnedMessagesBannerState {
 
     fun currentPinnedMessageIndex() = when (this) {
         is Hidden -> 0
-        is Loading -> 0
+        is Loading -> realPinnedMessagesCount - 1
         is Loaded -> currentPinnedMessageIndex
     }
 

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerView.kt

@@ -74,22 +74,11 @@ fun PinnedMessagesBannerView(
     Box(modifier = modifier) {
         when (state) {
             PinnedMessagesBannerState.Hidden -> Unit
-            is PinnedMessagesBannerState.Loading -> {
+            is PinnedMessagesBannerState.Visible -> {
                 PinnedMessagesBannerRow(
                     state = state,
+                    onClick = onClick,
                     onViewAllClick = onViewAllClick,
-                    modifier = Modifier.clickable(onClick = { }),
-                )
-            }
-            is PinnedMessagesBannerState.Loaded -> {
-                PinnedMessagesBannerRow(
-                    state = state,
-                    onViewAllClick = onViewAllClick,
-                    modifier = Modifier.clickable(
-                        onClick = {
-                            onClick(state.currentPinnedMessage.eventId)
-                            state.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned)
-                        }),
                 )
             }
         }
@@ -99,6 +88,7 @@ fun PinnedMessagesBannerView(
 @Composable
 fun PinnedMessagesBannerRow(
     state: PinnedMessagesBannerState,
+    onClick: (EventId) -> Unit,
     onViewAllClick: () -> Unit,
     modifier: Modifier = Modifier,
 ) {
@@ -108,7 +98,13 @@ fun PinnedMessagesBannerRow(
             .background(color = ElementTheme.colors.bgCanvasDefault)
             .fillMaxWidth()
             .drawBorder(borderColor)
-            .heightIn(min = 64.dp),
+            .heightIn(min = 64.dp)
+            .clickable {
+                if (state is PinnedMessagesBannerState.Loaded) {
+                    onClick(state.currentPinnedMessage.eventId)
+                    state.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned)
+                }
+            },
         verticalAlignment = Alignment.CenterVertically,
         horizontalArrangement = spacedBy(10.dp)
     ) {
@@ -202,7 +198,6 @@ private fun PinIndicators(
         state = lazyListState,
         verticalArrangement = spacedBy(2.dp),
         userScrollEnabled = false,
-        reverseLayout = true
     ) {
         items(pinsCount) { index ->
             Box(

features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt

@@ -140,25 +140,25 @@ class PinnedMessagesBannerPresenterTest {
             skipItems(2)
             awaitItem().also { loadedState ->
                 loadedState as PinnedMessagesBannerState.Loaded
-                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(0)
+                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(1)
                 assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2)
-                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent1.toString())
+                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent2.toString())
                 loadedState.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned)
             }
 
             awaitItem().also { loadedState ->
                 loadedState as PinnedMessagesBannerState.Loaded
-                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(1)
+                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(0)
                 assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2)
-                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent2.toString())
+                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent1.toString())
                 loadedState.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned)
             }
 
             awaitItem().also { loadedState ->
                 loadedState as PinnedMessagesBannerState.Loaded
-                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(0)
+                assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(1)
                 assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2)
-                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent1.toString())
+                assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent2.toString())
             }
         }
     }