WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@millanp
Copy link
Contributor

@millanp millanp commented Mar 28, 2025

Adds a badge next to Link Light Rail arrival times indicating the number of cars on the arriving train. Looks like this:

lightrailbadgecars

@amrhossamdev amrhossamdev requested a review from Copilot March 28, 2025 04:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a badge next to Link Light Rail arrival times that displays the number of cars on the arriving train.

  • Introduces a new TextView (carCount) in the arrivals list adapter to display the car count.
  • Implements a new method getNumCars() in ObaArrivalInfo to derive and format the number of cars from the vehicle ID.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListAdapterStyleA.java Adds UI elements and logic for displaying the car count badge.
onebusaway-android/src/main/java/org/onebusaway/android/io/elements/ObaArrivalInfo.java Implements logic to extract the number of cars from the vehicle ID.
Files not reviewed (1)
  • onebusaway-android/src/main/res/layout/arrivals_list_item.xml: Language not supported
Comments suppressed due to low confidence (1)

onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListAdapterStyleA.java:145

  • Verify that carCount's background is always an instance of GradientDrawable to prevent a potential ClassCastException. Consider checking with 'instanceof' before casting or ensuring the drawable resource always returns a GradientDrawable.
((GradientDrawable) carCount.getBackground()).setColor(color);

@amrhossamdev amrhossamdev self-requested a review March 28, 2025 04:56
Copy link
Member

@amrhossamdev amrhossamdev left a comment

Choose a reason for hiding this comment

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

Well done, @millanp... I was testing it with a large font. Can you figure out how to make that responsive 🤔

image

@millanp
Copy link
Contributor Author

millanp commented Mar 28, 2025

Thanks for getting back so quickly! I just made the "4 car" badge much smaller, and this seems to get rid of the problem with large font size (?) I thought a bit about making it responsive, but I'm not sure if it's worth the effort, given that most of the existing UI elements are not responsive and given that statically shrinking it seems to solve the problem. Let me know your thoughts.

@barbeau
Copy link
Member

barbeau commented Mar 28, 2025

This is cool! Note that this probably won't work outside of Seattle (and maybe just Link Light Rail) because the vehicle ID format isn't standardized.

@millanp
Copy link
Contributor Author

millanp commented Mar 28, 2025

Interesting, I didn't know that this wasn't standardized! This vehicle ID format has been stable for quite some time for the link light rail, and I haven't seen it in any other transit systems or routes. Do you think this is a blocker for displaying this info on the app?

@barbeau
Copy link
Member

barbeau commented Mar 28, 2025

Do you think this is a blocker for displaying this info on the app?

That's up to @amrhossamdev 🙂

@millanp
Copy link
Contributor Author

millanp commented Mar 28, 2025

My case for including this is that it works right now across all transit systems and routes: it either displays an accurate car count, or it doesn't display anything. The format of vehicle IDs has been stable for a long time, certainly for over a year.

If something breaks it is easy to turn this feature off, and the worst that can happen is briefly displaying an inaccurate car count.

Thanks again for looking at this!

@amrhossamdev
Copy link
Member

amrhossamdev commented Mar 29, 2025

Do you guarantee that it won’t show up if we don’t have a stable vehicle ID, or will it cause a problem? 🤔 @millanp

if the vehicle id == null or there is no car number, it will be gone right?

@amrhossamdev
Copy link
Member

For responsive, I think it's looking great now well done @millanp 👏

image

Copy link
Member

@amrhossamdev amrhossamdev left a comment

Choose a reason for hiding this comment

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

Well done @millanp 👏 please check my comments.

@amrhossamdev
Copy link
Member

Thanks for your participation, @barbeau! Could you elaborate more on the different formats of vehicle IDs? Are you suggesting that we need to find a way to parse each format differently based on the region? 🤔 Thanks!

@barbeau
Copy link
Member

barbeau commented Mar 29, 2025

Could you elaborate more on the different formats of vehicle IDs?

AFAIK there is typically no defined format, assuming these come from GTFS Realtime data sources. IDs could be 1234, a UUID, or passed through from some internal data source from the agency. So you'd need to investigate each agency/AVL vendor to see if they use some kind of internal proprietary format that happens to have the number of cars embedded similar to Link Light Rail.

@millanp
Copy link
Contributor Author

millanp commented Mar 29, 2025

Another option is I could explicitly turn this off for everything but the link light rail

@amrhossamdev
Copy link
Member

@millanp Looks good to me! Would love to hear your thoughts as well, @aaronbrethorst.

@millanp
Copy link
Contributor Author

millanp commented Mar 30, 2025

Forgot to say this earlier, but one thing I find cool is that no other app gives this information! This would be useful data available only on OneBusAway.

@aaronbrethorst
Copy link
Member

I agree with @amrhossamdev's feedback, and I like your approach, @millanp, regarding "Another option is I could explicitly turn this off for everything but the link light rail". I'd like to see the following changes before merging:

  1. Implement all of the code review feedback that Amr offered
  2. Only enable this feature when the region is Puget Sound and the agency in question is 40 (I think you should have access to that information...If not, then just check for matching region)

thanks!

@johnhelmuth-st
Copy link

Rather than relying on parsing an ID whose format is not included in any specification and may change at any time, I might recommend using the experimental GTFS-RT VehiclePosition's multi_carriage_details property, as a more reliable way to get the number of cars on a train.

Sound Transit publishes its light rail GTFS RT VehiclePosition feed via the Puget Sound region's OneBusAway server, which does not support the multi_carriage_details property, as far as I can tell. However, Sound Transit's upstream CAD/AVL system for their light rail service does produce the multi_carriage_details property, in preparation for including congestion information per-car. (ST's APC system still needs to be wired into that feed for the congestion information to be included.)

A way forward for this feature is to add support to the server's OBA APIs to include the multi_carriage property, and then use that, instead of the vehicle ID, to count the number of cars in the train.

@aaronbrethorst
Copy link
Member

@johnhelmuth-st If the OBA REST API server added support for ingesting and publishing this data, would ST update their deployed OBA server version to include that?

@johnhelmuth-st
Copy link

I am checking with the vendor that supports the OBA service for us.

@johnhelmuth-st
Copy link

@aaronbrethorst We will get the code deployed to pass through the multiple_carriage_details GTFS RT feeds for Trip Updates and Vehicle Positions once it is available.

@amrhossamdev
Copy link
Member

@millanp any updates?

@millanp
Copy link
Contributor Author

millanp commented Apr 6, 2025

Should I proceed with Aaron's idea of activating the feature only for region 40?

@amrhossamdev
Copy link
Member

@aaronbrethorst Hi, Aaron. Can we have your opinion on that?

@aaronbrethorst
Copy link
Member

Yeah, go for it.

@millanp
Copy link
Contributor Author

millanp commented Apr 25, 2025

Sorry for the delay here! I addressed the code review comments and restricted the feature to the Puget Sound region. I couldn't find an easy way to extract the agency ID from the arrival info, but hopefully this is OK. Let me know if there are more issues.

@amrhossamdev amrhossamdev self-requested a review April 25, 2025 23:26
Copy link
Member

@amrhossamdev amrhossamdev left a comment

Choose a reason for hiding this comment

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

Well done @millanp 👏 please look at my comments before merging.

@millanp
Copy link
Contributor Author

millanp commented Apr 26, 2025

I don't think I can merge it myself, but I'm not sure. Also @aaronbrethorst @amrhossamdev do you think we can get this out to the Play Store soon? I'm excited to see it in the wild!

@amrhossamdev amrhossamdev merged commit 24ec490 into OneBusAway:main Apr 26, 2025
2 checks passed
@amrhossamdev
Copy link
Member

@millanp it will be uploaded to the beta channel soon 🚀

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.

5 participants