fix: Show correct headsigns and groupings on all GL branches #472
Conversation
Since some atypical patterns from every westbound branch serve the GLX branches, now that atypical patterns are included to handle properly displaying those direction labels in predictions, it was causing predictions on the GLX to always be grouped under a direction label, even if only typical routes serving the expected headsign were running at the moment. This takes each grouped static direction, and checks it against typical and realtime patterns. If there is only a single typical and/or predicted route going in a direction, then the grouped direction is instead split up into however many headsigns are typical or predicted in that direction.
EmmaSimon
force-pushed
the
es-gl-atypical-direction-bugs
branch
from
42092d3
to
d9c7749
Compare
| @@ -11,11 +11,12 @@ data class Direction( | |||
| directionId: Int, | |||
| route: Route, | |||
| stop: Stop? = null, | |||
| routeStopIds: List<String>? = null | |||
| routeStopIds: List<String>? = null, | |||
| patternDestination: String? = null | |||
There was a problem hiding this comment.
suggestion (non-blocking): We haven't been doing much in the way of constructor docs, but consider adding some documentation here about the fallback behavior
| } else { | ||
| when (val it = typicalHere.first()) { | ||
| is StaticPatterns.ByDirection -> it.direction | ||
| is StaticPatterns.ByHeadsign -> it.direction |
There was a problem hiding this comment.
suggestion: A comment here about the fallback behavior would be useful!
Direction is only included if this is a ByHeadsign within a line. Is that right? Part of me wonders if introducing a 3rd type could help make this case be more explicit... but that could be more trouble than it is worth.
There was a problem hiding this comment.
That's right, yeah. I think that would add additional confusion to all of the other places we use this, and here we would still need to do something in the case where it was a regular StaticPatterns.ByHeadsign, even though that should never be possible.
| // Even if a direction can serve multiple routes according to the static data, it's possible | ||
| // that only one of those routes is typical, in which case we don't want to display it as a | ||
| // grouped direction in the UI. If there are only predicted trips on a single route, this | ||
| // will split the grouped direction up into as many headsign rows as there are headsigns. |
There was a problem hiding this comment.
praise: this is very descriptive, thank you!
| // that only one of those routes is typical, in which case we don't want to display it as a | ||
| // grouped direction in the UI. If there are only predicted trips on a single route, this | ||
| // will split the grouped direction up into as many headsign rows as there are headsigns. | ||
| fun resolveRealtimePatternForDirection( |
There was a problem hiding this comment.
suggestion: this function is pretty long - consider breaking out some of the helper steps into separate functions
| return headsignsToDisplay.map { (headsign, patternIds) -> | ||
| val patternsForHeadsign = patternIds.mapNotNull { patternsById[it] } | ||
| RealtimePatterns.ByHeadsign( | ||
| NearbyStaticData.StaticPatterns.ByHeadsign( |
There was a problem hiding this comment.
question (non-blocking): would it be possible / simplify things at all to use the RealtimePatterns.ByHeadsign constructor that doesn't take a StaticPatterns.ByHeadsign?
There was a problem hiding this comment.
I actually set it up like that initially, by adding a new constructor for RealtimePatterns.ByHeadsign that took a StaticPatterns.ByDirection, then decided that doing it like this was actually the most straightforward approach.
| routeEntry.value.groupBy({ it.first }, { it.second }) | ||
| } | ||
|
|
||
| val predictedPatternsByRoute = |
There was a problem hiding this comment.
question (non-blocking): Confirming my own learning - if we skip the step of checking predicted patterns by route, then we'd break down into showing individual headsigns prematurely because there could be some late-at-night atypical headsigns.
There was a problem hiding this comment.
Yes, not even just late at night, they can happen any time. If an atypical route is predicted, we want to group it into "Copley & West" along with any typical Heath St trips, but if there are no atypical trips predicted currently, then it's safe to just show the single headsign.
| staticData.patterns | ||
| .filter { | ||
| tripsByPattern[it.id]?.any { trip -> | ||
| trip.prediction != null && trip.vehicle?.tripId == trip.trip.id |
There was a problem hiding this comment.
question: why check the vehicle's trip ID here? Wouldn't we still show the prediction even if the vehicle is on another trip?
There was a problem hiding this comment.
Hmm, maybe it's a bad idea. I wanted to prevent cases where we would be displaying the grouping without actually displaying the atypical trip. So like, if there was a prediction for a C which is currently approaching Medford Tufts to start a new trip, but two E trains in front of it heading west, we would still show the group even though only the E trains are shown in the upcoming predictions. But maybe that's fine and this is too cautious and will hide predictions when we don't want it to.
| @@ -300,4 +300,309 @@ class PatternsByStopTest { | |||
| assertEquals(listOf(alert), patternsByStop.alertsHereFor(0, global)) | |||
| assertEquals(listOf(alert), patternsByStop.alertsHereFor(1, global)) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `resolveRealtimePatternForDirection splits direction groups into headsigns when needed`() { | |||
There was a problem hiding this comment.
suggestion: I'm having a bit of a hard time understanding this test since there is a lot of data defined & different cases asserted. Consider breaking this down into smaller tests that potentially share relevant data
Summary
Ticket: GL showing duplicate Eastbound @ Gov Center with no data
Most of the added lines here are in the tests, sorry for the large PR! 😅
After #454, there were issues introduced where we would only ever display grouped nearby headsigns on the Medford/Tufts branch. This changes the headsign grouping to split grouped routes into separate headsigns if only a single route pattern is predicted in one direction. This means that it's technically possible (but very unlikely) for a grouping to be displayed when an atypical trip is predicted but has multiple other predictions before it and so wouldn't be displayed in nearby, but since this only applies to a stretch of 5 stops that rarely have more than 2 trains headed inbound at any time, this shouldn't happen often.
This also includes changes to use the pattern headsign to generate directions, since the destination names in the route only apply to the route's typical pattern, and includes a default null
Directionobject in theStaticPatterns.ByHeadsign, because we still want the special casedDirectionobject (like "Park St & North") used ingroupedDirectionto set the overall directions inStopPatterns.ForLinefor displaying in the filtered stop details direction toggle.Screenshots of correct behavior after these changes:
Testing
Added tests for newly added functionality and updated Direction behavior.