Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaces subtitle in Search Rooms with room context rather than last event #5860

Merged
merged 24 commits into from
May 30, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0250f61
Replaces izPublic with isPublic
ericdecanini Apr 26, 2022
a3367d4
Replaces else cases in when branches to RoomListDisplayMode.FILTERED
ericdecanini Apr 26, 2022
9e53e6c
Adds space name to rooms in filtered search
ericdecanini Apr 28, 2022
70cded2
Adds user id and canonical alias to search result subtitles
ericdecanini Apr 28, 2022
87ad35d
Disables typing indicator in filtered search
ericdecanini Apr 28, 2022
3347560
Adds canonical named argument to RoomSummaryUpdater
ericdecanini Apr 28, 2022
b280358
Adds more named arguments to RoomSummaryUpdater
ericdecanini Apr 28, 2022
4784717
Fixes lint error
ericdecanini Apr 28, 2022
962e9ab
Fixes lint error
ericdecanini Apr 28, 2022
7e415e8
Fixes lint error
ericdecanini Apr 28, 2022
7cc79fe
Refactors RoomSummaryItem
ericdecanini Apr 29, 2022
a355b62
Adds displayMode to RoomSummaryListController
ericdecanini Apr 29, 2022
f70a24d
Changes IncomingShareController display mode to FILTERED
ericdecanini Apr 29, 2022
47493fc
Replaces method for getting the space parents of rooms
ericdecanini May 2, 2022
c9b32fe
Changes ordering of room subtitles used
ericdecanini May 2, 2022
b46794d
Adds changelog file
ericdecanini May 2, 2022
52c404a
Merge remote-tracking branch 'origin/develop' into feature/eric/repla…
ericdecanini May 12, 2022
21fe5a2
Adds vertical centering of title when no subtitle is present
ericdecanini May 13, 2022
50839c2
Adds flattenParents field to RoomSummary and corresponding mapping
ericdecanini May 13, 2022
7c1d1c3
Adds centering of items with no subtitles
ericdecanini May 16, 2022
83bd9bc
Fixes lint error
ericdecanini May 16, 2022
03acf45
Uses second layout to center room summary item title
ericdecanini May 17, 2022
d12ab17
Fixes lint errors
ericdecanini May 18, 2022
a5dc8ec
Only gets flattenParents if specifically requested
ericdecanini May 27, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5860.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Adds space or user id as a subtitle under rooms in search
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ data class RoomSummary(
val roomType: String? = null,
val spaceParents: List<SpaceParentInfo>? = null,
val spaceChildren: List<SpaceChildInfo>? = null,
val flattenParents: List<SpaceParentInfo> = emptyList(),
val flattenParentIds: List<String> = emptyList(),
val roomEncryptionAlgorithm: RoomEncryptionAlgorithm? = null
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import org.matrix.android.sdk.api.session.room.UpdatableLivePageResult
import org.matrix.android.sdk.api.session.room.model.Membership
import org.matrix.android.sdk.api.session.room.model.RoomSummary
import org.matrix.android.sdk.api.session.room.model.RoomType
import org.matrix.android.sdk.api.session.room.model.SpaceParentInfo
import org.matrix.android.sdk.api.session.room.roomSummaryQueryParams
import org.matrix.android.sdk.api.session.room.spaceSummaryQueryParams
import org.matrix.android.sdk.api.session.room.summary.RoomAggregateNotificationCount
Expand Down Expand Up @@ -193,7 +194,7 @@ internal class RoomSummaryDataSource @Inject constructor(
}
val dataSourceFactory = realmDataSourceFactory.map {
roomSummaryMapper.map(it)
}
}.map { it.getWithParents() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cool to have an overload of map in RoomSummaryMapper to have a param to request flattened parent

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better also. Like that it won't impact perf for those who don't need to load additional things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout!


val boundaries = MutableLiveData(ResultBoundaries())

Expand Down Expand Up @@ -232,6 +233,20 @@ internal class RoomSummaryDataSource @Inject constructor(
}
}

private fun RoomSummary.getWithParents(): RoomSummary {
val parents = flattenParentIds.mapNotNull { parentId ->
getRoomSummary(parentId)?.let { parentSummary ->
SpaceParentInfo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return the RoomSummaries here instead of fake parentInfo with fake/wrong canonical & viaservers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk why I didn't do this to start with lmao

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

parentId = parentSummary.flattenParentIds.firstOrNull(),
roomSummary = parentSummary,
canonical = true,
viaServers = emptyList()
)
}
}
return copy(flattenParents = parents)
}

fun getCountLive(queryParams: RoomSummaryQueryParams): LiveData<Int> {
val liveRooms = monarchy.findAllManagedWithChanges {
roomSummariesQuery(it, queryParams)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package org.matrix.android.sdk.internal.session.room.summary

import io.realm.Realm
import io.realm.RealmList
import io.realm.kotlin.createObject
import kotlinx.coroutines.runBlocking
import org.matrix.android.sdk.api.extensions.orFalse
Expand Down Expand Up @@ -350,34 +349,39 @@ internal class RoomSummaryUpdater @Inject constructor(
}

val acyclicGraph = graph.withoutEdges(backEdges)
// Timber.v("## SPACES: acyclicGraph $acyclicGraph")
val flattenSpaceParents = acyclicGraph.flattenDestination().map {
it.key.name to it.value.map { it.name }
}.toMap()
// Timber.v("## SPACES: flattenSpaceParents ${flattenSpaceParents.map { it.key.name to it.value.map { it.name } }.joinToString("\n") {
// it.first + ": [" + it.second.joinToString(",") + "]"
// }}")

// Timber.v("## SPACES: lookup map ${lookupMap.map { it.key.name to it.value.map { it.name } }.toMap()}")

lookupMap.entries
.filter { it.key.roomType == RoomType.SPACE && it.key.membership == Membership.JOIN }
.forEach { entry ->
val parent = RoomSummaryEntity.where(realm, entry.key.roomId).findFirst()
if (parent != null) {
// Timber.v("## SPACES: check hierarchy of ${parent.name} id ${parent.roomId}")
// Timber.v("## SPACES: flat known parents of ${parent.name} are ${flattenSpaceParents[parent.roomId]}")
val flattenParentsIds = (flattenSpaceParents[parent.roomId] ?: emptyList()) + listOf(parent.roomId)
// Timber.v("## SPACES: flatten known parents of children of ${parent.name} are ${flattenParentsIds}")
entry.value.forEach { child ->
RoomSummaryEntity.where(realm, child.roomId).findFirst()?.let { childSum ->
// TODO: Revisit
childSum.parents.add(SpaceParentSummaryEntity(
canonical = true,
parentRoomId = parent.roomId,
parentSummaryEntity = parent,
viaServers = RealmList()
))

// Timber.w("## SPACES: ${childSum.name} is ${childSum.roomId} fc: ${childSum.flattenParentIds}")
// var allParents = childSum.flattenParentIds ?: ""
if (childSum.flattenParentIds == null) childSum.flattenParentIds = ""
flattenParentsIds.forEach {
if (childSum.flattenParentIds?.contains(it) != true) {
if (childSum.flattenParentIds?.isEmpty() == false) {
childSum.flattenParentIds += "|"
}
childSum.flattenParentIds += it
childSum.flattenParentIds += "|$it"
}
}
// childSum.flattenParentIds = "$allParents|"

// Timber.v("## SPACES: flatten of ${childSum.name} is ${childSum.flattenParentIds}")
}
}
}
Expand Down Expand Up @@ -407,6 +411,7 @@ internal class RoomSummaryUpdater @Inject constructor(
// we keep real m.child/m.parent relations and add the one for common memberships
dmRoom.flattenParentIds += "|${flattenRelated.joinToString("|")}|"
}
// Timber.v("## SPACES: flatten of ${dmRoom.otherMemberIds.joinToString(",")} is ${dmRoom.flattenParentIds}")
}

// Maybe a good place to count the number of notifications for spaces?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ class RoomListFragment @Inject constructor(
}
}
else -> {
pagedControllerFactory.createRoomSummaryListController()
pagedControllerFactory.createRoomSummaryListController(roomListParams.displayMode)
.also { controller ->
section.liveList?.observe(viewLifecycleOwner) { list ->
controller.setData(list)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ package im.vector.app.features.home.room.list

import android.view.HapticFeedbackConstants
import android.view.View
import android.view.ViewGroup
import android.widget.ImageView
import android.widget.Space
import android.widget.TextView
import androidx.constraintlayout.widget.ConstraintLayout
import androidx.core.view.isInvisible
import androidx.core.view.isVisible
import com.airbnb.epoxy.EpoxyAttribute
Expand Down Expand Up @@ -105,14 +106,14 @@ abstract class RoomSummaryItem : VectorEpoxyModel<RoomSummaryItem.Holder>() {

override fun bind(holder: Holder) {
super.bind(holder)

renderDisplayMode(holder)
holder.rootView.onClick(itemClickListener)
holder.rootView.setOnLongClickListener {
it.performHapticFeedback(HapticFeedbackConstants.LONG_PRESS)
itemLongClickListener?.onLongClick(it) ?: false
}
holder.titleView.text = matrixItem.getBestName()
holder.lastEventTimeView.text = lastEventTime
holder.subtitleView.text = getTextForLastEventView()
holder.unreadCounterBadgeView.render(UnreadCounterBadgeView.State(unreadNotificationCount, showHighlighted))
holder.unreadIndentIndicator.isVisible = hasUnreadMessage
holder.draftView.isVisible = hasDraft
Expand All @@ -122,22 +123,29 @@ abstract class RoomSummaryItem : VectorEpoxyModel<RoomSummaryItem.Holder>() {
holder.roomAvatarFailSendingImageView.isVisible = hasFailedSending
renderSelection(holder, showSelected)
holder.roomAvatarPresenceImageView.render(showPresence, userPresence)
showTypingViewIfNecessary(holder)
}

private fun showTypingViewIfNecessary(holder: Holder) {
if (displayMode != RoomListDisplayMode.FILTERED) {
holder.typingView.setTextOrHide(typingMessage)
holder.subtitleView.isInvisible = holder.typingView.isVisible
}
private fun renderDisplayMode(holder: Holder) = when (displayMode) {
RoomListDisplayMode.ROOMS,
RoomListDisplayMode.PEOPLE,
RoomListDisplayMode.NOTIFICATIONS -> renderForDefaultDisplayMode(holder)
RoomListDisplayMode.FILTERED -> renderForFilteredDisplayMode(holder)
}

private fun getTextForLastEventView(): CharSequence {
return if (displayMode == RoomListDisplayMode.FILTERED) {
subtitle
} else {
lastFormattedEvent.charSequence
}
private fun renderForDefaultDisplayMode(holder: Holder) {
holder.subtitleView.text = lastFormattedEvent.charSequence
holder.lastEventTimeView.text = lastEventTime
holder.typingView.setTextOrHide(typingMessage)
holder.subtitleView.isInvisible = holder.typingView.isVisible
}

private fun renderForFilteredDisplayMode(holder: Holder) {
holder.subtitleView.text = subtitle
holder.centerTitle(shouldCenter = subtitle.isEmpty())
}

private fun Holder.centerTitle(shouldCenter: Boolean) {
centerTitleSpace.isVisible = shouldCenter
}

override fun unbind(holder: Holder) {
Expand Down Expand Up @@ -173,6 +181,7 @@ abstract class RoomSummaryItem : VectorEpoxyModel<RoomSummaryItem.Holder>() {
val roomAvatarPublicDecorationImageView by bind<ImageView>(R.id.roomAvatarPublicDecorationImageView)
val roomAvatarFailSendingImageView by bind<ImageView>(R.id.roomAvatarFailSendingImageView)
val roomAvatarPresenceImageView by bind<PresenceStateImageView>(R.id.roomAvatarPresenceImageView)
val rootView by bind<ViewGroup>(R.id.itemRoomLayout)
val rootView by bind<ConstraintLayout>(R.id.itemRoomLayout)
val centerTitleSpace by bind<Space>(R.id.centerTitleSpace)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,10 @@ class RoomSummaryItemFactory @Inject constructor(private val displayableEventFor
}

private fun getSearchResultSubtitle(roomSummary: RoomSummary): String {
val spaceName = roomSummary.spaceParents?.firstOrNull()?.roomSummary?.name
val userId = roomSummary.directUserId
val spaceName = roomSummary.spaceParents?.firstOrNull()?.roomSummary?.name
val canonicalAlias = roomSummary.canonicalAlias

return (spaceName ?: userId ?: canonicalAlias).orEmpty()
return (userId ?: spaceName ?: canonicalAlias).orEmpty()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ import im.vector.app.features.home.RoomListDisplayMode
import org.matrix.android.sdk.api.session.room.model.RoomSummary

class RoomSummaryListController(
private val roomSummaryItemFactory: RoomSummaryItemFactory
private val roomSummaryItemFactory: RoomSummaryItemFactory,
private val displayMode: RoomListDisplayMode
) : CollapsableTypedEpoxyController<List<RoomSummary>>() {

var listener: RoomListListener? = null

override fun buildModels(data: List<RoomSummary>?) {
data?.forEach {
add(roomSummaryItemFactory.create(it, emptyMap(), emptySet(), RoomListDisplayMode.ROOMS /* TODO: change */, listener))
add(roomSummaryItemFactory.create(it, emptyMap(), emptySet(), displayMode, listener))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class RoomSummaryPagedControllerFactory @Inject constructor(
return RoomSummaryPagedController(roomSummaryItemFactory, displayMode)
}

fun createRoomSummaryListController(): RoomSummaryListController {
return RoomSummaryListController(roomSummaryItemFactory)
fun createRoomSummaryListController(displayMode: RoomListDisplayMode): RoomSummaryListController {
return RoomSummaryListController(roomSummaryItemFactory, displayMode)
}

fun createSuggestedRoomListController(): SuggestedRoomListController {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,13 @@ class IncomingShareController @Inject constructor(private val roomSummaryItemFac
} else {
roomSummaries.forEach { roomSummary ->
roomSummaryItemFactory
.createRoomItem(roomSummary, data.selectedRoomIds, RoomListDisplayMode.ROOMS /* TODO: Change */, callback?.let { it::onRoomClicked }, callback?.let { it::onRoomLongClicked })
.createRoomItem(
roomSummary,
data.selectedRoomIds,
RoomListDisplayMode.FILTERED,
callback?.let { it::onRoomClicked },
callback?.let { it::onRoomLongClicked }
)
.addTo(this)
}
}
Expand Down
18 changes: 16 additions & 2 deletions vector/src/main/res/layout/item_room.xml
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,27 @@
app:layout_constraintTop_toBottomOf="@id/roomAvatarContainer"
tools:layout_marginStart="20dp" />

<Space
android:id="@+id/topMarginSpace"
android:layout_width="0dp"
android:layout_height="12dp"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent" />

<Space
android:id="@+id/centerTitleSpace"
android:layout_width="match_parent"
android:layout_height="15dp"
android:visibility="gone"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/topMarginSpace" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird way to align text vertically. If the font size is changed, it will not work properly.
Is is possible to do better using ConstraintLayout attributes?
Maybe adding a new sublayout if there is no easier solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this with changing consraints programatically, but that affects other bindings as you scroll. There's no easy way to 'reset' the view to its original xml state.

Can you explain more what you mean by sublayout?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sublayout: add a new ConstraintLayout to pack the Title and the Subtitle.
I did not check if this is possible, there are other views around.
I will try to play a bit on this layout to see if I find something better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to do this imo is to inflate an entirely different Epoxy item based on whether there is a subtitle or not. What are your thoughts on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will improve that in the future, there is another bug related, with the placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I am removing myself from the reviewer now.


<TextView
android:id="@+id/roomNameView"
style="@style/Widget.Vector.TextView.Subtitle"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginStart="@dimen/layout_horizontal_margin"
android:layout_marginTop="12dp"
android:layout_marginEnd="8dp"
android:duplicateParentState="true"
android:ellipsize="end"
Expand All @@ -130,7 +144,7 @@
app:layout_constraintHorizontal_bias="0.0"
app:layout_constraintHorizontal_chainStyle="packed"
app:layout_constraintStart_toEndOf="@id/roomAvatarContainer"
app:layout_constraintTop_toTopOf="parent"
app:layout_constraintTop_toBottomOf="@id/centerTitleSpace"
tools:text="@sample/users.json/data/displayName" />

<ImageView
Expand Down