Skip to content

Conversation

@ouchadam
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Updates the toActiveSpaceOrOrphanRooms usages to avoid the nullsafe fallback, the extension already handles the null case for us (this is the fix!)
  • Replaces the nullable SpaceFilter in favour of a dedicated NoFilter type, should help address the ambiguity of the different states

Motivation and context

Fixes #6665

Screenshots / GIFs

BEFORE AFTER
before-show-rooms after-rooms-in-home

Tests

  • Have rooms across spaces
  • Notice Home is always showing all rooms
  • Notice the Show all rooms in Home preference has no effect

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28

@ouchadam ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Jul 28, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
}
} ?: SpaceFilter.NoFilter
Copy link
Member

Choose a reason for hiding this comment

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

In this case I think a if/else would be clearer:

if (vectorPreferences.prefSpacesShowAllRoomInHome()) {
    SpaceFilter.NoFilter
} else {
    SpaceFilter.OrphanRooms
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, will update 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
}
} ?: SpaceFilter.NoFilter
Copy link
Member

Choose a reason for hiding this comment

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

Same remark

this.spaceFilter = SpaceFilter.OrphanRooms.takeIf {
!vectorPreferences.prefSpacesShowAllRoomInHome()
}
} ?: SpaceFilter.NoFilter
Copy link
Member

Choose a reason for hiding this comment

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

same remark

* 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.

}
.onEach { selectedSpaceOption ->
val selectedSpace = selectedSpaceOption.orNull()
val strategy = if (!vectorPreferences.prefSpacesShowAllRoomInHome()) {
Copy link
Contributor Author

@ouchadam ouchadam Jul 28, 2022

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()
}

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update.

.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!

@ouchadam
Copy link
Contributor Author

going to skip waiting for the instrumentation tests to complete so that the RC process can start

@ouchadam ouchadam merged commit b409431 into develop Jul 28, 2022
@ouchadam ouchadam deleted the feature/adm/missing-space-rooms branch July 28, 2022 11:10
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

48.6% 48.6% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Z-NextRelease For issues and PRs which should be included in the NextRelease.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show all rooms in Home preference has no effect

3 participants