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

feat(android): leave / rejoin predictions & alerts after backgrounding #588

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

KaylaBrady
Copy link
Collaborator

Summary

Ticket: 🤖 | Stability | Leave realtime channels when backgrounding

What is this PR for?

Follow-up to #579, applies the same pattern to predictions & alerts.

iOS

  • If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?
    • Add temporary machine translations, marked "Needs Review"
      android
  • All user-facing strings added to strings resource

Testing

What testing have you done?

  • Ran locally, confirmed I can background & revive the app without crashing
  • Updated unit tests

@KaylaBrady KaylaBrady requested a review from a team as a code owner December 16, 2024 17:34
@KaylaBrady KaylaBrady requested review from EmmaSimon and removed request for a team December 16, 2024 17:34
@@ -98,7 +98,7 @@ fun HomeMapView(
var railRouteLineData: List<RouteLineData>? by remember { mutableStateOf(null) }
var stopSourceData: FeatureCollection? by remember { mutableStateOf(null) }

val now = timer(updateInterval = 10.seconds)
val now = timer(updateInterval = 300.seconds)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this was causing lag / jankiness and saw the iOS value is 300 seconds, so updated to match.

This can be further improved by moving getAlertsByStop to a background thread for calculation since the calculation alone takes ~1 second. That wasn't straight forward to do within the remember func though, and would probably need to be moved into the ViewModel. I can make a separate ticket for that

connectToPredictions()
override fun onCleared() {
super.onCleared()
predictionsRepository.disconnect()
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Should this call the vm's disconnect() instead?

@KaylaBrady KaylaBrady mentioned this pull request Dec 17, 2024
3 tasks
@KaylaBrady KaylaBrady force-pushed the kb-android-backgrounding branch from 46dc1ed to 8b36601 Compare December 17, 2024 20:32
Base automatically changed from kb-android-backgrounding to main December 17, 2024 21:42
@KaylaBrady KaylaBrady force-pushed the kb-android-backgrounding-2 branch from b2d38b7 to b5740cd Compare December 17, 2024 21:44
@KaylaBrady
Copy link
Collaborator Author

android-only change, ran iOS app locally and confirmed unaffected. Merging before iOS workflows complete

@KaylaBrady KaylaBrady merged commit f80ff4c into main Dec 17, 2024
5 of 7 checks passed
@KaylaBrady KaylaBrady deleted the kb-android-backgrounding-2 branch December 17, 2024 22:12
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.

2 participants