wip: draft of foundational data types with a bunch of placeholders

[KaylaBrady]

Summary

Ticket: Group by direction | Foundational data structure

What is this PR for?

Currently a draft to seek feedback on this approach before I sink more time into actually getting all the pieces together.

This introduces a new structure, RouteCardData as a replacement for the existing NearbyStaticData, PatternsByStop, etc. classes which have grown in complexity over time as we organically added functionality. These new structures aim to simplify the data representation with simplified naming and hiearchy.

I've tried on using the builder pattern here. It felt like a good option since we have a complex series of steps; hopefully pulling all those steps into discrete functions will help with readability.

The supporing Builder classes are strongly based in the NearbyHierarchy classes. I opted to make a new file instead of modifying the old pieces for simplicity. There are a few key differences though:

• NearbyHierarchy was intended to support adding headsigns to the list of headsigns at a stop for a station that might not otherwise be there. We don't need that functionality since we are no longer grouping by headsigns, which I think allows us to simplify some of the functionality.
  • NearbyHiearchy uses generics to achieve merging together hierarchies of different types. With the above simplification, I think there will be few enough steps where we can omit the use of generics and instead use a LeafBuilder type to represent the leaf data that we are incrementally building up at each step.
• NearbyHiearchy was an internal data type that was ultimately transformed into PatternsByStop. In this approach, I tried to keep the internal pieces limited to the supporting Builder classes and expose a top-level structure that we could use comfortably in views models + views.

Please leave any feedback you have at this stage - structural, naming, nitpicks, new ideas, all welcome!

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 in alphabetical order
• Expensive calculations are run in withContext(Dispatchers.Default) where possible

Testing

What testing have you done?

[KaylaBrady]

We have used Transit in some places to mean LineOrRoute, could adopt that here also. TBH I always call the UI component RouteCard in my head when thinking about it and just keep the caveat in my head that it also is a line card b/c GL is weird. We could also consider calling it RouteCard and just document that sometimes we are treating lines like routes sometimes.

Very to other naming ideas!

[EmmaSimon]

I'm good with that, I've had some regret about the structure of the RouteCard component, I think it would be a lot cleaner if there was just a single RouteCard with a spec object to handle routes and lines differently, so that we could get rid of LineCard and TransitCard entirely, but it was created really early on when I was still very much fumbling around trying to figure out how Swift worked, then it just got copied into Android.

[KaylaBrady]

Yeah the existing pattern made sense to me while we were doing it, but I like the idea of moving towards a spec! We should def keep that in mind as we do the barebones UX - I don't have a sense yet of whether we'd have an opportunity to make that change then while introducing new components, or if we'll just want to make a separate refactor ticket for after these data types are fully adopted

[boringcactus]

What if we had a RouteGroup that contains a usually-single-element routes: List<Route> and a usually-null line: Line?

[KaylaBrady]

Oh interesting, so we don't have to do as much special casing? Though I guess then we'd probably end up doing conditional expressions in the UI depending on whether line is null or not, so I'd lean towards still preferring representing them as distinct data types. But I am open to this option - maybe whoever picks up the barebones UX ticket can use that as an opportunity to re-consider if this will make things easier to work with & switch if so, since it should be a fairly minor change.

[KaylaBrady]

pulled props from withRealtimeInfoViaTripHeadsigns

[EmmaSimon]

I don't totally understand the utility of includeMinorAlerts being on the top level of creating NearbyStaticData in the existing code, is this still necessary?

[KaylaBrady]

It looks like we only set it to true for stop details. I assume some of these other booleans are also driven by the page type - maybe we can condense them all down into a context variable like we use for TripInstantDisplay to simplify the number of params here

[KaylaBrady]

Just did a push with that change to pass 1 context var instead of 3 booleans.

[boringcactus]

As far as I remember, LineOrRoute.Line containing the routes is what causes the Kenmore bug: the static data knows that only B, C, D go to Kenmore, but the realtime data doesn't know that, and it can't build a LineOrRoute.Line without a list of routes, so it assumes B, C, D, E which then fails to match the static data because the LineOrRoute.Lines are different.

In the final object, we'll probably still need the list of routes included in a line, but in the intermediate state it might work to use just the line as a key and then have a separate Map<String, List<Route>> for the routes-by-line attested by the static data at any given stop. It might be simpler to just count all GL branches as always included at every GL stop, though; I'm not sure if that'd actually break anything but if it wouldn't it'd be way simpler.

[KaylaBrady]

Ah thanks for that detail, I forgot the specifics of the Kenmore bug. Will think through if there are any further issues from always including all GL routes

[boringcactus]

I think builder pattern makes a lot of sense here to keep the state manageable, and I agree that it makes sense to try to subsume the entire old code path including NearbyStaticData and RealtimePatterns rather than only defining temporary structures for within the call to withRealtimeInfo - I think I was approaching trip headsigns as just the feature itself rather than as a refactor opportunity to enable the feature and pay down some tech debt.

[KaylaBrady]

Thanks for the review @boringcactus!

I think I was approaching trip headsigns as just the feature itself rather than as a refactor opportunity to enable the feature and pay down some tech debt.

I 100% think this was the right approach at the time of trip headsigns! The larger refactor was definitely out of scope at that point w/ the release timeline. Glad we have some more time to revisit it now that the initial releases are behind us!