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

Do not display empty room list state before the loading one #2402

Merged

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Feb 15, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Unify both room list summaries and the loadingState into a single value containing both (RoomList.State).
  • Use this newly built RoomList.State to transparently decide whether to display the empty, loading, or loaded state with no flickers in the room list screen.

Motivation and context

When the room list took a bit longer than expected to load, the empty screen could be seen for a moment before the loading one appeared.

Tests

With an account having a single room in the room list, test:

  • The loading state appears, then the room list does.
  • When exiting and coming back to the screen there are no unexpected intermediate states.
  • When leaving the room the empty state is displayed instead, even when restarting the app.
  • If joining or creating a new room, the loaded state appears.

Tested devices

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

Checklist

Copy link
Contributor

github-actions bot commented Feb 15, 2024

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/SE2tVi

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4daf2b0) 72.31% compared to head (e2ff547) 72.32%.
Report is 11 commits behind head on develop.

Files Patch % Lines
...res/roomlist/impl/datasource/RoomListDataSource.kt 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2402   +/-   ##
========================================
  Coverage    72.31%   72.32%           
========================================
  Files         1364     1364           
  Lines        32236    32232    -4     
  Branches      6297     6295    -2     
========================================
- Hits         23312    23311    -1     
+ Misses        5659     5658    -1     
+ Partials      3265     3263    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmartinesp
Copy link
Member Author

I tried using a simpler solution and keeping the MutableStateFlow for room summaries, but I ran into matrix-org/matrix-rust-sdk#3135.

@jmartinesp jmartinesp force-pushed the fix/jme/do-not-display-empty-state-before-loading-roomlist branch from 98d8faf to e0e1be4 Compare February 16, 2024 16:09
@jmartinesp jmartinesp marked this pull request as ready for review February 16, 2024 16:15
@jmartinesp jmartinesp requested a review from a team as a code owner February 16, 2024 16:15
@jmartinesp jmartinesp requested review from bmarty and removed request for a team February 16, 2024 16:15
@frebib
Copy link
Contributor

frebib commented Feb 17, 2024

I pulled this in but noticed that my room list is truncated to only ~2 screens worth of rooms. Scrolling to the bottom doesn't load more. Also, strangely, many rooms are missing even when I search for them

@jmartinesp jmartinesp force-pushed the fix/jme/do-not-display-empty-state-before-loading-roomlist branch from 2ad3343 to 1262a48 Compare February 19, 2024 12:16
@jmartinesp
Copy link
Member Author

I pulled this in but noticed that my room list is truncated to only ~2 screens worth of rooms. Scrolling to the bottom doesn't load more. Also, strangely, many rooms are missing even when I search for them

The pagination logic was affected by the changes in the PR. I've rolled back those changes, since they caused some other side effects and went back to the initial implementation.

@jmartinesp jmartinesp force-pushed the fix/jme/do-not-display-empty-state-before-loading-roomlist branch from 80ff7e8 to e2ff547 Compare February 19, 2024 15:20
Copy link

sonarcloud bot commented Feb 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Feb 21, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Feb 21, 2024
@jmartinesp jmartinesp merged commit 598bf96 into develop Feb 21, 2024
18 checks passed
@jmartinesp jmartinesp deleted the fix/jme/do-not-display-empty-state-before-loading-roomlist branch February 21, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants