Abstract London Underground information from watch-side logic#2
Abstract London Underground information from watch-side logic#2CometDog wants to merge 1 commit intoC-D-Lewis:masterfrom
Conversation
|
Thank you! I will read this today. I really like the idea of extending the UI to support more transit systems (and provide a pluggable way for others to add more!) - ideally for me the one repo would be able to continue publishing Tube Status in addition to some other version with all the extra backends. Maybe two package files? |
|
I think two package files would make the most sense since I wouldn't want to lose the purpose-built nature of Tube Status. The only potential deviation would be if one wanted to provide a way in-app, rather than through configuration, to select different backend systems. That code would have to be gated behind something that wouldn't invade Tube Status either functionally or in app size. One piece I hadn't considered either is the backend service for Tube Status that seems to be pushing Timelime Pins or App Glances. That would probably be something that would remain exclusive to Tube Status unless one felt compelled to rebuild that for every backend. So having the separate package is important for that as well |
C-D-Lewis
left a comment
There was a problem hiding this comment.
Thanks! With a way to publish Tube Status and some new app that has additional transit system options, this is a neat abstraction!
| reason = reason?.substring(0, MAX_REASON_LENGTH - 4) + '...'; | ||
| } | ||
| for (const config of configs) { | ||
| const dict = { |
There was a problem hiding this comment.
I think AppMessage buffers can be large enough to send config + status in one message
| if (packet_contains_key(iter, MESSAGE_KEY_LineStatusSeverity)) { | ||
| line_data->severity = (StatusSeverity)packet_get_integer(iter, MESSAGE_KEY_LineStatusSeverity); | ||
| } else { | ||
| line_data->severity = StatusSeverityGood; |
There was a problem hiding this comment.
I'm wary of assuming good if data goes missing - should there be an 'unknown' state?
| /** | ||
| * London Underground / TfL transit backend | ||
| */ | ||
| class LondonUndergroundBackend extends TransitBackend { |
There was a problem hiding this comment.
'TfL' (Transport for London) might be a more succinct term to use in code symbols than 'londonunderground'
There was a problem hiding this comment.
I can do this. I switched it because conceptually other lines could exist so I didn't want to name it after the authority versus the actual line name.
However, and especially for the original Tube Status implementation, I can still change it back if that makes more sense for you
There was a problem hiding this comment.
I used TfL because they provide the data, and it encompasses all the available lines. No big deal really.
| index: number; | ||
| name: string; | ||
| color: number; // Hex color as uint32 | ||
| striped: boolean; // Renders a visual difference, can be used to indicate special lines |
There was a problem hiding this comment.
The use of the stripe on TfL lines seems arbitrary or biased towards newer ones that have re-used colors or colors similar to older lines
There was a problem hiding this comment.
I figured keeping it in the abstraction could be useful for other systems that are or aren't as arbitrary as London Underground.
For example JR East Kanto specifies a few lines with short codes like JY or JK while other lines, mainly local lines, lack short codes. On my own version, I'm passing those with short codes as stripes to designate that.
Do you think this should be more internal in the UI? Where if a color is to be reused it'll stripe it?
There was a problem hiding this comment.
I think the backend should just say which are striped to align with a map (TfL) where known or however the dev wants to make use of it themselves.
| const NUM_LINES = 19; | ||
| /** Max reason length */ | ||
| const MAX_REASON_LENGTH = 512; | ||
| // TODO: Configure ACTIVE_BACKEND with settings in the future |
There was a problem hiding this comment.
This could be a simple Clay config page
There was a problem hiding this comment.
Yeah I can remove the TODO until a PR is in place to handle two separate apps. I had considered this PR just abstraction and no route to a second .json file, so it doesn't actually need config at all, as Tube Status, right now.
I am actually considering something in the UI to enable this rather than Clay. Only because there are cases you'd have multiple lines overlapping and want to switch (JR East Kanto and Tokyo Metro operate in the same stations at times)
There was a problem hiding this comment.
Having an option in the UI makes sense - switch without getting your phone out
|
I'll be following up with all of this but I'll also be on vacation for 3 weeks so it won't be until after that that I round back! |
|
@CometDog No rush! Enjoy the vacation! I'll prototype multiple package files one day soon |
|
I'm back and at it. I'm working at my day job again so it'll be after work hours I get to this but I plan to write out my mindset for Tube Status vs Transit Status (what I'm currently generically calling it) as a proposal for you and potentially some commits to either demonstrate or fully propose the multiple packages for building. I intended for this to just be a more simple abstraction PR that causes no impact to Tube Status, but I think without at least a POC of how Transit Status would function as an extension of Tube Status, some of this either doesn't make sense or isn't fully solved enough to merge. As an aside, I've tested my implementations I have for JR East Kanto and JR Central Tokaido-Sanyo Shinkansen during very bad delays recently and it works exactly as I'd expect Tube Status to work for London. Multiple alerts for the same line (multiple direction, pass thrus, etc) work as well |
|
Thanks! There's no rush. I experimented with the package files but since your PR is from a main branch I could not add any commits. |
|
I'm currently finishing out a POC of the two apps side by side with transit system selection to hopefully give a fuller picture. When I do I'll have it in another branch to maybe restart the review process there (and I'll address comments here to not have to carry them over). Then we can compare strategies for separate packages because it's definitely a no man's land. Sorry about the clunk on this review so far. I thought breaking it apart would be easier but it seems it wasn't going to be. Then posting it right before my vacation was a choice. |
|
This is my concept - a script that builds one of two app package definitions, and use of a https://github.com/C-D-Lewis/pebble-dev/compare/multi-packages?expand=1 |
|
I did something fairly different, using just the wscript though it could just get split to a separate file just as well. It uses a separate package.json only to override values from the existing Tube Status, so in this case just the name and UUID. Then it does similar where it defines something you can check in C. This is all off the build command so you just pass the variant flag to pebble build. It puts the state of the files back to Tube Status after build finishes, so it's only ever Transit Status at build time. The work I'm doing now is to make it so Tube Status can follow the same code flow as Transit Status, just omitting the ability to select a system and instead always requesting London. I've got that all figured out but I am working on the selection screen now. I'll have something fairly complete in a few days as time allows and then I'll spin up a new PR to discuss that completely, and look over your work above on how to manage two apps in one project as well |
|
Lets switch this over to the full POC since that seems like it'll be more relevant to the conversation than just this PR which was generifying Tube Status on its own. I've taken notes of your comments here and will address them in that PR as well |
We've discussed this briefly in private but I wanted to raise this as a reivew-able for if you're interested with no timeline or pressure to merge back at all.
For my own purposes to make Tube Status work for JR East Kanto, I've gone through a few iterations of design to get the London Underground information off of the watch so it can just take any data from the phone and present it the same way it does exactly today.
The goals here are:
Changes:
Again I don't need you to merge this or not. I am going to continue working on it as my own project to be able to switch between transit systems that I feel are useful to me. But if it feels sufficient to you or when you have time discuss it, I think even if you never decide to support more systems than London in the Tube Status app, this at least sets up an easier system to work with overall even just for London.
London Underground:

JR East Kanto:

NYC MTA Subways:
