From 8a786fe3632d82c170d7f32253559350a928eab4 Mon Sep 17 00:00:00 2001 From: Tomek Zebrowski Date: Thu, 8 Oct 2026 12:10:33 +0200 Subject: [PATCH] fix(performance,trip-info): list bottom-row dialogs in the default order fix(performance,trip-info): list bottom-row dialogs in the default order Opening the gauge / bottom-row dialog stores the checked rows' listed order as the drag order, which beats the default order in PidOrder. The rows came from a Set, so merely opening the dialog fixed a random gauge order. List both dialogs in the profile / default bottom order, and move PidOrder to :datalogger so the dialogs and the renderers share one comparator. --- CLAUDE.md | 7 ++++++- .../preferences/pid/PidDefinitionViewModel.kt | 20 +++++++++---------- .../bl/query/PerformanceQueryStrategy.kt | 11 ++++++++++ .../java/org/obd/graphs/bl/query}/PidOrder.kt | 6 +++--- .../graphs/bl/query/TripInfoQueryStrategy.kt | 11 ++++++++++ .../bl/query/PerformanceQueryStrategyTest.kt | 10 ++++++++++ .../bl/query/TripInfoQueryStrategyTest.kt | 14 +++++++++++++ doc/specs/performance-dynamic-pids.md | 5 ++++- doc/specs/trip-info-dynamic-pids.md | 2 ++ .../performance/PerformanceMetrics.kt | 2 +- .../graphs/renderer/trip/TripInfoMetrics.kt | 2 +- .../performance/PerformanceMetricsTest.kt | 13 ++++++++++++ .../renderer/trip/TripInfoMetricsTest.kt | 12 +++++++++++ 13 files changed, 98 insertions(+), 17 deletions(-) rename {screen_renderer/src/main/java/org/obd/graphs/renderer => datalogger/src/main/java/org/obd/graphs/bl/query}/PidOrder.kt (93%) diff --git a/CLAUDE.md b/CLAUDE.md index 11527c1e..7b4d6e7c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -188,8 +188,13 @@ Gradle needs JDK 17+ (Crashlytics plugin); the shell default may be JDK 11 and f could be read half-applied. Grid fit reuses `TripInfoMetrics.grid(count, 5, 3)` (unchanged up to 15). * A stored gauge / bottom-row selection is a `Set`, so it carries no order. Undragged PIDs fall back to the profile's bottom order (Trip Info: `TRIP_INFO_DEFAULT_BOTTOM_PIDS`) before id order in - `renderer/PidOrder.kt` (shared by both screens — keep one copy, not in either screen's Metrics); + `bl/query/PidOrder.kt` (`:datalogger`, shared by both screens and their dialogs — keep one copy); plain id order reshuffled the row as soon as one PID was unchecked. +* **Opening a PID dialog stores a drag order.** `PidDefinitionViewModel.sortItems()` saves the checked + rows' listed order whenever none is stored, and a stored order beats `PidOrder`'s default. So the + bottom/gauge dialogs must *list* in the default order (`tripInfoBottomDialogItems`, + `performanceBottomDialogItems`); listed in a `Set`'s order, merely opening the dialog fixed a random + order. Orders stored before that fix stay as stored — they cannot be told apart from a real drag. * "Select all" in the Trip Info / Performance PID dialogs (full registry) only warns via toast, by the user's choice — no cap. Gate: `PidDefinitionDialogMode.warnsOnSelectAll`. * `MetricsCache` serves phone and AA alike, so the AA gauge pref and order keys also drive the phone diff --git a/app/src/main/java/org/obd/graphs/preferences/pid/PidDefinitionViewModel.kt b/app/src/main/java/org/obd/graphs/preferences/pid/PidDefinitionViewModel.kt index 2badf868..21ce6037 100644 --- a/app/src/main/java/org/obd/graphs/preferences/pid/PidDefinitionViewModel.kt +++ b/app/src/main/java/org/obd/graphs/preferences/pid/PidDefinitionViewModel.kt @@ -33,19 +33,21 @@ import org.obd.graphs.bl.datalogger.VehicleCapabilitiesManager import org.obd.graphs.bl.datalogger.dataLoggerSettings import org.obd.graphs.bl.datalogger.isUserCustom import org.obd.graphs.bl.datalogger.serialize -import org.obd.graphs.bl.query.PERFORMANCE_STATUS_PIDS +import org.obd.graphs.bl.query.PREF_QUERY_PERFORMANCE_BOTTOM import org.obd.graphs.bl.query.PREF_QUERY_PERFORMANCE_BOTTOM_SELECTED import org.obd.graphs.bl.query.PREF_QUERY_PERFORMANCE_HIDDEN import org.obd.graphs.bl.query.PREF_QUERY_PERFORMANCE_SELECTED import org.obd.graphs.bl.query.PREF_QUERY_TRIP_INFO_SELECTED import org.obd.graphs.bl.query.Query import org.obd.graphs.bl.query.QueryStrategyType -import org.obd.graphs.bl.query.TRIP_INFO_STATUS_PIDS +import org.obd.graphs.bl.query.performanceBottomDialogItems import org.obd.graphs.bl.query.performanceBottomDialogSelection import org.obd.graphs.bl.query.performanceGaugeOverflow import org.obd.graphs.bl.query.prunedPerformanceBottomSelection +import org.obd.graphs.bl.query.tripInfoBottomDialogItems import org.obd.graphs.bl.query.tripInfoBottomDialogSelection import org.obd.graphs.preferences.Prefs +import org.obd.graphs.preferences.getLongList import org.obd.graphs.preferences.getLongSet import org.obd.graphs.preferences.getStringSet import org.obd.graphs.preferences.updateStringSet @@ -294,9 +296,7 @@ class PidDefinitionViewModel( pinned + findPidDefinitionByPriority(all) { !pinnedIds.contains(it.id) } } dialogMode is PidDefinitionDialogMode.TripInfoBottom -> { - Prefs.getStringSet(PREF_QUERY_TRIP_INFO_SELECTED) - .map { it.toLong() } - .filter { !TRIP_INFO_STATUS_PIDS.contains(it) } + tripInfoBottomDialogItems(Prefs.getStringSet(PREF_QUERY_TRIP_INFO_SELECTED).map { it.toLong() }) .mapNotNull { pidRegistry.findBy(it) } .map { PidDefinitionDetails(it, checked = false, supported = true) } } @@ -311,11 +311,11 @@ class PidDefinitionViewModel( pinned + findPidDefinitionByPriority(all) { !pinnedIds.contains(it.id) } } dialogMode is PidDefinitionDialogMode.PerformanceBottom -> { - val hidden = Prefs.getLongSet(PREF_QUERY_PERFORMANCE_HIDDEN) - Prefs.getStringSet(PREF_QUERY_PERFORMANCE_SELECTED) - .map { it.toLong() } - .filter { !PERFORMANCE_STATUS_PIDS.contains(it) && !hidden.contains(it) } - .mapNotNull { pidRegistry.findBy(it) } + performanceBottomDialogItems( + Prefs.getStringSet(PREF_QUERY_PERFORMANCE_SELECTED).map { it.toLong() }, + Prefs.getLongSet(PREF_QUERY_PERFORMANCE_HIDDEN), + Prefs.getLongList(PREF_QUERY_PERFORMANCE_BOTTOM) + ).mapNotNull { pidRegistry.findBy(it) } .map { PidDefinitionDetails(it, checked = false, supported = true) } } individualQuery -> { diff --git a/datalogger/src/main/java/org/obd/graphs/bl/query/PerformanceQueryStrategy.kt b/datalogger/src/main/java/org/obd/graphs/bl/query/PerformanceQueryStrategy.kt index 0980fba1..ed943727 100644 --- a/datalogger/src/main/java/org/obd/graphs/bl/query/PerformanceQueryStrategy.kt +++ b/datalogger/src/main/java/org/obd/graphs/bl/query/PerformanceQueryStrategy.kt @@ -63,6 +63,17 @@ fun performanceBottomDialogSelection( listed: Collection ): Set = (persisted ?: Prefs.getLongList(PREF_QUERY_PERFORMANCE_BOTTOM)).filter { listed.contains(it) }.toSet() +/** The PIDs the gauge dialog lists, in the profile's gauge order. See [tripInfoBottomDialogItems]. */ +fun performanceBottomDialogItems( + selected: Collection, + hidden: Set, + profileBottom: List +): List = + selected + .filter { !PERFORMANCE_STATUS_PIDS.contains(it) && !hidden.contains(it) } + .distinct() + .sortedWith(PidOrder.comparator(null, profileBottom)) + const val PREF_QUERY_PERFORMANCE_BRAKE_BOOSTING_GAS_METRIC = "pref.query.performance.break_boosting.gas_pid" const val PREF_QUERY_PERFORMANCE_BRAKE_BOOSTING_ARBITRARY_METRIC = diff --git a/screen_renderer/src/main/java/org/obd/graphs/renderer/PidOrder.kt b/datalogger/src/main/java/org/obd/graphs/bl/query/PidOrder.kt similarity index 93% rename from screen_renderer/src/main/java/org/obd/graphs/renderer/PidOrder.kt rename to datalogger/src/main/java/org/obd/graphs/bl/query/PidOrder.kt index c7557e7a..1ef2647d 100644 --- a/screen_renderer/src/main/java/org/obd/graphs/renderer/PidOrder.kt +++ b/datalogger/src/main/java/org/obd/graphs/bl/query/PidOrder.kt @@ -14,10 +14,10 @@ * express or implied. See the License for the specific language governing permissions and * limitations under the License. */ -package org.obd.graphs.renderer +package org.obd.graphs.bl.query -/** Display order of user-selected PIDs, shared by the Trip Info and Performance plans. */ -internal object PidOrder { +/** Display order of user-selected PIDs, shared by the Trip Info / Performance plans and their PID dialogs. */ +object PidOrder { // Undragged PIDs fall back to [defaultOrder]: a stored selection is a set, and id order would // reshuffle the row as soon as one PID was unchecked. fun comparator( diff --git a/datalogger/src/main/java/org/obd/graphs/bl/query/TripInfoQueryStrategy.kt b/datalogger/src/main/java/org/obd/graphs/bl/query/TripInfoQueryStrategy.kt index 38ba834f..98df4a06 100644 --- a/datalogger/src/main/java/org/obd/graphs/bl/query/TripInfoQueryStrategy.kt +++ b/datalogger/src/main/java/org/obd/graphs/bl/query/TripInfoQueryStrategy.kt @@ -56,6 +56,17 @@ fun tripInfoBottomDialogSelection( listed: Collection ): Set = (persisted ?: TRIP_INFO_DEFAULT_BOTTOM_PIDS).filter { listed.contains(it) }.toSet() +/** + * The PIDs the bottom row dialog lists, in the order the bottom row draws them undragged. The dialog + * stores the order of its checked rows when first opened, and that stored order beats the default + * one in [PidOrder]; listed in the Trip Info selection's set order it reshuffled the row. + */ +fun tripInfoBottomDialogItems(selected: Collection): List = + selected + .filter { !TRIP_INFO_STATUS_PIDS.contains(it) } + .distinct() + .sortedWith(PidOrder.comparator(null, TRIP_INFO_DEFAULT_BOTTOM_PIDS)) + internal class TripInfoQueryStrategy : QueryStrategy() { private val defaults = setOf( diff --git a/datalogger/src/test/java/org/obd/graphs/bl/query/PerformanceQueryStrategyTest.kt b/datalogger/src/test/java/org/obd/graphs/bl/query/PerformanceQueryStrategyTest.kt index 391f9c9e..e881e231 100644 --- a/datalogger/src/test/java/org/obd/graphs/bl/query/PerformanceQueryStrategyTest.kt +++ b/datalogger/src/test/java/org/obd/graphs/bl/query/PerformanceQueryStrategyTest.kt @@ -96,6 +96,16 @@ class PerformanceQueryStrategyTest : TestSetup() { assertEquals(emptySet(), performanceBottomDialogSelection(persisted = emptySet(), listed = listOf(20L))) } + @Test + fun `gauge dialog lists the profile gauges first in their order, without hidden or status PIDs`() { + val selected = linkedSetOf(40L, 22L, 36L, PERFORMANCE_STATUS_PIDS.first(), 20L, 30L) + + assertEquals( + listOf(20L, 22L, 30L, 40L), + performanceBottomDialogItems(selected, hidden = setOf(36L), profileBottom = listOf(20L, 22L)) + ) + } + @Test fun `deselecting a PID drops it from the stored gauges`() { assertEquals(setOf(20L), prunedPerformanceBottomSelection(stored = setOf(20L, 21L), selected = setOf(10L, 20L))) diff --git a/datalogger/src/test/java/org/obd/graphs/bl/query/TripInfoQueryStrategyTest.kt b/datalogger/src/test/java/org/obd/graphs/bl/query/TripInfoQueryStrategyTest.kt index daac1198..91c8d324 100644 --- a/datalogger/src/test/java/org/obd/graphs/bl/query/TripInfoQueryStrategyTest.kt +++ b/datalogger/src/test/java/org/obd/graphs/bl/query/TripInfoQueryStrategyTest.kt @@ -75,6 +75,20 @@ class TripInfoQueryStrategyTest : TestSetup() { ) } + @Test + fun `bottom dialog lists the default row first in its order, without status PIDs`() { + val selected = + linkedSetOf( + Pid.COOLANT_TEMP_PID_ID.id, + Pid.ENGINE_TORQUE_PID_ID.id, + Pid.AMBIENT_TEMP_PID_ID.id, + Pid.OIL_PRESSURE_PID_ID.id, + Pid.INTAKE_PRESSURE_PID_ID.id + ) + + assertEquals(TRIP_INFO_DEFAULT_BOTTOM_PIDS + Pid.COOLANT_TEMP_PID_ID.id, tripInfoBottomDialogItems(selected)) + } + @Test fun `bottom dialog shows the persisted row limited to the listed PIDs`() { assertEquals( diff --git a/doc/specs/performance-dynamic-pids.md b/doc/specs/performance-dynamic-pids.md index c6e1a7e3..74e07851 100644 --- a/doc/specs/performance-dynamic-pids.md +++ b/doc/specs/performance-dynamic-pids.md @@ -82,7 +82,9 @@ handled by `SurfaceRendererScreen` like a Performance selection change. New stri (`maxWidth`): the default comes from the Trip Info layout cache, which Performance never lays out. 6. **`PidDefinitionViewModel` (`:app`).** The `Performance` source is the profile PIDs plus the current selection (unfiltered), plus the filtered registry. The new `PerformanceBottom` source is the - selection minus status and hidden PIDs; its checked state and save check use + selection minus status and hidden PIDs, listed in the profile's gauge order + (`performanceBottomDialogItems`) because the dialog stores its listed order as the drag order on + first open; its checked state and save check use `performanceBottomDialogSelection`. Saving the `Performance` selection prunes the stored gauges; saving the gauge dialog with more than 5 checked shows the overflow toast (`gaugeOverflow()`). @@ -110,6 +112,7 @@ handled by `SurfaceRendererScreen` like a Performance selection change. New stri | selected gauges are ordered, capped and the rest moves to the grid | Gauge order honoured, max 5, overflow and former gauges go to the grid | | undragged gauges keep the profile order after one is removed | A stored set without a drag order follows the profile's bottom order, not id order | | dragged gauges follow the drag order, then the profile order | Dragged first, then profile order, then the rest by id | +| the order the gauge dialog stores on opening keeps the profile order | The drag order the dialog stores on first open does not reorder the gauges | | empty gauge selection means no gauges | Former gauges move to the grid | | gauges that are not queried are skipped | Only queried ids | | grid is unchanged up to three full rows | 0–15 items: 5 columns, scale 1 | diff --git a/doc/specs/trip-info-dynamic-pids.md b/doc/specs/trip-info-dynamic-pids.md index ece5f058..8d75d909 100644 --- a/doc/specs/trip-info-dynamic-pids.md +++ b/doc/specs/trip-info-dynamic-pids.md @@ -50,6 +50,7 @@ The bottom setting is in `preferences.xml` under AA → Trip Info → displayed 7. **`PidDefinitionViewModel` (`:app`).** - The `TripInfo` source is the defaults plus the current selection (unfiltered, so a selected PID hidden by a filter is not dropped on save), plus the filtered registry. - The new `TripInfoBottom` source is the PIDs in the main selection, minus `TRIP_INFO_STATUS_PIDS`. + - The bottom dialog lists its PIDs via `tripInfoBottomDialogItems()` (`:datalogger`): the default row first in its order, then by id. The dialog stores its checked rows' order as the drag order on first open, and a stored drag order beats the default one, so listing in the selection's `Set` order made opening the dialog reorder the row. - For the bottom dialog, `persistedSelection()` uses `tripInfoBottomDialogSelection(persisted, listed)` (`:datalogger`): the stored row, or the default row when unset, **limited to the PIDs the dialog lists**. It drives both the checked state and the "did it change" check on save, so saving the dialog unchanged writes nothing — even when a default bottom PID is not selected for Trip Info, which would otherwise persist a shortened row and lose that PID for good. ## Backward compatibility @@ -72,6 +73,7 @@ The bottom setting is in `preferences.xml` under AA → Trip Info → displayed | other PIDs get the generic formatting | Generic descriptor defaults | | selected bottom row is ordered, capped and the rest moves to the grid | Order honoured, max 4, overflow and unchosen default bottom PIDs appear in the grid | | undragged bottom row keeps the default order once stored | A stored set without a drag order follows `TRIP_INFO_DEFAULT_BOTTOM_PIDS` order, not id order | +| the order the bottom dialog stores on opening keeps the default row order | The drag order the dialog stores on first open does not reorder the row | | empty bottom selection means no bottom row | Empty set means no bottom row; former bottom PIDs move to the grid | | odometer in the bottom row still shows the trip distance | Distance chosen for the bottom row keeps `diff` | | vehicle status PID added to every query by the status panel is not drawn | `VEHICLE_STATUS` (added by `QueryStrategyOrchestrator` when the status panel or disconnect-when-off is on) leaves both rows unchanged | diff --git a/screen_renderer/src/main/java/org/obd/graphs/renderer/performance/PerformanceMetrics.kt b/screen_renderer/src/main/java/org/obd/graphs/renderer/performance/PerformanceMetrics.kt index fbb4ca57..ec89e340 100644 --- a/screen_renderer/src/main/java/org/obd/graphs/renderer/performance/PerformanceMetrics.kt +++ b/screen_renderer/src/main/java/org/obd/graphs/renderer/performance/PerformanceMetrics.kt @@ -18,7 +18,7 @@ package org.obd.graphs.renderer.performance import org.obd.graphs.bl.query.PERFORMANCE_MAX_GAUGES import org.obd.graphs.bl.query.PERFORMANCE_STATUS_PIDS -import org.obd.graphs.renderer.PidOrder +import org.obd.graphs.bl.query.PidOrder import org.obd.graphs.renderer.trip.TripInfoGrid import org.obd.graphs.renderer.trip.TripInfoMetrics diff --git a/screen_renderer/src/main/java/org/obd/graphs/renderer/trip/TripInfoMetrics.kt b/screen_renderer/src/main/java/org/obd/graphs/renderer/trip/TripInfoMetrics.kt index 7d388cf8..67e5e8cc 100644 --- a/screen_renderer/src/main/java/org/obd/graphs/renderer/trip/TripInfoMetrics.kt +++ b/screen_renderer/src/main/java/org/obd/graphs/renderer/trip/TripInfoMetrics.kt @@ -17,9 +17,9 @@ package org.obd.graphs.renderer.trip import org.obd.graphs.bl.datalogger.Pid +import org.obd.graphs.bl.query.PidOrder import org.obd.graphs.bl.query.TRIP_INFO_DEFAULT_BOTTOM_PIDS import org.obd.graphs.bl.query.TRIP_INFO_STATUS_PIDS -import org.obd.graphs.renderer.PidOrder internal const val MAX_BOTTOM_ITEMS = 4 diff --git a/screen_renderer/src/test/java/org/obd/graphs/renderer/performance/PerformanceMetricsTest.kt b/screen_renderer/src/test/java/org/obd/graphs/renderer/performance/PerformanceMetricsTest.kt index 9e88304b..4d87e6a4 100644 --- a/screen_renderer/src/test/java/org/obd/graphs/renderer/performance/PerformanceMetricsTest.kt +++ b/screen_renderer/src/test/java/org/obd/graphs/renderer/performance/PerformanceMetricsTest.kt @@ -20,6 +20,7 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test import org.obd.graphs.bl.datalogger.Pid +import org.obd.graphs.bl.query.performanceBottomDialogItems private const val CUSTOM_PID_1 = 6001L private const val CUSTOM_PID_2 = 6002L @@ -138,6 +139,18 @@ class PerformanceMetricsTest { assertEquals(listOf(7007L, 7028L, 7005L, CUSTOM_PID_1), result.bottom) } + @Test + fun `the order the gauge dialog stores on opening keeps the profile order`() { + // The dialog stores its checked rows' order when first opened, and a stored order beats the profile one. + val listed = performanceBottomDialogItems(profileSelection.reversed().toSet(), profileHidden, profileBottom) + val gauges = setOf(7028L, 7005L, 7007L) + val storedOrder = listed.filter { gauges.contains(it) }.withIndex().associate { it.value to it.index } + + val result = plan(available = profileSelection, bottomSelection = gauges, bottomSortOrder = storedOrder) + + assertEquals(listOf(7028L, 7005L, 7007L), result.bottom) + } + @Test fun `empty gauge selection means no gauges`() { val result = plan(available = profileSelection, bottomSelection = emptySet()) diff --git a/screen_renderer/src/test/java/org/obd/graphs/renderer/trip/TripInfoMetricsTest.kt b/screen_renderer/src/test/java/org/obd/graphs/renderer/trip/TripInfoMetricsTest.kt index 0af07662..c3422acc 100644 --- a/screen_renderer/src/test/java/org/obd/graphs/renderer/trip/TripInfoMetricsTest.kt +++ b/screen_renderer/src/test/java/org/obd/graphs/renderer/trip/TripInfoMetricsTest.kt @@ -21,6 +21,7 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test import org.obd.graphs.bl.datalogger.Pid +import org.obd.graphs.bl.query.tripInfoBottomDialogItems private const val CUSTOM_PID_1 = 6001L private const val CUSTOM_PID_2 = 6002L @@ -165,6 +166,17 @@ class TripInfoMetricsTest { assertEquals(legacyBottom + CUSTOM_PID_1, plan.bottom.map { it.id }) } + @Test + fun `the order the bottom dialog stores on opening keeps the default row order`() { + // The dialog stores its checked rows' order when first opened, and a stored order beats the default one. + val listed = tripInfoBottomDialogItems((legacySelection + CUSTOM_PID_1).reversed().toSet()) + val storedOrder = listed.filter { legacyBottom.contains(it) }.withIndex().associate { it.value to it.index } + + val plan = TripInfoMetrics.plan(legacySelection, legacyBottom.toSet(), null, storedOrder) + + assertEquals(legacyBottom, plan.bottom.map { it.id }) + } + @Test fun `empty bottom selection means no bottom row`() { val plan = TripInfoMetrics.plan(legacySelection, emptySet(), null, null)