From 6bf1deb99bd75818b9620dd8ec83771181251178 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 10 May 2019 12:14:40 +0200 Subject: [PATCH] Code Review --- .../room/timeline/DefaultTimelineService.kt | 4 +- .../matrix/android/internal/util/Monarchy.kt | 2 +- .../core/utils/DebouncedClickListener.kt | 3 +- .../home/room/detail/RoomDetailFragment.kt | 10 +- .../timeline/TimelineEventController.kt | 4 +- .../action/MessageActionsBottomSheet.kt | 4 +- .../timeline/action/QuickReactionFragment.kt | 42 ++++---- .../timeline/factory/MessageItemFactory.kt | 71 +++++++------- .../timeline/item/MessageInformationData.kt | 1 - .../adapter_item_action_quick_reaction.xml | 95 +++++++------------ vector/src/main/res/values/strings.xml | 6 -- vector/src/main/res/values/strings_riotX.xml | 6 ++ 12 files changed, 102 insertions(+), 146 deletions(-) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/timeline/DefaultTimelineService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/timeline/DefaultTimelineService.kt index 4ca41ba7..8b949721 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/timeline/DefaultTimelineService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/room/timeline/DefaultTimelineService.kt @@ -23,7 +23,7 @@ import im.vector.matrix.android.api.session.room.timeline.TimelineService import im.vector.matrix.android.internal.database.model.EventEntity import im.vector.matrix.android.internal.database.query.where import im.vector.matrix.android.internal.task.TaskExecutor -import im.vector.matrix.android.internal.util.fetchMappedCopied +import im.vector.matrix.android.internal.util.fetchCopyMap internal class DefaultTimelineService(private val roomId: String, private val monarchy: Monarchy, @@ -38,7 +38,7 @@ internal class DefaultTimelineService(private val roomId: String, } override fun getTimeLineEvent(eventId: String): TimelineEvent? { - return monarchy.fetchMappedCopied({ + return monarchy.fetchCopyMap({ EventEntity.where(it, eventId = eventId).findFirst() }, { entity, realm -> timelineEventFactory.create(entity, realm) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/Monarchy.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/Monarchy.kt index 572b8434..6b547db1 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/Monarchy.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/util/Monarchy.kt @@ -42,7 +42,7 @@ fun Monarchy.fetchCopied(query: (Realm) -> T?): T? { return fetch(query, true) } -fun Monarchy.fetchMappedCopied(query: (Realm) -> T?, map: (T, realm: Realm) -> U): U? { +fun Monarchy.fetchCopyMap(query: (Realm) -> T?, map: (T, realm: Realm) -> U): U? { val ref = AtomicReference() doWithRealm { realm -> val result = query.invoke(realm)?.let { diff --git a/vector/src/main/java/im/vector/riotredesign/core/utils/DebouncedClickListener.kt b/vector/src/main/java/im/vector/riotredesign/core/utils/DebouncedClickListener.kt index dbfbe4fe..c2f47b92 100644 --- a/vector/src/main/java/im/vector/riotredesign/core/utils/DebouncedClickListener.kt +++ b/vector/src/main/java/im/vector/riotredesign/core/utils/DebouncedClickListener.kt @@ -1,6 +1,7 @@ package im.vector.riotredesign.core.utils import android.view.View +import java.util.* /** @@ -8,7 +9,7 @@ import android.view.View * Safe to use in different views */ class DebouncedClickListener(val original: View.OnClickListener, private val minimumInterval: Long = 400) : View.OnClickListener { - private val lastClickMap = HashMap() + private val lastClickMap = WeakHashMap() override fun onClick(clickedView: View) { val previousClickTimestamp = lastClickMap[clickedView] diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/RoomDetailFragment.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/RoomDetailFragment.kt index 2168c03e..e6a7f902 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/RoomDetailFragment.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/RoomDetailFragment.kt @@ -133,7 +133,7 @@ class RoomDetailFragment : override fun getLayoutResId() = R.layout.fragment_room_detail - lateinit var actionViewModel: ActionsHandler + private lateinit var actionViewModel: ActionsHandler override fun onActivityCreated(savedInstanceState: Bundle?) { super.onActivityCreated(savedInstanceState) @@ -425,11 +425,11 @@ class RoomDetailFragment : vectorBaseActivity.notImplemented() } - override fun onEventCellClicked(eventId: String, informationData: MessageInformationData, messageContent: MessageContent, view: View) { + override fun onEventCellClicked(informationData: MessageInformationData, messageContent: MessageContent, view: View) { } - override fun onEventLongClicked(eventId: String, informationData: MessageInformationData, messageContent: MessageContent, view: View): Boolean { + override fun onEventLongClicked(informationData: MessageInformationData, messageContent: MessageContent, view: View): Boolean { view.performHapticFeedback(HapticFeedbackConstants.LONG_PRESS) val roomId = (arguments?.get(MvRx.KEY_ARG) as? RoomDetailArgs)?.roomId if (roomId.isNullOrBlank()) { @@ -437,7 +437,7 @@ class RoomDetailFragment : return false } MessageActionsBottomSheet - .newInstance(eventId, roomId, informationData) + .newInstance(roomId, informationData) .show(requireActivity().supportFragmentManager, "MESSAGE_CONTEXTUAL_ACTIONS") return true } @@ -478,7 +478,7 @@ class RoomDetailFragment : override fun onSuccess(image: File?) { if (image != null) - shareMedia(requireContext(), image!!, "image/*") + shareMedia(requireContext(), image, "image/*") } override fun onFail(error: Exception?) {} diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/TimelineEventController.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/TimelineEventController.kt index 32b2f873..4f480203 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/TimelineEventController.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/TimelineEventController.kt @@ -54,8 +54,8 @@ class TimelineEventController(private val dateFormatter: TimelineDateFormatter, fun onVideoMessageClicked(messageVideoContent: MessageVideoContent, mediaData: VideoContentRenderer.Data, view: View) fun onFileMessageClicked(messageFileContent: MessageFileContent) fun onAudioMessageClicked(messageAudioContent: MessageAudioContent) - fun onEventCellClicked(eventId: String, informationData: MessageInformationData, messageContent: MessageContent, view: View) - fun onEventLongClicked(eventId: String, informationData: MessageInformationData, messageContent: MessageContent, view: View): Boolean + fun onEventCellClicked(informationData: MessageInformationData, messageContent: MessageContent, view: View) + fun onEventLongClicked(informationData: MessageInformationData, messageContent: MessageContent, view: View): Boolean fun onAvatarClicked(informationData: MessageInformationData) } diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/MessageActionsBottomSheet.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/MessageActionsBottomSheet.kt index 7f1609d9..320332ba 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/MessageActionsBottomSheet.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/MessageActionsBottomSheet.kt @@ -143,10 +143,10 @@ class MessageActionsBottomSheet : BaseMvRxBottomSheetDialog() { ) : Parcelable companion object { - fun newInstance(eventId: String, roomId: String, informationData: MessageInformationData): MessageActionsBottomSheet { + fun newInstance(roomId: String, informationData: MessageInformationData): MessageActionsBottomSheet { val args = Bundle() val parcelableArgs = ParcelableArgs( - eventId, + informationData.eventId, roomId, informationData ) diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/QuickReactionFragment.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/QuickReactionFragment.kt index 1e3a7b6b..a8593a2e 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/QuickReactionFragment.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/action/QuickReactionFragment.kt @@ -39,16 +39,6 @@ class QuickReactionFragment : BaseMvRxFragment() { @BindView(R.id.root_layout) lateinit var rootLayout: ConstraintLayout - - @BindView(R.id.quick_react_1) - lateinit var quickReact1: View - @BindView(R.id.quick_react_2) - lateinit var quickReact2: View - @BindView(R.id.quick_react_3) - lateinit var quickReact3: View - @BindView(R.id.quick_react_4) - lateinit var quickReact4: View - @BindView(R.id.quick_react_1_text) lateinit var quickReact1Text: TextView @@ -78,16 +68,16 @@ class QuickReactionFragment : BaseMvRxFragment() { quickReact4Text.text = viewModel.likeNegative //configure click listeners - quickReact1.setOnClickListener { + quickReact1Text.setOnClickListener { viewModel.toggleAgree(true) } - quickReact2.setOnClickListener { + quickReact2Text.setOnClickListener { viewModel.toggleAgree(false) } - quickReact3.setOnClickListener { + quickReact3Text.setOnClickListener { viewModel.toggleLike(true) } - quickReact4.setOnClickListener { + quickReact4Text.setOnClickListener { viewModel.toggleLike(false) } @@ -98,32 +88,32 @@ class QuickReactionFragment : BaseMvRxFragment() { TransitionManager.beginDelayedTransition(rootLayout) when (it.agreeTrigleState) { TriggleState.NONE -> { - quickReact1.alpha = 1f - quickReact2.alpha = 1f + quickReact1Text.alpha = 1f + quickReact2Text.alpha = 1f } TriggleState.FIRST -> { - quickReact1.alpha = 1f - quickReact2.alpha = 0.2f + quickReact1Text.alpha = 1f + quickReact2Text.alpha = 0.2f } TriggleState.SECOND -> { - quickReact1.alpha = 0.2f - quickReact2.alpha = 1f + quickReact1Text.alpha = 0.2f + quickReact2Text.alpha = 1f } } when (it.likeTriggleState) { TriggleState.NONE -> { - quickReact3.alpha = 1f - quickReact4.alpha = 1f + quickReact3Text.alpha = 1f + quickReact4Text.alpha = 1f } TriggleState.FIRST -> { - quickReact3.alpha = 1f - quickReact4.alpha = 0.2f + quickReact3Text.alpha = 1f + quickReact4Text.alpha = 0.2f } TriggleState.SECOND -> { - quickReact3.alpha = 0.2f - quickReact4.alpha = 1f + quickReact3Text.alpha = 0.2f + quickReact4Text.alpha = 1f } } diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/factory/MessageItemFactory.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/factory/MessageItemFactory.kt index 2dc3db94..f7849ff1 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/factory/MessageItemFactory.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/factory/MessageItemFactory.kt @@ -83,19 +83,18 @@ class MessageItemFactory(private val colorProvider: ColorProvider, // val all = event.root.toContent() // val ev = all.toModel() return when (messageContent) { - is MessageEmoteContent -> buildEmoteMessageItem(eventId, messageContent, informationData, callback) - is MessageTextContent -> buildTextMessageItem(eventId, event.sendState, messageContent, informationData, callback) - is MessageImageContent -> buildImageMessageItem(eventId, messageContent, informationData, callback) - is MessageNoticeContent -> buildNoticeMessageItem(eventId, messageContent, informationData, callback) - is MessageVideoContent -> buildVideoMessageItem(eventId, messageContent, informationData, callback) - is MessageFileContent -> buildFileMessageItem(eventId, messageContent, informationData, callback) - is MessageAudioContent -> buildAudioMessageItem(eventId, messageContent, informationData, callback) + is MessageEmoteContent -> buildEmoteMessageItem(messageContent, informationData, callback) + is MessageTextContent -> buildTextMessageItem(event.sendState, messageContent, informationData, callback) + is MessageImageContent -> buildImageMessageItem(messageContent, informationData, callback) + is MessageNoticeContent -> buildNoticeMessageItem(messageContent, informationData, callback) + is MessageVideoContent -> buildVideoMessageItem(messageContent, informationData, callback) + is MessageFileContent -> buildFileMessageItem(messageContent, informationData, callback) + is MessageAudioContent -> buildAudioMessageItem(messageContent, informationData, callback) else -> buildNotHandledMessageItem(messageContent) } } - private fun buildAudioMessageItem(eventId: String, messageContent: MessageAudioContent, - informationData: MessageInformationData, + private fun buildAudioMessageItem(messageContent: MessageAudioContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageFileItem? { return MessageFileItem_() .informationData(informationData) @@ -107,20 +106,19 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .clickListener( DebouncedClickListener(View.OnClickListener { _ -> callback?.onAudioMessageClicked(messageContent) })) .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } - private fun buildFileMessageItem(eventId: String, messageContent: MessageFileContent, - informationData: MessageInformationData, + private fun buildFileMessageItem(messageContent: MessageFileContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageFileItem? { return MessageFileItem_() .informationData(informationData) @@ -132,16 +130,16 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) + .longClickListener { view -> + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) + ?: false + } .clickListener( DebouncedClickListener(View.OnClickListener { _ -> callback?.onFileMessageClicked(messageContent) })) - .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) - ?: false - } } private fun buildNotHandledMessageItem(messageContent: MessageContent): DefaultItem? { @@ -149,8 +147,7 @@ class MessageItemFactory(private val colorProvider: ColorProvider, return DefaultItem_().text(text) } - private fun buildImageMessageItem(eventId: String, messageContent: MessageImageContent, - informationData: MessageInformationData, + private fun buildImageMessageItem(messageContent: MessageImageContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageImageVideoItem? { val (maxWidth, maxHeight) = timelineMediaSizeProvider.getMaxSize() @@ -178,17 +175,16 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } - private fun buildVideoMessageItem(eventId: String, messageContent: MessageVideoContent, - informationData: MessageInformationData, + private fun buildVideoMessageItem(messageContent: MessageVideoContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageImageVideoItem? { val (maxWidth, maxHeight) = timelineMediaSizeProvider.getMaxSize() @@ -217,17 +213,16 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .clickListener { view -> callback?.onVideoMessageClicked(messageContent, videoData, view) } .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } - private fun buildTextMessageItem(eventId: String, sendState: SendState, - messageContent: MessageTextContent, + private fun buildTextMessageItem(sendState: SendState, messageContent: MessageTextContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageTextItem? { @@ -246,20 +241,19 @@ class MessageItemFactory(private val colorProvider: ColorProvider, //click on the text .clickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } - private fun buildNoticeMessageItem(eventId: String, messageContent: MessageNoticeContent, - informationData: MessageInformationData, + private fun buildNoticeMessageItem(messageContent: MessageNoticeContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageTextItem? { val message = messageContent.body.let { @@ -279,16 +273,15 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } - private fun buildEmoteMessageItem(eventId: String, messageContent: MessageEmoteContent, - informationData: MessageInformationData, + private fun buildEmoteMessageItem(messageContent: MessageEmoteContent, informationData: MessageInformationData, callback: TimelineEventController.Callback?): MessageTextItem? { val message = messageContent.body.let { @@ -304,10 +297,10 @@ class MessageItemFactory(private val colorProvider: ColorProvider, })) .cellClickListener( DebouncedClickListener(View.OnClickListener { view -> - callback?.onEventCellClicked(eventId, informationData, messageContent, view) + callback?.onEventCellClicked(informationData, messageContent, view) })) .longClickListener { view -> - return@longClickListener callback?.onEventLongClicked(eventId, informationData, messageContent, view) + return@longClickListener callback?.onEventLongClicked(informationData, messageContent, view) ?: false } } diff --git a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/item/MessageInformationData.kt b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/item/MessageInformationData.kt index 34b5dcd6..cf763803 100644 --- a/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/item/MessageInformationData.kt +++ b/vector/src/main/java/im/vector/riotredesign/features/home/room/detail/timeline/item/MessageInformationData.kt @@ -29,6 +29,5 @@ data class MessageInformationData( val time: CharSequence? = null, val avatarUrl: String?, val memberName: CharSequence? = null, - val userId: String? = null, val showInformation: Boolean = true ) : Parcelable \ No newline at end of file diff --git a/vector/src/main/res/layout/adapter_item_action_quick_reaction.xml b/vector/src/main/res/layout/adapter_item_action_quick_reaction.xml index 6bdcc7dc..ba39f58c 100644 --- a/vector/src/main/res/layout/adapter_item_action_quick_reaction.xml +++ b/vector/src/main/res/layout/adapter_item_action_quick_reaction.xml @@ -2,39 +2,31 @@ - - + app:layout_constraintVertical_chainStyle="packed" + tools:text="👍" /> - - - - - app:layout_constraintStart_toEndOf="@id/quick_react_1" - app:layout_constraintTop_toTopOf="@id/quick_react_1"> - - - + app:layout_constraintTop_toBottomOf="@id/quick_react_1_text" /> - + app:layout_constraintTop_toTopOf="@id/quick_react_1_text" + tools:text="😀" /> - - - - - - - + app:layout_constraintStart_toEndOf="@id/quick_react_3_text" + app:layout_constraintTop_toTopOf="@id/quick_react_3_text" + tools:text="😞" /> Riot detected a custom server configuration for your userId domain \"%s\":\n%s Use Config - - Reactions - Agree - Like - Add Reaction - diff --git a/vector/src/main/res/values/strings_riotX.xml b/vector/src/main/res/values/strings_riotX.xml index 97af4f71..d5b91853 100644 --- a/vector/src/main/res/values/strings_riotX.xml +++ b/vector/src/main/res/values/strings_riotX.xml @@ -5,4 +5,10 @@ "Retry" "Join a room to start using the app." + + Reactions + Agree + Like + Add Reaction + \ No newline at end of file