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.

Sign up for GitHub

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

Home always showing all rooms #6666

Merged
merged 6 commits into from
Jul 28, 2022
Merged

Home always showing all rooms #6666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ba8d29
fixing rooms from other spaces being included in home due to wrong fi…
ouchadam Jul 28, 2022
e847688
providing a type for the NoFilter to avoid the ambiguity of the nulla…
ouchadam Jul 28, 2022
2d8ef99
adding changelog entry
ouchadam Jul 28, 2022
09d8405
replacing inverted takeIf chain with positive when conditions
ouchadam Jul 28, 2022
8ef0850
removing unused import
ouchadam Jul 28, 2022
e4640f1
adding sdk changelog
ouchadam Jul 28, 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/6665.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes the room list not taking into account the Show all rooms in Home preference
1 change: 1 addition & 0 deletions changelog.d/6666.sdk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SDK - The SpaceFilter is query parameter is no longer nullable, use SpaceFilter.NoFilter instead
10 changes: 10 additions & 0 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/query/SpaceFilter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,19 @@ sealed interface SpaceFilter {
* Used to get all the rooms that do not have the provided space in their parent hierarchy.
*/
data class ExcludeSpace(val spaceId: String) : SpaceFilter

/**
* Used to apply no filtering to the space.
*/
object NoFilter : SpaceFilter
}

/**
* Return a [SpaceFilter.ActiveSpace] if the String is not null, or [SpaceFilter.OrphanRooms].
*/
fun String?.toActiveSpaceOrOrphanRooms(): SpaceFilter = this?.let { SpaceFilter.ActiveSpace(it) } ?: SpaceFilter.OrphanRooms

/**
* Return a [SpaceFilter.ActiveSpace] if the String is not null, or [SpaceFilter.NoFilter].
*/
fun String?.toActiveSpaceOrNoFilter(): SpaceFilter = this?.let { SpaceFilter.ActiveSpace(it) } ?: SpaceFilter.NoFilter
4 changes: 2 additions & 2 deletions ...k-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomSummaryQueryParams.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ data class RoomSummaryQueryParams(
/**
* Used to filter room using the current space.
*/
val spaceFilter: SpaceFilter?,
val spaceFilter: SpaceFilter,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention this API change in a file 6666.sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e4640f1

) {

/**
Expand All @@ -101,7 +101,7 @@ data class RoomSummaryQueryParams(
var roomTagQueryFilter: RoomTagQueryFilter? = null
var excludeType: List<String?>? = listOf(RoomType.SPACE)
var includeType: List<String?>? = null
var spaceFilter: SpaceFilter? = null
var spaceFilter: SpaceFilter = SpaceFilter.NoFilter

fun build() = RoomSummaryQueryParams(
displayName = displayName,
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ internal class RoomSummaryDataSource @Inject constructor(
is SpaceFilter.ExcludeSpace -> {
query.not().contains(RoomSummaryEntityFields.FLATTEN_PARENT_IDS, queryParams.spaceFilter.spaceId)
}
null -> Unit // nop
SpaceFilter.NoFilter -> Unit // nop
}

return query
Expand Down
6 changes: 3 additions & 3 deletions vector/src/main/java/im/vector/app/features/home/HomeDetailViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import org.matrix.android.sdk.api.query.RoomCategoryFilter
import org.matrix.android.sdk.api.query.SpaceFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrNoFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrOrphanRooms
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.crypto.NewSessionListener
Expand Down Expand Up @@ -235,7 +235,7 @@ class HomeDetailViewModel @AssistedInject constructor(
roomSummaryQueryParams {
memberships = listOf(Membership.INVITE)
roomCategoryFilter = RoomCategoryFilter.ONLY_DM
spaceFilter = activeSpaceRoomId?.let { SpaceFilter.ActiveSpace(it) }
spaceFilter = activeSpaceRoomId.toActiveSpaceOrNoFilter()
}
).size

Expand All @@ -252,7 +252,7 @@ class HomeDetailViewModel @AssistedInject constructor(
roomSummaryQueryParams {
memberships = listOf(Membership.JOIN)
roomCategoryFilter = RoomCategoryFilter.ONLY_DM
spaceFilter = activeSpaceRoomId?.let { SpaceFilter.ActiveSpace(it) }
spaceFilter = activeSpaceRoomId.toActiveSpaceOrNoFilter()
}
)

Expand Down
5 changes: 3 additions & 2 deletions vector/src/main/java/im/vector/app/features/home/UnreadMessagesSharedViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,9 @@ class UnreadMessagesSharedViewModel @AssistedInject constructor(
val totalCount = roomService.getNotificationCountForRooms(
roomSummaryQueryParams {
this.memberships = listOf(Membership.JOIN)
this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
this.spaceFilter = when {
vectorPreferences.prefSpacesShowAllRoomInHome() -> SpaceFilter.NoFilter
else -> SpaceFilter.OrphanRooms
}
}
)
Expand Down
9 changes: 5 additions & 4 deletions vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.query.RoomCategoryFilter
import org.matrix.android.sdk.api.query.RoomTagQueryFilter
import org.matrix.android.sdk.api.query.SpaceFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrNoFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrOrphanRooms
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.getRoomSummary
Expand Down Expand Up @@ -378,7 +379,7 @@ class RoomListSectionBuilder(
activeSpaceUpdaters.add(object : RoomListViewModel.ActiveSpaceQueryUpdater {
override fun updateForSpaceId(roomId: String?) {
filteredPagedRoomSummariesLive.queryParams = roomQueryParams.copy(
spaceFilter = roomId?.toActiveSpaceOrOrphanRooms()
spaceFilter = roomId.toActiveSpaceOrOrphanRooms()
)
liveQueryParams.update { filteredPagedRoomSummariesLive.queryParams }
}
Expand All @@ -393,7 +394,7 @@ class RoomListSectionBuilder(
)
} else {
filteredPagedRoomSummariesLive.queryParams = roomQueryParams.copy(
spaceFilter = null
spaceFilter = SpaceFilter.NoFilter
)
}
liveQueryParams.update { filteredPagedRoomSummariesLive.queryParams }
Expand Down Expand Up @@ -444,12 +445,12 @@ class RoomListSectionBuilder(
return when (spaceFilter) {
RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL -> {
copy(
spaceFilter = currentSpace?.toActiveSpaceOrOrphanRooms()
spaceFilter = currentSpace.toActiveSpaceOrOrphanRooms()
)
}
RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL -> {
copy(
spaceFilter = currentSpace?.let { SpaceFilter.ActiveSpace(it) }
spaceFilter = currentSpace.toActiveSpaceOrNoFilter()
)
}
RoomListViewModel.SpaceFilterStrategy.NONE -> this
Expand Down
22 changes: 6 additions & 16 deletions vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import im.vector.app.core.di.MavericksAssistedViewModelFactory
import im.vector.app.core.di.hiltMavericksViewModelFactory
import im.vector.app.core.platform.StateView
import im.vector.app.core.platform.VectorViewModel
import im.vector.app.features.home.room.list.RoomListViewModel
import im.vector.app.features.settings.VectorPreferences
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.MutableSharedFlow
Expand All @@ -39,6 +38,7 @@ import kotlinx.coroutines.flow.onStart
import kotlinx.coroutines.launch
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.query.SpaceFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrNoFilter
import org.matrix.android.sdk.api.query.toActiveSpaceOrOrphanRooms
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.getRoom
Expand Down Expand Up @@ -106,13 +106,8 @@ class HomeRoomListViewModel @AssistedInject constructor(
}
.onEach { selectedSpaceOption ->
val selectedSpace = selectedSpaceOption.orNull()
val strategy = if (!vectorPreferences.prefSpacesShowAllRoomInHome()) {
Copy link
Contributor Author

@ouchadam ouchadam Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

cc @fedrunov this condition looked inverted and has been updated to match the usages in other screens

when {
  vectorPreferences.prefSpacesShowAllRoomInHome() -> selectedSpaceId.toActiveSpaceOrNoFilter()
  else -> selectedSpaceId.toActiveSpaceOrOrphanRooms()
}

RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL
} else {
RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL
}
filteredPagedRoomSummariesLive.queryParams = filteredPagedRoomSummariesLive.queryParams.copy(
spaceFilter = getSpaceFilter(selectedSpaceId = selectedSpace?.roomId, strategy)
spaceFilter = getSpaceFilter(selectedSpaceId = selectedSpace?.roomId)
)
}.launchIn(viewModelScope)

Expand All @@ -121,15 +116,10 @@ class HomeRoomListViewModel @AssistedInject constructor(
)
}

private fun getSpaceFilter(selectedSpaceId: String?, strategy: RoomListViewModel.SpaceFilterStrategy): SpaceFilter? {
return when (strategy) {
RoomListViewModel.SpaceFilterStrategy.ORPHANS_IF_SPACE_NULL -> {
selectedSpaceId?.toActiveSpaceOrOrphanRooms()
}
RoomListViewModel.SpaceFilterStrategy.ALL_IF_SPACE_NULL -> {
selectedSpaceId?.let { SpaceFilter.ActiveSpace(it) }
}
RoomListViewModel.SpaceFilterStrategy.NONE -> null
private fun getSpaceFilter(selectedSpaceId: String?): SpaceFilter {
return when {
vectorPreferences.prefSpacesShowAllRoomInHome() -> selectedSpaceId.toActiveSpaceOrNoFilter()
else -> selectedSpaceId.toActiveSpaceOrOrphanRooms()
}
}

Expand Down
13 changes: 7 additions & 6 deletions vector/src/main/java/im/vector/app/features/spaces/SpaceListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,7 @@ class SpaceListViewModel @AssistedInject constructor(
session.roomService().getPagedRoomSummariesLive(
roomSummaryQueryParams {
this.memberships = listOf(Membership.JOIN)
this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
}
this.spaceFilter = roomsInSpaceFilter()
}, sortOrder = RoomSortOrder.NONE
).asFlow()
.sample(300)
Expand All @@ -115,9 +113,7 @@ class SpaceListViewModel @AssistedInject constructor(
val totalCount = session.roomService().getNotificationCountForRooms(
roomSummaryQueryParams {
this.memberships = listOf(Membership.JOIN)
this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
}
this.spaceFilter = roomsInSpaceFilter()
}
)
val counts = RoomAggregateNotificationCount(
Expand All @@ -134,6 +130,11 @@ class SpaceListViewModel @AssistedInject constructor(
.launchIn(viewModelScope)
}

private fun roomsInSpaceFilter() = when {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not change it now, but the fun name is a bit strange to me.
Maybe something like getSpaceFilter() would be better.
On the fund, I do not really get why we have such filter here, since IIUC this ViewModel is used by the screen which displays only Spaces. So having orphans rooms here is strange to me.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK, it's to update the notification count. Please ignore me!

vectorPreferences.prefSpacesShowAllRoomInHome() -> SpaceFilter.NoFilter
else -> SpaceFilter.OrphanRooms
}

override fun handle(action: SpaceListAction) {
when (action) {
is SpaceListAction.SelectSpace -> handleSelectSpace(action)
Expand Down