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

Fixes Room List not getting updated when fragment is not in focus #7186

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Sep 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

We had a bug where if you join or leave a room from an activity/fragment other than room list, the list would often not update and would sometimes also crash. This fixes that by converting the room paged list livedata instead of passing livedata in a mavericks state update and observing it there

Motivation and context

Fixes #7152

Screenshots / GIFs

Tests

  • Open room list
  • Go into room
  • Leave room from room settings
  • Go back to list
  • See list is updated

Try the above scenario but also for joining rooms

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 13

Checklist

@ericdecanini ericdecanini marked this pull request as ready for review September 20, 2022 22:36
@ouchadam ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 21, 2022
.onEach {
setState { copy(roomsPagedList = it) }
}
.flowOn(Dispatchers.Default)
Copy link
Member

Choose a reason for hiding this comment

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

No need to change dispatcher for this

roomListViewModel.onEach(HomeRoomListViewState::roomsPagedList) { roomsList ->
roomsList?.let {
roomsController.submitList(it)
if (it.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit weird to have both call to roomsController, this should be his responsability to handle empty list

Copy link
Contributor

@fedrunov fedrunov Sep 21, 2022

Choose a reason for hiding this comment

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

What exactly do you mean? The problem here is that there are different empty states for different tabs, so we need to force rebuild models, to show different empty state view. But I can just create method in controller and move this code there 🤔

Copy link
Member

@ganfra ganfra Sep 21, 2022

Choose a reason for hiding this comment

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

The controller should be able to handle this regarding the ViewState, but keep it like this for now.

Copy link
Member

Choose a reason for hiding this comment

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

there are different empty states for different tabs, so we need to force rebuild models, to show different empty state view

This should be a comment in the code, since this is not usual to call requestForcedModelBuild()

@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2022

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication


roomsFlowJob = liveResults.livePagedList
.asFlow()
.onEach {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could use setOnEach

@bmarty bmarty merged commit 52d7369 into develop Sep 21, 2022
@bmarty bmarty deleted the bugfix/eric/fixes-room-list-updates branch September 21, 2022 10:14
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.

Leaving a room doesn't remove it from "All Chat" list with New App Layout Enabled
5 participants