From 0de56e29933f94740e3cd9e6034fb5a7d10be360 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 11 May 2021 19:11:15 +0200 Subject: [PATCH] Code Review --- .../app/core/ui/list/GenericPillItem.kt | 13 ++-- .../room/list/SpaceRoomListSectionBuilder.kt | 71 +++++++++++-------- .../features/spaces/SpaceSummaryController.kt | 24 +++---- .../features/spaces/SubSpaceSummaryItem.kt | 36 +++++----- 4 files changed, 79 insertions(+), 65 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/ui/list/GenericPillItem.kt b/vector/src/main/java/im/vector/app/core/ui/list/GenericPillItem.kt index 5750fd93b4..ce9e35c007 100644 --- a/vector/src/main/java/im/vector/app/core/ui/list/GenericPillItem.kt +++ b/vector/src/main/java/im/vector/app/core/ui/list/GenericPillItem.kt @@ -20,6 +20,7 @@ import android.view.Gravity import android.widget.ImageView import android.widget.TextView import androidx.annotation.DrawableRes +import androidx.core.view.isVisible import androidx.core.widget.ImageViewCompat import com.airbnb.epoxy.EpoxyAttribute import com.airbnb.epoxy.EpoxyModelClass @@ -30,10 +31,7 @@ import im.vector.app.core.extensions.setTextOrHide import im.vector.app.features.themes.ThemeUtils /** - * A generic list item. - * Displays an item with a title, and optional description. - * Can display an accessory on the right, that can be an image or an indeterminate progress. - * If provided with an action, will display a button at the bottom of the list item. + * A generic list item with a rounded corner background and an optional icon */ @EpoxyModelClass(layout = R.layout.item_generic_pill_footer) abstract class GenericPillItem : VectorEpoxyModel() { @@ -65,7 +63,12 @@ abstract class GenericPillItem : VectorEpoxyModel() { holder.textView.textSize = style.toTextSize() holder.textView.gravity = if (centered) Gravity.CENTER_HORIZONTAL else Gravity.START - imageRes?.let { holder.imageView.setImageResource(it) } + if (imageRes != null) { + holder.imageView.setImageResource(imageRes!!) + holder.imageView.isVisible = true + } else { + holder.imageView.isVisible = false + } if (tintIcon) { val iconTintColor = ThemeUtils.getColor(holder.view.context, R.attr.riotx_text_secondary) ImageViewCompat.setImageTintList(holder.imageView, ColorStateList.valueOf(iconTintColor)) diff --git a/vector/src/main/java/im/vector/app/features/home/room/list/SpaceRoomListSectionBuilder.kt b/vector/src/main/java/im/vector/app/features/home/room/list/SpaceRoomListSectionBuilder.kt index 2cd0199062..266adf6b0c 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/list/SpaceRoomListSectionBuilder.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/list/SpaceRoomListSectionBuilder.kt @@ -93,8 +93,11 @@ class SpaceRoomListSectionBuilder( activeSpaceUpdaters = activeSpaceAwareQueries, nameRes = R.string.invitations_header, notifyOfLocalEcho = true, - spaceFilterStrategy = RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL.takeIf { onlyOrphansInHome } - ?: RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL, + spaceFilterStrategy = if (onlyOrphansInHome) { + RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL + } else { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + }, countRoomAsNotif = true ) { it.memberships = listOf(Membership.INVITE) @@ -102,12 +105,15 @@ class SpaceRoomListSectionBuilder( } addSection( - sections, - activeSpaceAwareQueries, - R.string.bottom_action_rooms, - false, - RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL.takeIf { onlyOrphansInHome } - ?: RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + sections = sections, + activeSpaceUpdaters = activeSpaceAwareQueries, + nameRes = R.string.bottom_action_rooms, + notifyOfLocalEcho = false, + spaceFilterStrategy = if (onlyOrphansInHome) { + RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL + } else { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + } ) { it.memberships = listOf(Membership.JOIN) it.roomCategoryFilter = RoomCategoryFilter.ONLY_WITH_NOTIFICATIONS @@ -155,12 +161,15 @@ class SpaceRoomListSectionBuilder( } addSection( - sections, - activeSpaceAwareQueries, - R.string.bottom_action_rooms, - false, - RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL.takeIf { onlyOrphansInHome } - ?: RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + sections = sections, + activeSpaceUpdaters = activeSpaceAwareQueries, + nameRes = R.string.bottom_action_rooms, + notifyOfLocalEcho = false, + spaceFilterStrategy = if (onlyOrphansInHome) { + RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL + } else { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + } ) { it.memberships = listOf(Membership.JOIN) it.roomCategoryFilter = RoomCategoryFilter.ONLY_ROOMS @@ -168,12 +177,15 @@ class SpaceRoomListSectionBuilder( } addSection( - sections, - activeSpaceAwareQueries, - R.string.low_priority_header, - false, - RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL.takeIf { onlyOrphansInHome } - ?: RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + sections = sections, + activeSpaceUpdaters = activeSpaceAwareQueries, + nameRes = R.string.low_priority_header, + notifyOfLocalEcho = false, + spaceFilterStrategy = if (onlyOrphansInHome) { + RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL + } else { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + } ) { it.memberships = listOf(Membership.JOIN) it.roomCategoryFilter = RoomCategoryFilter.ONLY_ROOMS @@ -181,12 +193,15 @@ class SpaceRoomListSectionBuilder( } addSection( - sections, - activeSpaceAwareQueries, - R.string.system_alerts_header, - false, - RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL.takeIf { onlyOrphansInHome } - ?: RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + sections = sections, + activeSpaceUpdaters = activeSpaceAwareQueries, + nameRes = R.string.system_alerts_header, + notifyOfLocalEcho = false, + spaceFilterStrategy = if (onlyOrphansInHome) { + RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL + } else { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL + } ) { it.memberships = listOf(Membership.JOIN) it.roomCategoryFilter = RoomCategoryFilter.ONLY_ROOMS @@ -372,7 +387,7 @@ class SpaceRoomListSectionBuilder( activeSpaceFilter = ActiveSpaceFilter.ActiveSpace(currentSpace) ) } - RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL -> { + RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL -> { if (currentSpace == null) { copy( activeSpaceFilter = ActiveSpaceFilter.None @@ -383,7 +398,7 @@ class SpaceRoomListSectionBuilder( ) } } - RoomListViewModel.SpaceFilterStrategy.NONE -> this + RoomListViewModel.SpaceFilterStrategy.NONE -> this } } } diff --git a/vector/src/main/java/im/vector/app/features/spaces/SpaceSummaryController.kt b/vector/src/main/java/im/vector/app/features/spaces/SpaceSummaryController.kt index bd190519aa..33e4b23047 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/SpaceSummaryController.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/SpaceSummaryController.kt @@ -179,31 +179,31 @@ class SpaceSummaryController @Inject constructor( private fun buildSubSpace(summaries: List?, expandedStates: Map, selected: RoomGroupingMethod, - childSum: SpaceChildInfo, currentDepth: Int, maxDepth: Int) { + info: SpaceChildInfo, currentDepth: Int, maxDepth: Int) { if (currentDepth >= maxDepth) return - val childSum = summaries?.firstOrNull { it.roomId == childSum.childRoomId } ?: return + val childSummary = summaries?.firstOrNull { it.roomId == info.childRoomId } ?: return // does it have children? - val subSpaces = childSum.spaceChildren?.filter { childInfo -> + val subSpaces = childSummary.spaceChildren?.filter { childInfo -> summaries.indexOfFirst { it.roomId == childInfo.childRoomId } != -1 }?.sortedWith(subSpaceComparator) - val expanded = expandedStates[childSum.roomId] == true - val isSelected = selected is RoomGroupingMethod.BySpace && childSum.roomId == selected.space()?.roomId + val expanded = expandedStates[childSummary.roomId] == true + val isSelected = selected is RoomGroupingMethod.BySpace && childSummary.roomId == selected.space()?.roomId subSpaceSummaryItem { avatarRenderer(avatarRenderer) - id(childSum.roomId) + id(childSummary.roomId) hasChildren(!subSpaces.isNullOrEmpty()) selected(isSelected) expanded(expanded) - onMore { callback?.onSpaceSettings(childSum) } - matrixItem(childSum.toMatrixItem()) - listener { callback?.onSpaceSelected(childSum) } - toggleExpand { callback?.onToggleExpand(childSum) } + onMore { callback?.onSpaceSettings(childSummary) } + matrixItem(childSummary.toMatrixItem()) + listener { callback?.onSpaceSelected(childSummary) } + toggleExpand { callback?.onToggleExpand(childSummary) } indent(currentDepth) countState( UnreadCounterBadgeView.State( - childSum.notificationCount, - childSum.highlightCount > 0 + childSummary.notificationCount, + childSummary.highlightCount > 0 ) ) } diff --git a/vector/src/main/java/im/vector/app/features/spaces/SubSpaceSummaryItem.kt b/vector/src/main/java/im/vector/app/features/spaces/SubSpaceSummaryItem.kt index b98f1ca7af..db58353e5c 100644 --- a/vector/src/main/java/im/vector/app/features/spaces/SubSpaceSummaryItem.kt +++ b/vector/src/main/java/im/vector/app/features/spaces/SubSpaceSummaryItem.kt @@ -20,7 +20,6 @@ import android.widget.ImageView import android.widget.Space import android.widget.TextView import androidx.core.content.ContextCompat -import androidx.core.view.isGone import androidx.core.view.isVisible import androidx.core.view.updateLayoutParams import com.airbnb.epoxy.EpoxyAttribute @@ -37,16 +36,16 @@ import org.matrix.android.sdk.api.util.MatrixItem @EpoxyModelClass(layout = R.layout.item_sub_space) abstract class SubSpaceSummaryItem : VectorEpoxyModel() { - @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) lateinit var avatarRenderer: AvatarRenderer + @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) lateinit var avatarRenderer: AvatarRenderer @EpoxyAttribute lateinit var matrixItem: MatrixItem @EpoxyAttribute var selected: Boolean = false @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) var listener: (() -> Unit)? = null - @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) var onMore: (() -> Unit)? = null - @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) var toggleExpand: (() -> Unit)? = null + @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) var onMore: (() -> Unit)? = null + @EpoxyAttribute(EpoxyAttribute.Option.DoNotHash) var toggleExpand: (() -> Unit)? = null @EpoxyAttribute var expanded: Boolean = false @EpoxyAttribute var hasChildren: Boolean = false @EpoxyAttribute var indent: Int = 0 - @EpoxyAttribute var countState : UnreadCounterBadgeView.State = UnreadCounterBadgeView.State(0, false) + @EpoxyAttribute var countState: UnreadCounterBadgeView.State = UnreadCounterBadgeView.State(0, false) override fun bind(holder: Holder) { super.bind(holder) @@ -64,21 +63,18 @@ abstract class SubSpaceSummaryItem : VectorEpoxyModel - toggleExpand?.invoke() - }) - ) - } else { - holder.collapseIndicator.isGone = true - } + holder.collapseIndicator.setImageDrawable( + ContextCompat.getDrawable(holder.view.context, + if (expanded) R.drawable.ic_expand_less else R.drawable.ic_expand_more + ) + ) + holder.collapseIndicator.setOnClickListener( + DebouncedClickListener({ _ -> + toggleExpand?.invoke() + }) + ) + + holder.collapseIndicator.isVisible = hasChildren holder.indentSpace.isVisible = indent > 0 holder.indentSpace.updateLayoutParams {