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.

Sign up for GitHub

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

refactor(iOS): Move stop details fetching into VM #599

Merged
merged 6 commits into from
Dec 19, 2024
+292 −303
Merged

refactor(iOS): Move stop details fetching into VM #599

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9582f34
refactor(iOS): Move stop details fetching into VM
EmmaSimon Dec 18, 2024
5655ffa
Merge branch 'main' into es-stop-details-page-vm-refactor
EmmaSimon Dec 19, 2024
a6c0736
Merge branch 'main' into es-stop-details-page-vm-refactor
EmmaSimon Dec 19, 2024
d998583
Merge branch 'main' into es-stop-details-page-vm-refactor
EmmaSimon Dec 19, 2024
262fdd2
fix: Restore existing appear tracking
EmmaSimon Dec 19, 2024
35910b5
Merge branch 'main' into es-stop-details-page-vm-refactor
EmmaSimon Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions iosApp/iosApp.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@
9A2A6B6E2D07F7EB00E39AF5 /* StopDetailsViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A2A6B6D2D07F7EB00E39AF5 /* StopDetailsViewModelTests.swift */; };
9A2BCBDE2CED365200FB2913 /* StopDetailsPageTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A2BCBDD2CED365200FB2913 /* StopDetailsPageTests.swift */; };
9A2BCBE02CED366300FB2913 /* StopDetailsViewTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A2BCBDF2CED366300FB2913 /* StopDetailsViewTests.swift */; };
9A2BCBE22CEE8A9F00FB2913 /* StopDetailsPageHandlerExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A2BCBE12CEE8A9F00FB2913 /* StopDetailsPageHandlerExtension.swift */; };
9A320F962CD3E4CF0096D7B1 /* UpcomingTripAccessibilityFormatters.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A320F952CD3E4CF0096D7B1 /* UpcomingTripAccessibilityFormatters.swift */; };
9A37F3052BACCC40001714FE /* DoubleRoundedExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A37F3042BACCC40001714FE /* DoubleRoundedExtension.swift */; };
9A37F3072BACCCA5001714FE /* CoordinateExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A37F3062BACCCA5001714FE /* CoordinateExtension.swift */; };
Expand Down Expand Up @@ -423,7 +422,6 @@
9A2A6B6D2D07F7EB00E39AF5 /* StopDetailsViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StopDetailsViewModelTests.swift; sourceTree = "<group>"; };
9A2BCBDD2CED365200FB2913 /* StopDetailsPageTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StopDetailsPageTests.swift; sourceTree = "<group>"; };
9A2BCBDF2CED366300FB2913 /* StopDetailsViewTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StopDetailsViewTests.swift; sourceTree = "<group>"; };
9A2BCBE12CEE8A9F00FB2913 /* StopDetailsPageHandlerExtension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StopDetailsPageHandlerExtension.swift; sourceTree = "<group>"; };
9A320F952CD3E4CF0096D7B1 /* UpcomingTripAccessibilityFormatters.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UpcomingTripAccessibilityFormatters.swift; sourceTree = "<group>"; };
9A37F3042BACCC40001714FE /* DoubleRoundedExtension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DoubleRoundedExtension.swift; sourceTree = "<group>"; };
9A37F3062BACCCA5001714FE /* CoordinateExtension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoordinateExtension.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1125,7 +1123,6 @@
9AF29DF52CF54319005AA4A3 /* DepartureTile.swift */,
9AF093772BD943A4001DF39F /* DirectionPicker.swift */,
9ACE4FCF2CE6707900FEB006 /* StopDetailsPage.swift */,
9A2BCBE12CEE8A9F00FB2913 /* StopDetailsPageHandlerExtension.swift */,
9AF29E052CFE2C4C005AA4A3 /* StopDetailsFilteredDepartureDetails.swift */,
9AF29DFF2CF63330005AA4A3 /* StopDetailsFilteredHeader.swift */,
9AF29DF72CF5454E005AA4A3 /* StopDetailsFilteredView.swift */,
Expand Down Expand Up @@ -1583,7 +1580,6 @@
6E2027902BD989AC0037554F /* ProductionAppView.swift in Sources */,
6E04D4302C1A17340055FD99 /* StopDeparturesSummaryList.swift in Sources */,
9A74A2112BE2D71400E57102 /* AnnotationLabel.swift in Sources */,
9A2BCBE22CEE8A9F00FB2913 /* StopDetailsPageHandlerExtension.swift in Sources */,
6EEF219E2BF2927E0023A3E9 /* VehicleCardView.swift in Sources */,
9A9E7DCF2C2200C9000DA1FD /* LineHeader.swift in Sources */,
8C05C5812CD568DE000381E8 /* MoreNavLink.swift in Sources */,
Expand Down
14 changes: 8 additions & 6 deletions iosApp/iosApp/ContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,16 +286,18 @@ struct ContentView: View {
viewportProvider: viewportProvider
)
.toolbar(.hidden, for: .tabBar)
.onAppear {
let filtered = stopFilter != nil
screenTracker.track(
screen: filtered ? .stopDetailsFiltered : .stopDetailsUnfiltered
)
}
}
// Set id per stop so that transitioning from one stop to another is handled by removing
// the existing stop view & creating a new one
.id(stopId)
.onChange(of: stopId) { nextStopId in stopDetailsVM.handleStopChange(nextStopId) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

question(non-blocking): can the stopId field be removed as a prop of StopDetailsPage entirely? That way it can't get out of sync with what is in the VM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting. That does seem temping, but I don't think it's possible, it might cause issues with the loading state and page transitions. The ID in StopData is only populated once the schedules have loaded, so StopDetailsPage couldn't start checking auto filters until after the network request, and it uses the prop stop ID to make sure that it doesn't change the departures if it doesn't match the ID in StopData, which I think was the fix for the issue I was seeing with double prediction loads.

.onAppear {
stopDetailsVM.handleStopAppear(stopId)
screenTracker.track(
screen: stopFilter != nil ? .stopDetailsFiltered : .stopDetailsUnfiltered
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry @EmmaSimon one last thought - does this affect how visits to stop details are counted in GA? Wondering if it would alter the stats about how users progress through the funnel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might actually, I should move it back to where it was and add a second onAppear here.

)
}
.onDisappear { stopDetailsVM.leaveStopPredictions() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion(non-blocking): I think the calls to stopDetailsVM.handleStopAppear and leaveStopPredictions might feel more natural within the StopDetailsPage. Is there a benefit to having them a level higher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my notes in the PR description, StopDetailsPage gets torn down and recreated every time the stop ID or stop filter changes, which was resulting in some issues. The onChange handles disconnecting and reconnecting from the channel, so we only actually want onAppear or onDisappear to be called when the page is changed entirely, not just changed to a different stop/filter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, thank you, I wasn't connecting the dots on how this fit in with that.

One other non-blocking thought that comes to mind based on that - it could help to clarify the behavior to add a new component to the stop details hierarchy to clarify w/ naming and/or comments that there is this stable stop details container that is unchanging and presented once, and that the inner piece is more volatile. Something like StopDetailsPageContainer (stable) -> StopDetailsPage -> StopDetailsView.

Page vs Container doesn't feel meaningfully distinctive as a name, but I think clarifying the difference between these layers could help with understandability down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I like that idea, I can try to fit that into one of the follow up PRs!

.transition(transition)

case let .legacyStopDetails(stop, filter):
Expand Down
52 changes: 42 additions & 10 deletions iosApp/iosApp/Pages/StopDetails/StopDetailsPage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,37 +72,69 @@ struct StopDetailsPage: View {

var body: some View {
stopDetails
.onChange(of: stopId) { nextStopId in changeStop(nextStopId) }
.onChange(of: stopDetailsVM.global) { _ in updateDepartures() }
.onChange(of: stopDetailsVM.pinnedRoutes) { _ in updateDepartures() }
.onChange(of: stopDetailsVM.predictionsByStop) { _ in updateDepartures() }
.onChange(of: stopDetailsVM.schedulesResponse) { _ in updateDepartures() }
.onChange(of: stopDetailsVM.stopData) { stopData in
errorBannerVM.loadingWhenPredictionsStale = !(stopData?.predictionsLoaded ?? true)
updateDepartures()
}
.onChange(of: stopFilter) { nextStopFilter in setTripFilter(stopFilter: nextStopFilter) }
.onChange(of: internalDepartures) { _ in
let nextStopFilter = setStopFilter()
setTripFilter(stopFilter: nextStopFilter)
}
.onAppear { loadEverything() }
.onReceive(inspection.notice) { inspection.visit(self, $0) }
.task(id: stopId) {
while !Task.isCancelled {
now = Date.now
updateDepartures()
checkPredictionsStale()
stopDetailsVM.checkStopPredictionsStale()
try? await Task.sleep(for: .seconds(5))
}
}
.onDisappear { stopDetailsVM.leavePredictions() }
.onReceive(inspection.notice) { inspection.visit(self, $0) }
.withScenePhaseHandlers(
onActive: {
stopDetailsVM.returnFromBackground()
joinPredictions()
stopDetailsVM.joinStopPredictions(stopId)
},
onInactive: stopDetailsVM.leavePredictions,
onInactive: stopDetailsVM.leaveStopPredictions,
onBackground: {
stopDetailsVM.leavePredictions()
stopDetailsVM.leaveStopPredictions()
errorBannerVM.loadingWhenPredictionsStale = true
}
)
}

func setStopFilter() -> StopDetailsFilter? {
let nextStopFilter = stopFilter ?? internalDepartures?.autoStopFilter()
if stopFilter != nextStopFilter {
nearbyVM.setLastStopDetailsFilter(stopId, nextStopFilter)
}
return nextStopFilter
}

func setTripFilter(stopFilter: StopDetailsFilter?) {
let tripFilter = internalDepartures?.autoTripFilter(
stopFilter: stopFilter,
currentTripFilter: tripFilter,
filterAtTime: now.toKotlinInstant()
)
nearbyVM.setLastTripDetailsFilter(stopId, tripFilter)
}

func updateDepartures() {
Task {
if stopId != stopDetailsVM.stopData?.stopId { return }
let nextDepartures = stopDetailsVM.getDepartures(
stopId: stopId,
alerts: nearbyVM.alerts,
useTripHeadsigns: nearbyVM.tripHeadsignsEnabled,
now: now
)
Task { @MainActor in
nearbyVM.setDepartures(stopId, nextDepartures)
internalDepartures = nextDepartures
}
}
}
}
111 changes: 0 additions & 111 deletions iosApp/iosApp/Pages/StopDetails/StopDetailsPageHandlerExtension.swift
View file
Open in desktop

This file was deleted.

Loading
Loading