Add home screen widget (#1075)#1538
Open
robertgodfrey wants to merge 1 commit into
Open
Conversation
robertgodfrey
force-pushed
the
stop-widget
branch
from
March 20, 2026 06:03
2c41ece to
76ff9e3
Compare
Member
|
Neat, thanks @robertgodfrey! |
Member
|
@robertgodfrey it looks like CI is failing because you need to add the new resources to our example white label apps (Agency X and Agency Y) as well. Let me know if you have questions: https://opentransitsoftwarefoundation.org/join-our-slack |
robertgodfrey
force-pushed
the
stop-widget
branch
from
March 20, 2026 06:10
76ff9e3 to
a6541d3
Compare
Contributor
Author
|
Ah! Okay got it, I'll get the tests working tomorrow Edit - looks like that last change I pushed did it |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
✅ Files skipped from review due to trivial changes (14)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 WalkthroughWalkthroughAdds a Stop Times home-screen widget with configuration and stop picker UIs, background worker + caching, an AppWidgetProvider that schedules refreshes and renders cached arrivals, and required layouts, drawables, strings, styles, and manifest entries. ChangesStop Times Widget Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.java (1)
33-38: ⚡ Quick winConsider adding validation and defensive copying.
For defensive coding and true immutability:
- Validate that required fields (
stopId,stopName,widgetName) are non-null- Make a defensive copy of
routeShortNamesto prevent external mutation🛡️ Suggested improvements
public WidgetConfig(String stopId, String stopName, String widgetName, Map<String, String> routeShortNames) { + if (stopId == null || stopName == null || widgetName == null) { + throw new IllegalArgumentException("stopId, stopName, and widgetName must not be null"); + } this.stopId = stopId; this.stopName = stopName; this.widgetName = widgetName; - this.routeShortNames = routeShortNames; + this.routeShortNames = routeShortNames != null ? new HashMap<>(routeShortNames) : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.java` around lines 33 - 38, The WidgetConfig constructor currently assigns parameters directly; add null checks for stopId, stopName, and widgetName (throw IllegalArgumentException or Objects.requireNonNull) and defensively copy routeShortNames (e.g., new HashMap<>(routeShortNames) or Collections.emptyMap() when null) before assigning to this.routeShortNames to prevent external mutation and ensure required fields are present; update the WidgetConfig(String stopId, String stopName, String widgetName, Map<String,String> routeShortNames) constructor accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.java`:
- Around line 247-260: The PendingIntent request codes for widget button clicks
(bindRefreshIntent) and scheduled alarms (scheduleExactAlarm) collide because
both use appWidgetId-derived integers; modify both bindRefreshIntent and the
alarm-scheduling methods (e.g., scheduleExactAlarm and any callers that create
ACTION_REFRESH_WIDGET or ACTION_UPDATE_RELATIVE_TIMES PIs) to use a single,
shared request-code generator function (or consistent offset scheme) that
encodes both the widget id and the PI-purpose (click vs alarm) into the
requestCode (for example combine appWidgetId with a small constant tag or
bitmask) so each PendingIntent.getBroadcast call (for ACTION_REFRESH_WIDGET and
ACTION_UPDATE_RELATIVE_TIMES) produces a unique requestCode per purpose per
widget, and update all four places that create these broadcast PIs to use that
generator.
- Around line 317-337: scheduleExactAlarm currently always uses
ELAPSED_REALTIME_WAKEUP which unnecessarily wakes the device for the per-minute
relative-times UI update; modify the call sites (the
ACTION_UPDATE_RELATIVE_TIMES per-minute chain vs the 5-minute API refresh) or
change scheduleExactAlarm signature to accept a boolean/useWakeup flag so that
when scheduling ACTION_UPDATE_RELATIVE_TIMES you use
AlarmManager.ELAPSED_REALTIME (non-wakeup) instead of ELAPSED_REALTIME_WAKEUP,
keeping the wakeup behavior for the 5-minute refresh; update the logic in
scheduleExactAlarm to choose the alarm type based on that flag and retain the
existing PendingIntent creation and trigger time calculation.
- Around line 105-126: When handling ACTION_APPLY_PENDING_CONFIG, check that
widgetId is not AppWidgetManager.INVALID_APPWIDGET_ID before persisting or
scheduling: if widgetId equals INVALID_APPWIDGET_ID, return early and do not
call WidgetPrefs.saveConfig(context, widgetId, config),
scheduleRepeatingRefreshBroadcasts(context, widgetId) or refreshWidget; perform
this guard at the start of the ACTION_APPLY_PENDING_CONFIG branch in
StopTimesWidget so bogus IDs never corrupt prefs or register alarms.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java`:
- Around line 173-187: The click listener on selectStopButton always launches
StopPickerActivity even when a stop is selected; change it so if selectedStopId
is null it launches mStopPickerLauncher as before, but if selectedStopId is
non-null it clears the selection (set selectedStopId and selectedStopName to
null or empty), reset the UI by calling updateStopButton() and
updateSaveButton(), and clear any loaded routes (e.g., call
loadRoutesForStop(null) or remove displayed routes) so the chip X behaves as a
remove affordance rather than re-opening the picker. Ensure this logic lives
where selectStopButton.setOnClickListener(...) is set so updateStopButton() and
onSave() continue to work correctly.
- Around line 199-219: The generation check currently runs on the background
thread before enqueuing the UI update, allowing a race where a newer generation
can be set after the check but before mainHandler.post executes; move or repeat
the check inside the Runnable posted to mainHandler so you only call
populateRouteChips(routes) when mLoadGeneration.get() == generation (i.e., keep
the existing generation local variable and perform the comparison inside the
lambda passed to mainHandler.post) and remove or keep the bg-thread check as a
fast-fail but ensure the atomic check-and-update happens in the Runnable that
calls populateRouteChips.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.java`:
- Around line 93-126: fetchArrivals currently treats zero-length arrival arrays
as a fetch failure and keeps expanding the look‑ahead, which causes doWork to
always retry and never persist an empty snapshot; change fetchArrivals (and its
usage in doWork) so that ObaArrivalInfoResponse is returned as soon as response
!= null and response.getArrivalInfo() != null (even if length == 0), only
retry/expand when response == null or response.getArrivalInfo() == null, and let
doWork proceed to buildSnapshot, WidgetPrefs.saveSnapshot and return
Result.success() for honest empty results so the widget can persist an empty
snapshot instead of looping retries.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.java`:
- Around line 43-44: The single global PENDING_CONFIG_KEY in WidgetPrefs can be
clobbered by concurrent pin flows; change the persistence to use a per-request
identifier (e.g., a generated requestId) propagated through the pin flow
(callback Intent extras) and use that id to compose the prefs key instead of the
constant "pending_pin_config"; update all read/write sites referenced in
WidgetPrefs (including the logic around the methods/blocks covering lines
~79-96) to accept/require the requestId, persist/load using the request-specific
key, and read the same requestId from the incoming callback extras so multiple
in-flight flows do not collide.
In `@onebusaway-android/src/main/res/layout/stop_times_widget_preview.xml`:
- Around line 34-36: The layout uses android:paddingHorizontal and
android:paddingVertical (in stop_times_widget_preview.xml) which require API 26;
replace each paddingHorizontal with android:paddingLeft and
android:paddingRight, and each paddingVertical with android:paddingTop and
android:paddingBottom for API-21 compatibility, updating all occurrences
referenced (originally at lines 34, 36, 65, 80, 81, 99) so the layout no longer
uses API-26-only attributes.
In `@onebusaway-android/src/main/res/xml/stop_times_widget_info.xml`:
- Around line 19-20: The previewImage attribute is pointing to a layout
(android:previewImage="@layout/stop_times_widget") which is invalid; update the
widget info by changing android:previewImage to reference a drawable (e.g.,
android:previewImage="@drawable/your_preview_drawable") or, if you need a layout
preview on Android 12+, replace/add
android:previewLayout="@layout/stop_times_widget" instead—adjust the resource
name (stop_times_widget) accordingly and ensure the drawable resource exists.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.java`:
- Around line 33-38: The WidgetConfig constructor currently assigns parameters
directly; add null checks for stopId, stopName, and widgetName (throw
IllegalArgumentException or Objects.requireNonNull) and defensively copy
routeShortNames (e.g., new HashMap<>(routeShortNames) or Collections.emptyMap()
when null) before assigning to this.routeShortNames to prevent external mutation
and ensure required fields are present; update the WidgetConfig(String stopId,
String stopName, String widgetName, Map<String,String> routeShortNames)
constructor accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e2354634-cf09-46b2-ac88-e5c563b86711
📒 Files selected for processing (30)
onebusaway-android/src/main/AndroidManifest.xmlonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopPickerActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalSnapshot.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.javaonebusaway-android/src/main/res/drawable-v21/widget_background.xmlonebusaway-android/src/main/res/drawable/ic_chip_check.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_early.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_late.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_on_time.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_scheduled.xmlonebusaway-android/src/main/res/drawable/widget_header_bg.xmlonebusaway-android/src/main/res/layout/activity_stop_picker.xmlonebusaway-android/src/main/res/layout/list_item_empty_section.xmlonebusaway-android/src/main/res/layout/stop_times_widget.xmlonebusaway-android/src/main/res/layout/stop_times_widget_config.xmlonebusaway-android/src/main/res/layout/stop_times_widget_preview.xmlonebusaway-android/src/main/res/menu-v14/arrivals_list.xmlonebusaway-android/src/main/res/menu/arrivals_list.xmlonebusaway-android/src/main/res/values/attrs.xmlonebusaway-android/src/main/res/values/colors.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/main/res/values/styles.xmlonebusaway-android/src/main/res/values/themes.xmlonebusaway-android/src/main/res/xml-v31/stop_times_widget_info.xmlonebusaway-android/src/main/res/xml/stop_times_widget_info.xml
Comment on lines
+173
to
+187
| selectStopButton.setOnClickListener(v -> | ||
| mStopPickerLauncher.launch(new Intent(this, StopPickerActivity.class))); | ||
|
|
||
| if (selectedStopId != null) { | ||
| loadRoutesForStop(selectedStopId); | ||
| } | ||
|
|
||
| updateSaveButton(); | ||
| saveButton.setOnClickListener(v -> onSave()); | ||
| } | ||
|
|
||
| private void updateStopButton() { | ||
| selectStopButton.setText(selectedStopName); | ||
| selectStopButton.setIconResource(R.drawable.ic_close); | ||
| } |
There was a problem hiding this comment.
"X" icon on the stop button is a remove affordance, but clicking it re-launches the picker.
After a stop is selected, updateStopButton sets ic_close on the chip-style button (the PR description and screenshot describe this as the remove option). However the click listener installed at line 173 always launches StopPickerActivity, regardless of whether a stop is already selected. Users who tap the visible X to clear their selection will instead be sent back into the picker — and there is no way to deselect the stop from this screen. Either change the click behavior when a stop is already selected to clear selectedStopId/selectedStopName, the chips, and the save button, or replace ic_close with a "change/edit" affordance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java`
around lines 173 - 187, The click listener on selectStopButton always launches
StopPickerActivity even when a stop is selected; change it so if selectedStopId
is null it launches mStopPickerLauncher as before, but if selectedStopId is
non-null it clears the selection (set selectedStopId and selectedStopName to
null or empty), reset the UI by calling updateStopButton() and
updateSaveButton(), and clear any loaded routes (e.g., call
loadRoutesForStop(null) or remove displayed routes) so the chip X behaves as a
remove affordance rather than re-opening the picker. Ensure this logic lives
where selectStopButton.setOnClickListener(...) is set so updateStopButton() and
onSave() continue to work correctly.
Comment on lines
+43
to
+44
| private static final String PENDING_CONFIG_KEY = "pending_pin_config"; | ||
|
|
There was a problem hiding this comment.
Single pending-pin slot can misapply config across concurrent widget adds.
Using one global pending_pin_config key means a second pin flow overwrites the first before it is consumed, so the wrong config can be attached to a widget.
Suggested direction
- private static final String PENDING_CONFIG_KEY = "pending_pin_config";
+ // Use a per-request key (or queue) instead of a single global slot.
+ private static final String PENDING_CONFIG_PREFIX = "pending_pin_config_";Use a request identifier propagated through the pin flow (e.g., in callback extras) and persist/load by that key so multiple in-flight flows do not collide.
Also applies to: 79-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.java`
around lines 43 - 44, The single global PENDING_CONFIG_KEY in WidgetPrefs can be
clobbered by concurrent pin flows; change the persistence to use a per-request
identifier (e.g., a generated requestId) propagated through the pin flow
(callback Intent extras) and use that id to compose the prefs key instead of the
constant "pending_pin_config"; update all read/write sites referenced in
WidgetPrefs (including the logic around the methods/blocks covering lines
~79-96) to accept/require the requestId, persist/load using the request-specific
key, and read the same requestId from the incoming callback extras so multiple
in-flight flows do not collide.
Contributor
Author
|
Oh nice, we have coderabbit now. I'll address these comments |
robertgodfrey
force-pushed
the
stop-widget
branch
from
May 10, 2026 02:11
acb5bc2 to
5adb57f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.java`:
- Around line 101-102: The call to buildSnapshot(...) passes
config.getRouteShortNameMap() without null/empty checks which can throw if
preferences are malformed; before calling buildSnapshot (where
WidgetArrivalSnapshot is created), retrieve the map into a local variable, test
for null or empty and substitute Collections.emptyMap() or new HashMap<>() when
missing, then pass that safe map into buildSnapshot; apply the same guard to
every other call site that passes config.getRouteShortNameMap() to ensure
graceful recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f48c8db5-2b8f-4bb6-9064-de09f32470e6
📒 Files selected for processing (31)
onebusaway-android/src/main/AndroidManifest.xmlonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopPickerActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalSnapshot.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.javaonebusaway-android/src/main/res/drawable-v21/widget_background.xmlonebusaway-android/src/main/res/drawable/ic_chip_check.xmlonebusaway-android/src/main/res/drawable/ic_edit.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_early.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_late.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_on_time.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_scheduled.xmlonebusaway-android/src/main/res/drawable/widget_header_bg.xmlonebusaway-android/src/main/res/layout/activity_stop_picker.xmlonebusaway-android/src/main/res/layout/list_item_empty_section.xmlonebusaway-android/src/main/res/layout/stop_times_widget.xmlonebusaway-android/src/main/res/layout/stop_times_widget_config.xmlonebusaway-android/src/main/res/layout/stop_times_widget_preview.xmlonebusaway-android/src/main/res/menu-v14/arrivals_list.xmlonebusaway-android/src/main/res/menu/arrivals_list.xmlonebusaway-android/src/main/res/values/attrs.xmlonebusaway-android/src/main/res/values/colors.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/main/res/values/styles.xmlonebusaway-android/src/main/res/values/themes.xmlonebusaway-android/src/main/res/xml-v31/stop_times_widget_info.xmlonebusaway-android/src/main/res/xml/stop_times_widget_info.xml
✅ Files skipped from review due to trivial changes (15)
- onebusaway-android/src/main/res/xml-v31/stop_times_widget_info.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_scheduled.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_early.xml
- onebusaway-android/src/main/res/values/attrs.xml
- onebusaway-android/src/main/res/drawable/ic_edit.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_late.xml
- onebusaway-android/src/main/res/drawable/ic_chip_check.xml
- onebusaway-android/src/main/res/drawable-v21/widget_background.xml
- onebusaway-android/src/main/res/menu-v14/arrivals_list.xml
- onebusaway-android/src/main/res/xml/stop_times_widget_info.xml
- onebusaway-android/src/main/res/drawable/widget_header_bg.xml
- onebusaway-android/src/main/res/values/styles.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_on_time.xml
- onebusaway-android/src/main/res/values/strings.xml
- onebusaway-android/src/main/res/values/colors.xml
🚧 Files skipped from review as they are similar to previous changes (15)
- onebusaway-android/src/main/res/menu/arrivals_list.xml
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.java
- onebusaway-android/src/main/res/layout/list_item_empty_section.xml
- onebusaway-android/src/main/res/layout/activity_stop_picker.xml
- onebusaway-android/src/main/res/layout/stop_times_widget_config.xml
- onebusaway-android/src/main/res/values/themes.xml
- onebusaway-android/src/main/AndroidManifest.xml
- onebusaway-android/src/main/res/layout/stop_times_widget_preview.xml
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalSnapshot.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.java
- onebusaway-android/src/main/res/layout/stop_times_widget.xml
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopPickerActivity.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java
robertgodfrey
force-pushed
the
stop-widget
branch
from
May 10, 2026 02:22
5adb57f to
d5dae28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java (1)
249-268: ⚡ Quick winEnforce the 3-route cap while chips are being toggled.
Right now the UI allows an invalid 4+ route selection and only rejects it later in
onSave(). Disabling additional unchecked chips once 3 are selected would keep the screen consistent with the “Select 1–3 routes” contract and avoid a failed save round-trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java` around lines 249 - 268, The chip UI currently lets users select 4+ routes; enforce the 1–3 limit by adding a CompoundButton.OnCheckedChangeListener to each Chip (in StopTimesWidgetConfigActivity where chips are created/added to chipGroup) that recalculates the number of checked chips (using chip.isChecked() over chipGroup children or a selectedCount) and when the count reaches 3 disables all unchecked chips (chip.setEnabled(false) / setClickable(false)) and when the count falls below 3 re-enables them; ensure you also apply this logic once after creating chips (so existingRouteFilter initial state disables extras) and leave onSave() validation intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java`:
- Around line 202-214: The code currently builds the selectable routes from
response.getArrivalInfo() (ObaArrivalInfoRequest) which excludes routes that
serve the stop but have no upcoming arrivals; instead, fetch the full list of
routes that serve the stop (e.g., via response.getStop().getRoutes() if
available or by issuing an ObaStop/StopWithRoutes request for stopId), iterate
that list to construct Route objects (using the route id and any available
shortName), and merge/override shortName values from response.getArrivalInfo()
when present to preserve nicer labels; keep the seen dedupe set and sort by
compareRouteNames as before (update the code around the Route construction in
StopTimesWidgetConfigActivity to use the stop’s route list rather than only
response.getArrivalInfo()).
- Around line 254-264: The code currently disables the Chip minimum touch target
via chip.ensureAccessibleTouchTarget(0) in StopTimesWidgetConfigActivity, which
reduces accessibility; remove that call or replace it with a proper minimum
touch-target call such as chip.setEnsureMinTouchTargetSize(true) or call
ensureAccessibleTouchTarget(...) with a converted 48dp-to-px value so the Chip
meets the 48dp accessibility guideline; update the code around the chip
configuration (the block that sets chip.setChipIcon...,
chip.setChipEndPadding(...), chip.ensureAccessibleTouchTarget(0)) to use one of
those fixes.
---
Nitpick comments:
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java`:
- Around line 249-268: The chip UI currently lets users select 4+ routes;
enforce the 1–3 limit by adding a CompoundButton.OnCheckedChangeListener to each
Chip (in StopTimesWidgetConfigActivity where chips are created/added to
chipGroup) that recalculates the number of checked chips (using chip.isChecked()
over chipGroup children or a selectedCount) and when the count reaches 3
disables all unchecked chips (chip.setEnabled(false) / setClickable(false)) and
when the count falls below 3 re-enables them; ensure you also apply this logic
once after creating chips (so existingRouteFilter initial state disables extras)
and leave onSave() validation intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e36fed72-2e48-4b6a-bf6f-3674e37536d8
📒 Files selected for processing (31)
onebusaway-android/src/main/AndroidManifest.xmlonebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopPickerActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalSnapshot.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.javaonebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.javaonebusaway-android/src/main/res/drawable-v21/widget_background.xmlonebusaway-android/src/main/res/drawable/ic_chip_check.xmlonebusaway-android/src/main/res/drawable/ic_edit.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_early.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_late.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_on_time.xmlonebusaway-android/src/main/res/drawable/widget_eta_bg_scheduled.xmlonebusaway-android/src/main/res/drawable/widget_header_bg.xmlonebusaway-android/src/main/res/layout/activity_stop_picker.xmlonebusaway-android/src/main/res/layout/list_item_empty_section.xmlonebusaway-android/src/main/res/layout/stop_times_widget.xmlonebusaway-android/src/main/res/layout/stop_times_widget_config.xmlonebusaway-android/src/main/res/layout/stop_times_widget_preview.xmlonebusaway-android/src/main/res/menu-v14/arrivals_list.xmlonebusaway-android/src/main/res/menu/arrivals_list.xmlonebusaway-android/src/main/res/values/attrs.xmlonebusaway-android/src/main/res/values/colors.xmlonebusaway-android/src/main/res/values/strings.xmlonebusaway-android/src/main/res/values/styles.xmlonebusaway-android/src/main/res/values/themes.xmlonebusaway-android/src/main/res/xml-v31/stop_times_widget_info.xmlonebusaway-android/src/main/res/xml/stop_times_widget_info.xml
✅ Files skipped from review due to trivial changes (16)
- onebusaway-android/src/main/res/drawable-v21/widget_background.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_scheduled.xml
- onebusaway-android/src/main/res/layout/list_item_empty_section.xml
- onebusaway-android/src/main/res/drawable/widget_header_bg.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_early.xml
- onebusaway-android/src/main/res/xml-v31/stop_times_widget_info.xml
- onebusaway-android/src/main/res/values/attrs.xml
- onebusaway-android/src/main/res/drawable/ic_chip_check.xml
- onebusaway-android/src/main/res/layout/activity_stop_picker.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_on_time.xml
- onebusaway-android/src/main/res/drawable/widget_eta_bg_late.xml
- onebusaway-android/src/main/res/drawable/ic_edit.xml
- onebusaway-android/src/main/res/values/colors.xml
- onebusaway-android/src/main/res/xml/stop_times_widget_info.xml
- onebusaway-android/src/main/res/menu-v14/arrivals_list.xml
- onebusaway-android/src/main/res/layout/stop_times_widget_config.xml
🚧 Files skipped from review as they are similar to previous changes (14)
- onebusaway-android/src/main/res/values/styles.xml
- onebusaway-android/src/main/res/layout/stop_times_widget_preview.xml
- onebusaway-android/src/main/res/menu/arrivals_list.xml
- onebusaway-android/src/main/AndroidManifest.xml
- onebusaway-android/src/main/res/layout/stop_times_widget.xml
- onebusaway-android/src/main/res/values/themes.xml
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalSnapshot.java
- onebusaway-android/src/main/res/values/strings.xml
- onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListFragment.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetArrivalWorker.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetConfig.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopPickerActivity.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/WidgetPrefs.java
- onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidget.java
Comment on lines
+202
to
+214
| final ObaArrivalInfoResponse response = | ||
| ObaArrivalInfoRequest.newRequest(this, stopId, MAX_MINUTES_AFTER).call(); | ||
|
|
||
| final List<Route> routes = new ArrayList<>(); | ||
| final Set<String> seen = new HashSet<>(); | ||
| if (response != null && response.getArrivalInfo() != null) { | ||
| for (ObaArrivalInfo info : response.getArrivalInfo()) { | ||
| if (seen.add(info.getRouteId())) { | ||
| routes.add(new Route(info.getRouteId(), info.getShortName())); | ||
| } | ||
| } | ||
| } | ||
| Collections.sort(routes, (a, b) -> compareRouteNames(a.shortName, b.shortName)); |
There was a problem hiding this comment.
Route selection is currently limited to routes that already have arrival rows.
This list is built only from response.getArrivalInfo(), so any route that serves the stop but has no trips in the next 24 hours never becomes selectable here. That conflicts with the widget behavior of showing configured routes as N/A, and it blocks valid configurations for late-night / infrequent service.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@onebusaway-android/src/main/java/org/onebusaway/android/ui/widget/StopTimesWidgetConfigActivity.java`
around lines 202 - 214, The code currently builds the selectable routes from
response.getArrivalInfo() (ObaArrivalInfoRequest) which excludes routes that
serve the stop but have no upcoming arrivals; instead, fetch the full list of
routes that serve the stop (e.g., via response.getStop().getRoutes() if
available or by issuing an ObaStop/StopWithRoutes request for stopId), iterate
that list to construct Route objects (using the route id and any available
shortName), and merge/override shortName values from response.getArrivalInfo()
when present to preserve nicer labels; keep the seen dedupe set and sort by
compareRouteNames as before (update the code around the Route construction in
StopTimesWidgetConfigActivity to use the stop’s route list rather than only
response.getArrivalInfo()).
Contributor
Author
There was a problem hiding this comment.
I don't think this is a real issue
There was a problem hiding this comment.
@robertgodfrey Fair point. With MAX_MINUTES_AFTER = 1440 (24 hours), virtually every route that serves a stop will have at least one trip in that window, so the limitation I described is an extremely marginal edge case in practice. I'll accept your judgment here.
robertgodfrey
force-pushed
the
stop-widget
branch
from
May 10, 2026 22:21
d5dae28 to
08d98f5
Compare
Member
|
@robertgodfrey I haven't forgotten about you! I'm just a one person show on Android and several other projects right now 🙃 |
Contributor
Author
|
No worries! I know it's a pretty big PR 😄 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #698 and #1075: Adds a home screen widget that shows arrival information. I've been wanting a widget with this functionality forever! 😁
Design decisions open for discussion
N/Ainstead of a timeNotes
Android 8 relaunches the config screen after the widget is placed via
requestPinAppWidget. I initially wanted to close this silently, but I couldn't find a reliable way to distinguish between the initial relaunch and a user-initiated edit (available Android 12+). The current behavior on Android 8 shows the config screen pre-filled and the user taps Save or Back to dismiss it.Main Files
StopTimesWidget: AppWidgetProvider managing widget lifecycle, alarm scheduling, and RemoteViews renderingWidgetArrivalWorker: WorkManager worker that fetches arrivals from the OBA APIStopTimesWidgetConfigActivity: dialog configuration screenStopPickerActivity: stop selection screen showing starred and recent stops from the local databaseWidgetPrefs,WidgetConfig,WidgetArrivalSnapshot: persistence layer using SharedPreferences + GsonScreenshots
Videos (too big to attach here and I don't feel like editing them, so they're in a CloudFlare bucket):
Apply the
AndroidStyle.xmlstyle template to your code in Android Studio.Run the unit tests with
gradlew connectedObaGoogleDebugAndroidTestto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them for the initial submission of the pull request. When addressing comments on a pull request, please push a new commit per comment when possible (reviewers will squash and merge using GitHub merge tool)
Summary by CodeRabbit