From 80366ee93801a420d791251018baa132f977184f Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 10 May 2021 14:38:36 +0200 Subject: [PATCH] Code review --- .../database/mapper/RoomSummaryMapper.kt | 1 - .../im/vector/app/core/extensions/EditText.kt | 16 ++++++++ .../app/features/form/FormEditTextItem.kt | 10 ++--- .../form/FormEditTextWithButtonItem.kt | 9 ++--- .../form/FormMultiLineEditTextItem.kt | 10 ++--- .../createroom/RoomAliasEditItem.kt | 8 +--- .../settings/RoomSettingsController.kt | 1 - .../settings/RoomSettingsViewState.kt | 36 +++++++++--------- .../manage/SpaceChildInfoMatchFilter.kt | 37 +++++++++++++++++++ .../spaces/manage/SpaceManageActivity.kt | 20 ++++++---- .../manage/SpaceManageRoomViewEvents.kt | 4 +- .../manage/SpaceManageRoomsController.kt | 18 --------- .../spaces/manage/SpaceManageRoomsFragment.kt | 7 +--- .../manage/SpaceManageRoomsViewModel.kt | 23 +++++------- .../spaces/manage/SpaceSettingsController.kt | 2 +- .../spaces/manage/SpaceSettingsFragment.kt | 4 +- vector/src/main/res/values/styles_riot.xml | 2 +- 17 files changed, 111 insertions(+), 97 deletions(-) create mode 100644 vector/src/main/java/im/vector/app/features/spaces/manage/SpaceChildInfoMatchFilter.kt diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomSummaryMapper.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomSummaryMapper.kt index d52e1f995b..fbecbf37be 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomSummaryMapper.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomSummaryMapper.kt @@ -90,7 +90,6 @@ internal class RoomSummaryMapper @Inject constructor(private val timelineEventMa autoJoin = it.autoJoin ?: false, viaServers = it.viaServers.toList(), parentRoomId = roomSummaryEntity.roomId, - // What to do here? suggested = it.suggested ) }, diff --git a/vector/src/main/java/im/vector/app/core/extensions/EditText.kt b/vector/src/main/java/im/vector/app/core/extensions/EditText.kt index 05b70def3d..e65dc4d2c6 100644 --- a/vector/src/main/java/im/vector/app/core/extensions/EditText.kt +++ b/vector/src/main/java/im/vector/app/core/extensions/EditText.kt @@ -23,7 +23,9 @@ import android.view.View import android.view.inputmethod.EditorInfo import android.widget.EditText import androidx.annotation.DrawableRes +import com.google.android.material.textfield.TextInputEditText import im.vector.app.R +import im.vector.app.core.epoxy.VectorEpoxyHolder import im.vector.app.core.platform.SimpleTextWatcher fun EditText.setupAsSearch(@DrawableRes searchIconRes: Int = R.drawable.ic_search, @@ -57,3 +59,17 @@ fun EditText.setupAsSearch(@DrawableRes searchIconRes: Int = R.drawable.ic_searc return@OnTouchListener false }) } + +/** + * Set the initial value of the textEdit. + * Avoids issue with two way bindings, the value is only set the first time + */ +fun TextInputEditText.setValueOnce(value: String?, holder: VectorEpoxyHolder) { + if (holder.view.isAttachedToWindow) { + // the view is attached to the window + // So it is a rebind of new data and you could ignore it assuming this is text that was already inputted into the view. + // Downside is if you ever wanted to programmatically change the content of the edit text while it is on screen you would not be able to + } else { + setText(value) + } +} diff --git a/vector/src/main/java/im/vector/app/features/form/FormEditTextItem.kt b/vector/src/main/java/im/vector/app/features/form/FormEditTextItem.kt index 74f088d739..9ab939945c 100644 --- a/vector/src/main/java/im/vector/app/features/form/FormEditTextItem.kt +++ b/vector/src/main/java/im/vector/app/features/form/FormEditTextItem.kt @@ -27,6 +27,7 @@ import com.google.android.material.textfield.TextInputLayout import im.vector.app.R import im.vector.app.core.epoxy.VectorEpoxyHolder import im.vector.app.core.epoxy.VectorEpoxyModel +import im.vector.app.core.extensions.setValueOnce import im.vector.app.core.platform.SimpleTextWatcher @EpoxyModelClass(layout = R.layout.item_form_text_input) @@ -75,13 +76,8 @@ abstract class FormEditTextItem : VectorEpoxyModel() { holder.textInputLayout.error = errorMessage holder.textInputLayout.endIconMode = endIconMode ?: TextInputLayout.END_ICON_NONE - if (holder.view.isAttachedToWindow) { - // the view is attached to the window - // So it is a rebind of new data and you could ignore it assuming this is text that was already inputted into the view. - // Downside is if you ever wanted to programmatically change the content of the edit text while it is on screen you would not be able to - } else { - holder.textInputEditText.setText(value) - } + holder.textInputEditText.setValueOnce(value, holder) + holder.textInputEditText.isEnabled = enabled inputType?.let { holder.textInputEditText.inputType = it } holder.textInputEditText.isSingleLine = singleLine ?: false diff --git a/vector/src/main/java/im/vector/app/features/form/FormEditTextWithButtonItem.kt b/vector/src/main/java/im/vector/app/features/form/FormEditTextWithButtonItem.kt index 227e93d2d4..ef3ee2a371 100644 --- a/vector/src/main/java/im/vector/app/features/form/FormEditTextWithButtonItem.kt +++ b/vector/src/main/java/im/vector/app/features/form/FormEditTextWithButtonItem.kt @@ -26,6 +26,7 @@ import com.google.android.material.textfield.TextInputLayout import im.vector.app.R import im.vector.app.core.epoxy.VectorEpoxyHolder import im.vector.app.core.epoxy.VectorEpoxyModel +import im.vector.app.core.extensions.setValueOnce import im.vector.app.core.platform.SimpleTextWatcher @EpoxyModelClass(layout = R.layout.item_form_text_input_with_button) @@ -60,12 +61,8 @@ abstract class FormEditTextWithButtonItem : VectorEpoxyModel() holder.textInputLayout.isEnabled = enabled holder.textInputLayout.error = errorMessage - if (holder.view.isAttachedToWindow) { - // the view is attached to the window - // So it is a rebind of new data and you could ignore it assuming this is text that was already inputted into the view. - } else { - holder.textInputEditText.setText(value) - } + holder.textInputEditText.setValueOnce(value, holder) holder.textInputEditText.isEnabled = enabled holder.textInputEditText.addTextChangedListener(onTextChangeListener) holder.homeServerText.text = homeServer diff --git a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsController.kt b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsController.kt index a2a8fcb0c2..b12a6d52eb 100644 --- a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsController.kt +++ b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsController.kt @@ -27,7 +27,6 @@ import im.vector.app.features.form.formEditableAvatarItem import im.vector.app.features.form.formSwitchItem import im.vector.app.features.home.AvatarRenderer import im.vector.app.features.home.room.detail.timeline.format.RoomHistoryVisibilityFormatter -import im.vector.app.features.roomprofile.settings.RoomSettingsViewState.Companion.getJoinRuleWording import im.vector.app.features.settings.VectorPreferences import org.matrix.android.sdk.api.session.room.model.GuestAccess import org.matrix.android.sdk.api.session.room.model.RoomJoinRules diff --git a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewState.kt b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewState.kt index f1931e4add..bb97acde61 100644 --- a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewState.kt +++ b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewState.kt @@ -70,26 +70,24 @@ data class RoomSettingsViewState( ) { fun hasChanged() = newJoinRules != null || newGuestAccess != null } +} - companion object { - fun RoomSettingsViewState.getJoinRuleWording(stringProvider: StringProvider): String { - return when (val joinRule = newRoomJoinRules.newJoinRules ?: currentRoomJoinRules) { - RoomJoinRules.INVITE -> { - stringProvider.getString(R.string.room_settings_room_access_private_title) - } - RoomJoinRules.PUBLIC -> { - stringProvider.getString(R.string.room_settings_room_access_public_title) - } - RoomJoinRules.KNOCK -> { - stringProvider.getString(R.string.room_settings_room_access_entry_knock) - } - RoomJoinRules.RESTRICTED -> { - stringProvider.getString(R.string.room_settings_room_access_restricted_title) - } - else -> { - stringProvider.getString(R.string.room_settings_room_access_entry_unknown, joinRule.value) - } - } +fun RoomSettingsViewState.getJoinRuleWording(stringProvider: StringProvider): String { + return when (val joinRule = newRoomJoinRules.newJoinRules ?: currentRoomJoinRules) { + RoomJoinRules.INVITE -> { + stringProvider.getString(R.string.room_settings_room_access_private_title) + } + RoomJoinRules.PUBLIC -> { + stringProvider.getString(R.string.room_settings_room_access_public_title) + } + RoomJoinRules.KNOCK -> { + stringProvider.getString(R.string.room_settings_room_access_entry_knock) + } + RoomJoinRules.RESTRICTED -> { + stringProvider.getString(R.string.room_settings_room_access_restricted_title) + } + else -> { + stringProvider.getString(R.string.room_settings_room_access_entry_unknown, joinRule.value) } } } diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceChildInfoMatchFilter.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceChildInfoMatchFilter.kt new file mode 100644 index 0000000000..17f5d13e19 --- /dev/null +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceChildInfoMatchFilter.kt @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2021 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 im.vector.app.features.spaces.manage + +import io.reactivex.functions.Predicate +import org.matrix.android.sdk.api.extensions.orFalse +import org.matrix.android.sdk.api.session.room.model.SpaceChildInfo + +class SpaceChildInfoMatchFilter : Predicate { + var filter: String = "" + + override fun test(spaceChildInfo: SpaceChildInfo): Boolean { + if (filter.isEmpty()) { + // No filter + return true + } + // if filter is "Jo Do", it should match "John Doe" + return filter.split(" ").all { + spaceChildInfo.name?.contains(it, ignoreCase = true).orFalse() + || spaceChildInfo.topic?.contains(it, ignoreCase = true).orFalse() + } + } +} diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageActivity.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageActivity.kt index f3bf3eda09..a4585df474 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageActivity.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageActivity.kt @@ -111,17 +111,19 @@ class SpaceManageActivity : VectorBaseActivity(), } ManageType.Settings -> { val simpleName = SpaceSettingsFragment::class.java.simpleName - if (supportFragmentManager.findFragmentByTag(simpleName) == null) { + if (supportFragmentManager.findFragmentByTag(simpleName) == null && args?.spaceId != null) { supportFragmentManager.commitTransaction { replace(R.id.simpleFragmentContainer, SpaceSettingsFragment::class.java, - Bundle().apply { this.putParcelable(MvRx.KEY_ARG, RoomProfileArgs(args?.spaceId ?: "")) }, + Bundle().apply { this.putParcelable(MvRx.KEY_ARG, RoomProfileArgs(args.spaceId)) }, simpleName ) } } } - ManageType.ManageRooms -> TODO() + ManageType.ManageRooms -> { + // no direct access for now + } } } } @@ -145,11 +147,13 @@ class SpaceManageActivity : VectorBaseActivity(), ) } SpaceManagedSharedViewEvents.NavigateToManageRooms -> { - addFragmentToBackstack( - R.id.simpleFragmentContainer, - SpaceManageRoomsFragment::class.java, - SpaceManageArgs(args?.spaceId ?: "", ManageType.ManageRooms) - ) + args?.spaceId?.let { spaceId -> + addFragmentToBackstack( + R.id.simpleFragmentContainer, + SpaceManageRoomsFragment::class.java, + SpaceManageArgs(spaceId, ManageType.ManageRooms) + ) + } } } } diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomViewEvents.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomViewEvents.kt index b135fd89f5..8ba7398a2f 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomViewEvents.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomViewEvents.kt @@ -19,6 +19,6 @@ package im.vector.app.features.spaces.manage import im.vector.app.core.platform.VectorViewEvents sealed class SpaceManageRoomViewEvents : VectorViewEvents { - object BulkActionSuccess: SpaceManageRoomViewEvents() - data class BulkActionFailure(val errorList: List) : SpaceManageRoomViewEvents() +// object BulkActionSuccess: SpaceManageRoomViewEvents() + data class BulkActionFailure(val errorList: List) : SpaceManageRoomViewEvents() } diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsController.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsController.kt index 853333faa6..a94a2d9242 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsController.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsController.kt @@ -24,8 +24,6 @@ import im.vector.app.core.epoxy.loadingItem import im.vector.app.core.error.ErrorFormatter import im.vector.app.core.utils.DebouncedClickListener import im.vector.app.features.home.AvatarRenderer -import io.reactivex.functions.Predicate -import org.matrix.android.sdk.api.extensions.orFalse import org.matrix.android.sdk.api.session.room.model.RoomType import org.matrix.android.sdk.api.session.room.model.SpaceChildInfo import org.matrix.android.sdk.api.util.toMatrixItem @@ -83,19 +81,3 @@ class SpaceManageRoomsController @Inject constructor( } } } - -class SpaceChildInfoMatchFilter : Predicate { - var filter: String = "" - - override fun test(spaceChildInfo: SpaceChildInfo): Boolean { - if (filter.isEmpty()) { - // No filter - return true - } - // if filter is "Jo Do", it should match "John Doe" - return filter.split(" ").all { - spaceChildInfo.name?.contains(it, ignoreCase = true).orFalse() - || spaceChildInfo.topic?.contains(it, ignoreCase = true).orFalse() - } - } -} diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsFragment.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsFragment.kt index 9e1463cb68..b918af0b68 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsFragment.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsFragment.kt @@ -91,12 +91,7 @@ class SpaceManageRoomsFragment @Inject constructor( viewModel.observeViewEvents { when (it) { is SpaceManageRoomViewEvents.BulkActionFailure -> { - it.errorList.forEach { - vectorBaseActivity.toast(it) - } - } - SpaceManageRoomViewEvents.BulkActionSuccess -> { - // + vectorBaseActivity.toast(errorFormatter.toHumanReadable(it.errorList.firstOrNull())) } } } diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsViewModel.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsViewModel.kt index c018b9afd0..d1e1d8f0fb 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceManageRoomsViewModel.kt @@ -27,7 +27,6 @@ import com.airbnb.mvrx.ViewModelContext import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject -import im.vector.app.core.error.ErrorFormatter import im.vector.app.core.mvrx.runCatchingToAsync import im.vector.app.core.platform.VectorViewModel import im.vector.app.features.session.coroutineScope @@ -37,8 +36,7 @@ import org.matrix.android.sdk.api.session.Session class SpaceManageRoomsViewModel @AssistedInject constructor( @Assisted val initialState: SpaceManageRoomViewState, - private val session: Session, - private val errorFormatter: ErrorFormatter + private val session: Session ) : VectorViewModel(initialState) { init { @@ -79,20 +77,20 @@ class SpaceManageRoomsViewModel @AssistedInject constructor( override fun handle(action: SpaceManageRoomViewAction) { when (action) { - is SpaceManageRoomViewAction.ToggleSelection -> handleToggleSelection(action) - is SpaceManageRoomViewAction.UpdateFilter -> { + is SpaceManageRoomViewAction.ToggleSelection -> handleToggleSelection(action) + is SpaceManageRoomViewAction.UpdateFilter -> { setState { copy(currentFilter = action.filter) } } - SpaceManageRoomViewAction.ClearSelection -> { + SpaceManageRoomViewAction.ClearSelection -> { setState { copy(selectedRooms = emptyList()) } } - SpaceManageRoomViewAction.BulkRemove -> { + SpaceManageRoomViewAction.BulkRemove -> { handleBulkRemove() } is SpaceManageRoomViewAction.MarkAllAsSuggested -> { handleBulkMarkAsSuggested(action.suggested) } - SpaceManageRoomViewAction.RefreshFromServer -> { + SpaceManageRoomViewAction.RefreshFromServer -> { refreshSummaryAPI() } } @@ -112,10 +110,10 @@ class SpaceManageRoomsViewModel @AssistedInject constructor( } if (errorList.isEmpty()) { // success - _viewEvents.post(SpaceManageRoomViewEvents.BulkActionSuccess) } else { - _viewEvents.post(SpaceManageRoomViewEvents.BulkActionFailure(errorList.map { errorFormatter.toHumanReadable(it) })) + _viewEvents.post(SpaceManageRoomViewEvents.BulkActionFailure(errorList)) } + refreshSummaryAPI() setState { copy(actionState = Uninitialized) } } } @@ -142,11 +140,10 @@ class SpaceManageRoomsViewModel @AssistedInject constructor( } if (errorList.isEmpty()) { // success - _viewEvents.post(SpaceManageRoomViewEvents.BulkActionSuccess) - refreshSummaryAPI() } else { - _viewEvents.post(SpaceManageRoomViewEvents.BulkActionFailure(errorList.map { errorFormatter.toHumanReadable(it) })) + _viewEvents.post(SpaceManageRoomViewEvents.BulkActionFailure(errorList)) } + refreshSummaryAPI() setState { copy(actionState = Uninitialized) } } } diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsController.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsController.kt index c92692a800..34263bcb32 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsController.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsController.kt @@ -28,7 +28,7 @@ import im.vector.app.features.form.formMultiLineEditTextItem import im.vector.app.features.form.formSwitchItem import im.vector.app.features.home.AvatarRenderer import im.vector.app.features.roomprofile.settings.RoomSettingsViewState -import im.vector.app.features.roomprofile.settings.RoomSettingsViewState.Companion.getJoinRuleWording +import im.vector.app.features.roomprofile.settings.getJoinRuleWording import im.vector.app.features.settings.VectorPreferences import org.matrix.android.sdk.api.session.room.model.RoomJoinRules import org.matrix.android.sdk.api.util.toMatrixItem diff --git a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsFragment.kt b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsFragment.kt index e33f461f47..85c73ac8ef 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsFragment.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/manage/SpaceSettingsFragment.kt @@ -195,7 +195,9 @@ class SpaceSettingsFragment @Inject constructor( viewModel.handle(RoomSettingsAction.SetRoomTopic(topic)) } - override fun onHistoryVisibilityClicked() {} + override fun onHistoryVisibilityClicked() { + // N/A for space settings screen + } override fun onJoinRuleClicked() = withState(viewModel) { state -> val currentJoinRule = state.newRoomJoinRules.newJoinRules ?: state.currentRoomJoinRules diff --git a/vector/src/main/res/values/styles_riot.xml b/vector/src/main/res/values/styles_riot.xml index c3b8247582..a48cecf5f2 100644 --- a/vector/src/main/res/values/styles_riot.xml +++ b/vector/src/main/res/values/styles_riot.xml @@ -11,7 +11,7 @@ ?riotx_background -