From f3d48355f6c0a88762390cd77a7fca7efc40f0f3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 5 Jul 2024 12:00:04 +0200 Subject: [PATCH 1/2] Add Konsist test to ensure that we invoke callback method on all the Callback instances. --- .../tests/konsist/KonsistCallbackTest.kt | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistCallbackTest.kt diff --git a/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistCallbackTest.kt b/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistCallbackTest.kt new file mode 100644 index 0000000000..4937595ef9 --- /dev/null +++ b/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistCallbackTest.kt @@ -0,0 +1,33 @@ +/* + * Copyright (c) 2024 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.tests.konsist + +import com.lemonappdev.konsist.api.Konsist +import com.lemonappdev.konsist.api.verify.assertFalse +import org.junit.Test + +class KonsistCallbackTest { + @Test + fun `we should not invoke Callback Input directly, we should use forEach`() { + Konsist + .scopeFromProduction() + .files + .assertFalse { + it.text.contains("callback?.") + } + } +} From 27e4b5d82f69d704ab44c551175716c3a648bce0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 5 Jul 2024 12:13:21 +0200 Subject: [PATCH 2/2] Ensure that all the callback instances are invoked. --- .../messages/impl/MessagesFlowNode.kt | 11 +++--- .../features/messages/impl/MessagesNode.kt | 34 +++++++++++-------- .../securebackup/impl/SecureBackupFlowNode.kt | 6 ++-- 3 files changed, 29 insertions(+), 22 deletions(-) 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 132b8020da..3999a5fe0c 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 @@ -107,6 +107,7 @@ class MessagesFlowNode @AssistedInject constructor( plugins = plugins ) { data class Inputs(val focusedEventId: EventId?) : NodeInputs + private val inputs = inputs() sealed interface NavTarget : Parcelable { @@ -148,7 +149,7 @@ class MessagesFlowNode @AssistedInject constructor( data class EditPoll(val eventId: EventId) : NavTarget } - private val callback = plugins().firstOrNull() + private val callbacks = plugins() private val mentionSpanProvider = mentionSpanProviderFactory.create(room.sessionId.value) @@ -167,7 +168,7 @@ class MessagesFlowNode @AssistedInject constructor( is NavTarget.Messages -> { val callback = object : MessagesNode.Callback { override fun onRoomDetailsClick() { - callback?.onRoomDetailsClick() + callbacks.forEach { it.onRoomDetailsClick() } } override fun onEventClick(event: TimelineItem.Event): Boolean { @@ -179,11 +180,11 @@ class MessagesFlowNode @AssistedInject constructor( } override fun onUserDataClick(userId: UserId) { - callback?.onUserDataClick(userId) + callbacks.forEach { it.onUserDataClick(userId) } } override fun onPermalinkClick(data: PermalinkData) { - callback?.onPermalinkClick(data) + callbacks.forEach { it.onPermalinkClick(data) } } override fun onShowEventDebugInfoClick(eventId: EventId?, debugInfo: TimelineItemDebugInfo) { @@ -250,7 +251,7 @@ class MessagesFlowNode @AssistedInject constructor( val inputs = ForwardMessagesNode.Inputs(navTarget.eventId) val callback = object : ForwardMessagesNode.Callback { override fun onForwardedToSingleRoom(roomId: RoomId) { - this@MessagesFlowNode.callback?.onForwardedToSingleRoom(roomId) + callbacks.forEach { it.onForwardedToSingleRoom(roomId) } } } createNode(buildContext, listOf(inputs, callback)) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt index d1e3f87fe3..b2ee1053a6 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesNode.kt @@ -78,7 +78,7 @@ class MessagesNode @AssistedInject constructor( private val timelineController: TimelineController, ) : Node(buildContext, plugins = plugins), MessagesNavigator { private val presenter = presenterFactory.create(this) - private val callback = plugins().firstOrNull() + private val callbacks = plugins() data class Inputs(val focusedEventId: EventId?) : NodeInputs @@ -113,19 +113,25 @@ class MessagesNode @AssistedInject constructor( } private fun onRoomDetailsClick() { - callback?.onRoomDetailsClick() + callbacks.forEach { it.onRoomDetailsClick() } } private fun onEventClick(event: TimelineItem.Event): Boolean { - return callback?.onEventClick(event).orFalse() + // Note: cannot use `callbacks.all { it.onEventClick(event) }` because: + // - if callbacks is empty, it will return true and we want to return false. + // - if a callback returns false, the other callback will not be invoked. + return callbacks.takeIf { it.isNotEmpty() } + ?.map { it.onEventClick(event) } + ?.all { it } + .orFalse() } private fun onPreviewAttachments(attachments: ImmutableList) { - callback?.onPreviewAttachments(attachments) + callbacks.forEach { it.onPreviewAttachments(attachments) } } private fun onUserDataClick(userId: UserId) { - callback?.onUserDataClick(userId) + callbacks.forEach { it.onUserDataClick(userId) } } private fun onLinkClick( @@ -137,7 +143,7 @@ class MessagesNode @AssistedInject constructor( is PermalinkData.UserLink -> { // Open the room member profile, it will fallback to // the user profile if the user is not in the room - callback?.onUserDataClick(permalink.userId) + callbacks.forEach { it.onUserDataClick(permalink.userId) } } is PermalinkData.RoomLink -> { handleRoomLinkClick(permalink, eventSink) @@ -159,36 +165,36 @@ class MessagesNode @AssistedInject constructor( context.toast("Already viewing this room!") } } else { - callback?.onPermalinkClick(roomLink) + callbacks.forEach { it.onPermalinkClick(roomLink) } } } override fun onShowEventDebugInfoClick(eventId: EventId?, debugInfo: TimelineItemDebugInfo) { - callback?.onShowEventDebugInfoClick(eventId, debugInfo) + callbacks.forEach { it.onShowEventDebugInfoClick(eventId, debugInfo) } } override fun onForwardEventClick(eventId: EventId) { - callback?.onForwardEventClick(eventId) + callbacks.forEach { it.onForwardEventClick(eventId) } } override fun onReportContentClick(eventId: EventId, senderId: UserId) { - callback?.onReportMessage(eventId, senderId) + callbacks.forEach { it.onReportMessage(eventId, senderId) } } override fun onEditPollClick(eventId: EventId) { - callback?.onEditPollClick(eventId) + callbacks.forEach { it.onEditPollClick(eventId) } } private fun onSendLocationClick() { - callback?.onSendLocationClick() + callbacks.forEach { it.onSendLocationClick() } } private fun onCreatePollClick() { - callback?.onCreatePollClick() + callbacks.forEach { it.onCreatePollClick() } } private fun onJoinCallClick() { - callback?.onJoinCallClick(room.roomId) + callbacks.forEach { it.onJoinCallClick(room.roomId) } } @Composable diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/SecureBackupFlowNode.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/SecureBackupFlowNode.kt index e03aecad43..f54bfaee96 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/SecureBackupFlowNode.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/SecureBackupFlowNode.kt @@ -81,7 +81,7 @@ class SecureBackupFlowNode @AssistedInject constructor( data object CreateNewRecoveryKey : NavTarget } - private val callback = plugins().firstOrNull() + private val callbacks = plugins() override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node { return when (navTarget) { @@ -130,8 +130,8 @@ class SecureBackupFlowNode @AssistedInject constructor( NavTarget.EnterRecoveryKey -> { val callback = object : SecureBackupEnterRecoveryKeyNode.Callback { override fun onEnterRecoveryKeySuccess() { - if (callback != null) { - callback.onDone() + if (callbacks.isNotEmpty()) { + callbacks.forEach { it.onDone() } } else { backstack.pop() }