-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix the logic of the room list banner state #3615
Fix the logic of the room list banner state #3615
Conversation
112b742
to
27e087b
Compare
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
27e087b
to
0b2ae98
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3615 +/- ##
========================================
Coverage 82.65% 82.65%
========================================
Files 1735 1735
Lines 41187 41191 +4
Branches 5000 4999 -1
========================================
+ Hits 34043 34047 +4
- Misses 5380 5381 +1
+ Partials 1764 1763 -1 ☔ View full report in Codecov by Sentry. |
@@ -176,27 +175,23 @@ class RoomListPresenter @Inject constructor( | |||
private fun securityBannerState( | |||
securityBannerDismissed: Boolean, | |||
needsSlidingSyncMigration: Boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not because needsSlidingSyncMigration
needs to be wrapped in a state like securityBannerDismissed
, with rememberUpdatedState
?
…the-migration-banner-is-displayed
deriveStateOf
preventing the banner state from updating0ee4b83
to
230abd3
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
} | ||
|
||
return SecurityBannerState.None | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content
needsSlidingSyncMigration
also generates a state forderivedStateOf
.derivedStateOf
block: it fixes an issue that hid the migration banner, gets rid of thesyncState
and can now be extended with more cases that fallthrough to the next ones.Motivation and context
Fix the migration banner not being displayed in some cases that @pixlwave reported. To be honest I have no idea why this issue appeared now, since it was tested several times before and it worked as expected. Maybe an update of the compose compiler broke it?
Tests
riot.grin.hu
).Tested devices
Checklist