Add a toggle to hide scheduled traffics if needed#1524
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Amelia, this is a really useful feature — I've seen users ask for a way to focus on real-time predictions and ignore schedule-only data, so it's great to see someone tackling issue #1293. The filtering logic in shouldAddArrival is clean, and I appreciate that you wrote tests for the three core scenarios. There are a few things I need you to fix before we can merge:
Critical
1. Test dependency shipped in production APK
build.gradle:258 — implementation 'androidx.test.ext:junit:1.3.0' adds a test-only library as a regular dependency. This means androidx.test.ext:junit and its transitive dependencies will be bundled into the production APK, increasing its size and shipping test code to users. The project already uses androidTestImplementation for test dependencies (e.g., line 270). Change it to:
androidTestImplementation 'androidx.test.ext:junit:1.3.0'2. Preference key in translatable strings.xml instead of donottranslate.xml
strings.xml:434 — The string preference_key_hide_scheduled_arrivals is a preference key, not a user-facing string. Every other preference key in the project is defined in donottranslate.xml (lines 25-49). If a translator localizes this key, the preference lookup will silently break for users in that locale — the toggle would appear to reset every time they open the app.
Move this line from strings.xml to donottranslate.xml:
<string name="preference_key_hide_scheduled_arrivals">preference_key_hide_scheduled_arrivals</string>The two user-facing strings (stop_info_option_hide_scheduled_arrivals and stop_info_option_show_scheduled_arrivals) should stay in strings.xml — those are correct.
Important
3. Test method names use PascalCase instead of project convention
ArrivalInfoUtilsTest.java — All existing test methods in this project use testCamelCase naming (e.g., testOrientationToDirection, testSerializeRouteDisplayNames). The AOSP code style specified in CLAUDE.md also uses camelCase for method names. Please rename:
TestShowAllByDefault→testShowAllByDefaultTestHideScheduledWithBothKinds→testHideScheduledWithBothKindsTestHideScheduleWithAllSchedule→testHideScheduledWithAllScheduled(also fixes the missing "d" on both words)
4. Add a null guard on the new menu item in onPrepareOptionsMenu
ArrivalsListFragment.java:617 — menu.findItem(R.id.toggle_scheduled_arrivals) is used without a null check. The surrounding code guards other items (e.g., the mHeader != null check for menuItemHeaderArrivals). While the item is always present in the current XML, adding a guard keeps this defensive and consistent:
MenuItem menuItemScheduledArrivals = menu.findItem(R.id.toggle_scheduled_arrivals);
if (menuItemScheduledArrivals != null) {
boolean hideScheduled = Application.getPrefs()
.getBoolean(getString(R.string.preference_key_hide_scheduled_arrivals), false);
title = hideScheduled ?
getString(R.string.stop_info_option_show_scheduled_arrivals) :
getString(R.string.stop_info_option_hide_scheduled_arrivals);
menuItemScheduledArrivals.setTitle(title);
menuItemScheduledArrivals.setTitleCondensed(title);
}Fit and Finish
5. Consider using the project's JSON fixture pattern for tests
ArrivalInfoUtilsTest.java:162-195 — The tests construct ObaArrivalInfo objects via reflection (getDeclaredConstructor, setAccessible, field-by-field setting). This works, but it's fragile — if field names are refactored, these tests break with cryptic reflection errors instead of compile errors. The existing test suite (e.g., ArrivalInfoRequestTest.java, UIUtilTest.java) uses mock JSON responses from /res/raw/ files deserialized through the normal Jackson pipeline. Not blocking, but worth considering for a follow-up to align with the rest of the test suite.
6. Consider adding a test for the route-filter branch
The production code has two distinct branches in convertObaArrivalInfo — one when filter != null and one without. All three tests pass null as the filter. Adding one test that passes a non-null filter and verifies that hideScheduled still works within the filtered set would cover both branches. Not blocking.
Strengths
- Clean, minimal filtering logic —
shouldAddArrivaldelegates toshouldAddEtarather than duplicating the negative-ETA check - Good test coverage of the three core behavioral scenarios (default off, mixed types, all-scheduled)
- Proper
setUp/tearDownfor preference state management in tests - The
createArrivalhelper is well-structured and reusable - Menu toggle follows the existing show/hide pattern used by
doShowHideHeaderArrivals
Recommended Action
- Fix critical items 1-2 (dependency scope, preference key location)
- Address important items 3-4 (test naming, null guard)
- Consider suggestions 5-6 for follow-up
dec40d5 to
40630b8
Compare
|
Hi Aaron! Thank you so much for the review! I’ve addressed changes 1, 2,3, 4, and 6, and will raise another PR for 5.
All tests passed. Please let me know if there’s anything else you’d like me to adjust. Thanks again for the helpful feedback! |
Summary
This PR solves Issue #1293. It adds a feature to allow users to hide scheduled arrivals and display only real-time traffic if they prefer.
I have added a toggle in the arrivals list menu to filter this behavior (Last one in the list)
Key Changes
Tests
Added tests to verify the new behavior in
util/test/ArrivalInfoUtilsTest.javaTestShowAllByDefault()– both scheduled and real-time arrivals are shown.TestHideScheduledWithBothKinds()– Scheduled arrivals are removed while real-time arrivals remain.TestHideScheduleWithAllSchedule()– When there are no real-time arrivals and filtering is enabled, the list returns empty.All tests have passed.
Screenshots
Please make sure these boxes are checked before submitting your pull request - thanks!
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)