From a7bf0b4900bb22d6fbf1e9845ac1e33f1ca61a5d Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 6 Aug 2024 19:05:08 +0200 Subject: [PATCH] Pinned events : use correct ordering logic --- .../banner/PinnedMessagesBannerPresenter.kt | 12 ++++----- .../banner/PinnedMessagesBannerState.kt | 7 +++--- .../pinned/banner/PinnedMessagesBannerView.kt | 25 ++++++++----------- .../PinnedMessagesBannerPresenterTest.kt | 16 ++++++------ 4 files changed, 28 insertions(+), 32 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenter.kt index 6604114fc5..b04b6e656a 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenter.kt +++ b/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>(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, 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( diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerState.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerState.kt index 886cef81de..c06003cc7c 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerState.kt +++ b/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 } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerView.kt index 34b531ff55..3a8fa448ef 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerView.kt +++ b/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( diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt index 33e1b74aba..6e78b2c446 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/banner/PinnedMessagesBannerPresenterTest.kt @@ -138,14 +138,6 @@ class PinnedMessagesBannerPresenterTest { val presenter = createPinnedMessagesBannerPresenter(room = room) presenter.test { skipItems(2) - awaitItem().also { loadedState -> - loadedState as PinnedMessagesBannerState.Loaded - assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(0) - assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2) - assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent1.toString()) - loadedState.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned) - } - awaitItem().also { loadedState -> loadedState as PinnedMessagesBannerState.Loaded assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(1) @@ -159,6 +151,14 @@ class PinnedMessagesBannerPresenterTest { assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(0) assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2) assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent1.toString()) + loadedState.eventSink(PinnedMessagesBannerEvents.MoveToNextPinned) + } + + awaitItem().also { loadedState -> + loadedState as PinnedMessagesBannerState.Loaded + assertThat(loadedState.currentPinnedMessageIndex).isEqualTo(1) + assertThat(loadedState.knownPinnedMessagesCount).isEqualTo(2) + assertThat(loadedState.currentPinnedMessage.formatted.text).isEqualTo(messageContent2.toString()) } } }