From a37a6d249f2b7e6dde48236b3c4015eb5539ae58 Mon Sep 17 00:00:00 2001 From: ganfra Date: Thu, 5 Sep 2024 17:36:16 +0200 Subject: [PATCH] Pinned messages list : improve and fix code after PR review. --- .../android/appnav/LoggedInFlowNode.kt | 7 +-- .../messages/api/MessagesEntryPoint.kt | 2 +- .../messages/impl/MessagesFlowNode.kt | 2 +- .../list/PinnedMessagesListPresenter.kt | 2 +- .../list/PinnedMessagesListStateProvider.kt | 49 ++++++++++++++++++- ...MessagesListTimelineActionPostProcessor.kt | 17 ++----- .../list/PinnedMessagesListPresenterTest.kt | 22 +++++++-- .../libraries/matrix/api/timeline/Timeline.kt | 2 +- .../matrix/impl/room/RustMatrixRoom.kt | 2 +- .../matrix/impl/timeline/RustTimeline.kt | 2 +- .../RoomBeginningPostProcessor.kt | 2 +- 11 files changed, 80 insertions(+), 29 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index d218d49dee..1d1fa2bea6 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -36,7 +36,6 @@ import com.bumble.appyx.core.plugin.plugins import com.bumble.appyx.navmodel.backstack.BackStack import com.bumble.appyx.navmodel.backstack.operation.push import com.bumble.appyx.navmodel.backstack.operation.replace -import com.bumble.appyx.navmodel.backstack.operation.singleTop import dagger.assisted.Assisted import dagger.assisted.AssistedInject import im.vector.app.features.analytics.plan.JoinedRoom @@ -86,6 +85,7 @@ import kotlinx.coroutines.launch import kotlinx.parcelize.Parcelize import timber.log.Timber import java.util.Optional +import java.util.UUID @ContributesNode(SessionScope::class) class LoggedInFlowNode @AssistedInject constructor( @@ -204,7 +204,8 @@ class LoggedInFlowNode @AssistedInject constructor( val serverNames: List = emptyList(), val trigger: JoinedRoom.Trigger? = null, val roomDescription: RoomDescription? = null, - val initialElement: RoomNavigationTarget = RoomNavigationTarget.Messages() + val initialElement: RoomNavigationTarget = RoomNavigationTarget.Messages(), + val targetId: UUID = UUID.randomUUID(), ) : NavTarget @Parcelize @@ -311,7 +312,7 @@ class LoggedInFlowNode @AssistedInject constructor( if (pushToBackstack) { backstack.push(target) } else { - backstack.singleTop(target) + backstack.replace(target) } } is PermalinkData.FallbackLink, diff --git a/features/messages/api/src/main/kotlin/io/element/android/features/messages/api/MessagesEntryPoint.kt b/features/messages/api/src/main/kotlin/io/element/android/features/messages/api/MessagesEntryPoint.kt index 1bd1bfe756..219faae6b3 100644 --- a/features/messages/api/src/main/kotlin/io/element/android/features/messages/api/MessagesEntryPoint.kt +++ b/features/messages/api/src/main/kotlin/io/element/android/features/messages/api/MessagesEntryPoint.kt @@ -46,7 +46,7 @@ interface MessagesEntryPoint : FeatureEntryPoint { interface Callback : Plugin { fun onRoomDetailsClick() fun onUserDataClick(userId: UserId) - fun onPermalinkClick(data: PermalinkData, pushToBackstack: Boolean = true) + fun onPermalinkClick(data: PermalinkData, pushToBackstack: Boolean) fun onForwardedToSingleRoom(roomId: RoomId) } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesFlowNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesFlowNode.kt index 78832fa101..1f1b3f9b5d 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesFlowNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesFlowNode.kt @@ -194,7 +194,7 @@ class MessagesFlowNode @AssistedInject constructor( } override fun onPermalinkClick(data: PermalinkData) { - callbacks.forEach { it.onPermalinkClick(data) } + callbacks.forEach { it.onPermalinkClick(data, pushToBackstack = true) } } override fun onShowEventDebugInfoClick(eventId: EventId?, debugInfo: TimelineItemDebugInfo) { diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenter.kt index 6c9ad5e4c9..c2e1a3baca 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenter.kt @@ -74,7 +74,7 @@ class PinnedMessagesListPresenter @AssistedInject constructor( fun create(navigator: PinnedMessagesListNavigator): PinnedMessagesListPresenter } - private val actionListPresenter = actionListPresenterFactory.create(PinnedMessagesListTimelineActionPostProcessor) + private val actionListPresenter = actionListPresenterFactory.create(PinnedMessagesListTimelineActionPostProcessor()) @Composable override fun present(): PinnedMessagesListState { diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt index c98d3d0b75..eff0ebf961 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListStateProvider.kt @@ -21,10 +21,17 @@ import io.element.android.features.messages.impl.UserEventPermissions import io.element.android.features.messages.impl.actionlist.ActionListState import io.element.android.features.messages.impl.actionlist.anActionListState import io.element.android.features.messages.impl.timeline.TimelineRoomInfo -import io.element.android.features.messages.impl.timeline.aTimelineItemList +import io.element.android.features.messages.impl.timeline.aTimelineItemDaySeparator +import io.element.android.features.messages.impl.timeline.aTimelineItemEvent +import io.element.android.features.messages.impl.timeline.aTimelineItemReactions import io.element.android.features.messages.impl.timeline.aTimelineRoomInfo import io.element.android.features.messages.impl.timeline.model.TimelineItem +import io.element.android.features.messages.impl.timeline.model.TimelineItemGroupPosition +import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemAudioContent +import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemFileContent +import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemPollContent import io.element.android.features.messages.impl.timeline.model.event.aTimelineItemTextContent +import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList open class PinnedMessagesListStateProvider : PreviewParameterProvider { @@ -34,7 +41,45 @@ open class PinnedMessagesListStateProvider : PreviewParameterProvider): List { return buildList { add(TimelineItemAction.ViewInTimeline) - addAll(actions.filter(::predicate)) - } - } - - private fun predicate(action: TimelineItemAction): Boolean { - return when (action) { - is TimelineItemAction.Unpin, - is TimelineItemAction.Redact, - is TimelineItemAction.Forward, - is TimelineItemAction.ViewSource -> true - else -> false + actions.firstOrNull { it is TimelineItemAction.Unpin }?.let(::add) + actions.firstOrNull { it is TimelineItemAction.Forward }?.let(::add) + actions.firstOrNull { it is TimelineItemAction.ViewSource }?.let(::add) + actions.firstOrNull { it is TimelineItemAction.Redact }?.let(::add) } } } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt index 44a4563720..6f927123d8 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/pinned/list/PinnedMessagesListPresenterTest.kt @@ -33,6 +33,7 @@ import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem import io.element.android.libraries.matrix.api.timeline.item.TimelineItemDebugInfo import io.element.android.libraries.matrix.test.AN_EVENT_ID +import io.element.android.libraries.matrix.test.A_THROWABLE import io.element.android.libraries.matrix.test.A_UNIQUE_ID import io.element.android.libraries.matrix.test.room.FakeMatrixRoom import io.element.android.libraries.matrix.test.room.aRoomInfo @@ -172,10 +173,10 @@ class PinnedMessagesListPresenterTest { @Test fun `present - unpin event`() = runTest { - val unpinEventLambda = lambdaRecorder { _: EventId? -> Result.success(true) } - val pinnedEventsTimeline = createPinnedMessagesTimeline().apply { - this.unpinEventLambda = unpinEventLambda - } + val successUnpinEventLambda = lambdaRecorder { _: EventId? -> Result.success(true) } + val failureUnpinEventLambda = lambdaRecorder { _: EventId? -> Result.failure(A_THROWABLE) } + val pinnedEventsTimeline = createPinnedMessagesTimeline() + val room = FakeMatrixRoom( pinnedEventsTimelineResult = { Result.success(pinnedEventsTimeline) }, canRedactOwnResult = { Result.success(true) }, @@ -189,9 +190,20 @@ class PinnedMessagesListPresenterTest { skipItems(3) val filledState = awaitItem() as PinnedMessagesListState.Filled val eventItem = filledState.timelineItems.first() as TimelineItem.Event + + pinnedEventsTimeline.unpinEventLambda = successUnpinEventLambda filledState.eventSink(PinnedMessagesListEvents.HandleAction(TimelineItemAction.Unpin, eventItem)) + + pinnedEventsTimeline.unpinEventLambda = failureUnpinEventLambda + filledState.eventSink(PinnedMessagesListEvents.HandleAction(TimelineItemAction.Unpin, eventItem)) + cancelAndIgnoreRemainingEvents() - assert(unpinEventLambda) + + assert(successUnpinEventLambda) + .isCalledOnce() + .with(value(AN_EVENT_ID)) + + assert(failureUnpinEventLambda) .isCalledOnce() .with(value(AN_EVENT_ID)) } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt index 2eea2a6c2f..b4bc54d825 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt @@ -50,7 +50,7 @@ interface Timeline : AutoCloseable { enum class Mode { LIVE, FOCUSED_ON_EVENT, - FOCUSED_ON_PINNED_EVENTS + PINNED_EVENTS } val membershipChangeEventReceived: Flow diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt index 1430ad8025..abd1442c11 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt @@ -199,7 +199,7 @@ class RustMatrixRoom( internalIdPrefix = "pinned_events", maxEventsToLoad = 100u, ).let { inner -> - createTimeline(inner, mode = Timeline.Mode.FOCUSED_ON_PINNED_EVENTS) + createTimeline(inner, mode = Timeline.Mode.PINNED_EVENTS) } }.onFailure { if (it is CancellationException) { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt index c2320d6a25..58a66c4db4 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt @@ -137,7 +137,7 @@ class RustTimeline( private val lastForwardIndicatorsPostProcessor = LastForwardIndicatorsPostProcessor(mode) private val backPaginationStatus = MutableStateFlow( - Timeline.PaginationStatus(isPaginating = false, hasMoreToLoad = mode != Timeline.Mode.FOCUSED_ON_PINNED_EVENTS) + Timeline.PaginationStatus(isPaginating = false, hasMoreToLoad = mode != Timeline.Mode.PINNED_EVENTS) ) private val forwardPaginationStatus = MutableStateFlow( diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/RoomBeginningPostProcessor.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/RoomBeginningPostProcessor.kt index 2e90da88b5..fa53de6d2a 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/RoomBeginningPostProcessor.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/RoomBeginningPostProcessor.kt @@ -37,7 +37,7 @@ class RoomBeginningPostProcessor(private val mode: Timeline.Mode) { hasMoreToLoadBackwards: Boolean ): List { return when { - mode == Timeline.Mode.FOCUSED_ON_PINNED_EVENTS -> items + mode == Timeline.Mode.PINNED_EVENTS -> items hasMoreToLoadBackwards -> items isDm -> processForDM(items) else -> processForRoom(items)