feat: add hide all alerts banner above active alerts #1534#1553
feat: add hide all alerts banner above active alerts #1534#1553Hitanshi7556 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a more prominent “hide all alerts” control by inserting a new banner-style alert row above the active alerts list, and updates related string resources.
Changes:
- Add a new
HideAllAlertbanner item inArrivalsListHeaderand refresh it during header refresh. - Count currently-visible alerts and conditionally insert the hide-all banner at the top of the alert list.
- Update
strings.xmlplurals and add a new(hide all)string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| onebusaway-android/src/main/res/values/strings.xml | Updates alert filter plural text and adds a new hide-all label string. |
| onebusaway-android/src/main/java/org/onebusaway/android/ui/ArrivalsListHeader.java | Inserts a new banner-style alert to hide all alerts and wires it into the refresh cycle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <plurals name="alert_filter_text"> | ||
| <item quantity="one">%1$d active alert is hidden</item> | ||
| <item quantity="one">%1$d active alert shown</item> | ||
| <!-- Entire element below should be on a single line if XML is reformatted - see #99 --> | ||
| <item quantity="other"> | ||
| <xliff:g id="count">%1$d</xliff:g> | ||
| active alerts are hidden | ||
| active alerts shown | ||
| </item> | ||
| </plurals> |
There was a problem hiding this comment.
alert_filter_text is used by refreshHiddenAlerts() to describe the number of hidden alerts (see ArrivalsListHeader.java:1490-1492), but it was changed here to say "shown". This will make the existing "show hidden alerts" banner display incorrect text (e.g., it will report hidden alerts as "shown"). Consider keeping alert_filter_text as the hidden-count text and introducing a new plurals resource for the visible/active alerts count used by the new hide-all banner.
| if (visibleAlertCount > 0) { | ||
| CharSequence activeAlertsText = mContext.getResources().getQuantityString( | ||
| R.plurals.alert_filter_text, visibleAlertCount, visibleAlertCount); | ||
| CharSequence hideAllText = mContext.getResources().getString(R.string.alert_hide_all); | ||
| CharSequence combinedText = activeAlertsText + " " + hideAllText; | ||
|
|
||
| mHideAllAlert = new HideAllAlert(combinedText, mController); | ||
| alerts.insert(mHideAllAlert, 0); | ||
| } |
There was a problem hiding this comment.
The filter banner UI uses alert_item.xml's @id/show_all_alerts TextView, whose text is hardcoded to @string/alert_filter_showall ("(show all)") and is what the adapter makes clickable. Since HideAllAlert reuses TYPE_SHOW_HIDDEN_ALERTS, the row will still display "(show all)" while executing hideAllAlerts() on click, and combinedText also embeds "(hide all)", likely resulting in contradictory/duplicated UI. To make this work, the adapter/layout needs a way to set the action label per alert (e.g., introduce a new alert type for hide-all or add an action-label field/method and set showAllView text accordingly), and avoid embedding the action label into the count text.
| // Count visible alerts (excluding response errors and special alerts) | ||
| int visibleAlertCount = 0; | ||
| for (int i = 0; i < alerts.getCount(); i++) { | ||
| AlertList.Alert alert = alerts.getItem(i); | ||
| int type = alert.getType(); | ||
| if (type == AlertList.Alert.TYPE_ERROR || type == AlertList.Alert.TYPE_WARNING || | ||
| type == AlertList.Alert.TYPE_INFO) { | ||
| visibleAlertCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
visibleAlertCount currently counts any TYPE_ERROR/TYPE_WARNING/TYPE_INFO entries, which includes the "STATIC: RESPONSE ERROR" banner inserted by refreshError() (it returns TYPE_ERROR). This can cause the hide-all banner to appear even when the only alert is a response error, and clicking "hide all" won't clear that response error. Consider excluding ResponseError (or excluding alerts with getId() starting with "STATIC:") from the visible-alert count so the banner only appears when there are hideable service alerts.
| if (mHideAllAlert != null) { | ||
| alerts.remove(mHideAllAlert); | ||
| } | ||
|
|
There was a problem hiding this comment.
If visibleAlertCount is 0, mHideAllAlert is removed from the list but the field is left non-null, so subsequent refreshes will keep calling alerts.remove(mHideAllAlert) on an object that's no longer in the adapter. It would be cleaner to set mHideAllAlert = null when the banner isn't shown.
|
@Hitanshi7556 this feature has been superseded by a UI modernization - would you like to re-implement it against the latest |
Description
Addresses issue #1534 - Make the "hide all alerts" button more visible and accessible.
Previously, the hide alerts functionality was hidden in the menu drawer. This change adds a prominent "Hide all alerts" button directly above the active alerts list, making it discoverable and easy to use.
Problem
Solution
Changes
Fixes #1534
[ ✔️] Apply the
AndroidStyle.xmlstyle template to your code in Android Studio.[ ✔️] Run the unit tests with
gradlew connectedObaGoogleDebugAndroidTestto make sure you didn't break anything[✔️ ] If 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)