From ccc3ae260c7a74dbcd1e75e8891d5a56029cd99d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 5 Sep 2026 16:15:33 -0700 Subject: [PATCH 01/11] feat(metrics): add a temperature and power page to the carousel (ADFA-5499) A third carousel page charts battery temperature against instantaneous power draw, with thermal throttling shaded behind the plot and the battery level shown in the corner. Design decisions, and what was rejected: - Instantaneous power, not cumulative. A running total only ever rises and says nothing about which piece of work cost anything; instantaneous draw lines up with the spikes on the memory and network pages. - Battery readings only. The per-zone CPU, GPU and skin temperatures need android.permission.DEVICE_POWER, which is prot=signature|role|module -- it cannot be granted to an installed app, so there is no prompt to defer and no fallback worth attempting. PowerSource is an interface so a privileged build can supply better readings without the chart changing. - Throttling is shaded, not plotted. The platform reports an ordinal level, not a temperature, so plotting it against degrees would invent a scale. Alpha rises with severity so the bands read as a gradient of concern. - Battery level is a readout, not a series: it moves about a percent every few minutes, so over the chart's window a line would be flat, spending an axis on a constant. Hidden while charging, when a rising level would contradict a chart about power being spent. Charging periods are not shaded. - Two value axes, the only page with them. Degrees and milliwatts share no unit, so each series declares its axis; a series left on the default would be drawn against labels that do not describe it. - Power is plotted as a magnitude. The battery current reverses while charging, and a line dipping below zero would read as negative power spent. Two defects found on-device, both invisible to passing unit tests -- the same class of failure as the black-on-black axis labels and the black-tinted arrows earlier in this stack: - Shading never reached the screen. setDrawGridBackground(true) fills the plot opaquely inside super.onDraw, so spans painted before it were covered. SafeLineChart now overrides drawGridBackground and paints the spans straight after that fill, which also puts them under the grid lines and the data. - A single-sample throttle had zero width. Spans ran centre to centre, so one sample mapped to one pixel column and two adjacent runs left a sample-wide gap. Each span now covers its samples' full cells. Also wired up two things that were built but unreachable: the power page's x-axis tap now opens the sampling-rate chooser like the other pages, and batteryReadout() now has a view to write to. Verified on a Pixel 6 Pro (arm64) with `cmd thermalservice override-status` stepped through levels 1, 3 and 6 and `dumpsys battery unplug`: three bands appear, deepen with severity, abut without gaps, and stop when the override clears. Checked at font scale 1.0 and 2.0 -- the title, arrows and battery readout all grow without clipping. The chart's own axis and legend text is drawn by MPAndroidChart in dp and does not scale, which is a pre-existing limitation of the library recorded under ADFA-5486, not new here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../activities/editor/BaseEditorActivity.kt | 6 + .../androidide/ui/MetricsCarouselAdapter.kt | 23 ++ .../ui/MetricsCarouselController.kt | 51 +++- .../androidide/ui/PowerUsageChartRenderer.kt | 260 ++++++++++++++++ .../com/itsaky/androidide/ui/SafeLineChart.kt | 62 ++++ .../androidide/utils/DevicePowerSource.kt | 139 +++++++++ .../androidide/utils/PowerUsageWatcher.kt | 281 ++++++++++++++++++ .../androidide/viewmodel/MetricsViewModel.kt | 16 +- .../res/layout/item_metrics_power_chart.xml | 13 + app/src/main/res/layout/layout_mem_usage.xml | 15 + .../ui/PowerUsageChartRendererTest.kt | 255 ++++++++++++++++ .../androidide/utils/PowerUsageWatcherTest.kt | 188 ++++++++++++ resources/src/main/res/values/strings.xml | 4 + 13 files changed, 1310 insertions(+), 3 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt create mode 100644 app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt create mode 100644 app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt create mode 100644 app/src/main/res/layout/item_metrics_power_chart.xml create mode 100644 app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt index 72299abfc4..48ed604014 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt @@ -196,10 +196,13 @@ abstract class BaseEditorActivity : protected val networkUsageWatcher get() = metricsViewModel.networkUsageWatcher + protected val powerUsageWatcher get() = metricsViewModel.powerUsageWatcher + protected val metricsCarousel by lazy { MetricsCarouselController( memoryUsageWatcher = memoryUsageWatcher, networkUsageWatcher = networkUsageWatcher, + powerUsageWatcher = powerUsageWatcher, lineColorFor = ::getMemUsageLineColorFor, annotations = metricsViewModel.annotations, ) @@ -1072,6 +1075,9 @@ abstract class BaseEditorActivity : if (!networkUsageWatcher.isWatching) { networkUsageWatcher.startWatching() } + if (!powerUsageWatcher.isWatching) { + powerUsageWatcher.startWatching() + } if (!isMetricsCarouselUndocked()) { // Draw whatever was sampled while away, rather than waiting for the next tick. diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt index e9ff65f7f8..cc2b1bcc51 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt @@ -42,6 +42,11 @@ sealed interface MetricsPage { data class NetworkChart( @StringRes override val title: Int, ) : MetricsPage + + /** The live temperature and power chart, rendered by [PowerUsageChartRenderer]. */ + data class PowerChart( + @StringRes override val title: Int, + ) : MetricsPage } /** @@ -58,6 +63,7 @@ class MetricsCarouselAdapter( private val pages: List, private val memoryChartRenderer: MemoryUsageChartRenderer, private val networkChartRenderer: NetworkUsageChartRenderer, + private val powerChartRenderer: PowerUsageChartRenderer, ) : RecyclerView.Adapter() { sealed class PageViewHolder( view: View, @@ -69,6 +75,10 @@ class MetricsCarouselAdapter( class NetworkChart( val chart: SafeLineChart, ) : PageViewHolder(chart) + + class PowerChart( + val chart: SafeLineChart, + ) : PageViewHolder(chart) } override fun getItemCount(): Int = pages.size @@ -77,6 +87,7 @@ class MetricsCarouselAdapter( when (pages[position]) { is MetricsPage.MemoryChart -> VIEW_TYPE_MEMORY_CHART is MetricsPage.NetworkChart -> VIEW_TYPE_NETWORK_CHART + is MetricsPage.PowerChart -> VIEW_TYPE_POWER_CHART } override fun onCreateViewHolder( @@ -97,6 +108,12 @@ class MetricsCarouselAdapter( ) } + VIEW_TYPE_POWER_CHART -> { + PageViewHolder.PowerChart( + inflater.inflate(R.layout.item_metrics_power_chart, parent, false) as SafeLineChart, + ) + } + else -> { throw IllegalArgumentException("Unknown metrics page view type: $viewType") } @@ -115,6 +132,10 @@ class MetricsCarouselAdapter( is MetricsPage.NetworkChart -> { networkChartRenderer.attach((holder as PageViewHolder.NetworkChart).chart) } + + is MetricsPage.PowerChart -> { + powerChartRenderer.attach((holder as PageViewHolder.PowerChart).chart) + } } } @@ -125,11 +146,13 @@ class MetricsCarouselAdapter( when (holder) { is PageViewHolder.MemoryChart -> memoryChartRenderer.detachIfAttached(holder.chart) is PageViewHolder.NetworkChart -> networkChartRenderer.detachIfAttached(holder.chart) + is PageViewHolder.PowerChart -> powerChartRenderer.detachIfAttached(holder.chart) } } private companion object { const val VIEW_TYPE_MEMORY_CHART = 0 const val VIEW_TYPE_NETWORK_CHART = 1 + const val VIEW_TYPE_POWER_CHART = 2 } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index bd756ee7bf..cc63c6e114 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.ui import android.widget.Toast import androidx.annotation.UiThread +import androidx.core.view.isVisible import androidx.viewpager2.widget.ViewPager2 import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.databinding.LayoutMemUsageBinding @@ -30,6 +31,7 @@ import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.MetricsSamplingRates import com.itsaky.androidide.utils.MetricsSnapshot import com.itsaky.androidide.utils.NetworkUsageWatcher +import com.itsaky.androidide.utils.PowerUsageWatcher /** * Drives one metrics carousel: its pages, its renderers, and the title that names the current page. @@ -48,6 +50,7 @@ import com.itsaky.androidide.utils.NetworkUsageWatcher class MetricsCarouselController( private val memoryUsageWatcher: MemoryUsageWatcher, private val networkUsageWatcher: NetworkUsageWatcher, + private val powerUsageWatcher: PowerUsageWatcher, lineColorFor: (MemoryUsageWatcher.ProcessMemoryInfo) -> Int, annotations: MetricsAnnotationStore? = null, ) { @@ -72,8 +75,23 @@ class MetricsCarouselController( // (ADFA-5489), replacing the brand-mark placeholder that ADFA-5487 shipped. MetricsPage.MemoryChart(title = string.metrics_title_memory), MetricsPage.NetworkChart(title = string.metrics_title_network), + MetricsPage.PowerChart(title = string.metrics_title_power), ) + private val powerRenderer = + PowerUsageChartRenderer( + usageProvider = { powerUsageWatcher.getUsage() }, + batteryProvider = { powerUsageWatcher.latestBattery }, + annotations = annotations, + sampleIntervalMillis = { powerUsageWatcher.updateInterval }, + ) + + private val powerListener = + PowerUsageWatcher.PowerUsageListener { usage -> + powerRenderer.onUsageChanged(usage) + updateBatteryReadout() + } + private val memoryListener = MemoryUsageWatcher.MemoryUsageListener { memoryUsage -> memoryRenderer.onUsagesChanged(memoryUsage) @@ -101,7 +119,7 @@ class MetricsCarouselController( fun bind(binding: LayoutMemUsageBinding) { this.binding = binding - binding.metricsPager.adapter = MetricsCarouselAdapter(pages, memoryRenderer, networkRenderer) + binding.metricsPager.adapter = MetricsCarouselAdapter(pages, memoryRenderer, networkRenderer, powerRenderer) val showTitleFor = { position: Int -> pages.getOrNull(position)?.let { page -> @@ -114,9 +132,11 @@ class MetricsCarouselController( override fun onPageSelected(position: Int) { showTitleFor(position) updateArrows(position) + updateBatteryReadout() // A page left zoomed would keep claiming horizontal drags when swiped back to. memoryRenderer.resetZoom() networkRenderer.resetZoom() + powerRenderer.resetZoom() } }.also { binding.metricsPager.registerOnPageChangeCallback(it) } @@ -132,6 +152,9 @@ class MetricsCarouselController( memoryRenderer.onXAxisTap = { showSamplingRateDialog() } networkRenderer.onXAxisTap = { showSamplingRateDialog() } + powerRenderer.onXAxisTap = { showSamplingRateDialog() } + + updateBatteryReadout() // A camera button in the graph's bottom-right corner exports the chart. The gestures over // the chart are all spoken for, so this is a control rather than another gesture. @@ -146,6 +169,7 @@ class MetricsCarouselController( memoryUsageWatcher.listener = memoryListener networkUsageWatcher.listener = networkListener + powerUsageWatcher.listener = powerListener } /** @@ -160,9 +184,13 @@ class MetricsCarouselController( if (networkUsageWatcher.listener === networkListener) { networkUsageWatcher.listener = null } + if (powerUsageWatcher.listener === powerListener) { + powerUsageWatcher.listener = null + } memoryRenderer.onXAxisTap = null networkRenderer.onXAxisTap = null + powerRenderer.onXAxisTap = null binding?.metricsSnapshot?.setOnClickListener(null) binding?.metricsPrevious?.setOnClickListener(null) binding?.metricsNext?.setOnClickListener(null) @@ -172,6 +200,7 @@ class MetricsCarouselController( binding?.metricsPager?.adapter = null memoryRenderer.detach() networkRenderer.detach() + powerRenderer.detach() binding = null } @@ -199,6 +228,22 @@ class MetricsCarouselController( binding.metricsNext.alpha = if (position < pages.lastIndex) 1f else DISABLED_ARROW_ALPHA } + /** + * Shows the battery level beside the power chart, and nowhere else (ADFA-5499). + * + * It is a readout rather than a plotted series because the level moves about a percent every few + * minutes: over the chart's window a line would be flat, spending an axis on a constant. + */ + @UiThread + private fun updateBatteryReadout() { + val binding = this.binding ?: return + val onPowerPage = pages.getOrNull(binding.metricsPager.currentItem) is MetricsPage.PowerChart + val readout = if (onPowerPage) powerRenderer.batteryReadout() else null + + binding.metricsBattery.text = readout.orEmpty() + binding.metricsBattery.isVisible = readout != null + } + /** * The renderer behind the page currently on screen, or `null` when nothing is bound. */ @@ -207,6 +252,7 @@ class MetricsCarouselController( return when (pages.getOrNull(binding.metricsPager.currentItem)) { is MetricsPage.MemoryChart -> memoryRenderer is MetricsPage.NetworkChart -> networkRenderer + is MetricsPage.PowerChart -> powerRenderer null -> null } } @@ -263,6 +309,7 @@ class MetricsCarouselController( private fun setSamplingInterval(intervalMillis: Long) { memoryUsageWatcher.updateInterval = intervalMillis networkUsageWatcher.updateInterval = intervalMillis + powerUsageWatcher.updateInterval = intervalMillis refresh() } @@ -289,6 +336,7 @@ class MetricsCarouselController( when (page) { is MetricsPage.MemoryChart -> memoryRenderer is MetricsPage.NetworkChart -> networkRenderer + is MetricsPage.PowerChart -> powerRenderer } val label = context.getString(page.title) @@ -316,6 +364,7 @@ class MetricsCarouselController( fun refresh() { memoryRenderer.rebuild() networkRenderer.rebuild() + powerRenderer.rebuild() } /** diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt new file mode 100644 index 0000000000..09fb52d14b --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -0,0 +1,260 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.graphics.Color +import androidx.annotation.UiThread +import androidx.core.graphics.ColorUtils +import com.github.mikephil.charting.components.AxisBase +import com.github.mikephil.charting.components.YAxis +import com.github.mikephil.charting.data.Entry +import com.github.mikephil.charting.data.LineDataSet +import com.github.mikephil.charting.formatter.IAxisValueFormatter +import com.itsaky.androidide.R +import com.itsaky.androidide.utils.MetricsAnnotationStore +import com.itsaky.androidide.utils.PowerUsageWatcher +import com.itsaky.androidide.utils.PowerUsageWatcher.PowerUsage +import com.itsaky.androidide.utils.resolveAttr +import kotlin.math.abs +import kotlin.math.roundToLong + +/** + * Renders [PowerUsageWatcher] samples: battery temperature against power draw (ADFA-5499). + * + * The only page with two value axes. Degrees and milliwatts differ in unit and by orders of + * magnitude, so temperature takes the left axis and power the right. Both series therefore have to + * declare which axis they belong to -- a dataset left on the default would be drawn against an axis + * whose labels do not describe it, which is a bug this codebase has already shipped once. + * + * Thermal throttling is shown as background shading rather than as a line: the platform reports an + * ordinal level, not a temperature, so plotting it against degrees would invent a scale. The level + * is sampled alongside the readings, so a shaded band is simply a run of equal levels. + */ +class PowerUsageChartRenderer( + private val usageProvider: () -> PowerUsage, + private val batteryProvider: () -> PowerUsageWatcher.BatteryState, + annotations: MetricsAnnotationStore? = null, + sampleIntervalMillis: () -> Long = { PowerUsageWatcher.DEFAULT_UPDATE_INTERVAL }, +) : MetricsChartRenderer( + sampleIntervalMillis = sampleIntervalMillis, + annotations = annotations, + ) { + @UiThread + override fun rebuild() { + val chart = this.chart ?: return + val usage = usageProvider() + val context = chart.context + + val datasets = + arrayOf( + series( + values = usage.temperatureMilliCelsius, + label = context.getString(R.string.metrics_power_temperature), + lineColor = TEMPERATURE_COLOR, + axis = YAxis.AxisDependency.LEFT, + transform = ::milliCelsiusToCelsius, + ), + series( + values = usage.powerMicroWatts, + label = context.getString(R.string.metrics_power_draw), + lineColor = POWER_COLOR, + axis = YAxis.AxisDependency.RIGHT, + transform = ::microWattsToMilliWatts, + ), + ) + + setData(chart, datasets) + applyThermalShading(chart, usage) + } + + /** + * Redraws from a fresh sample. Rebuilds rather than mutating in place: this chart samples + * relatively slowly and has two short series, so the saving is not worth a second code path + * that can disagree with the first. + */ + @UiThread + fun onUsageChanged(usage: PowerUsage) { + chart ?: return + rebuild() + } + + /** + * Paints a band behind the chart for each stretch of throttling, deepening with the level. + * + * Unthrottled and unknown stretches are left unpainted: shading everything would say nothing. + */ + private fun applyThermalShading( + chart: SafeLineChart, + usage: PowerUsage, + ) { + val levels = usage.thermalStatus + val spans = mutableListOf() + + var index = 0 + while (index < levels.size) { + val level = levels[index].toInt() + var end = index + while (end + 1 < levels.size && levels[end + 1].toInt() == level) { + end++ + } + + shadeFor(chart, level)?.let { color -> + // Half a sample either side, so each sample covers its own cell: a single-sample + // spike would otherwise have zero width and never be drawn, and two adjacent runs + // would leave a sample-wide gap between them. + spans += SafeLineChart.Span(index - HALF_SAMPLE, end + HALF_SAMPLE, color) + } + index = end + 1 + } + + chart.backgroundSpans = spans + } + + /** + * The shade for a throttling level, or `null` where there is nothing to say. + * + * Alpha rises with severity so the bands read as a gradient of concern rather than as separate + * categories, and stays low enough throughout that the plotted lines remain the foreground. + */ + private fun shadeFor( + chart: SafeLineChart, + level: Int, + ): Int? { + val alpha = + when (level) { + THERMAL_LIGHT -> 24 + THERMAL_MODERATE -> 40 + THERMAL_SEVERE -> 64 + THERMAL_CRITICAL -> 88 + THERMAL_EMERGENCY, THERMAL_SHUTDOWN -> 112 + else -> return null + } + + val base = chart.context.resolveAttr(R.attr.colorError) + return ColorUtils.setAlphaComponent(base, alpha) + } + + private fun series( + values: LongArray, + label: String, + lineColor: Int, + axis: YAxis.AxisDependency, + transform: (Long) -> Float, + ): LineDataSet = + LineDataSet( + values.mapIndexed { index, value -> Entry(index.toFloat(), transform(value)) }, + label, + ).apply { + axisDependency = axis + color = lineColor + setDrawIcons(false) + setDrawCircles(false) + setDrawCircleHole(false) + setDrawValues(false) + formLineWidth = 1f + formSize = 15f + isHighlightEnabled = false + this.label = labelFor(label, values.lastOrNull(), axis) + } + + private fun labelFor( + label: String, + latest: Long?, + axis: YAxis.AxisDependency, + ): String { + val value = latest ?: PowerUsageWatcher.UNAVAILABLE + if (value == PowerUsageWatcher.UNAVAILABLE) { + return "%s - n/a".format(label) + } + + return if (axis == YAxis.AxisDependency.LEFT) { + "%s - %.1fC".format(label, milliCelsiusToCelsius(value)) + } else { + "%s - %.0fmW".format(label, milliWattsMagnitude(value)) + } + } + + override fun configure(chart: SafeLineChart) { + super.configure(chart) + + // Two units, two axes: the base class disables the left one because every other page has a + // single series family. + chart.axisLeft.isEnabled = true + chart.axisLeft.valueFormatter = + object : IAxisValueFormatter { + override fun getFormattedValue( + value: Float, + axis: AxisBase?, + ): String = "%dC".format(value.roundToLong()) + } + + chart.axisRight.valueFormatter = + object : IAxisValueFormatter { + override fun getFormattedValue( + value: Float, + axis: AxisBase?, + ): String = "%dmW".format(value.roundToLong()) + } + } + + /** + * The battery line for the legend, or `null` while charging. + * + * Level is a readout rather than a series because it moves about a percent every few minutes: + * over the chart's window a plotted line would be flat, spending an axis on a constant. It is + * hidden while charging, when a rising level would contradict a chart about power being spent. + */ + @UiThread + fun batteryReadout(): String? { + val battery = batteryProvider() + if (battery.isCharging || battery.levelPercent < 0) { + return null + } + return "%d%%".format(battery.levelPercent) + } + + private companion object { + val TEMPERATURE_COLOR = Color.rgb(255, 138, 101) + val POWER_COLOR = Color.rgb(129, 212, 250) + + /** Half the x-axis width of one sample, which is 1 because x values are sample indices. */ + const val HALF_SAMPLE = 0.5f + + const val THERMAL_LIGHT = 1 + const val THERMAL_MODERATE = 2 + const val THERMAL_SEVERE = 3 + const val THERMAL_CRITICAL = 4 + const val THERMAL_EMERGENCY = 5 + const val THERMAL_SHUTDOWN = 6 + } +} + +/** + * An unavailable reading plots at zero rather than breaking the line. + */ +private fun milliCelsiusToCelsius(milliCelsius: Long): Float = + if (milliCelsius == PowerUsageWatcher.UNAVAILABLE) 0f else milliCelsius / 1000f + +/** + * Power is plotted as a magnitude. The battery current reverses while charging, and a line that + * dips below zero would read as the device spending negative power. + */ +private fun microWattsToMilliWatts(microWatts: Long): Float = + if (microWatts == PowerUsageWatcher.UNAVAILABLE) 0f else abs(microWatts) / 1000f + +private fun milliWattsMagnitude(microWatts: Long): Float = abs(microWatts) / 1000f diff --git a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt index 3eda88b076..3f93c67ba3 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -19,8 +19,10 @@ package com.itsaky.androidide.ui import android.content.Context import android.graphics.Canvas +import android.graphics.Paint import android.util.AttributeSet import com.github.mikephil.charting.charts.LineChart +import com.github.mikephil.charting.components.YAxis import org.slf4j.LoggerFactory /** @@ -53,6 +55,66 @@ class SafeLineChart : LineChart { private var skippedFrames = 0L + /** + * Bands painted behind the data, in x-value coordinates (ADFA-5499's thermal shading). + * + * Drawn here rather than by the caller because the chart owns the transformer that maps an + * x value to a pixel, and that mapping changes with every zoom, pan and layout. + */ + var backgroundSpans: List = emptyList() + set(value) { + field = value + invalidate() + } + + /** + * A shaded range of the x axis. + * + * @property startX First x value covered, inclusive. + * @property endX Last x value covered, inclusive. + * @property color Fill colour, expected to carry its own alpha. + */ + data class Span( + val startX: Float, + val endX: Float, + val color: Int, + ) + + private val spanPaint = Paint(Paint.ANTI_ALIAS_FLAG) + + /** + * Draws the spans immediately after the grid background, which is an opaque fill of the plot: a + * span painted before [onDraw] delegates upwards is covered by it and never reaches the screen. + * Landing here also puts the shading under the grid lines and the data, where it belongs. + */ + override fun drawGridBackground(canvas: Canvas) { + super.drawGridBackground(canvas) + drawBackgroundSpans(canvas) + } + + private fun drawBackgroundSpans(canvas: Canvas) { + if (backgroundSpans.isEmpty()) { + return + } + + val content = viewPortHandler.contentRect + val transformer = getTransformer(YAxis.AxisDependency.LEFT) ?: return + + backgroundSpans.forEach { span -> + val left = transformer.getPixelForValues(span.startX, 0f).x.toFloat() + val right = transformer.getPixelForValues(span.endX, 0f).x.toFloat() + // A span scrolled out of view still maps to a pixel, so clip to the plot. + val clippedLeft = left.coerceAtLeast(content.left) + val clippedRight = right.coerceAtMost(content.right) + if (clippedRight <= clippedLeft) { + return@forEach + } + + spanPaint.color = span.color + canvas.drawRect(clippedLeft, content.top, clippedRight, content.bottom, spanPaint) + } + } + override fun onDraw(canvas: Canvas) { try { super.onDraw(canvas) diff --git a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt new file mode 100644 index 0000000000..882d7de1b3 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt @@ -0,0 +1,139 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.content.Context +import android.content.Intent +import android.content.IntentFilter +import android.os.BatteryManager +import android.os.Build +import android.os.PowerManager +import androidx.core.content.getSystemService +import com.itsaky.androidide.services.builder.ThermalInfo +import com.itsaky.androidide.services.builder.ThermalState +import com.itsaky.androidide.utils.PowerUsageWatcher.BatteryState +import com.itsaky.androidide.utils.PowerUsageWatcher.PowerReading + +/** + * Reads temperature and power from the battery, which is all a normally-installed app can see + * (ADFA-5499). + * + * `ACTION_BATTERY_CHANGED` is a sticky broadcast, so the current values can be read on demand with a + * null receiver rather than by registering one and waiting -- which suits being polled on the + * sampling tick. + * + * Not read here, deliberately: the per-zone CPU, GPU and skin temperatures from + * `HardwarePropertiesManager`. Those need `android.permission.DEVICE_POWER`, which is signature + * level and cannot be granted to an installed app, so there is nothing to ask for and no fallback + * worth attempting. A privileged build would supply a different `PowerSource`. + */ +class DevicePowerSource( + private val context: Context, +) : PowerUsageWatcher.PowerSource { + private val batteryManager = context.getSystemService() + private val powerManager = context.getSystemService() + + override fun read(): PowerReading { + val battery = context.registerReceiver(null, IntentFilter(Intent.ACTION_BATTERY_CHANGED)) + + return PowerReading( + temperatureMilliCelsius = readTemperature(battery), + powerMicroWatts = readPower(battery), + thermalStatus = readThermalStatus(), + battery = readBatteryState(battery), + ) + } + + /** + * Battery temperature. The broadcast reports tenths of a degree, which is coarser than the + * millidegrees stored, but storing the finer unit keeps the arithmetic honest if a privileged + * source ever supplies something better. + */ + private fun readTemperature(battery: Intent?): Long { + val tenthsCelsius = battery?.getIntExtra(BatteryManager.EXTRA_TEMPERATURE, Int.MIN_VALUE) + if (tenthsCelsius == null || tenthsCelsius == Int.MIN_VALUE) { + return PowerUsageWatcher.UNAVAILABLE + } + return tenthsCelsius.toLong() * 100L + } + + /** + * Instantaneous draw, from current and voltage. + * + * Microamps times millivolts is nanowatts, so the product is scaled down to microwatts. The sign + * follows the battery current: negative while charging, because current is then flowing in. + */ + private fun readPower(battery: Intent?): Long { + val microAmps = batteryManager?.getIntProperty(BatteryManager.BATTERY_PROPERTY_CURRENT_NOW) + val milliVolts = battery?.getIntExtra(BatteryManager.EXTRA_VOLTAGE, Int.MIN_VALUE) + + if (microAmps == null || microAmps == Int.MIN_VALUE || + milliVolts == null || milliVolts <= 0 + ) { + return PowerUsageWatcher.UNAVAILABLE + } + + return microAmps.toLong() * milliVolts.toLong() / NANOWATTS_PER_MICROWATT + } + + /** + * The platform's throttling level, which is what the chart shades by. + * + * Only API 29 and above report a graded level. Below that [ThermalInfo] can still say whether + * the device is throttled at all, which gives one shade instead of several. + */ + private fun readThermalStatus(): Int { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + val status = runCatching { powerManager?.currentThermalStatus }.getOrNull() + if (status != null) { + return status + } + } + + return when (ThermalInfo.getThermalState(context)) { + ThermalState.Throttled -> PowerManager.THERMAL_STATUS_SEVERE + ThermalState.NotThrottled -> PowerManager.THERMAL_STATUS_NONE + else -> PowerUsageWatcher.THERMAL_UNKNOWN + } + } + + private fun readBatteryState(battery: Intent?): BatteryState { + battery ?: return BatteryState.UNKNOWN + + val level = battery.getIntExtra(BatteryManager.EXTRA_LEVEL, -1) + val scale = battery.getIntExtra(BatteryManager.EXTRA_SCALE, -1) + val status = battery.getIntExtra(BatteryManager.EXTRA_STATUS, BatteryManager.BATTERY_STATUS_UNKNOWN) + + val percent = + if (level < 0 || scale <= 0) { + -1 + } else { + level * 100 / scale + } + + return BatteryState( + levelPercent = percent, + isCharging = status == BatteryManager.BATTERY_STATUS_CHARGING || status == BatteryManager.BATTERY_STATUS_FULL, + ) + } + + private companion object { + /** Microamps times millivolts gives nanowatts; this scales the product to microwatts. */ + const val NANOWATTS_PER_MICROWATT = 1_000L + } +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt new file mode 100644 index 0000000000..7c1e8b987a --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -0,0 +1,281 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import androidx.annotation.VisibleForTesting +import com.itsaky.androidide.tasks.cancelIfActive +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.DelicateCoroutinesApi +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExecutorCoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.newSingleThreadContext +import kotlinx.coroutines.withContext +import org.slf4j.LoggerFactory +import java.util.concurrent.atomic.AtomicBoolean +import kotlin.coroutines.CoroutineContext + +/** + * Samples the device's temperature and power draw (ADFA-5499). + * + * What a normally-installed app can read is narrower than it sounds. Battery temperature and the + * current/voltage pair behind power come from the battery, free of any permission. The per-zone CPU + * and skin temperatures the platform itself can see need `android.permission.DEVICE_POWER`, which is + * signature-level and cannot be granted to an installed app at all -- hence [PowerSource], so a + * privileged build could supply better readings without the chart changing. + * + * Power is instantaneous rather than cumulative: a running total only ever rises and says nothing + * about which piece of work cost anything, whereas power lines up with the spikes on the memory and + * network pages. + * + * @param updateInterval Milliseconds between samples. + * @param source Where readings come from. Injectable so tests need no device. + */ +class PowerUsageWatcher + @OptIn(ExperimentalCoroutinesApi::class, DelicateCoroutinesApi::class) + constructor( + updateInterval: Long = DEFAULT_UPDATE_INTERVAL, + private val source: PowerSource, + private val coroutineDispatcher: CoroutineContext = newSingleThreadContext("PowerUsageWatcher"), + private val mainDispatcher: CoroutineContext = Dispatchers.Main.immediate, + ) { + private val coroutineScope = CoroutineScope(SupervisorJob() + coroutineDispatcher) + private val watching = AtomicBoolean(false) + + /** The running sampling loop, so [stopWatching] can actually stop it. */ + private var samplingJob: Job? = null + + /** Guards the ring buffers: the sampler writes them, the UI thread snapshots them. */ + private val historyLock = Any() + + private val temperature = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + private val power = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + + /** + * The thermal throttling level at each sample, or [THERMAL_UNKNOWN]. + * + * Kept per sample rather than as a separate timestamped log so the chart's shading lines up + * with the sample grid exactly: a shaded span is just a run of equal values here. + */ + private val thermal = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + + /** + * Milliseconds between samples. Changing it clears the history, for the reason given on + * [MemoryUsageWatcher.updateInterval]. + */ + var updateInterval: Long = updateInterval + set(value) { + if (field == value) { + return + } + field = value + clearHistory() + } + + /** The most recent battery reading, for the chart's legend. */ + @Volatile + var latestBattery: BatteryState = BatteryState.UNKNOWN + private set + + val isWatching: Boolean + get() = watching.get() + + /** Notified on the main thread after each sample. */ + var listener: PowerUsageListener? = null + + /** + * A snapshot of the sampled history, oldest first. The arrays are copies; handing out the + * live ring buffers would let a reader see them mid-append. + */ + fun getUsage(): PowerUsage = + synchronized(historyLock) { + PowerUsage(temperature.snapshotArray(), power.snapshotArray(), thermal.snapshotArray()) + } + + fun clearHistory() { + synchronized(historyLock) { + temperature.clear() + power.clear() + thermal.clear() + } + } + + fun startWatching() { + if (!watching.compareAndSet(false, true)) { + log.warn("Power usage is already being watched") + return + } + + samplingJob = + coroutineScope.launch { + while (isWatching) { + runCatching { + sampleOnce() + + listener?.also { listener -> + val usage = getUsage() + withContext(mainDispatcher) { + listener.onPowerUsageChanged(usage) + } + } + }.onFailure { failure -> + if (failure is CancellationException) { + throw failure + } + log.error("Power usage sampling failed; continuing", failure) + } + + delay(updateInterval) + } + } + } + + fun stopWatching() { + watching.set(false) + samplingJob?.cancel() + samplingJob = null + } + + /** Stops sampling and releases the sampling thread. The watcher cannot be started again. */ + fun close() { + stopWatching() + listener = null + coroutineScope.cancelIfActive("Watcher closed") + (coroutineDispatcher as? ExecutorCoroutineDispatcher)?.close() + } + + /** + * Takes one sample. The loop calls this once per [updateInterval]; tests call it directly. + */ + @VisibleForTesting + internal fun sampleOnce() { + val reading = source.read() + latestBattery = reading.battery + + synchronized(historyLock) { + append(temperature, reading.temperatureMilliCelsius) + append(power, reading.powerMicroWatts) + append(thermal, reading.thermalStatus.toLong()) + } + } + + private fun append( + history: MutableShiftedLongArray, + value: Long, + ) { + // Newest entry goes in at index 0 and the shift makes it the last element, matching + // MemoryUsageWatcher and NetworkUsageWatcher. + history[0] = value + history.shift(1) + } + + /** + * One sample's worth of readings. + * + * @property temperatureMilliCelsius Battery temperature, or [UNAVAILABLE]. + * @property powerMicroWatts Instantaneous draw, or [UNAVAILABLE]. Negative while charging, + * because the battery current reverses. + * @property thermalStatus The platform throttling level, or [THERMAL_UNKNOWN]. + * @property battery Level and charging state, for the legend. + */ + data class PowerReading( + val temperatureMilliCelsius: Long, + val powerMicroWatts: Long, + val thermalStatus: Int, + val battery: BatteryState, + ) + + /** + * @property levelPercent Charge remaining, or -1 if unknown. + * @property isCharging Whether the battery is being charged. + */ + data class BatteryState( + val levelPercent: Int, + val isCharging: Boolean, + ) { + companion object { + val UNKNOWN = BatteryState(levelPercent = -1, isCharging = false) + } + } + + /** + * Where readings come from. An interface because the best available source depends on how + * the app is installed: a privileged build can read per-zone temperatures that an installed + * one cannot. + */ + fun interface PowerSource { + fun read(): PowerReading + } + + /** + * Sampled history, oldest first. + * + * @property temperatureMilliCelsius Battery temperature per sample. + * @property powerMicroWatts Instantaneous draw per sample. + * @property thermalStatus Throttling level per sample, for the chart's shading. + */ + data class PowerUsage( + val temperatureMilliCelsius: LongArray, + val powerMicroWatts: LongArray, + val thermalStatus: LongArray, + ) { + override fun equals(other: Any?): Boolean = + this === other || + ( + other is PowerUsage && + temperatureMilliCelsius.contentEquals(other.temperatureMilliCelsius) && + powerMicroWatts.contentEquals(other.powerMicroWatts) && + thermalStatus.contentEquals(other.thermalStatus) + ) + + override fun hashCode(): Int { + var result = temperatureMilliCelsius.contentHashCode() + result = 31 * result + powerMicroWatts.contentHashCode() + result = 31 * result + thermalStatus.contentHashCode() + return result + } + } + + fun interface PowerUsageListener { + fun onPowerUsageChanged(usage: PowerUsage) + } + + companion object { + /** Samples retained per series, matching the other watchers. */ + const val MAX_USAGE_ENTRIES = 10000 + const val DEFAULT_UPDATE_INTERVAL = 1000L + + /** A reading the device does not provide. */ + const val UNAVAILABLE = Long.MIN_VALUE + + /** No throttling level could be read -- an API 28 device, or the call failed. */ + const val THERMAL_UNKNOWN = -1 + + private val log = LoggerFactory.getLogger(PowerUsageWatcher::class.java) + } + } + +/** + * Copies this ring buffer into a plain array in logical order, oldest first. + */ +private fun ShiftedLongArray.snapshotArray(): LongArray = LongArray(size) { this[it] } diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/MetricsViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/MetricsViewModel.kt index bcf61bd48d..2400fe4cc5 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/MetricsViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/MetricsViewModel.kt @@ -17,10 +17,13 @@ package com.itsaky.androidide.viewmodel -import androidx.lifecycle.ViewModel +import android.app.Application +import androidx.lifecycle.AndroidViewModel +import com.itsaky.androidide.utils.DevicePowerSource import com.itsaky.androidide.utils.MemoryUsageWatcher import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.NetworkUsageWatcher +import com.itsaky.androidide.utils.PowerUsageWatcher /** * Owns the sample history behind the editor's metrics carousel. @@ -34,11 +37,19 @@ import com.itsaky.androidide.utils.NetworkUsageWatcher * This survives configuration changes and activity recreation. It does not survive the process being * killed -- see ADFA-5494. */ -class MetricsViewModel : ViewModel() { +class MetricsViewModel( + application: Application, +) : AndroidViewModel(application) { val memoryUsageWatcher = MemoryUsageWatcher() val networkUsageWatcher = NetworkUsageWatcher() + /** + * Temperature and power (ADFA-5499). Needs a Context for the battery broadcast, which is why + * this is an AndroidViewModel. + */ + val powerUsageWatcher = PowerUsageWatcher(source = DevicePowerSource(application)) + /** Significant events for the charts to annotate (ADFA-5486). */ val annotations = MetricsAnnotationStore() @@ -48,5 +59,6 @@ class MetricsViewModel : ViewModel() { // dedicated sampling thread that newSingleThreadContext keeps alive until it is closed. memoryUsageWatcher.close() networkUsageWatcher.close() + powerUsageWatcher.close() } } diff --git a/app/src/main/res/layout/item_metrics_power_chart.xml b/app/src/main/res/layout/item_metrics_power_chart.xml new file mode 100644 index 0000000000..aa319feba1 --- /dev/null +++ b/app/src/main/res/layout/item_metrics_power_chart.xml @@ -0,0 +1,13 @@ + + + diff --git a/app/src/main/res/layout/layout_mem_usage.xml b/app/src/main/res/layout/layout_mem_usage.xml index 5190312432..f267ae6466 100644 --- a/app/src/main/res/layout/layout_mem_usage.xml +++ b/app/src/main/res/layout/layout_mem_usage.xml @@ -88,6 +88,21 @@ app:layout_constraintBottom_toBottomOf="@id/metrics_pager" app:layout_constraintEnd_toEndOf="@id/metrics_pager" /> + + + diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt new file mode 100644 index 0000000000..d34ff1081a --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -0,0 +1,255 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.components.YAxis +import com.github.mikephil.charting.data.LineDataSet +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.PowerUsageWatcher +import com.itsaky.androidide.utils.PowerUsageWatcher.BatteryState +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Pins the three decisions ADFA-5499 was scoped around: temperature and power get an axis each + * because they share no unit, throttling is shaded rather than plotted because the platform reports + * an ordinal and not a temperature, and the battery level is hidden while charging. + */ +@RunWith(RobolectricTestRunner::class) +class PowerUsageChartRendererTest { + private val context = ApplicationProvider.getApplicationContext() + + private fun usage( + temperature: LongArray, + power: LongArray = LongArray(temperature.size), + thermal: LongArray = LongArray(temperature.size), + ) = PowerUsageWatcher.PowerUsage(temperature, power, thermal) + + private fun rendererFor( + usage: PowerUsageWatcher.PowerUsage, + battery: BatteryState = BatteryState(levelPercent = 80, isCharging = false), + ): Pair { + val chart = SafeLineChart(context) + val renderer = + PowerUsageChartRenderer( + usageProvider = { usage }, + batteryProvider = { battery }, + ) + renderer.attach(chart) + return renderer to chart + } + + private fun dataset( + chart: SafeLineChart, + index: Int, + ) = chart.data.getDataSetByIndex(index) as LineDataSet + + @Test + fun `temperature and power are plotted against separate axes`() { + val (_, chart) = + rendererFor( + usage( + temperature = longArrayOf(30_000L, 31_000L), + power = longArrayOf(1_000_000L, 4_000_000L), + ), + ) + + assertThat(chart.data.dataSetCount).isEqualTo(2) + // Degrees and milliwatts differ by orders of magnitude; a series left on the default axis + // would be drawn against labels that do not describe it. + assertThat(dataset(chart, 0).axisDependency).isEqualTo(YAxis.AxisDependency.LEFT) + assertThat(dataset(chart, 1).axisDependency).isEqualTo(YAxis.AxisDependency.RIGHT) + assertThat(chart.axisLeft.isEnabled).isTrue() + assertThat(chart.axisRight.isEnabled).isTrue() + } + + @Test + fun `temperature is plotted in degrees and power in milliwatts`() { + val (_, chart) = + rendererFor( + usage( + temperature = longArrayOf(29_700L), + power = longArrayOf(6_358_064L), + ), + ) + + assertThat(dataset(chart, 0).entries.last().y).isWithin(0.01f).of(29.7f) + assertThat(dataset(chart, 1).entries.last().y).isWithin(0.01f).of(6358.064f) + } + + @Test + fun `power is plotted as a magnitude, so charging does not dip below zero`() { + val (_, chart) = + rendererFor( + usage( + temperature = longArrayOf(30_000L, 30_000L), + // The battery current reverses while charging. + power = longArrayOf(2_000_000L, -3_000_000L), + ), + ) + + val ys = dataset(chart, 1).entries.map { it.y } + + assertThat(ys).containsExactly(2000f, 3000f).inOrder() + assertThat(ys.none { it < 0f }).isTrue() + } + + @Test + fun `an unavailable reading plots at zero rather than at Long MIN_VALUE`() { + val (_, chart) = + rendererFor( + usage( + temperature = longArrayOf(PowerUsageWatcher.UNAVAILABLE, 30_000L), + power = longArrayOf(PowerUsageWatcher.UNAVAILABLE, 1_000_000L), + ), + ) + + // Plotted as MIN_VALUE the point would put the axis range into the billions and flatten + // every real reading onto one line. + assertThat(dataset(chart, 0).entries.first().y).isEqualTo(0f) + assertThat(dataset(chart, 1).entries.first().y).isEqualTo(0f) + } + + @Test + fun `the legend says n slash a for a reading the device does not provide`() { + val (_, chart) = + rendererFor( + usage( + temperature = longArrayOf(PowerUsageWatcher.UNAVAILABLE), + power = longArrayOf(PowerUsageWatcher.UNAVAILABLE), + ), + ) + + assertThat(dataset(chart, 0).label).endsWith("n/a") + assertThat(dataset(chart, 1).label).endsWith("n/a") + } + + @Test + fun `a run of one throttling level becomes one shaded span`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(6) { 30_000L }, + thermal = longArrayOf(0L, 0L, 2L, 2L, 2L, 0L), + ), + ) + + assertThat(chart.backgroundSpans).hasSize(1) + val span = chart.backgroundSpans.single() + // Samples 2..4, each covering its own cell rather than just its centre point. + assertThat(span.startX).isEqualTo(1.5f) + assertThat(span.endX).isEqualTo(4.5f) + } + + @Test + fun `a single throttled sample still gets a span with width`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(3) { 30_000L }, + thermal = longArrayOf(0L, 3L, 0L), + ), + ) + + // Drawn from centre to centre this span would be zero pixels wide and never appear. + val span = chart.backgroundSpans.single() + assertThat(span.endX - span.startX).isEqualTo(1f) + } + + @Test + fun `adjacent runs leave no unshaded gap between them`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(4) { 30_000L }, + thermal = longArrayOf(1L, 1L, 3L, 3L), + ), + ) + + val (first, second) = chart.backgroundSpans + assertThat(first.endX).isEqualTo(second.startX) + } + + @Test + fun `adjacent levels shade separately, and deeper for the worse one`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(4) { 30_000L }, + thermal = longArrayOf(1L, 1L, 4L, 4L), + ), + ) + + assertThat(chart.backgroundSpans).hasSize(2) + val (light, critical) = chart.backgroundSpans + // The bands read as a gradient of concern rather than as unrelated categories. + assertThat(critical.color ushr 24).isGreaterThan(light.color ushr 24) + } + + @Test + fun `no shading where there is nothing to say`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(4) { 30_000L }, + // Not throttled, then a device that reports no level at all. + thermal = longArrayOf(0L, 0L, -1L, -1L), + ), + ) + + // Shading everything would say nothing. + assertThat(chart.backgroundSpans).isEmpty() + } + + @Test + fun `the battery readout is hidden while charging`() { + val (charging, _) = + rendererFor( + usage(temperature = longArrayOf(30_000L)), + battery = BatteryState(levelPercent = 62, isCharging = true), + ) + + // A level climbing while the chart is about power being spent reads as a contradiction. + assertThat(charging.batteryReadout()).isNull() + } + + @Test + fun `the battery readout shows the level on battery power`() { + val (renderer, _) = + rendererFor( + usage(temperature = longArrayOf(30_000L)), + battery = BatteryState(levelPercent = 62, isCharging = false), + ) + + assertThat(renderer.batteryReadout()).isEqualTo("62%") + } + + @Test + fun `an unknown battery level shows nothing rather than a negative percentage`() { + val (renderer, _) = + rendererFor( + usage(temperature = longArrayOf(30_000L)), + battery = BatteryState.UNKNOWN, + ) + + assertThat(renderer.batteryReadout()).isNull() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt b/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt new file mode 100644 index 0000000000..f7cf75f39b --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt @@ -0,0 +1,188 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.PowerUsageWatcher.BatteryState +import com.itsaky.androidide.utils.PowerUsageWatcher.PowerReading +import org.junit.After +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Pins what ADFA-5499 records per sample: temperature, instantaneous power and the throttling + * level land on one shared sample grid, and a reading the device does not provide stays + * distinguishable from a real zero. + * + * These drive [PowerUsageWatcher.sampleOnce] directly rather than starting the sampling loop, so + * there is no waiting and no dependence on thread timing. + */ +@RunWith(RobolectricTestRunner::class) +class PowerUsageWatcherTest { + /** Every watcher built here, so the sampling threads they allocate are released. */ + private val created = mutableListOf() + + @After + fun tearDown() { + created.forEach { it.close() } + created.clear() + } + + /** A watcher fed a scripted sequence of readings, advancing one step per sample. */ + private inner class Fixture( + private val readings: List, + ) { + private var index = -1 + + val watcher = + PowerUsageWatcher( + source = { readings[index.coerceIn(0, readings.lastIndex)] }, + ).also { created += it } + + fun sample(count: Int) { + repeat(count) { + index++ + watcher.sampleOnce() + } + } + } + + private fun reading( + temperature: Long = 30_000L, + power: Long = 1_000_000L, + thermal: Int = 0, + battery: BatteryState = BatteryState(levelPercent = 80, isCharging = false), + ) = PowerReading(temperature, power, thermal, battery) + + private fun LongArray.recent(count: Int): List = takeLast(count) + + @Test + fun `history is all zeros before the first sample`() { + val fixture = Fixture(listOf(reading())) + + val usage = fixture.watcher.getUsage() + + assertThat(usage.temperatureMilliCelsius).hasLength(PowerUsageWatcher.MAX_USAGE_ENTRIES) + assertThat(usage.powerMicroWatts.sum()).isEqualTo(0L) + assertThat(usage.thermalStatus.sum()).isEqualTo(0L) + } + + @Test + fun `records temperature, power and throttling level on one sample grid`() { + val fixture = + Fixture( + listOf( + reading(temperature = 30_000L, power = 1_000_000L, thermal = 0), + reading(temperature = 31_500L, power = 4_500_000L, thermal = 2), + reading(temperature = 32_000L, power = 2_250_000L, thermal = 2), + ), + ) + + fixture.sample(3) + val usage = fixture.watcher.getUsage() + + // Index n of each array is the same instant, which is what lets the chart shade a run of + // equal levels by sample index rather than by a separate timeline. + assertThat(usage.temperatureMilliCelsius.recent(3)).containsExactly(30_000L, 31_500L, 32_000L).inOrder() + assertThat(usage.powerMicroWatts.recent(3)).containsExactly(1_000_000L, 4_500_000L, 2_250_000L).inOrder() + assertThat(usage.thermalStatus.recent(3)).containsExactly(0L, 2L, 2L).inOrder() + } + + @Test + fun `an unavailable reading is recorded as unavailable, not as zero`() { + val fixture = Fixture(listOf(reading(temperature = PowerUsageWatcher.UNAVAILABLE, power = PowerUsageWatcher.UNAVAILABLE))) + + fixture.sample(1) + val usage = fixture.watcher.getUsage() + + // A device with no readable current would otherwise plot a flat, believable 0 mW. + assertThat(usage.temperatureMilliCelsius.last()).isEqualTo(PowerUsageWatcher.UNAVAILABLE) + assertThat(usage.powerMicroWatts.last()).isEqualTo(PowerUsageWatcher.UNAVAILABLE) + } + + @Test + fun `negative power is kept as recorded, because charging reverses the current`() { + val fixture = Fixture(listOf(reading(power = -3_000_000L))) + + fixture.sample(1) + + // The watcher records the sign; deciding how to plot it is the renderer's job. + assertThat( + fixture.watcher + .getUsage() + .powerMicroWatts + .last(), + ).isEqualTo(-3_000_000L) + } + + @Test + fun `the latest battery state is exposed for the legend`() { + val fixture = + Fixture( + listOf( + reading(battery = BatteryState(levelPercent = 80, isCharging = false)), + reading(battery = BatteryState(levelPercent = 79, isCharging = true)), + ), + ) + + fixture.sample(2) + + assertThat(fixture.watcher.latestBattery).isEqualTo(BatteryState(levelPercent = 79, isCharging = true)) + } + + @Test + fun `the ring buffer keeps only the most recent samples`() { + val capacity = PowerUsageWatcher.MAX_USAGE_ENTRIES + val readings = List(capacity + 2) { reading(temperature = it.toLong()) } + val fixture = Fixture(readings) + + fixture.sample(readings.size) + val usage = fixture.watcher.getUsage() + + assertThat(usage.temperatureMilliCelsius).hasLength(capacity) + assertThat(usage.temperatureMilliCelsius.last()).isEqualTo((readings.size - 1).toLong()) + assertThat(usage.temperatureMilliCelsius.first()).isEqualTo(2L) + } + + @Test + fun `changing the sampling interval clears the history`() { + val fixture = Fixture(listOf(reading())) + fixture.sample(5) + + fixture.watcher.updateInterval = 5_000L + + // Samples taken at two rates in one buffer would misdate the older ones. + val usage = fixture.watcher.getUsage() + assertThat(usage.temperatureMilliCelsius.sum()).isEqualTo(0L) + assertThat(usage.powerMicroWatts.sum()).isEqualTo(0L) + } + + @Test + fun `getUsage returns a copy, not the live buffer`() { + val fixture = Fixture(listOf(reading(temperature = 30_000L), reading(temperature = 40_000L))) + + fixture.sample(1) + val first = fixture.watcher.getUsage() + val asHandedOut = first.temperatureMilliCelsius.copyOf() + fixture.sample(1) + + assertThat(first.temperatureMilliCelsius).isEqualTo(asHandedOut) + assertThat(fixture.watcher.getUsage().temperatureMilliCelsius).isNotEqualTo(asHandedOut) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index bdba956b6b..5a4a57ae9e 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1682,6 +1682,10 @@ Sampling rate Every %1$s %1$s (needs a 64-bit device) + Temperature and power + Temperature and power chart + Battery temp + Power Previous metric Next metric Save chart image From 7f34c6e5d3c69066f058f87788d83f7113310c38 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 5 Sep 2026 17:09:23 -0700 Subject: [PATCH 02/11] feat(metrics): colour the throttle bands, label power in watts, stagger annotations Four changes to the temperature and power page (ADFA-5499) and one to the annotations shared by every page (ADFA-5486). Throttle shading is now hue-coded rather than one colour at six depths: green, cyan, yellow, orange, rust, red for levels 1 to 6, at one fixed alpha. Level 0 and an unreadable level stay unshaded. Ranking seven ordinals by depth of a single colour asks the eye to compare shades that are never side by side; the bands are separated in time, so distinct hues stay tellable apart wherever on the chart they fall. The source is unchanged: PowerManager.getCurrentThermalStatus() on API 29+, with ThermalInfo behind it for API 28, which minSdk still admits. The power axis is labelled in whole watts. A build peaks in single-digit watts, so milliwatt labels spent three characters each on trailing zeros. Granularity is pinned to 1 W as well: left to choose its own spacing the axis puts gridlines a fraction of a watt apart on an idle device, and rounding those to whole watts prints the same label several times over. The legend keeps finer units, falling back to milliwatts below a watt, where "0W" would lose the only value it exists to show. Each value axis takes the colour of the line it describes -- orange for temperature on the left, blue for power on the right. With two axes carrying unrelated units, colour is what says which reads which. That last one needed a hook. setData repaints both axes in the surface's text colour on every redraw, so anything a subclass set in configure was overwritten within a frame; it now calls an open styleValueAxes, which the power page overrides. The test caught this -- the same shape as the two defects in the previous commit, and this time it was caught before the device. Annotation labels are staggered across eight rows, cycling. Gradle fires tasks in bursts, so several markers land within a few pixels of each other and their labels, all drawn on one row, overwrote each other into an unreadable smear. The row comes from a new Annotation.sequence, counted from the first annotation of the session, rather than from a position in the visible list: that list shifts as older entries age out, so a label would hop rows while merely sitting still. Nothing covered the drawing of annotations before this, only the store behind them, which is how the smear came to ship. MetricsAnnotationRenderingTest now covers it; its three stagger tests were confirmed to fail with the offset held constant, and the row-stability test to fail when the row is taken from the visible list. Verified on a Pixel 6 Pro against a newly created Compose Activity project, so the Gradle run was long and task-dense: three annotations drawn on three different rows, the right axis reading 0W through 6W, the left axis orange and the right blue, and all six throttle hues distinct under `cmd thermalservice override-status`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsChartRenderer.kt | 42 ++++- .../androidide/ui/PowerUsageChartRenderer.kt | 106 +++++++++--- .../utils/MetricsAnnotationStore.kt | 16 +- .../ui/MetricsAnnotationRenderingTest.kt | 158 ++++++++++++++++++ .../ui/PowerUsageChartRendererTest.kt | 76 +++++++-- .../utils/MetricsAnnotationStoreTest.kt | 39 +++++ 6 files changed, 398 insertions(+), 39 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index c087e102d2..0254ed9940 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -285,8 +285,6 @@ abstract class MetricsChartRenderer( chart.apply { data = LineData(*datasets) - axisRight.textColor = textColor - axisLeft.textColor = textColor legend.textColor = textColor // MPAndroidChart defaults every component's text to Color.BLACK. The y axis and legend // were given a themed colour and the x axis never was, so its labels have always been @@ -295,6 +293,7 @@ abstract class MetricsChartRenderer( xAxis.textColor = textColor data.setValueTextColor(textColor) + styleValueAxes(this, textColor) setBackgroundColor(bgColor) setGridBackgroundColor(bgColor) notifyDataSetChanged() @@ -304,6 +303,22 @@ abstract class MetricsChartRenderer( chart.invalidate() } + /** + * Colours the value axes' labels. Called from [setData], not [configure], because the styling + * here is re-applied on every redraw and would otherwise overwrite whatever a subclass had set + * up once at configuration time. + * + * The default paints both in the surface's text colour, which suits a page whose series all + * share one unit. A page with two unrelated axes overrides this. + */ + protected open fun styleValueAxes( + chart: SafeLineChart, + defaultTextColor: Int, + ) { + chart.axisLeft.textColor = defaultTextColor + chart.axisRight.textColor = defaultTextColor + } + /** * Draws a vertical marker for each recent significant event (ADFA-5486). * @@ -311,6 +326,10 @@ abstract class MetricsChartRenderer( * shifts under them. Age converts to an x position here: the newest sample sits at the buffer's * last index, and every [sampleIntervalMillis] before that is one index to the left. Anything * older than the buffer holds falls outside the axis and is not drawn. + * + * Labels are staggered across [ANNOTATION_LABEL_SLOTS] rows. Gradle fires tasks in bursts, so + * several markers land within a few pixels of each other and their labels, all drawn on one + * row, overwrite each other into an unreadable smear. */ private fun applyAnnotations(chart: SafeLineChart) { val store = annotations ?: return @@ -337,11 +356,19 @@ abstract class MetricsChartRenderer( textColor = markerColor enableDashedLine(ANNOTATION_DASH_LENGTH, ANNOTATION_DASH_LENGTH, 0f) labelPosition = LimitLine.LimitLabelPosition.RIGHT_BOTTOM + // Rows are counted up from the bottom of the plot, and the offset is in dp: + // LimitLine converts it on the way in. + yOffset = ANNOTATION_LABEL_ROW_HEIGHT_DP * slotFor(annotation.sequence) }, ) } } + /** + * The row an annotation's label sits on, cycling so that neighbours never share one. + */ + private fun slotFor(sequence: Long): Int = (sequence % ANNOTATION_LABEL_SLOTS).toInt() + /** * Redraws after the attached series have been mutated in place. */ @@ -368,5 +395,16 @@ abstract class MetricsChartRenderer( const val ANNOTATION_LINE_WIDTH = 1f const val ANNOTATION_DASH_LENGTH = 6f + + /** + * Rows the annotation labels cycle through, counted up from the bottom of the plot. + * + * Eight rows at [ANNOTATION_LABEL_ROW_HEIGHT_DP] apiece stay inside the strip's plot area + * while spreading a burst of Gradle tasks far enough apart to read. + */ + const val ANNOTATION_LABEL_SLOTS = 8 + + /** One row, in dp. The label text is 10dp, so this leaves a little air between rows. */ + const val ANNOTATION_LABEL_ROW_HEIGHT_DP = 12f } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 09fb52d14b..89e090d21d 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -29,21 +29,25 @@ import com.itsaky.androidide.R import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.PowerUsageWatcher import com.itsaky.androidide.utils.PowerUsageWatcher.PowerUsage -import com.itsaky.androidide.utils.resolveAttr import kotlin.math.abs import kotlin.math.roundToLong /** * Renders [PowerUsageWatcher] samples: battery temperature against power draw (ADFA-5499). * - * The only page with two value axes. Degrees and milliwatts differ in unit and by orders of - * magnitude, so temperature takes the left axis and power the right. Both series therefore have to - * declare which axis they belong to -- a dataset left on the default would be drawn against an axis - * whose labels do not describe it, which is a bug this codebase has already shipped once. + * The only page with two value axes. Degrees and watts differ in unit and by orders of magnitude, + * so temperature takes the left axis and power the right. Both series therefore have to declare + * which axis they belong to -- a dataset left on the default would be drawn against an axis whose + * labels do not describe it, which is a bug this codebase has already shipped once. Each axis's + * labels are drawn in its series' colour, so which axis reads which line needs no explaining. * * Thermal throttling is shown as background shading rather than as a line: the platform reports an * ordinal level, not a temperature, so plotting it against degrees would invent a scale. The level * is sampled alongside the readings, so a shaded band is simply a run of equal levels. + * + * Severity is carried by hue, green through red, at one fixed alpha. Ranking seven ordinals by + * depth of a single colour asks the eye to compare shades that are never side by side; distinct + * hues stay tellable apart wherever on the chart they fall. */ class PowerUsageChartRenderer( private val usageProvider: () -> PowerUsage, @@ -74,7 +78,7 @@ class PowerUsageChartRenderer( label = context.getString(R.string.metrics_power_draw), lineColor = POWER_COLOR, axis = YAxis.AxisDependency.RIGHT, - transform = ::microWattsToMilliWatts, + transform = ::microWattsToWatts, ), ) @@ -113,7 +117,7 @@ class PowerUsageChartRenderer( end++ } - shadeFor(chart, level)?.let { color -> + shadeFor(level)?.let { color -> // Half a sample either side, so each sample covers its own cell: a single-sample // spike would otherwise have zero width and never be drawn, and two adjacent runs // would leave a sample-wide gap between them. @@ -128,25 +132,23 @@ class PowerUsageChartRenderer( /** * The shade for a throttling level, or `null` where there is nothing to say. * - * Alpha rises with severity so the bands read as a gradient of concern rather than as separate - * categories, and stays low enough throughout that the plotted lines remain the foreground. + * Level 0 is unthrottled and level -1 is a device that reports no level at all; neither is + * shaded, because shading everything would say nothing. The alpha is the same for every level, + * so hue alone ranks them, and low enough throughout that the plotted lines stay the foreground. */ - private fun shadeFor( - chart: SafeLineChart, - level: Int, - ): Int? { - val alpha = + private fun shadeFor(level: Int): Int? { + val hue = when (level) { - THERMAL_LIGHT -> 24 - THERMAL_MODERATE -> 40 - THERMAL_SEVERE -> 64 - THERMAL_CRITICAL -> 88 - THERMAL_EMERGENCY, THERMAL_SHUTDOWN -> 112 + THERMAL_LIGHT -> SHADE_LIGHT + THERMAL_MODERATE -> SHADE_MODERATE + THERMAL_SEVERE -> SHADE_SEVERE + THERMAL_CRITICAL -> SHADE_CRITICAL + THERMAL_EMERGENCY -> SHADE_EMERGENCY + THERMAL_SHUTDOWN -> SHADE_SHUTDOWN else -> return null } - val base = chart.context.resolveAttr(R.attr.colorError) - return ColorUtils.setAlphaComponent(base, alpha) + return ColorUtils.setAlphaComponent(hue, SHADE_ALPHA) } private fun series( @@ -185,7 +187,20 @@ class PowerUsageChartRenderer( return if (axis == YAxis.AxisDependency.LEFT) { "%s - %.1fC".format(label, milliCelsiusToCelsius(value)) } else { - "%s - %.0fmW".format(label, milliWattsMagnitude(value)) + "%s - %s".format(label, formatPower(value)) + } + } + + /** + * The latest draw, for the legend. Below a watt it is given in milliwatts: an idle device would + * otherwise read "0.0W", losing the very value the legend exists to show. + */ + private fun formatPower(microWatts: Long): String { + val watts = wattsMagnitude(microWatts) + return if (watts < 1f) { + "%.0fmW".format(abs(microWatts) / MICROWATTS_PER_MILLIWATT) + } else { + "%.1fW".format(watts) } } @@ -195,6 +210,7 @@ class PowerUsageChartRenderer( // Two units, two axes: the base class disables the left one because every other page has a // single series family. chart.axisLeft.isEnabled = true + chart.axisLeft.valueFormatter = object : IAxisValueFormatter { override fun getFormattedValue( @@ -203,13 +219,33 @@ class PowerUsageChartRenderer( ): String = "%dC".format(value.roundToLong()) } + // Watts, not milliwatts: a build peaks in single digit watts, so mW labels spent three + // characters on trailing zeros. Whole watts, so the labels carry no decimal point either. chart.axisRight.valueFormatter = object : IAxisValueFormatter { override fun getFormattedValue( value: Float, axis: AxisBase?, - ): String = "%dmW".format(value.roundToLong()) + ): String = "%dW".format(value.roundToLong()) } + + // Integer labels need integer gridlines to match. Left to pick its own spacing the axis + // will place lines a fraction of a watt apart on an idle device, and rounding those to + // whole watts prints the same label several times over. + chart.axisRight.granularity = 1f + chart.axisRight.isGranularityEnabled = true + } + + /** + * Each axis's labels take the colour of the line they describe. With two axes carrying + * unrelated units, colour is what says which reads which; one shared text colour cannot. + */ + override fun styleValueAxes( + chart: SafeLineChart, + defaultTextColor: Int, + ) { + chart.axisLeft.textColor = TEMPERATURE_COLOR + chart.axisRight.textColor = POWER_COLOR } /** @@ -235,6 +271,21 @@ class PowerUsageChartRenderer( /** Half the x-axis width of one sample, which is 1 because x values are sample indices. */ const val HALF_SAMPLE = 0.5f + /** + * Throttling shades, green through red. Deliberately six distinct hues rather than one + * colour at six depths: the bands are separated in time, so shades of one colour would have + * to be compared across the width of the chart. + */ + val SHADE_LIGHT = Color.rgb(76, 175, 80) + val SHADE_MODERATE = Color.rgb(0, 188, 212) + val SHADE_SEVERE = Color.rgb(253, 216, 53) + val SHADE_CRITICAL = Color.rgb(251, 140, 0) + val SHADE_EMERGENCY = Color.rgb(183, 65, 14) + val SHADE_SHUTDOWN = Color.rgb(229, 57, 53) + + /** Visible against the plot surface without drowning the lines drawn over it. */ + const val SHADE_ALPHA = 96 + const val THERMAL_LIGHT = 1 const val THERMAL_MODERATE = 2 const val THERMAL_SEVERE = 3 @@ -254,7 +305,10 @@ private fun milliCelsiusToCelsius(milliCelsius: Long): Float = * Power is plotted as a magnitude. The battery current reverses while charging, and a line that * dips below zero would read as the device spending negative power. */ -private fun microWattsToMilliWatts(microWatts: Long): Float = - if (microWatts == PowerUsageWatcher.UNAVAILABLE) 0f else abs(microWatts) / 1000f +private fun microWattsToWatts(microWatts: Long): Float = + if (microWatts == PowerUsageWatcher.UNAVAILABLE) 0f else abs(microWatts) / MICROWATTS_PER_WATT + +private fun wattsMagnitude(microWatts: Long): Float = abs(microWatts) / MICROWATTS_PER_WATT -private fun milliWattsMagnitude(microWatts: Long): Float = abs(microWatts) / 1000f +private const val MICROWATTS_PER_WATT = 1_000_000f +private const val MICROWATTS_PER_MILLIWATT = 1_000f diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt index 195c623471..ae90870f42 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -43,6 +43,9 @@ class MetricsAnnotationStore( */ private var lastRecordedAt: Long? = null + /** Hands each annotation its [Annotation.sequence]. */ + private var nextSequence: Long = 0L + /** * An annotated moment. * @@ -52,6 +55,16 @@ class MetricsAnnotationStore( data class Annotation( val atMillis: Long, val label: String, + /** + * Position in the order recorded, counted from the first annotation of the session. + * + * The chart staggers labels across rows to stop them overwriting each other, and picks the + * row from this. Its own position in [recentAnnotations] would not do: that list shifts as + * older entries age out of it, so a label would hop between rows while merely sitting + * still. Counting from the first annotation instead pins a label to one row for life, and + * makes consecutive annotations differ, which is when a collision is likeliest. + */ + val sequence: Long, ) /** @@ -68,7 +81,7 @@ class MetricsAnnotationStore( } lastRecordedAt = now - annotations.addLast(Annotation(now, label)) + annotations.addLast(Annotation(now, label, nextSequence++)) while (annotations.size > MAX_ANNOTATIONS) { annotations.removeFirst() } @@ -88,6 +101,7 @@ class MetricsAnnotationStore( fun clear() { annotations.clear() lastRecordedAt = null + nextSequence = 0L } companion object { diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt new file mode 100644 index 0000000000..98ccf47ea7 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt @@ -0,0 +1,158 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.data.Entry +import com.github.mikephil.charting.data.LineDataSet +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.MetricsAnnotationStore +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Pins how annotation labels are placed (ADFA-5486, ADFA-5499). + * + * Nothing covered the drawing of annotations before, only the store behind them, which is how a + * burst of Gradle tasks came to render its labels stacked on one row as an unreadable smear. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsAnnotationRenderingTest { + private val context = ApplicationProvider.getApplicationContext() + + /** A minimal renderer, so the placement is tested without a particular page's data. */ + private class TestRenderer( + private val sampleCount: Int, + annotations: MetricsAnnotationStore, + now: () -> Long, + ) : MetricsChartRenderer( + sampleIntervalMillis = { SAMPLE_INTERVAL_MS }, + annotations = annotations, + nowMillis = now, + ) { + override fun rebuild() { + val chart = this.chart ?: return + val entries = List(sampleCount) { Entry(it.toFloat(), 0f) } + setData(chart, arrayOf(LineDataSet(entries, "test"))) + } + } + + private class Fixture { + var now = 0L + val store = MetricsAnnotationStore(nowMillis = { now }) + + /** Records [count] annotations, spaced far enough apart to clear the store's throttle. */ + fun recordBurst(count: Int) { + repeat(count) { index -> + store.record("task $index") + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + } + } + } + + private fun render(fixture: Fixture): Pair { + val chart = SafeLineChart(context) + val renderer = TestRenderer(SAMPLE_COUNT, fixture.store, { fixture.now }) + renderer.attach(chart) + return renderer to chart + } + + private fun rowsOf(chart: SafeLineChart): List = chart.xAxis.limitLines.map { it.yOffset } + + @Test + fun `a marker is drawn for each annotation in the window`() { + val fixture = Fixture() + fixture.recordBurst(4) + + val (_, chart) = render(fixture) + + assertThat(chart.xAxis.limitLines).hasSize(4) + } + + @Test + fun `labels are staggered across rows rather than stacked on one`() { + val fixture = Fixture() + fixture.recordBurst(4) + + val (_, chart) = render(fixture) + + // All on one row is exactly the smear this exists to prevent. + assertThat(rowsOf(chart).toSet()).hasSize(4) + } + + @Test + fun `neighbouring labels never share a row`() { + val fixture = Fixture() + fixture.recordBurst(10) + + val (_, chart) = render(fixture) + + // Gradle fires tasks in bursts, so consecutive markers are the ones likeliest to collide. + val rows = rowsOf(chart) + assertThat(rows.zipWithNext().none { (earlier, later) -> earlier == later }).isTrue() + } + + @Test + fun `the rows cycle once more annotations than rows are drawn`() { + val fixture = Fixture() + fixture.recordBurst(10) + + val (_, chart) = render(fixture) + + // Ten annotations over eight rows: the ninth starts the cycle again. + val rows = rowsOf(chart) + assertThat(rows.toSet()).hasSize(8) + assertThat(rows[8]).isEqualTo(rows[0]) + assertThat(rows[9]).isEqualTo(rows[1]) + } + + @Test + fun `a label keeps its row as older annotations scroll out of the window`() { + val fixture = Fixture() + fixture.recordBurst(3) + + val (renderer, chart) = render(fixture) + assertThat(chart.xAxis.limitLines).hasSize(3) + val newestRowBefore = rowsOf(chart).last() + + // Age the chart until the first two annotations have fallen out of the buffer's span and + // only the third is still inside it. Nothing new is recorded. + fixture.now = SURVIVOR_ONLY_AT_MS + renderer.rebuild() + + // Rows come from the order recorded, not from a position in the visible list: taking the + // row from the latter would move this label from the third row to the first while it has + // merely sat still. + assertThat(chart.xAxis.limitLines).hasSize(1) + assertThat(rowsOf(chart).single()).isEqualTo(newestRowBefore) + } + + private companion object { + const val SAMPLE_INTERVAL_MS = 1_000L + const val SAMPLE_COUNT = 60 + + /** + * A time by which the burst's first two annotations are older than the buffer's span and + * its third is not: they were recorded at 0ms, 5000ms and 10000ms, and the buffer holds + * SAMPLE_COUNT * SAMPLE_INTERVAL_MS = 60000ms. + */ + const val SURVIVOR_ONLY_AT_MS = 66_000L + } +} diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index d34ff1081a..cf7bf30652 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -82,7 +82,30 @@ class PowerUsageChartRendererTest { } @Test - fun `temperature is plotted in degrees and power in milliwatts`() { + fun `the power axis is labelled in whole watts`() { + val (_, chart) = rendererFor(usage(temperature = longArrayOf(30_000L), power = longArrayOf(8_400_000L))) + val axis = chart.axisRight + + assertThat(axis.valueFormatter.getFormattedValue(8.4f, axis)).isEqualTo("8W") + assertThat(axis.valueFormatter.getFormattedValue(0f, axis)).isEqualTo("0W") + // Without this the axis puts gridlines a fraction of a watt apart on an idle device, and + // rounding them to whole watts prints the same label several times over. + assertThat(axis.isGranularityEnabled).isTrue() + assertThat(axis.granularity).isEqualTo(1f) + } + + @Test + fun `each axis takes the colour of the line it describes`() { + val (_, chart) = rendererFor(usage(temperature = longArrayOf(30_000L), power = longArrayOf(1_000_000L))) + + // Two axes with unrelated units; colour is what pairs each with its series. + assertThat(chart.axisLeft.textColor).isEqualTo(dataset(chart, 0).color) + assertThat(chart.axisRight.textColor).isEqualTo(dataset(chart, 1).color) + assertThat(chart.axisLeft.textColor).isNotEqualTo(chart.axisRight.textColor) + } + + @Test + fun `temperature is plotted in degrees and power in watts`() { val (_, chart) = rendererFor( usage( @@ -92,7 +115,7 @@ class PowerUsageChartRendererTest { ) assertThat(dataset(chart, 0).entries.last().y).isWithin(0.01f).of(29.7f) - assertThat(dataset(chart, 1).entries.last().y).isWithin(0.01f).of(6358.064f) + assertThat(dataset(chart, 1).entries.last().y).isWithin(0.001f).of(6.358064f) } @Test @@ -108,7 +131,7 @@ class PowerUsageChartRendererTest { val ys = dataset(chart, 1).entries.map { it.y } - assertThat(ys).containsExactly(2000f, 3000f).inOrder() + assertThat(ys).containsExactly(2f, 3f).inOrder() assertThat(ys.none { it < 0f }).isTrue() } @@ -189,19 +212,43 @@ class PowerUsageChartRendererTest { } @Test - fun `adjacent levels shade separately, and deeper for the worse one`() { + fun `each throttling level gets its own hue, green through red`() { val (_, chart) = rendererFor( usage( - temperature = LongArray(4) { 30_000L }, - thermal = longArrayOf(1L, 1L, 4L, 4L), + temperature = LongArray(6) { 30_000L }, + thermal = longArrayOf(1L, 2L, 3L, 4L, 5L, 6L), + ), + ) + + assertThat(chart.backgroundSpans).hasSize(6) + assertThat(chart.backgroundSpans.map { it.color or OPAQUE }).isEqualTo(EXPECTED_HUES) + } + + @Test + fun `no two levels share a colour, and the alpha does not vary`() { + val (_, chart) = + rendererFor( + usage( + temperature = LongArray(6) { 30_000L }, + thermal = longArrayOf(1L, 2L, 3L, 4L, 5L, 6L), ), ) - assertThat(chart.backgroundSpans).hasSize(2) - val (light, critical) = chart.backgroundSpans - // The bands read as a gradient of concern rather than as unrelated categories. - assertThat(critical.color ushr 24).isGreaterThan(light.color ushr 24) + // Hue alone ranks the levels, so a repeat would make two of them indistinguishable... + assertThat(chart.backgroundSpans.map { it.color }.toSet()).hasSize(6) + // ...and a varying alpha would add a second, weaker ranking that disagrees with it. + assertThat(chart.backgroundSpans.map { it.color ushr 24 }.toSet()).hasSize(1) + } + + @Test + fun `the legend reports power in watts, and in milliwatts below a watt`() { + val (_, loaded) = rendererFor(usage(temperature = longArrayOf(30_000L), power = longArrayOf(6_358_064L))) + assertThat(dataset(loaded, 1).label).endsWith("6.4W") + + // An idle device reads 0.0W in watts, losing the value the legend exists to show. + val (_, idle) = rendererFor(usage(temperature = longArrayOf(30_000L), power = longArrayOf(6_000L))) + assertThat(dataset(idle, 1).label).endsWith("6mW") } @Test @@ -252,4 +299,13 @@ class PowerUsageChartRendererTest { assertThat(renderer.batteryReadout()).isNull() } + + private companion object { + const val OPAQUE = 0xFF000000.toInt() + + /** The palette ADFA-5499 specifies: green, cyan, yellow, orange, rust, red. */ + val EXPECTED_HUES = + listOf(0xFF4CAF50, 0xFF00BCD4, 0xFFFDD835, 0xFFFB8C00, 0xFFB7410E, 0xFFE53935) + .map { it.toInt() } + } } diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt index 03dc8b4b46..1e672ab2dd 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt @@ -104,4 +104,43 @@ class MetricsAnnotationStoreTest { // Without resetting the throttle, the next event would be swallowed for five seconds. assertThat(store.record("second")).isTrue() } + + @Test + fun `sequence numbers count from the first annotation of the session`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + repeat(3) { + store.record("task") + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + } + + // The chart picks a label's row from this, so it has to be stable and gap-free. + assertThat(store.recentAnnotations(60_000L).map { it.sequence }).containsExactly(0L, 1L, 2L).inOrder() + } + + @Test + fun `a throttled record consumes no sequence number`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + store.record("kept") + // Inside the throttle window, so this one is dropped rather than stored. + store.record("dropped") + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + store.record("kept too") + + // A gap here would leave a row unused and push neighbours together. + assertThat(store.recentAnnotations(60_000L).map { it.sequence }).containsExactly(0L, 1L).inOrder() + } + + @Test + fun `clear restarts the numbering`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + store.record("before") + + store.clear() + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + store.record("after") + + assertThat(store.recentAnnotations(60_000L).map { it.sequence }).containsExactly(0L) + } } From 59d6851afe3e71802433cd452593273341252fab Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 5 Sep 2026 17:23:05 -0700 Subject: [PATCH 03/11] fix(metrics): put the sampling-rate tap on the edge the x axis is drawn on (ADFA-5486) The chooser was reachable only from a blank strip above the plot, at the opposite end of the chart from the axis labels the gesture is named for. The hit test compared against contentTop while the axis is positioned BOTTOM, so tapping the labels did nothing and the rate could not be changed by anyone who did not already know where the hidden band was. The strip under the plot had been left alone for the carousel swipe. Paging is by the arrows now, so it is free, and the tap moves there. The two have to agree, and nothing said so: a comment on each site now points at the other. MetricsChartAxisTapTest covers all three bands. Confirmed to fail against the old hit test in both directions -- the tap below the plot not registering, and the tap above it still registering -- so it pins the edge rather than merely the existence of the gesture. A guard test asserts the chart was laid out first, without which every coordinate sits on the same edge and the others would pass vacuously. Verified on a Pixel 6 Pro: tapping the "-54s" labels opens the chooser, tapping the band above the plot does nothing, and picking "Every 5s" relabels the axis to -270s and clears the history as intended. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsChartRenderer.kt | 16 ++- .../androidide/ui/MetricsChartAxisTapTest.kt | 122 ++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 0254ed9940..7f594941ce 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -162,8 +162,9 @@ abstract class MetricsChartRenderer( setBackgroundColor(context.resolveAttr(R.attr.colorSurfaceDim)) setDrawGridBackground(true) - // Below the plot, so the strip under it can be reserved for the carousel swipe and the - // plot itself can pan when zoomed (ADFA-5486). + // Below the plot, which is also where a tap opens the sampling-rate chooser + // (ADFA-5486). The two have to agree: they disagreed once, and the gesture was + // unreachable at the labels it is named for. xAxis.position = XAxis.XAxisPosition.BOTTOM // The right axis carries the labels; the left is unused. @@ -207,15 +208,20 @@ abstract class MetricsChartRenderer( * Turns a tap in the x-axis band into [onXAxisTap]. * * The axis is drawn by the chart rather than being a view of its own, so there is nothing to - * attach a click listener to. `contentTop` is the top of the plotting area, and the axis labels - * sit above it, so a tap higher than that landed on the axis. + * attach a click listener to. `contentBottom` is the bottom of the plotting area and the axis + * is drawn below it (see [configure]), so a tap lower than that landed on the axis. + * + * This used to test `contentTop`, which put the only way to reach the sampling-rate chooser in + * an empty band at the *opposite* end of the chart from the labels it is named for. The strip + * under the plot had been left alone for the carousel swipe; paging is by the arrows now, so it + * is free. */ private inner class XAxisTapListener( private val chart: SafeLineChart, ) : OnChartGestureListener { override fun onChartSingleTapped(me: MotionEvent?) { val y = me?.y ?: return - if (y <= chart.viewPortHandler.contentTop()) { + if (y >= chart.viewPortHandler.contentBottom()) { onXAxisTap?.invoke() } } diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt new file mode 100644 index 0000000000..f965eabda6 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt @@ -0,0 +1,122 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.view.MotionEvent +import android.view.View +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.PowerUsageWatcher +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Pins where the sampling-rate chooser is reached from (ADFA-5486). + * + * The x axis is drawn by the chart rather than being a view of its own, so the tap is recognised by + * comparing coordinates against the plot area. That test and the axis's position have to agree: + * they disagreed once -- the axis at the bottom, the tap band at the top -- which left the only way + * to change the sampling rate in an empty strip at the far end of the chart from the labels the + * gesture is named for. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsChartAxisTapTest { + private val context = ApplicationProvider.getApplicationContext() + + private var taps = 0 + + private fun laidOutChart(): SafeLineChart { + val chart = SafeLineChart(context) + val renderer = + PowerUsageChartRenderer( + usageProvider = { + PowerUsageWatcher.PowerUsage( + LongArray(SAMPLES) { 30_000L }, + LongArray(SAMPLES) { 1_000_000L }, + LongArray(SAMPLES), + ) + }, + batteryProvider = { PowerUsageWatcher.BatteryState.UNKNOWN }, + ) + renderer.attach(chart) + renderer.onXAxisTap = { taps++ } + + // Without a layout pass the plot area has no extent, so every coordinate is on its edge. + chart.measure( + View.MeasureSpec.makeMeasureSpec(WIDTH, View.MeasureSpec.EXACTLY), + View.MeasureSpec.makeMeasureSpec(HEIGHT, View.MeasureSpec.EXACTLY), + ) + chart.layout(0, 0, WIDTH, HEIGHT) + return chart + } + + private fun tapAt( + chart: SafeLineChart, + y: Float, + ) { + val event = MotionEvent.obtain(0L, 0L, MotionEvent.ACTION_UP, 10f, y, 0) + chart.onChartGestureListener.onChartSingleTapped(event) + event.recycle() + } + + @Test + fun `the plot area has room for a tap to fall inside or outside it`() { + val chart = laidOutChart() + + // Guards the other tests: on an unlaid-out chart they would all tap the same edge. + assertThat(chart.viewPortHandler.contentBottom()).isGreaterThan(chart.viewPortHandler.contentTop()) + assertThat(chart.viewPortHandler.contentBottom()).isLessThan(HEIGHT.toFloat()) + } + + @Test + fun `a tap below the plot, where the axis is drawn, opens the chooser`() { + val chart = laidOutChart() + + tapAt(chart, chart.viewPortHandler.contentBottom() + 1f) + + assertThat(taps).isEqualTo(1) + } + + @Test + fun `a tap above the plot does not open the chooser`() { + val chart = laidOutChart() + + // Nothing is drawn up there. Answering taps here is what made the gesture unreachable. + tapAt(chart, chart.viewPortHandler.contentTop() - 1f) + + assertThat(taps).isEqualTo(0) + } + + @Test + fun `a tap inside the plot does not open the chooser`() { + val chart = laidOutChart() + + val handler = chart.viewPortHandler + tapAt(chart, (handler.contentTop() + handler.contentBottom()) / 2f) + + assertThat(taps).isEqualTo(0) + } + + private companion object { + const val WIDTH = 720 + const val HEIGHT = 400 + const val SAMPLES = 60 + } +} From 5028834f10ce6a41ba22e9a24774fae6792c8b2c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 5 Sep 2026 21:58:56 -0700 Subject: [PATCH 04/11] docs(metrics): correct the sign contract on the battery current (ADFA-5499) `BATTERY_PROPERTY_CURRENT_NOW` is positive for current entering the battery -- charging -- and negative for current leaving it. The KDoc on `PowerReading.powerMicroWatts` claimed the opposite, and a test name repeated the claim. No behaviour changes, and deliberately so. CodeRabbit's suggestion was to negate the reading to match the doc; that would make the stored value disagree with the platform it came from, which is the wrong half to move. Nothing consumes the sign: the renderer plots the magnitude, both because a line dipping below zero reads as negative power spent and because not every OEM signs this property the way the documentation says. That second reason is now written down where it belongs, next to the reading. Confirmed against the device the feature was built on: current_now reads positive while charging. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../java/com/itsaky/androidide/utils/DevicePowerSource.kt | 8 ++++++-- .../java/com/itsaky/androidide/utils/PowerUsageWatcher.kt | 5 +++-- .../itsaky/androidide/ui/PowerUsageChartRendererTest.kt | 5 +++-- .../com/itsaky/androidide/utils/PowerUsageWatcherTest.kt | 5 +++-- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt index 882d7de1b3..ceacbdff75 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt @@ -75,8 +75,12 @@ class DevicePowerSource( /** * Instantaneous draw, from current and voltage. * - * Microamps times millivolts is nanowatts, so the product is scaled down to microwatts. The sign - * follows the battery current: negative while charging, because current is then flowing in. + * Microamps times millivolts is nanowatts, so the product is scaled down to microwatts. + * + * The sign is the platform's, passed through unchanged: `BATTERY_PROPERTY_CURRENT_NOW` is + * positive for current entering the battery -- charging -- and negative for current leaving it. + * Not every OEM honours that, which is one reason the chart plots the magnitude rather than the + * signed value; the other is that a line dipping below zero reads as negative power spent. */ private fun readPower(battery: Intent?): Long { val microAmps = batteryManager?.getIntProperty(BatteryManager.BATTERY_PROPERTY_CURRENT_NOW) diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 7c1e8b987a..29d162094b 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -193,8 +193,9 @@ class PowerUsageWatcher * One sample's worth of readings. * * @property temperatureMilliCelsius Battery temperature, or [UNAVAILABLE]. - * @property powerMicroWatts Instantaneous draw, or [UNAVAILABLE]. Negative while charging, - * because the battery current reverses. + * @property powerMicroWatts Instantaneous draw, or [UNAVAILABLE]. Signed as the platform + * signs the battery current: positive while charging, negative while discharging. Recorded + * as read; the renderer decides how to plot it. * @property thermalStatus The platform throttling level, or [THERMAL_UNKNOWN]. * @property battery Level and charging state, for the legend. */ diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index cf7bf30652..678b94bcbb 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -119,12 +119,13 @@ class PowerUsageChartRendererTest { } @Test - fun `power is plotted as a magnitude, so charging does not dip below zero`() { + fun `power is plotted as a magnitude, whichever way the current is signed`() { val (_, chart) = rendererFor( usage( temperature = longArrayOf(30_000L, 30_000L), - // The battery current reverses while charging. + // The platform signs the battery current by direction, and not every OEM signs it + // the same way round, so both signs have to plot as spent power. power = longArrayOf(2_000_000L, -3_000_000L), ), ) diff --git a/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt b/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt index f7cf75f39b..6032bcbd8e 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/PowerUsageWatcherTest.kt @@ -117,12 +117,13 @@ class PowerUsageWatcherTest { } @Test - fun `negative power is kept as recorded, because charging reverses the current`() { + fun `the sign of the current is recorded, not interpreted`() { val fixture = Fixture(listOf(reading(power = -3_000_000L))) fixture.sample(1) - // The watcher records the sign; deciding how to plot it is the renderer's job. + // The watcher passes the platform's sign through. Deciding what it means -- and that the + // chart plots the magnitude either way -- is the renderer's job. assertThat( fixture.watcher .getUsage() From 9f15dd856a9cbba280c67040594f40eeb9d9b36f Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 06:54:16 -0700 Subject: [PATCH 05/11] fix(metrics): range the power axes, and give the watcher its siblings' guards (ADFA-5499) Two axis defects with one cause, plus two guards this watcher was simply missing. Neither power axis was bounded, so MPAndroidChart ranged both over every entry in the data -- which includes the buffer's unsampled prefix, ten thousand slots that plot as zero. The 29-33C band the page exists to show was therefore pressed into the top tenth of the plot with a negative gridline beneath it, and it stayed that way for the hours the buffer takes to fill. Both axes now range over the samples on screen, skipping the unsampled prefix and anything the device does not report, with a plausible fallback span until the first readable temperature arrives. The right axis is pinned to zero. Unpinned it picked up the chart's 10% bottom padding and printed a negative watt label -- under a series deliberately plotted as a magnitude precisely so it could never read as negative power spent. The axis was offering exactly the reading the transform exists to prevent. PowerUsageWatcher was missing both guards its siblings carry. Without the interval clamp a non-positive value reaches delay(), which does not suspend for one, so the loop spins -- and this watcher does a registerReceiver binder call per iteration, so it spins more expensively than the other two. Without the terminal closed flag, a start after close() flips isWatching to true and launches into a cancelled scope: power sampling is then dead, isWatching lies about it, and the editor's `if (!isWatching) startWatching()` never retries while memory and network keep working. Both axis fixes were confirmed to fail without them. Co-Authored-By: Claude Opus 5 --- .../androidide/ui/PowerUsageChartRenderer.kt | 63 +++++++++++++++++++ .../androidide/utils/PowerUsageWatcher.kt | 20 +++++- .../ui/PowerUsageChartRendererTest.kt | 55 ++++++++++++++++ 3 files changed, 135 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 89e090d21d..247a53392e 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -30,6 +30,10 @@ import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.PowerUsageWatcher import com.itsaky.androidide.utils.PowerUsageWatcher.PowerUsage import kotlin.math.abs +import kotlin.math.ceil +import kotlin.math.floor +import kotlin.math.max +import kotlin.math.min import kotlin.math.roundToLong /** @@ -83,6 +87,7 @@ class PowerUsageChartRenderer( ) setData(chart, datasets) + applyAxisRanges(chart, usage) applyThermalShading(chart, usage) } @@ -97,6 +102,51 @@ class PowerUsageChartRenderer( rebuild() } + /** + * Ranges both axes over the samples on screen. + * + * Two problems, one cause. Left to itself MPAndroidChart ranges over every entry, which + * includes the buffer's unsampled prefix -- ten thousand slots that plot as zero -- so the + * 29-33C band this page exists to show was pressed into the top tenth of the plot with a + * negative gridline beneath it. And the right axis, unpinned, picked up MPAndroidChart's 10% + * bottom padding: a negative watt label under a series deliberately plotted as a magnitude + * precisely so it could never read as negative power spent. + */ + private fun applyAxisRanges( + chart: SafeLineChart, + usage: PowerUsage, + ) { + val visible = visibleSampleRange(chart, usage.temperatureMilliCelsius.size) + + var hottest = Float.NEGATIVE_INFINITY + var coldest = Float.POSITIVE_INFINITY + var peakWatts = 0f + for (index in visible) { + val milliCelsius = usage.temperatureMilliCelsius[index] + // Skip the unsampled prefix and anything the device does not report: both plot at + // zero, and letting zero into the range is what flattened the real readings. + if (milliCelsius != PowerUsageWatcher.UNAVAILABLE && milliCelsius != 0L) { + val celsius = milliCelsiusToCelsius(milliCelsius) + hottest = max(hottest, celsius) + coldest = min(coldest, celsius) + } + peakWatts = max(peakWatts, microWattsToWatts(usage.powerMicroWatts[index])) + } + + // Power always starts at zero: it is a magnitude, so there is nothing below it. + chart.axisRight.axisMinimum = 0f + chart.axisRight.axisMaximum = max(peakWatts * AXIS_HEADROOM, MIN_AXIS_WATTS) + + if (hottest.isFinite() && coldest.isFinite()) { + chart.axisLeft.axisMinimum = floor(coldest) - TEMPERATURE_MARGIN_CELSIUS + chart.axisLeft.axisMaximum = ceil(hottest) + TEMPERATURE_MARGIN_CELSIUS + } else { + // Nothing readable yet; a plausible room-to-warm span beats a range built from zeros. + chart.axisLeft.axisMinimum = DEFAULT_MIN_CELSIUS + chart.axisLeft.axisMaximum = DEFAULT_MAX_CELSIUS + } + } + /** * Paints a band behind the chart for each stretch of throttling, deepening with the level. * @@ -268,6 +318,19 @@ class PowerUsageChartRenderer( val TEMPERATURE_COLOR = Color.rgb(255, 138, 101) val POWER_COLOR = Color.rgb(129, 212, 250) + /** Keeps the tallest line off the top frame of the plot. */ + const val AXIS_HEADROOM = 1.1f + + /** Floor for the power axis, so an idle device still has a readable scale. */ + const val MIN_AXIS_WATTS = 2f + + /** Air above and below the temperature range, so the line is not drawn on the frame. */ + const val TEMPERATURE_MARGIN_CELSIUS = 1f + + /** Shown until the first readable temperature arrives. */ + const val DEFAULT_MIN_CELSIUS = 20f + const val DEFAULT_MAX_CELSIUS = 40f + /** Half the x-axis width of one sample, which is 1 because x values are sample indices. */ const val HALF_SAMPLE = 0.5f diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 29d162094b..3551001fbd 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -62,6 +62,13 @@ class PowerUsageWatcher private val coroutineScope = CoroutineScope(SupervisorJob() + coroutineDispatcher) private val watching = AtomicBoolean(false) + /** + * Set by [close] and never cleared. Without it a start after a terminal teardown would flip + * [isWatching] to true and launch into a cancelled scope, leaving the watcher reporting + * that it is sampling when no loop exists -- and nothing ever retries. + */ + private val closed = AtomicBoolean(false) + /** The running sampling loop, so [stopWatching] can actually stop it. */ private var samplingJob: Job? = null @@ -83,12 +90,13 @@ class PowerUsageWatcher * Milliseconds between samples. Changing it clears the history, for the reason given on * [MemoryUsageWatcher.updateInterval]. */ - var updateInterval: Long = updateInterval + var updateInterval: Long = MetricsSamplingRates.coerceToSafeRange(updateInterval) set(value) { - if (field == value) { + val safe = MetricsSamplingRates.coerceToSafeRange(value) + if (field == safe) { return } - field = value + field = safe clearHistory() } @@ -121,6 +129,11 @@ class PowerUsageWatcher } fun startWatching() { + if (closed.get()) { + log.warn("Power usage watcher is closed and cannot be restarted") + return + } + if (!watching.compareAndSet(false, true)) { log.warn("Power usage is already being watched") return @@ -158,6 +171,7 @@ class PowerUsageWatcher /** Stops sampling and releases the sampling thread. The watcher cannot be started again. */ fun close() { + closed.set(true) stopWatching() listener = null coroutineScope.cancelIfActive("Watcher closed") diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index 678b94bcbb..30209b5f1a 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -18,6 +18,9 @@ package com.itsaky.androidide.ui import android.content.Context +import android.graphics.Bitmap +import android.graphics.Canvas +import android.view.View import androidx.test.core.app.ApplicationProvider import com.github.mikephil.charting.components.YAxis import com.github.mikephil.charting.data.LineDataSet @@ -301,7 +304,59 @@ class PowerUsageChartRendererTest { assertThat(renderer.batteryReadout()).isNull() } + private fun laidOut(chart: SafeLineChart) { + chart.measure( + View.MeasureSpec.makeMeasureSpec(WIDTH, View.MeasureSpec.EXACTLY), + View.MeasureSpec.makeMeasureSpec(HEIGHT, View.MeasureSpec.EXACTLY), + ) + chart.layout(0, 0, WIDTH, HEIGHT) + // The scroll to the newest samples is a job that only runs during a draw pass. + chart.draw(Canvas(Bitmap.createBitmap(WIDTH, HEIGHT, Bitmap.Config.ARGB_8888))) + } + + @Test + fun `the power axis starts at zero, never below it`() { + val (_, chart) = + rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L }, power = LongArray(SAMPLES) { 7_000_000L })) + laidOut(chart) + + // Unpinned, the chart's own 10% bottom padding prints a negative watt label under a series + // plotted as a magnitude precisely so it could never read as negative power. + assertThat(chart.axisRight.axisMinimum).isEqualTo(0f) + } + + @Test + fun `the temperature axis ignores the buffer's unsampled zeros`() { + // A real reading only in the newest slots; the rest of the buffer has never been written. + val temperature = LongArray(SAMPLES) + for (index in SAMPLES - 10 until SAMPLES) { + temperature[index] = 30_000L + } + val (_, chart) = rendererFor(usage(temperature = temperature)) + laidOut(chart) + + // Ranged over the zeros the 30C band is squeezed into the top tenth of the plot, with a + // negative gridline below it. + assertThat(chart.axisLeft.axisMinimum).isGreaterThan(20f) + assertThat(chart.axisLeft.axisMaximum).isLessThan(40f) + } + + @Test + fun `an unreadable temperature falls back to a plausible span`() { + val (_, chart) = + rendererFor(usage(temperature = LongArray(SAMPLES) { PowerUsageWatcher.UNAVAILABLE })) + laidOut(chart) + + // Nothing readable, so a sensible range beats one computed from placeholder zeros. + assertThat(chart.axisLeft.axisMinimum).isLessThan(chart.axisLeft.axisMaximum) + assertThat(chart.axisLeft.axisMaximum).isAtMost(40f) + } + private companion object { + const val WIDTH = 720 + const val HEIGHT = 400 + const val SAMPLES = 200 + const val OPAQUE = 0xFF000000.toInt() /** The palette ADFA-5499 specifies: green, cyan, yellow, orange, rust, red. */ From 14aae3f20855498a8db081918160ac4cf5012c07 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 09:27:00 -0700 Subject: [PATCH 06/11] ADFA-5499: address review findings on the power page One axis rules the plot. Both axes were drawing grid lines at their own pitch, so the plot carried two interleaved sets of horizontal rules -- nine of them, including a pair eight pixels apart. The left axis is enabled for its labels alone, so it no longer rules; that is now a base-class invariant rather than a per-page reminder. With the tight post-fix range the temperature axis also needed whole-degree granularity, or it printed "29C, 30C, 30C, 31C". onUsageChanged mutates the series in place. It used to discard the sample it was handed and call rebuild, which asked the watcher for another copy of all three buffers and allocated two datasets and twenty thousand entries -- every tick, on the UI thread. The KDoc justified that with "two short series"; they are MAX_USAGE_ENTRIES long. rebuild now takes the sample, so the fallback path cannot disagree with the fast one. DevicePowerSource: read EXTRA_PLUGGED rather than EXTRA_STATUS, since a full battery on the charger reports BATTERY_STATUS_FULL and read as discharging; map the pre-API-29 thermal fallback to THERMAL_STATUS_LIGHT rather than SEVERE, so a device that cannot report shading is not painted as if it were throttling hard; and drop a power reading outside a plausible envelope, because OEMs diverge on both the sign and the unit of BATTERY_PROPERTY_CURRENT_NOW and a microamp reading plots as kilowatts. SafeLineChart transforms span endpoints through a reused buffer instead of two pooled MPPointD instances it never recycled, in a method that runs for every span on every frame of every pan and zoom. Tests: the draw order of the spans against the grid background, asserted against pixels under Robolectric's native graphics -- the geometry tests could not see the bug, because backgroundSpans was correct all along and the shading was simply painted and then covered. Plus the gridline and granularity invariants, and both onUsageChanged paths. Also folds two identical private ShiftedLongArray snapshot extensions into one shared internal one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsChartRenderer.kt | 5 + .../androidide/ui/PowerUsageChartRenderer.kt | 79 ++++++- .../com/itsaky/androidide/ui/SafeLineChart.kt | 15 +- .../androidide/utils/DevicePowerSource.kt | 36 +++- .../androidide/utils/NetworkUsageWatcher.kt | 7 +- .../androidide/utils/PowerUsageWatcher.kt | 7 +- .../androidide/utils/ShiftedLongArray.kt | 204 +++++++++--------- .../ui/PowerUsageChartRendererTest.kt | 47 ++++ .../itsaky/androidide/ui/SafeLineChartTest.kt | 98 +++++++++ 9 files changed, 368 insertions(+), 130 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/SafeLineChartTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 574e220ae8..02914c74db 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -172,6 +172,11 @@ abstract class MetricsChartRenderer( // The right axis carries the labels; the left is unused. axisLeft.isEnabled = false + // The right axis rules the plot. Harmless while the left one is disabled, and it means + // a page that enables the left for a second unit gets its labels without a second set + // of grid lines at unrelated heights -- MPAndroidChart rules the plot once per enabled + // axis, and AxisBase defaults to drawing them. + axisLeft.setDrawGridLines(false) onChartGestureListener = XAxisTapListener(this) diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 247a53392e..121fdb5947 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -63,9 +63,17 @@ class PowerUsageChartRenderer( annotations = annotations, ) { @UiThread - override fun rebuild() { + override fun rebuild() = rebuild(usageProvider()) + + /** + * Replaces both series from [usage]. + * + * Takes the sample rather than fetching one so [onUsageChanged] can fall back to it without + * asking the watcher for a second, later copy of the buffers it was just handed. + */ + @UiThread + private fun rebuild(usage: PowerUsage) { val chart = this.chart ?: return - val usage = usageProvider() val context = chart.context val datasets = @@ -92,14 +100,62 @@ class PowerUsageChartRenderer( } /** - * Redraws from a fresh sample. Rebuilds rather than mutating in place: this chart samples - * relatively slowly and has two short series, so the saving is not worth a second code path - * that can disagree with the first. + * Redraws from the sample just taken, mutating the existing entries in place. + * + * It used to discard [usage] and call [rebuild], which asked the watcher for another copy of + * all three series and allocated two datasets and twenty thousand entries -- every tick, on + * the UI thread. The KDoc justified that with "two short series"; they are MAX_USAGE_ENTRIES + * long. Falls back to a full rebuild only when the chart's shape no longer matches. */ @UiThread fun onUsageChanged(usage: PowerUsage) { - chart ?: return - rebuild() + val chart = this.chart ?: return + val data = chart.data + val temperature = data?.getDataSetByIndex(TEMPERATURE_INDEX) as LineDataSet? + val power = data?.getDataSetByIndex(POWER_INDEX) as LineDataSet? + + if (temperature == null || power == null || + temperature.entryCount != usage.temperatureMilliCelsius.size || + power.entryCount != usage.powerMicroWatts.size + ) { + rebuild(usage) + return + } + + val context = chart.context + update( + dataset = temperature, + values = usage.temperatureMilliCelsius, + label = context.getString(R.string.metrics_power_temperature), + axis = YAxis.AxisDependency.LEFT, + transform = ::milliCelsiusToCelsius, + ) + update( + dataset = power, + values = usage.powerMicroWatts, + label = context.getString(R.string.metrics_power_draw), + axis = YAxis.AxisDependency.RIGHT, + transform = ::microWattsToWatts, + ) + + applyAxisRanges(chart, usage) + applyThermalShading(chart, usage) + redraw(chart) + } + + /** Rewrites one series' values in place and refreshes its legend entry. */ + private fun update( + dataset: LineDataSet, + values: LongArray, + label: String, + axis: YAxis.AxisDependency, + transform: (Long) -> Float, + ) { + for (index in values.indices) { + dataset.entries[index].y = transform(values[index]) + } + dataset.label = labelFor(label, values.lastOrNull(), axis) + dataset.notifyDataSetChanged() } /** @@ -261,6 +317,12 @@ class PowerUsageChartRenderer( // single series family. chart.axisLeft.isEnabled = true + // Integer labels need integer grid lines, exactly as the watt axis below does. Now that + // the range is tight -- 29 to 33 rather than 0 to 36 -- the axis would otherwise place + // lines half a degree apart and "%dC" would print 29C, 30C, 30C, 31C, 31C. + chart.axisLeft.granularity = 1f + chart.axisLeft.isGranularityEnabled = true + chart.axisLeft.valueFormatter = object : IAxisValueFormatter { override fun getFormattedValue( @@ -349,6 +411,9 @@ class PowerUsageChartRenderer( /** Visible against the plot surface without drowning the lines drawn over it. */ const val SHADE_ALPHA = 96 + const val TEMPERATURE_INDEX = 0 + const val POWER_INDEX = 1 + const val THERMAL_LIGHT = 1 const val THERMAL_MODERATE = 2 const val THERMAL_SEVERE = 3 diff --git a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt index 3f93c67ba3..8b96943750 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -82,6 +82,9 @@ class SafeLineChart : LineChart { private val spanPaint = Paint(Paint.ANTI_ALIAS_FLAG) + /** Reused by [drawBackgroundSpans]: two (x, y) pairs, transformed in place. */ + private val spanPoints = FloatArray(4) + /** * Draws the spans immediately after the grid background, which is an opaque fill of the plot: a * span painted before [onDraw] delegates upwards is covered by it and never reaches the screen. @@ -101,8 +104,16 @@ class SafeLineChart : LineChart { val transformer = getTransformer(YAxis.AxisDependency.LEFT) ?: return backgroundSpans.forEach { span -> - val left = transformer.getPixelForValues(span.startX, 0f).x.toFloat() - val right = transformer.getPixelForValues(span.endX, 0f).x.toFloat() + // A reused buffer through pointValuesToPixel, not two getPixelForValues calls: those + // hand back pooled MPPointD instances that have to be recycled, and this runs inside + // onDraw for every span on every frame of every pan and zoom. + spanPoints[0] = span.startX + spanPoints[1] = 0f + spanPoints[2] = span.endX + spanPoints[3] = 0f + transformer.pointValuesToPixel(spanPoints) + val left = spanPoints[0] + val right = spanPoints[2] // A span scrolled out of view still maps to a pixel, so clip to the plot. val clippedLeft = left.coerceAtLeast(content.left) val clippedRight = right.coerceAtMost(content.right) diff --git a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt index ceacbdff75..c05b5d8b33 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt @@ -28,6 +28,7 @@ import com.itsaky.androidide.services.builder.ThermalInfo import com.itsaky.androidide.services.builder.ThermalState import com.itsaky.androidide.utils.PowerUsageWatcher.BatteryState import com.itsaky.androidide.utils.PowerUsageWatcher.PowerReading +import kotlin.math.abs /** * Reads temperature and power from the battery, which is all a normally-installed app can see @@ -92,7 +93,18 @@ class DevicePowerSource( return PowerUsageWatcher.UNAVAILABLE } - return microAmps.toLong() * milliVolts.toLong() / NANOWATTS_PER_MICROWATT + val microWatts = microAmps.toLong() * milliVolts.toLong() / NANOWATTS_PER_MICROWATT + + // The sign of CURRENT_NOW is documented and not always honoured; the unit is the same + // story. Several OEM kernels report milliamps, which makes a five-watt build read as five + // milliwatts -- indistinguishable from an idle device, with no error path at all. Outside + // a plausible envelope, report the reading as unavailable rather than as a believable lie. + val magnitude = abs(microWatts) + return if (magnitude == 0L || magnitude in MIN_PLAUSIBLE_MICROWATTS..MAX_PLAUSIBLE_MICROWATTS) { + microWatts + } else { + PowerUsageWatcher.UNAVAILABLE + } } /** @@ -110,8 +122,14 @@ class DevicePowerSource( } return when (ThermalInfo.getThermalState(context)) { - ThermalState.Throttled -> PowerManager.THERMAL_STATUS_SEVERE + // LIGHT, not SEVERE. The fallback knows only throttled or not, and its own + // PowerManager mapping counts LIGHT and MODERATE as not throttled -- so one mild trip + // point was painted with the middle hue of a six-level severity scale. Claim the least + // the reading could mean. + ThermalState.Throttled -> PowerManager.THERMAL_STATUS_LIGHT + ThermalState.NotThrottled -> PowerManager.THERMAL_STATUS_NONE + else -> PowerUsageWatcher.THERMAL_UNKNOWN } } @@ -121,7 +139,11 @@ class DevicePowerSource( val level = battery.getIntExtra(BatteryManager.EXTRA_LEVEL, -1) val scale = battery.getIntExtra(BatteryManager.EXTRA_SCALE, -1) - val status = battery.getIntExtra(BatteryManager.EXTRA_STATUS, BatteryManager.BATTERY_STATUS_UNKNOWN) + // EXTRA_PLUGGED rather than EXTRA_STATUS. A device held at a charge cap -- Adaptive + // Charging, or any battery-protection limit -- reports NOT_CHARGING while plugged in, so + // testing the status showed the battery readout for a device on mains power with its + // current still reversed. Plugged is the question the readout actually asks. + val plugged = battery.getIntExtra(BatteryManager.EXTRA_PLUGGED, 0) val percent = if (level < 0 || scale <= 0) { @@ -132,12 +154,18 @@ class DevicePowerSource( return BatteryState( levelPercent = percent, - isCharging = status == BatteryManager.BATTERY_STATUS_CHARGING || status == BatteryManager.BATTERY_STATUS_FULL, + isCharging = plugged != 0, ) } private companion object { /** Microamps times millivolts gives nanowatts; this scales the product to microwatts. */ const val NANOWATTS_PER_MICROWATT = 1_000L + + /** A milliwatt: below this a non-zero reading is likelier a unit mismatch than a real draw. */ + const val MIN_PLAUSIBLE_MICROWATTS = 1_000L + + /** A hundred watts: no phone draws this, so that is a unit mismatch the other way. */ + const val MAX_PLAUSIBLE_MICROWATTS = 100_000_000L } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt index f78a7728f0..0d7a7d69ee 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt @@ -135,7 +135,7 @@ class NetworkUsageWatcher */ fun getUsage(): NetworkUsage = synchronized(historyLock) { - NetworkUsage(received.snapshot(), transmitted.snapshot()) + NetworkUsage(received.toLongArray(), transmitted.toLongArray()) } /** @@ -313,8 +313,3 @@ class NetworkUsageWatcher private val log = LoggerFactory.getLogger(NetworkUsageWatcher::class.java) } } - -/** - * Copies this ring buffer into a plain array in logical order, oldest first. - */ -private fun ShiftedLongArray.snapshot(): LongArray = LongArray(size) { this[it] } diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 3551001fbd..dfc1c8a9a2 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -117,7 +117,7 @@ class PowerUsageWatcher */ fun getUsage(): PowerUsage = synchronized(historyLock) { - PowerUsage(temperature.snapshotArray(), power.snapshotArray(), thermal.snapshotArray()) + PowerUsage(temperature.toLongArray(), power.toLongArray(), thermal.toLongArray()) } fun clearHistory() { @@ -289,8 +289,3 @@ class PowerUsageWatcher private val log = LoggerFactory.getLogger(PowerUsageWatcher::class.java) } } - -/** - * Copies this ring buffer into a plain array in logical order, oldest first. - */ -private fun ShiftedLongArray.snapshotArray(): LongArray = LongArray(size) { this[it] } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ShiftedLongArray.kt b/app/src/main/java/com/itsaky/androidide/utils/ShiftedLongArray.kt index 4e2c524293..6392e21f7c 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ShiftedLongArray.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ShiftedLongArray.kt @@ -34,110 +34,104 @@ package com.itsaky.androidide.utils * @author Akash Yadav */ open class ShiftedLongArray( - protected val array: LongArray, - shift: Int = 0 + protected val array: LongArray, + shift: Int = 0, ) : Collection { + override val size: Int + get() = array.size + + var shift: Int = shift + protected set + + val normalizedShift: Int + get() = ((shift % size) + size) % size + + @Suppress("NOTHING_TO_INLINE") + protected inline fun checkIdx(idx: Int) { + if (idx < 0 || idx >= array.size) { + throw IndexOutOfBoundsException("Index $idx is out of bounds for array of size ${array.size}") + } + } + + /** + * Get the corresponding shifted-index for the given index. + */ + open fun getShiftedIndex(index: Int): Int { + val size = this.size + val idx = + if (shift < 0) { + size - index + } else { + index + } + return (idx + normalizedShift) % size + } + + /** + * Returns whether the contents of this array are equal to the specified array. + */ + fun contentEquals(array: ShiftedLongArray): Boolean = contentEquals(array.array) + + /** + * Returns whether the contents of this array are equal to the specified array. + */ + fun contentEquals(array: LongArray): Boolean = this.array.contentEquals(array) + + /** + * Returns the hash code value for the contents of this array. + */ + fun contentHashCode(): Int = array.contentHashCode() + + operator fun get(index: Int): Long { + checkIdx(index) + return array[getShiftedIndex(index)] + } + + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is ShiftedLongArray) return false + + if (!array.contentEquals(other.array)) return false + if (shift != other.shift) return false + + return true + } + + override fun hashCode(): Int { + var result = array.contentHashCode() + result = 31 * result + shift + return result + } + + override fun isEmpty(): Boolean = array.isEmpty() + + override fun containsAll(elements: Collection): Boolean = elements.all { array.contains(it) } + + override fun contains(element: Long): Boolean = array.contains(element) + + override fun iterator(): Iterator { + return object : Iterator { + var index = 0 + + override fun hasNext(): Boolean = index < array.size + + override fun next(): Long { + if (!hasNext()) { + throw NoSuchElementException() + } else { + return this@ShiftedLongArray[index++] + } + } + } + } + + override fun toString(): String = "ShiftedLongArray(array=${array.contentToString()}, shift=$shift)" +} - override val size: Int - get() = array.size - - var shift: Int = shift - protected set - - val normalizedShift: Int - get() = ((shift % size) + size) % size - - @Suppress("NOTHING_TO_INLINE") - protected inline fun checkIdx(idx: Int) { - if (idx < 0 || idx >= array.size) { - throw IndexOutOfBoundsException("Index $idx is out of bounds for array of size ${array.size}") - } - } - - /** - * Get the corresponding shifted-index for the given index. - */ - open fun getShiftedIndex(index: Int): Int { - val size = this.size - val idx = if (shift < 0) { - size - index - } else index - return (idx + normalizedShift) % size - } - - /** - * Returns whether the contents of this array are equal to the specified array. - */ - fun contentEquals(array: ShiftedLongArray): Boolean { - return contentEquals(array.array) - } - - /** - * Returns whether the contents of this array are equal to the specified array. - */ - fun contentEquals(array: LongArray): Boolean { - return this.array.contentEquals(array) - } - - /** - * Returns the hash code value for the contents of this array. - */ - fun contentHashCode(): Int { - return array.contentHashCode() - } - - operator fun get(index: Int): Long { - checkIdx(index) - return array[getShiftedIndex(index)] - } - - override fun equals(other: Any?): Boolean { - if (this === other) return true - if (other !is ShiftedLongArray) return false - - if (!array.contentEquals(other.array)) return false - if (shift != other.shift) return false - - return true - } - - override fun hashCode(): Int { - var result = array.contentHashCode() - result = 31 * result + shift - return result - } - - override fun isEmpty(): Boolean { - return array.isEmpty() - } - - override fun containsAll(elements: Collection): Boolean { - return elements.all { array.contains(it) } - } - - override fun contains(element: Long): Boolean { - return array.contains(element) - } - - override fun iterator(): Iterator { - return object : Iterator { - var index = 0 - - override fun hasNext(): Boolean { - return index < array.size - } - - override fun next(): Long { - if (!hasNext()) { - throw NoSuchElementException() - } else { - return this@ShiftedLongArray[index++] - } - } - } - } - - override fun toString(): String { - return "ShiftedLongArray(array=${array.contentToString()}, shift=$shift)" - } -} \ No newline at end of file +/** + * Copies this ring buffer into a plain array in logical order, oldest first. + * + * Shared so the watchers' snapshots cannot drift from [ShiftedLongArray]'s shift semantics; each + * of them had its own private copy of this one line. + */ +internal fun ShiftedLongArray.toLongArray(): LongArray = LongArray(size) { this[it] } diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index 30209b5f1a..79cbf3e120 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -341,6 +341,53 @@ class PowerUsageChartRendererTest { assertThat(chart.axisLeft.axisMaximum).isLessThan(40f) } + @Test + fun `only one axis rules the plot`() { + val (_, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) + laidOut(chart) + + // Both axes drew grid lines at their own pitch, so the plot carried two interleaved sets + // of horizontal rules -- nine of them, including a pair eight pixels apart. Only the + // labelled axis should rule the plot; the left axis is enabled for its labels alone. + assertThat(chart.axisLeft.isDrawGridLinesEnabled).isFalse() + assertThat(chart.axisRight.isDrawGridLinesEnabled).isTrue() + } + + @Test + fun `the temperature axis does not repeat a label`() { + val (_, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) + laidOut(chart) + + // Ranged over a few degrees and formatted without decimals, a finer pitch prints + // "29C, 30C, 30C, 31C". + assertThat(chart.axisLeft.granularity).isEqualTo(1f) + assertThat(chart.axisLeft.isGranularityEnabled).isTrue() + } + + @Test + fun `a new sample updates the existing series rather than replacing them`() { + val (renderer, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) + val before = dataset(chart, 0) + + renderer.onUsageChanged(usage(temperature = LongArray(SAMPLES) { 31_000L })) + + // Rebuilding allocated two datasets and 2 * MAX_USAGE_ENTRIES entries every tick, on the + // UI thread, and threw away the sample it had just been handed. + assertThat(dataset(chart, 0)).isSameInstanceAs(before) + assertThat(before.entries.last().y).isEqualTo(31f) + } + + @Test + fun `a series that no longer matches the sample is rebuilt`() { + val (renderer, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) + + // The buffer grows to its full length over the first minutes of a session, so an + // in-place update has to notice when the shape it is writing into is the wrong one. + renderer.onUsageChanged(usage(temperature = LongArray(SAMPLES + 1) { 31_000L })) + + assertThat(dataset(chart, 0).entryCount).isEqualTo(SAMPLES + 1) + } + @Test fun `an unreadable temperature falls back to a plausible span`() { val (_, chart) = diff --git a/app/src/test/java/com/itsaky/androidide/ui/SafeLineChartTest.kt b/app/src/test/java/com/itsaky/androidide/ui/SafeLineChartTest.kt new file mode 100644 index 0000000000..b209006670 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/SafeLineChartTest.kt @@ -0,0 +1,98 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.graphics.Bitmap +import android.graphics.Canvas +import android.graphics.Color +import android.view.View +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.data.Entry +import com.github.mikephil.charting.data.LineData +import com.github.mikephil.charting.data.LineDataSet +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.GraphicsMode + +/** + * Where [SafeLineChart] paints its background spans relative to the grid background. + * + * The spans first went in before the call up to `super.onDraw`, which was the one place they could + * not survive: the grid background is an opaque fill of the whole plot, so every span was painted + * and then covered. Nothing in the span geometry tests noticed -- they read + * [SafeLineChart.backgroundSpans], which was correct all along -- so this asserts against pixels. + */ +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +class SafeLineChartTest { + private val context = ApplicationProvider.getApplicationContext() + + private fun drawn(configure: SafeLineChart.() -> Unit): Bitmap { + val chart = SafeLineChart(context) + chart.setDrawGridBackground(true) + chart.setGridBackgroundColor(GRID_BACKGROUND) + chart.description.isEnabled = false + chart.legend.isEnabled = false + chart.axisLeft.axisMinimum = 0f + chart.axisLeft.axisMaximum = 10f + // Flat at the axis minimum, so the line itself stays clear of the sampled pixel. + chart.data = LineData(LineDataSet(List(SAMPLES) { Entry(it.toFloat(), 0f) }, "flat")) + chart.configure() + chart.measure( + View.MeasureSpec.makeMeasureSpec(WIDTH, View.MeasureSpec.EXACTLY), + View.MeasureSpec.makeMeasureSpec(HEIGHT, View.MeasureSpec.EXACTLY), + ) + chart.layout(0, 0, WIDTH, HEIGHT) + val bitmap = Bitmap.createBitmap(WIDTH, HEIGHT, Bitmap.Config.ARGB_8888) + chart.draw(Canvas(bitmap)) + return bitmap + } + + /** A pixel inside the plot, near its top, away from the flat data line. */ + private fun Bitmap.plotPixel(): Int = getPixel(WIDTH / 2, HEIGHT / 4) + + @Test + fun `a span reaches the screen instead of being covered by the grid background`() { + val shaded = + drawn { + backgroundSpans = + listOf(SafeLineChart.Span(startX = 0f, endX = SAMPLES.toFloat(), color = SPAN)) + } + + // Painted before the grid background this pixel came back GRID_BACKGROUND, every time. + assertThat(shaded.plotPixel()).isEqualTo(SPAN) + } + + @Test + fun `the grid background still shows through where nothing is shaded`() { + // The other half of the order: the span must not be a wash over the whole plot either. + assertThat(drawn { }.plotPixel()).isEqualTo(GRID_BACKGROUND) + } + + private companion object { + const val WIDTH = 720 + const val HEIGHT = 400 + const val SAMPLES = 20 + + val GRID_BACKGROUND = Color.WHITE + val SPAN = Color.RED + } +} From 7b5028265a2620e5860f67e08ac3ed13373720de Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 09:51:48 -0700 Subject: [PATCH 07/11] ADFA-5499: keep the battery readout off the topmost axis label The readout is anchored to the pager's top-right corner, over the chart, which is exactly where the right axis prints its highest label. At the default font scale it sits above the plot and the two never meet, so the collision was invisible; the strip is a fixed height, so at a 2.0 font scale the readout grows down into the plot and covers that label entirely. Reserving the readout's line height as the chart's extra top offset moves the plot instead, which scales with the text rather than against it, and gives the room back when the readout is hidden. setExtraTopOffset only stores the value -- the viewport is recomputed by the protected calculateOffsets, which otherwise runs only when the chart's size changes -- so this notifies the chart as well. The first version of the test failed for that reason, reporting an unchanged contentTop of 15.0. Found by looking at the page at font scales 1.0, 1.5 and 2.0 rather than only at 1.0. Verified there too: with the fix, at 2.0, "79%" ends at y=214 and the "2W" label begins at y=228. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../ui/MetricsCarouselController.kt | 10 +++++++++ .../androidide/ui/MetricsChartRenderer.kt | 19 +++++++++++++++++ .../ui/PowerUsageChartRendererTest.kt | 21 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index e2b3c68e86..8400228169 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -272,6 +272,16 @@ class MetricsCarouselController( binding.metricsBattery.text = readout.orEmpty() binding.metricsBattery.isVisible = readout != null + + // lineHeight rather than the measured height: this runs on bind, before the readout has + // been laid out, and it is the text's own size that grows with the font scale. + val reserved = + if (readout == null) { + 0f + } else { + binding.metricsBattery.lineHeight + binding.metricsBattery.paddingTop.toFloat() + } + powerRenderer.reserveTopSpace(reserved) } /** diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 02914c74db..9ce73d8036 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -82,6 +82,25 @@ abstract class MetricsChartRenderer( protected var chart: SafeLineChart? = null private set + /** + * Keeps [pixels] of the chart's top clear of the plot and its labels. + * + * The battery readout is anchored to the pager's top-right corner, over the chart, where the + * right axis prints its topmost label. At the default font scale the readout sits above the + * plot and the two do not meet; the strip is a fixed height, so at a 2.0 font scale the + * readout grows down into the plot and hides that label. Reserving its height moves the plot + * instead, which scales with the text rather than against it. + */ + @UiThread + fun reserveTopSpace(pixels: Float) { + val chart = this.chart ?: return + chart.setExtraTopOffset(pixels / chart.resources.displayMetrics.density) + // setExtraTopOffset only stores the value; the viewport is recomputed by calculateOffsets, + // which is protected and otherwise runs only when the chart's size changes. + chart.notifyDataSetChanged() + chart.invalidate() + } + /** * Attaches [chart], applies configuration, and renders the full current history. */ diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index 79cbf3e120..ae3e62b186 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -341,6 +341,24 @@ class PowerUsageChartRendererTest { assertThat(chart.axisLeft.axisMaximum).isLessThan(40f) } + @Test + fun `the battery readout gets room, and gives it back`() { + val (renderer, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) + laidOut(chart) + val unreserved = chart.viewPortHandler.contentTop() + + // The readout is anchored over the chart's top-right corner, where the right axis prints + // its topmost label; at a 2.0 font scale it grew down into the plot and hid that label. + renderer.reserveTopSpace(READOUT_HEIGHT_PX) + laidOut(chart) + assertThat(chart.viewPortHandler.contentTop()).isGreaterThan(unreserved) + + // Off the power page the readout is hidden, and the plot should have the room back. + renderer.reserveTopSpace(0f) + laidOut(chart) + assertThat(chart.viewPortHandler.contentTop()).isEqualTo(unreserved) + } + @Test fun `only one axis rules the plot`() { val (_, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) @@ -404,6 +422,9 @@ class PowerUsageChartRendererTest { const val HEIGHT = 400 const val SAMPLES = 200 + /** A readout two lines tall, which is roughly what a 2.0 font scale gives. */ + const val READOUT_HEIGHT_PX = 80f + const val OPAQUE = 0xFF000000.toInt() /** The palette ADFA-5499 specifies: green, cyan, yellow, orange, rust, red. */ From 6596564f17c0c185b00f815c9a9d81b620bd15a3 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 12:28:22 -0700 Subject: [PATCH 08/11] ADFA-5499: make the carousel adapter actually page-agnostic The adapter's KDoc has claimed since ADFA-5487 that a new display -- "including pages contributed by plugins" -- could be added without touching the class. That was false in three ways at once. MetricsPage was a sealed interface, so a plugin, being a different module, could not implement it at all. Four exhaustive `when` expressions inside this class named the three page types, and two more in the controller did, so a fourth page meant editing six sites. And each renderer was its own constructor parameter, so adding one changed the signature. Adding the third page is what made the shape untenable, so this is where it gets fixed. A page now says what it is called, what it is, and what draws it; nothing in the adapter or the controller names a metric. All six `when`s are gone, and the interface is no longer sealed, so the doc's claim is true rather than aspirational. The three per-metric layouts differed from each other in exactly one attribute -- the content description -- so they are one layout, set per page at bind time. Two things this could plausibly have broken, and did not: Charts are still never shared between pages. One view type per position, because a chart carries what its renderer put on it and some of that is written by one renderer and cleared by none of the others: the power page's thermal shading goes on through SafeLineChart.backgroundSpans, which no memory or network renderer touches. Verified on device by forcing thermal status 4, confirming the band covers 248 of 248 sampled columns on the power page, and then finding 0 of 248 on network and memory after paging away -- and the band still there on returning. Recycling still detaches the right renderer. The holder no longer carries its page's type, so it remembers the renderer it was bound to; onViewRecycled is not told the position and may be handed NO_POSITION. The battery readout moved from a page-type test to MetricsChartRenderer, which answers null unless a page has something to read out. That was the last place the carousel needed to know which page it was holding. Also: `pages` is now declared after the renderers. Holding its own renderer, the list read powerRenderer while that property was still null where it used to sit -- Kotlin initialises properties in declaration order. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsCarouselAdapter.kt | 147 +++++++--------- .../ui/MetricsCarouselController.kt | 59 ++++--- .../androidide/ui/MetricsChartRenderer.kt | 9 + .../androidide/ui/PowerUsageChartRenderer.kt | 2 +- ...emory_chart.xml => item_metrics_chart.xml} | 5 +- .../res/layout/item_metrics_network_chart.xml | 13 -- .../res/layout/item_metrics_power_chart.xml | 13 -- .../ui/MetricsCarouselAdapterTest.kt | 158 ++++++++++++++++++ .../ui/PowerUsageChartRendererTest.kt | 6 +- 9 files changed, 267 insertions(+), 145 deletions(-) rename app/src/main/res/layout/{item_metrics_memory_chart.xml => item_metrics_chart.xml} (79%) delete mode 100644 app/src/main/res/layout/item_metrics_network_chart.xml delete mode 100644 app/src/main/res/layout/item_metrics_power_chart.xml create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselAdapterTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt index 878ddb36e1..0ea5ba14fc 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselAdapter.kt @@ -18,7 +18,6 @@ package com.itsaky.androidide.ui import android.view.LayoutInflater -import android.view.View import android.view.ViewGroup import androidx.annotation.StringRes import androidx.recyclerview.widget.RecyclerView @@ -27,33 +26,47 @@ import com.itsaky.androidide.R /** * A page of the editor's metrics carousel. * + * A page says what it is called, what it is, and what draws it. Nothing else in the carousel needs + * to know which page it is holding, which is what lets [MetricsCarouselAdapter] be page-agnostic. + * + * Deliberately an ordinary interface rather than a sealed one. The adapter has always claimed that + * a new display -- including one contributed by a plugin -- could be added without touching it; + * while this was sealed that was impossible, since a plugin is a different module and could not + * implement it at all. + * * @property title Names the page. Shown below the carousel, and the only cue to which page is * showing, so every page needs one. + * @property contentDescription What the plot is, for a screen reader. + * @property renderer Draws this page and owns its axes, annotations and shading. */ -sealed interface MetricsPage { +interface MetricsPage { @get:StringRes val title: Int - /** The live memory-usage chart, rendered by [MemoryUsageChartRenderer]. */ - data class MemoryChart( - @StringRes override val title: Int, - ) : MetricsPage + @get:StringRes val contentDescription: Int - /** The live network-traffic chart, rendered by [NetworkUsageChartRenderer]. */ - data class NetworkChart( - @StringRes override val title: Int, - ) : MetricsPage - - /** The live temperature and power chart, rendered by [PowerUsageChartRenderer]. */ - data class PowerChart( - @StringRes override val title: Int, - ) : MetricsPage + val renderer: MetricsChartRenderer } +/** + * A page showing one line chart. + * + * There used to be a type per metric -- `MemoryChart`, `NetworkChart`, `PowerChart` -- each with a + * layout of its own that differed from its siblings by one attribute, plus a view type, a view + * holder subclass and a branch in four `when` expressions. They differed in nothing a chart page + * needs to differ in. + */ +data class ChartPage( + @StringRes override val title: Int, + @StringRes override val contentDescription: Int, + override val renderer: MetricsChartRenderer, +) : MetricsPage + /** * Backs the editor's horizontally swipeable carousel of metric displays. * - * [pages] is a constructor argument rather than a hardcoded list so that new displays -- a network - * traffic chart, or pages contributed by plugins -- can be added without touching this class. + * [pages] is a constructor argument rather than a hardcoded list so that new displays can be added + * without touching this class -- and now nothing here names a page or a metric, so that is true + * rather than aspirational. * * A chart page holds no sample state of its own: its renderer is attached when the page binds and * detached when it is recycled, and rebuilds the full history from its watcher each time. Moving @@ -61,98 +74,56 @@ sealed interface MetricsPage { */ class MetricsCarouselAdapter( private val pages: List, - private val memoryChartRenderer: MemoryUsageChartRenderer, - private val networkChartRenderer: NetworkUsageChartRenderer, - private val powerChartRenderer: PowerUsageChartRenderer, ) : RecyclerView.Adapter() { - sealed class PageViewHolder( - view: View, - ) : RecyclerView.ViewHolder(view) { - class MemoryChart( - val chart: SafeLineChart, - ) : PageViewHolder(chart) - - class NetworkChart( - val chart: SafeLineChart, - ) : PageViewHolder(chart) - - class PowerChart( - val chart: SafeLineChart, - ) : PageViewHolder(chart) + /** + * @property boundRenderer What was attached to [chart] at bind time, so [onViewRecycled] can + * detach the right renderer without being told the position -- which it is not. + */ + class PageViewHolder( + val chart: SafeLineChart, + ) : RecyclerView.ViewHolder(chart) { + var boundRenderer: MetricsChartRenderer? = null } override fun getItemCount(): Int = pages.size - override fun getItemViewType(position: Int): Int = - when (pages[position]) { - is MetricsPage.MemoryChart -> VIEW_TYPE_MEMORY_CHART - is MetricsPage.NetworkChart -> VIEW_TYPE_NETWORK_CHART - is MetricsPage.PowerChart -> VIEW_TYPE_POWER_CHART - } + /** + * One view type per page, so a chart is never recycled from one page onto another. + * + * Not a saving worth making here: a chart carries the state its renderer put on it, and some of + * that is written by one renderer and cleared by none of the others -- the thermal shading on + * the power page is set through [SafeLineChart.backgroundSpans], which a memory or network + * renderer has no reason to touch. A handful of pages, each keeping its own chart, costs + * nothing and cannot leak one page's decoration onto another. + */ + override fun getItemViewType(position: Int): Int = position override fun onCreateViewHolder( parent: ViewGroup, viewType: Int, ): PageViewHolder { - val inflater = LayoutInflater.from(parent.context) - return when (viewType) { - VIEW_TYPE_MEMORY_CHART -> { - PageViewHolder.MemoryChart( - inflater.inflate(R.layout.item_metrics_memory_chart, parent, false) as SafeLineChart, - ) - } - - VIEW_TYPE_NETWORK_CHART -> { - PageViewHolder.NetworkChart( - inflater.inflate(R.layout.item_metrics_network_chart, parent, false) as SafeLineChart, - ) - } - - VIEW_TYPE_POWER_CHART -> { - PageViewHolder.PowerChart( - inflater.inflate(R.layout.item_metrics_power_chart, parent, false) as SafeLineChart, - ) - } - - else -> { - throw IllegalArgumentException("Unknown metrics page view type: $viewType") - } - } + val chart = + LayoutInflater + .from(parent.context) + .inflate(R.layout.item_metrics_chart, parent, false) as SafeLineChart + return PageViewHolder(chart) } override fun onBindViewHolder( holder: PageViewHolder, position: Int, ) { - when (pages[position]) { - is MetricsPage.MemoryChart -> { - memoryChartRenderer.attach((holder as PageViewHolder.MemoryChart).chart) - } - - is MetricsPage.NetworkChart -> { - networkChartRenderer.attach((holder as PageViewHolder.NetworkChart).chart) - } - - is MetricsPage.PowerChart -> { - powerChartRenderer.attach((holder as PageViewHolder.PowerChart).chart) - } - } + val page = pages[position] + holder.chart.contentDescription = holder.chart.context.getString(page.contentDescription) + holder.boundRenderer = page.renderer + page.renderer.attach(holder.chart) } override fun onViewRecycled(holder: PageViewHolder) { // Only if this holder's chart is still the attached one: a rebind can create the replacement // before RecyclerView recycles the view it replaced, and detaching then would drop the new // chart instead of the old. - when (holder) { - is PageViewHolder.MemoryChart -> memoryChartRenderer.detachIfAttached(holder.chart) - is PageViewHolder.NetworkChart -> networkChartRenderer.detachIfAttached(holder.chart) - is PageViewHolder.PowerChart -> powerChartRenderer.detachIfAttached(holder.chart) - } - } - - private companion object { - const val VIEW_TYPE_MEMORY_CHART = 0 - const val VIEW_TYPE_NETWORK_CHART = 1 - const val VIEW_TYPE_POWER_CHART = 2 + holder.boundRenderer?.detachIfAttached(holder.chart) + holder.boundRenderer = null } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index 7acae11e8a..e02a7399e7 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -85,15 +85,6 @@ class MetricsCarouselController( sampleInterval = { networkUsageWatcher.updateInterval }, ) - private val pages = - listOf( - // The memory chart is the default page (ADFA-5487); network traffic is the second - // (ADFA-5489), replacing the brand-mark placeholder that ADFA-5487 shipped. - MetricsPage.MemoryChart(title = string.metrics_title_memory), - MetricsPage.NetworkChart(title = string.metrics_title_network), - MetricsPage.PowerChart(title = string.metrics_title_power), - ) - private val powerRenderer = PowerUsageChartRenderer( usageProvider = { powerUsageWatcher.getUsage() }, @@ -102,6 +93,34 @@ class MetricsCarouselController( sampleIntervalMillis = { powerUsageWatcher.updateInterval }, ) + /** + * The carousel's pages, in order. + * + * Declared after the renderers, not before: a page holds its own renderer, and Kotlin + * initialises properties in declaration order, so listing the pages first read powerRenderer + * while it was still null. + */ + private val pages: List = + listOf( + // The memory chart is the default page (ADFA-5487); network traffic is the second + // (ADFA-5489), replacing the brand-mark placeholder that ADFA-5487 shipped. + ChartPage( + title = string.metrics_title_memory, + contentDescription = string.metrics_carousel_memory_chart, + renderer = memoryRenderer, + ), + ChartPage( + title = string.metrics_title_network, + contentDescription = string.metrics_network_chart, + renderer = networkRenderer, + ), + ChartPage( + title = string.metrics_title_power, + contentDescription = string.metrics_power_chart, + renderer = powerRenderer, + ), + ) + private val powerListener = PowerUsageWatcher.PowerUsageListener { usage -> powerRenderer.onUsageChanged(usage) @@ -149,7 +168,7 @@ class MetricsCarouselController( this.binding = binding - binding.metricsPager.adapter = MetricsCarouselAdapter(pages, memoryRenderer, networkRenderer, powerRenderer) + binding.metricsPager.adapter = MetricsCarouselAdapter(pages) val showTitleFor = { position: Int -> pages.getOrNull(position)?.let { page -> @@ -267,8 +286,8 @@ class MetricsCarouselController( @UiThread private fun updateBatteryReadout() { val binding = this.binding ?: return - val onPowerPage = pages.getOrNull(binding.metricsPager.currentItem) is MetricsPage.PowerChart - val readout = if (onPowerPage) powerRenderer.batteryReadout() else null + val renderer = currentRenderer() + val readout = renderer?.readout() binding.metricsBattery.text = readout.orEmpty() binding.metricsBattery.isVisible = readout != null @@ -281,7 +300,7 @@ class MetricsCarouselController( } else { binding.metricsBattery.lineHeight + binding.metricsBattery.paddingTop.toFloat() } - powerRenderer.reserveTopSpace(reserved) + renderer?.reserveTopSpace(reserved) } /** @@ -289,12 +308,7 @@ class MetricsCarouselController( */ private fun currentRenderer(): MetricsChartRenderer? { val binding = this.binding ?: return null - return when (pages.getOrNull(binding.metricsPager.currentItem)) { - is MetricsPage.MemoryChart -> memoryRenderer - is MetricsPage.NetworkChart -> networkRenderer - is MetricsPage.PowerChart -> powerRenderer - null -> null - } + return pages.getOrNull(binding.metricsPager.currentItem)?.renderer } /** @@ -413,12 +427,7 @@ class MetricsCarouselController( val position = binding.metricsPager.currentItem val page = pages.getOrNull(position) ?: return false - val renderer = - when (page) { - is MetricsPage.MemoryChart -> memoryRenderer - is MetricsPage.NetworkChart -> networkRenderer - is MetricsPage.PowerChart -> powerRenderer - } + val renderer = page.renderer val label = context.getString(page.title) val bitmap = renderer.snapshot() diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 1e5862ac52..ac0fc3d610 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -82,6 +82,15 @@ abstract class MetricsChartRenderer( protected var chart: SafeLineChart? = null private set + /** + * A short readout to show beside this page's chart, or `null` if it has none. + * + * Asked of the renderer rather than decided from the page's type, so the carousel does not + * have to know which of its pages happens to have a battery on it. + */ + @UiThread + open fun readout(): String? = null + /** * Keeps [pixels] of the chart's top clear of the plot and its labels. * diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 121fdb5947..306cae5fe4 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -368,7 +368,7 @@ class PowerUsageChartRenderer( * hidden while charging, when a rising level would contradict a chart about power being spent. */ @UiThread - fun batteryReadout(): String? { + override fun readout(): String? { val battery = batteryProvider() if (battery.isCharging || battery.levelPercent < 0) { return null diff --git a/app/src/main/res/layout/item_metrics_memory_chart.xml b/app/src/main/res/layout/item_metrics_chart.xml similarity index 79% rename from app/src/main/res/layout/item_metrics_memory_chart.xml rename to app/src/main/res/layout/item_metrics_chart.xml index d6eaaa40ab..643645bd1f 100644 --- a/app/src/main/res/layout/item_metrics_memory_chart.xml +++ b/app/src/main/res/layout/item_metrics_chart.xml @@ -5,9 +5,10 @@ PURPOSE. See the ~ GNU General Public License for more details. ~ ~ You should have received a copy of the GNU General Public License ~ along with AndroidIDE. If not, see . --> + + android:layout_height="match_parent" /> diff --git a/app/src/main/res/layout/item_metrics_network_chart.xml b/app/src/main/res/layout/item_metrics_network_chart.xml deleted file mode 100644 index f011080f06..0000000000 --- a/app/src/main/res/layout/item_metrics_network_chart.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - diff --git a/app/src/main/res/layout/item_metrics_power_chart.xml b/app/src/main/res/layout/item_metrics_power_chart.xml deleted file mode 100644 index aa319feba1..0000000000 --- a/app/src/main/res/layout/item_metrics_power_chart.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselAdapterTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselAdapterTest.kt new file mode 100644 index 0000000000..b2cb9f205e --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselAdapterTest.kt @@ -0,0 +1,158 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.widget.FrameLayout +import androidx.appcompat.view.ContextThemeWrapper +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.data.Entry +import com.github.mikephil.charting.data.LineDataSet +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.R +import com.itsaky.androidide.resources.R.string +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Pins what the carousel adapter promises now that it no longer names any metric. + * + * It used to branch on the page's type in four places, with a view type, a view-holder subclass + * and a layout per metric; the layouts differed from each other by one attribute. These are the + * behaviours that branching was providing, asserted directly so the page-agnostic version cannot + * quietly drop one. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsCarouselAdapterTest { + private val context: Context = + ContextThemeWrapper(ApplicationProvider.getApplicationContext(), R.style.Theme_AndroidIDE) + + private val parent = FrameLayout(context) + + /** A renderer that records what it was attached to, and draws just enough to be attachable. */ + private class TestRenderer( + private val readout: String? = null, + ) : MetricsChartRenderer(sampleIntervalMillis = { 1_000L }) { + val attached = mutableListOf() + + override fun rebuild() { + val chart = this.chart ?: return + setData(chart, arrayOf(LineDataSet(listOf(Entry(0f, 0f)), "test"))) + attached += chart + } + + override fun readout(): String? = readout + + /** detachIfAttached is final, so detachment is observed through what it leaves behind. */ + val isAttached: Boolean + get() = chart != null + } + + private fun pageOf( + renderer: MetricsChartRenderer, + description: Int = string.metrics_carousel_memory_chart, + ) = ChartPage(title = string.metrics_title_memory, contentDescription = description, renderer = renderer) + + private fun bind( + adapter: MetricsCarouselAdapter, + position: Int, + ): MetricsCarouselAdapter.PageViewHolder { + val holder = adapter.onCreateViewHolder(parent, adapter.getItemViewType(position)) + adapter.onBindViewHolder(holder, position) + return holder + } + + @Test + fun `each page gets its own chart, never one recycled from another page`() { + val pages = List(3) { pageOf(TestRenderer()) } + val adapter = MetricsCarouselAdapter(pages) + + // One view type per position. Sharing a chart between pages would carry over whatever the + // previous renderer had put on it -- the power page's thermal shading is written through + // SafeLineChart.backgroundSpans, which no other renderer clears. + val types = pages.indices.map(adapter::getItemViewType) + assertThat(types.toSet()).hasSize(pages.size) + } + + @Test + fun `binding attaches that page's own renderer`() { + val first = TestRenderer() + val second = TestRenderer() + val adapter = MetricsCarouselAdapter(listOf(pageOf(first), pageOf(second))) + + val holder = bind(adapter, 1) + + assertThat(second.attached).containsExactly(holder.chart) + assertThat(first.attached).isEmpty() + } + + @Test + fun `binding describes the plot for a screen reader`() { + val adapter = + MetricsCarouselAdapter( + listOf(pageOf(TestRenderer(), description = string.metrics_power_chart)), + ) + + val holder = bind(adapter, 0) + + // This was the only thing the three per-metric layouts differed in, so it is the one + // thing collapsing them to one could have lost. + assertThat(holder.chart.contentDescription) + .isEqualTo(context.getString(string.metrics_power_chart)) + } + + @Test + fun `recycling detaches the renderer that was bound`() { + val first = TestRenderer() + val second = TestRenderer() + val adapter = MetricsCarouselAdapter(listOf(pageOf(first), pageOf(second))) + val holder = bind(adapter, 1) + assertThat(second.isAttached).isTrue() + + adapter.onViewRecycled(holder) + + // The holder no longer carries its page's type, so it has to remember its renderer: + // onViewRecycled is not told the position, and may be given NO_POSITION. + assertThat(second.isAttached).isFalse() + assertThat(holder.boundRenderer).isNull() + } + + @Test + fun `a rebind before the old view is recycled keeps the new chart attached`() { + val renderer = TestRenderer() + val adapter = MetricsCarouselAdapter(listOf(pageOf(renderer))) + val old = bind(adapter, 0) + val new = bind(adapter, 0) + + // RecyclerView can create the replacement before recycling what it replaced. Detaching + // unconditionally here would drop the new chart instead of the old one. + adapter.onViewRecycled(old) + + assertThat(renderer.isAttached).isTrue() + assertThat(new.boundRenderer).isSameInstanceAs(renderer) + } + + @Test + fun `a page with nothing to read out says so, without being asked what kind it is`() { + // The battery readout used to be reached by testing the page's type. Only the power page + // has one; every other renderer answers null from the base class. + assertThat(TestRenderer().readout()).isNull() + assertThat(TestRenderer(readout = "62%").readout()).isEqualTo("62%") + } +} diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index ae3e62b186..880ca7526d 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -279,7 +279,7 @@ class PowerUsageChartRendererTest { ) // A level climbing while the chart is about power being spent reads as a contradiction. - assertThat(charging.batteryReadout()).isNull() + assertThat(charging.readout()).isNull() } @Test @@ -290,7 +290,7 @@ class PowerUsageChartRendererTest { battery = BatteryState(levelPercent = 62, isCharging = false), ) - assertThat(renderer.batteryReadout()).isEqualTo("62%") + assertThat(renderer.readout()).isEqualTo("62%") } @Test @@ -301,7 +301,7 @@ class PowerUsageChartRendererTest { battery = BatteryState.UNKNOWN, ) - assertThat(renderer.batteryReadout()).isNull() + assertThat(renderer.readout()).isNull() } private fun laidOut(chart: SafeLineChart) { From 434174621cc22729bbe88d275487ccd897580613 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 13:30:19 -0700 Subject: [PATCH 09/11] ADFA-5499: set axis bounds before the notify, and publish watcher state Two findings from CodeRabbit, both confirmed and both wider than reported. Manual axis bounds never reached the transform. axisMinimum and axisMaximum only store a value; notifyDataSetChanged is what recomputes the axis values and the value-to-pixel mapping, and it is protected against being called directly. Two of the three renderers ranged after that notify, so until something else recalculated -- a layout change, or the next tick -- the chart drew through a transform built from the bounds MPAndroidChart had picked for itself. The network chart had it in the per-tick path, which runs once a second. The order is now the base class's, not each renderer's: setData and redraw take the ranging step and run it before the notify. No renderer chooses any more, which is the point -- all three had drifted to different orderings, and the comment on one of them ("After, not before: setData is what scrolls the window...") described a design that no longer exists, since visibleSampleRange stopped reading the viewport when it started keying on userHasZoomed. updateInterval and listener are now @Volatile on all three watchers. They are written on the UI thread and read on each watcher's own sampling thread, so a reader could go on seeing a cleared listener or a stale interval indefinitely. CodeRabbit flagged PowerUsageWatcher; MemoryUsageWatcher had the same on listener, and all three did on updateInterval. NetworkUsageWatcher.listener was already marked, so the knowledge was in the codebase and the sweep was what was missing -- the third time that shape has come up in this stack. The first version of the rebuild-path test passed with the bug still in place: it laid the chart out with a draw, and the draw recomputes the transform by itself. It now rebuilds after the layout and asserts without drawing again. Both tests fail against the old ordering. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MemoryUsageChartRenderer.kt | 10 ++--- .../androidide/ui/MetricsChartRenderer.kt | 17 +++++++- .../ui/NetworkUsageChartRenderer.kt | 10 +---- .../androidide/ui/PowerUsageChartRenderer.kt | 6 +-- .../androidide/utils/MemoryUsageWatcher.kt | 8 ++++ .../androidide/utils/NetworkUsageWatcher.kt | 4 ++ .../androidide/utils/PowerUsageWatcher.kt | 8 ++++ .../ui/NetworkUsageChartRendererTest.kt | 41 +++++++++++++++++++ 8 files changed, 85 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt index fd00fa02bc..e046c95e08 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt @@ -104,8 +104,7 @@ class MemoryUsageChartRenderer( } } - applyAxisRange(chart, processes) - setData(chart, datasets) + setData(chart, datasets) { applyAxisRange(it, processes) } } /** @@ -184,10 +183,11 @@ class MemoryUsageChartRenderer( if (dataChanged) { // From the samples already in hand: usagesProvider() copies every history, so calling // it again here would snapshot the whole buffer a second time per tick. - applyAxisRangeFor(chart) { visit -> - memoryUsage.forEachValue { visit(it) } + redraw(chart) { ranged -> + applyAxisRangeFor(ranged) { visit -> + memoryUsage.forEachValue { visit(it) } + } } - redraw(chart) } } diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index ac0fc3d610..5cbc153b3f 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -352,6 +352,7 @@ abstract class MetricsChartRenderer( protected fun setData( chart: SafeLineChart, datasets: Array, + applyAxisRanges: (SafeLineChart) -> Unit = {}, ) { val bgColor = chart.context.resolveAttr(R.attr.colorSurfaceDim) val textColor = chart.context.resolveAttr(R.attr.colorOnSurface) @@ -369,8 +370,14 @@ abstract class MetricsChartRenderer( styleValueAxes(this, textColor) setBackgroundColor(bgColor) setGridBackgroundColor(bgColor) - notifyDataSetChanged() } + // Ranges first, then the notify. setting axisMinimum and axisMaximum only stores them; + // what recomputes the axis values and the value-to-pixel transform is notifyDataSetChanged, + // and it is protected against being called directly. Ranged after the notify -- as two of + // the three renderers did -- the chart draws its next frame through a transform built from + // the bounds MPAndroidChart picked for itself. + applyAxisRanges(chart) + chart.notifyDataSetChanged() applyAnnotations(chart) showNewestWindow(chart) chart.invalidate() @@ -448,7 +455,13 @@ abstract class MetricsChartRenderer( /** * Redraws after the attached series have been mutated in place. */ - protected fun redraw(chart: SafeLineChart) { + protected fun redraw( + chart: SafeLineChart, + applyAxisRanges: (SafeLineChart) -> Unit = {}, + ) { + // Same order as [setData], and for the same reason: the bounds have to be in place before + // the notify that turns them into a transform. + applyAxisRanges(chart) chart.apply { data.notifyDataChanged() notifyDataSetChanged() diff --git a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt index 989139e676..d8ce7d0cec 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt @@ -78,11 +78,7 @@ class NetworkUsageChartRenderer( dataset(usage.transmitted, chart.context.getString(R.string.metrics_network_transmitted), TRANSMITTED_COLOR), ) - setData(chart, datasets) - // After, not before: setData is what scrolls the window to the newest samples, and the - // range is derived from what that window ends up showing. - applyAxisRange(chart, usage) - chart.invalidate() + setData(chart, datasets) { applyAxisRange(it, usage) } } /** @@ -112,9 +108,7 @@ class NetworkUsageChartRenderer( update(received, usage.received, chart.context.getString(R.string.metrics_network_received)) update(transmitted, usage.transmitted, chart.context.getString(R.string.metrics_network_transmitted)) - redraw(chart) - applyAxisRange(chart, usage) - chart.invalidate() + redraw(chart) { applyAxisRange(it, usage) } } private fun dataset( diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 306cae5fe4..31d6948ffb 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -94,8 +94,7 @@ class PowerUsageChartRenderer( ), ) - setData(chart, datasets) - applyAxisRanges(chart, usage) + setData(chart, datasets) { applyAxisRanges(it, usage) } applyThermalShading(chart, usage) } @@ -138,9 +137,8 @@ class PowerUsageChartRenderer( transform = ::microWattsToWatts, ) - applyAxisRanges(chart, usage) applyThermalShading(chart, usage) - redraw(chart) + redraw(chart) { applyAxisRanges(it, usage) } } /** Rewrites one series' values in place and refreshes its legend entry. */ diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index 31ca11b78b..229a103e27 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -63,7 +63,11 @@ class MemoryUsageWatcher * Milliseconds between samples. Changing it clears the history: the chart reads a sample's * age from its position, which assumes every sample is the same age apart, and a buffer * holding samples taken at two rates would silently misdate all the older ones (ADFA-5486). + * + * Volatile: written on the UI thread and read on the watcher's own sampling thread. + * Without it the reader can go on seeing a stale value indefinitely. */ + @Volatile var updateInterval: Long = MetricsSamplingRates.coerceToSafeRange(updateInterval) set(value) { val safe = MetricsSamplingRates.coerceToSafeRange(value) @@ -103,7 +107,11 @@ class MemoryUsageWatcher /** * The listener to be notified when the memory usage of a process changes. + * + * Volatile: written on the UI thread and read on the watcher's own sampling thread. + * Without it the reader can go on seeing a stale value indefinitely. */ + @Volatile var listener: MemoryUsageListener? = null companion object { diff --git a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt index 8735ff2d3d..2407c1673a 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt @@ -85,7 +85,11 @@ class NetworkUsageWatcher /** * Milliseconds between samples. Changing it clears the history, for the reason given on * [MemoryUsageWatcher.updateInterval]. + * + * Volatile: written on the UI thread and read on the watcher's own sampling thread. + * Without it the reader can go on seeing a stale value indefinitely. */ + @Volatile var updateInterval: Long = MetricsSamplingRates.coerceToSafeRange(updateInterval) set(value) { val safe = MetricsSamplingRates.coerceToSafeRange(value) diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index dfc1c8a9a2..4753e9e730 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -89,7 +89,14 @@ class PowerUsageWatcher /** * Milliseconds between samples. Changing it clears the history, for the reason given on * [MemoryUsageWatcher.updateInterval]. + * + * Volatile: written on the UI thread and read on the watcher's own sampling thread. + * Without it the reader can go on seeing a stale value indefinitely. + * + * Volatile: written on the UI thread and read on the watcher's own sampling thread. + * Without it the reader can go on seeing a stale value indefinitely. */ + @Volatile var updateInterval: Long = MetricsSamplingRates.coerceToSafeRange(updateInterval) set(value) { val safe = MetricsSamplingRates.coerceToSafeRange(value) @@ -109,6 +116,7 @@ class PowerUsageWatcher get() = watching.get() /** Notified on the main thread after each sample. */ + @Volatile var listener: PowerUsageListener? = null /** diff --git a/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt index 85cd37298c..3e8cf8f781 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt @@ -114,6 +114,47 @@ class NetworkUsageChartRendererTest { chart.draw(Canvas(Bitmap.createBitmap(WIDTH, HEIGHT, Bitmap.Config.ARGB_8888))) } + @Test + fun `a rebuild after layout leaves the bounds and the transform in step`() { + val chart = SafeLineChart(context) + var samples = LongArray(SAMPLE_COUNT) { 500L } + val renderer = NetworkUsageChartRenderer(usageProvider = { usage(samples) }) + renderer.attach(chart) + laidOut(chart) + + // Rebuild after the layout, and assert without drawing again: a draw recomputes the + // transform on its own, which is what made the first version of this test pass with the + // bug still in place. + samples = LongArray(SAMPLE_COUNT) { 900_000L } + renderer.rebuild() + + // Setting axisMinimum and axisMaximum only stores them; notifyDataSetChanged is what turns + // them into a value-to-pixel transform. + val ceiling = chart.axisRight.axisMaximum + val pixel = chart.getPixelForValues(0f, ceiling, YAxis.AxisDependency.RIGHT) + + assertThat(pixel.y.toFloat()).isWithin(1f).of(chart.viewPortHandler.contentTop()) + } + + @Test + fun `a tick keeps the bounds and the transform in step`() { + val chart = SafeLineChart(context) + var samples = LongArray(SAMPLE_COUNT) { 500L } + val renderer = NetworkUsageChartRenderer(usageProvider = { usage(samples) }) + renderer.attach(chart) + laidOut(chart) + + // A burst raises the ceiling. The per-tick path had the same ordering bug as the rebuild, + // and it is the one that runs once a second. + samples = LongArray(SAMPLE_COUNT) { 900_000L } + renderer.onUsageChanged(usage(samples)) + + val ceiling = chart.axisRight.axisMaximum + val pixel = chart.getPixelForValues(0f, ceiling, YAxis.AxisDependency.RIGHT) + + assertThat(pixel.y.toFloat()).isWithin(1f).of(chart.viewPortHandler.contentTop()) + } + @Test fun `the axis is scaled to what is on screen, not to the whole buffer`() { // A one-off gigabyte burst near the start of a long history, then quiet chatter. From a3c90adde1fb9fc839d565b288b42d1d908e2452 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 18:29:36 -0700 Subject: [PATCH 10/11] ADFA-5499: hide the battery readout when the carousel undocks setUndocked hid the pager, the title, both arrows and the snapshot button, and left the battery readout showing. This ticket added that readout after setUndocked was written and did not extend the list, so undocking left a battery level sitting over the "tap to bring them back" message. Hiding it is one way only. The readout belongs to the power page alone, and which page is showing is the controller's to know, not this view's: docking restores it on the rebind that follows. The controller's own test for it grows a second term, because that runs on every page change and every refresh -- without it the next battery tick put the readout straight back over the message. The test that should have caught this was already named for it -- "undocking hides every carousel control, not just the chart" -- and listed five ids by hand. It now enumerates the strip's children and asserts none is left showing, so a control added later cannot be missed the same way. Two cases fail without the fix with "expected to be empty but was: [metrics_battery]". The readout starts `gone` in the layout, so both cases show it first. Without that they passed against a strip where the readout had never been visible -- which is how the original test missed the defect, and how the first draft of this one passed with the fix removed. Found by CodeRabbit on #1790 as an "outside diff range" comment. Those cannot become review threads, so nothing tracked it: the PR showed no unresolved threads. Note for whoever runs the suite on this branch: MetricsViewModelTest fails here with "Cannot create an instance of class MetricsViewModel", before and after this change, and passes at the top of the stack. It is not this commit's, and it is filed separately. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsCarouselController.kt | 5 ++- .../androidide/ui/MetricsCarouselLayout.kt | 25 ++++++++++- .../ui/MetricsCarouselLayoutTest.kt | 42 +++++++++++++++---- 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index 3885016001..4a6523fa1c 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -349,7 +349,10 @@ class MetricsCarouselController( val readout = renderer?.readout() binding.metricsBattery.text = readout.orEmpty() - binding.metricsBattery.isVisible = readout != null + // Not on a page that has no readout, and not while the carousel is undocked: this runs on + // every page change and every refresh, so without the second test the next battery tick + // put the readout back over the "tap to bring them back" message. + binding.metricsBattery.isVisible = readout != null && !binding.root.isUndocked // lineHeight rather than the measured height: this runs on bind, before the readout has // been laid out, and it is the text's own size that grows with the font scale. diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt index c55989037d..b568754857 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt @@ -60,16 +60,31 @@ class MetricsCarouselLayout /** Invoked as each gesture begins. */ var onTouchDown: (() -> Unit)? = null + /** + * Whether the carousel has been moved out to a floating window. + * + * Read by the controller: a control whose visibility depends on something else as well -- + * the battery readout, which only belongs on the page that has one -- cannot be restored by + * [setUndocked] alone, so it has to be able to ask. + */ + var isUndocked = false + private set + /** * Shows either the carousel or the "it is in a floating window" message, never a mix. * * The whole strip switches, not just the pager. The arrows and the snapshot button are * chrome for a chart that is not here: left behind they sit over the message, and the * camera is inert anyway because undocking unbinds the controller that listens to it. - * Keeping the set here rather than at the call site is what stops a control added later - * from being forgotten again. + * + * Keeping the set here was supposed to stop a control added later from being forgotten. + * It did not: the battery readout arrived afterwards and was missed, so the readout sat + * over the message. A list in one place is still easier to extend than a list at every + * call site, but nothing about it is self-maintaining -- what actually guards this is the + * test, which enumerates the strip's children rather than naming them. */ fun setUndocked(undocked: Boolean) { + isUndocked = undocked val carouselIds = intArrayOf( R.id.metrics_pager, @@ -81,6 +96,12 @@ class MetricsCarouselLayout carouselIds.forEach { id -> findViewById(id)?.isVisible = !undocked } + // One way only. Undocking hides the battery readout like everything else, but docking + // must not show it: it belongs to the power page alone, and which page is showing is + // the controller's to say. It restores the readout on the rebind that follows. + if (undocked) { + findViewById(R.id.metrics_battery)?.isVisible = false + } findViewById(R.id.metrics_undocked_message)?.isVisible = undocked } diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselLayoutTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselLayoutTest.kt index 9697966d47..6a9847be89 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselLayoutTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselLayoutTest.kt @@ -23,6 +23,7 @@ import android.view.LayoutInflater import android.view.MotionEvent import android.view.ViewConfiguration import androidx.appcompat.view.ContextThemeWrapper +import androidx.core.view.children import androidx.core.view.isVisible import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat @@ -116,24 +117,37 @@ class MetricsCarouselLayoutTest { } } + /** Every control in the strip except the message that replaces them, named for a failure. */ + private fun MetricsCarouselLayout.stillShowing(): List = + children + .filter { it.id != R.id.metrics_undocked_message && it.isVisible } + .map { resources.getResourceEntryName(it.id) } + .toList() + @Test - fun `undocking hides every carousel control, not just the chart`() { + fun `undocking hides every control in the strip, whatever it is`() { val binding = inflatedStrip() + // The readout starts `gone` in the layout and is shown by the controller on the power page. + // It has to be showing before this, or the assertion runs against a strip where it never + // was -- which is how the defect survived a test already named for it, and how the first + // version of this one passed with the fix removed. + binding.metricsBattery.isVisible = true binding.root.setUndocked(true) // The arrows and the camera are chrome for a chart that is not here. Left visible they sit // over the message, and the camera is inert anyway because undocking unbinds its listener. - assertThat(binding.metricsPager.isVisible).isFalse() - assertThat(binding.metricsTitle.isVisible).isFalse() - assertThat(binding.metricsPrevious.isVisible).isFalse() - assertThat(binding.metricsNext.isVisible).isFalse() - assertThat(binding.metricsSnapshot.isVisible).isFalse() + // + // Enumerated from the layout rather than listed by hand. The hand-list this replaces was + // named for the invariant it did not check: it named five ids and missed the battery + // readout, which had been added to the strip after setUndocked was written, so the readout + // sat over the message. A list that reads the layout cannot be out of date. + assertThat(binding.root.stillShowing()).isEmpty() assertThat(binding.metricsUndockedMessage.isVisible).isTrue() } @Test - fun `re-docking brings every control back`() { + fun `re-docking brings the carousel back`() { val binding = inflatedStrip() binding.root.setUndocked(true) @@ -147,6 +161,20 @@ class MetricsCarouselLayoutTest { assertThat(binding.metricsUndockedMessage.isVisible).isFalse() } + @Test + fun `re-docking does not put the battery readout back by itself`() { + val binding = inflatedStrip() + + // It belongs to the power page alone, and which page is showing is not this view's to + // know. Restoring it here would show a battery level over every other chart; the + // controller puts it back on the rebind that follows a dock. + binding.metricsBattery.isVisible = true + binding.root.setUndocked(true) + binding.root.setUndocked(false) + + assertThat(binding.metricsBattery.isVisible).isFalse() + } + @Test fun `a two-finger tap fires the callback`() { var taps = 0 From 62464537d09808441f227027268bbc090a98b2f4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 06:13:10 -0700 Subject: [PATCH 11/11] ADFA-5499: fix the re-review findings on the power page Tier A and B from the second xhigh pass. Six items. SafeLineChart raced itself. drawBackgroundSpans mutated two instance fields -- a FloatArray scratch and a Paint -- inside onDraw, and the entire reason this class exists is that onDraw is entered from two threads at once: Sentry Session Replay records the screen by drawing the view hierarchy off the main thread. So the replay thread could overwrite all four coordinate slots, or the colour, between the main thread's write and its read, and a band would be painted at another span's coordinates or in another span's hue. The reused buffer was introduced to avoid pooled MPPointD churn; a local array and a local Paint per draw is still far less churn than that, and cannot be raced. backgroundSpans is volatile now too, since it is published across the same two threads. The power plausibility envelope did not catch the case its own comment named. The comment said a milliamp-reporting kernel makes "a five-watt build read as five milliwatts"; five watts misreported is 5,000uW, comfortably inside the old 1,000uW floor. A single sample cannot tell that from a genuinely tiny draw -- both are 5,000uW -- so this is a plausibility floor, not a detector, and the floor is now placed where a misreported build actually lands: 10mW, a thousandth of the 10W ceiling a phone can reach. The residual gaps are stated rather than papered over. The arithmetic is extracted as microWattsOrUnavailable so the envelope can be asserted without a BatteryManager, and the five-watt case fails against the old floor. Power buffers were filled with zero, so every unsampled slot plotted as a real 0 C and 0 W reading. That forced applyAxisRanges to special-case `!= 0L`, which also discarded a genuine freezing-battery sample -- a phone left in a car charted its own temperature as absent. The buffers fill with UNAVAILABLE now, clear() takes the fill value because zero is a measurement for memory and an absence for temperature, and the special case is gone. The test that encoded the workaround is rewritten, and a second case pins that a real 0 C is ranged over. reserveTopSpace claimed calculateOffsets was protected. It is public in AndroidChart 3.1.0.21 -- checked with javap -- which is why this went the long way round through notifyDataSetChanged(). That did far more work, and worse, returns early when the chart has no data: exactly the state at bind time, when this is first called, so the reserve silently did not apply until the next sample. It calls calculateOffsets directly now and skips an unchanged value, which also stops every power tick recomputing the viewport of whichever chart is on screen. powerUsageWatcher.listener was missing from the terminal teardown while its two siblings were there. Only metricsCarousel.unbind() was releasing it, under an identity check, and skipped entirely for an undocked carousel. MetricsViewModelTest has been failing since this ticket made the view model an AndroidViewModel: NewInstanceFactory reflects on a no-arg constructor that no longer exists. Nothing noticed because the only CI job that runs unit tests runs them with ignoreFailures set (ADFA-5559). It uses AndroidViewModelFactory now and asserts all three watchers rather than two. Only the memory watcher is asserted to *start*: the other two refuse when the platform cannot supply their metric, so requiring that would pin the test environment rather than the teardown. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../activities/editor/BaseEditorActivity.kt | 5 ++ .../androidide/ui/MetricsChartRenderer.kt | 22 +++++- .../androidide/ui/PowerUsageChartRenderer.kt | 2 +- .../com/itsaky/androidide/ui/SafeLineChart.kt | 43 ++++++----- .../androidide/utils/DevicePowerSource.kt | 43 +++++++++-- .../utils/MutableShiftedLongArray.kt | 13 +++- .../androidide/utils/PowerUsageWatcher.kt | 16 ++-- .../ui/PowerUsageChartRendererTest.kt | 25 ++++++- .../utils/DevicePowerEnvelopeTest.kt | 74 +++++++++++++++++++ .../viewmodel/MetricsViewModelTest.kt | 28 ++++++- 10 files changed, 227 insertions(+), 44 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/utils/DevicePowerEnvelopeTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt index a2ea7e2f27..f8179c6780 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt @@ -569,6 +569,11 @@ abstract class BaseEditorActivity : // recreation, so it must not be torn down whenever this activity goes away. memoryUsageWatcher.listener = null networkUsageWatcher.listener = null + // The third one too. It was missed when the power page was added, and only + // metricsCarousel.unbind() a few lines above was releasing it -- under an identity + // check, and skipped entirely for an undocked carousel. Asymmetry here is what hides + // which watcher is holding a dead controller. + powerUsageWatcher.listener = null editorActivityScope.cancelIfActive("Activity is being destroyed") unbindDebuggerService() diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 2d6a66927c..3e640942e6 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -76,6 +76,14 @@ abstract class MetricsChartRenderer( */ private var userHasZoomed = false + /** + * The top inset last reserved, so an unchanged value costs nothing. + * + * [reserveTopSpace] is called from the power listener on every sample, and the height it + * reserves changes only when the readout appears or disappears or the font scale moves. + */ + private var reservedTopPixels = Float.NaN + /** * The attached chart, or `null` when no carousel page is bound to this renderer. */ @@ -103,10 +111,18 @@ abstract class MetricsChartRenderer( @UiThread fun reserveTopSpace(pixels: Float) { val chart = this.chart ?: return + if (pixels == reservedTopPixels) { + return + } + reservedTopPixels = pixels chart.setExtraTopOffset(pixels / chart.resources.displayMetrics.density) - // setExtraTopOffset only stores the value; the viewport is recomputed by calculateOffsets, - // which is protected and otherwise runs only when the chart's size changes. - chart.notifyDataSetChanged() + // setExtraTopOffset only stores the value; calculateOffsets is what turns it into a + // viewport. It is public in AndroidChart 3.1.0.21 -- an earlier comment here called it + // protected, which is why this used to go the long way round through + // notifyDataSetChanged(). That did far more work (initBuffers, calcMinMax, three + // computeAxis calls, computeLegend) and, worse, returns early when the chart has no data + // yet -- which is exactly the state at bind time, when this is first called. + chart.calculateOffsets() chart.invalidate() } diff --git a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt index 31d6948ffb..042e2f19e6 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/PowerUsageChartRenderer.kt @@ -179,7 +179,7 @@ class PowerUsageChartRenderer( val milliCelsius = usage.temperatureMilliCelsius[index] // Skip the unsampled prefix and anything the device does not report: both plot at // zero, and letting zero into the range is what flattened the real readings. - if (milliCelsius != PowerUsageWatcher.UNAVAILABLE && milliCelsius != 0L) { + if (milliCelsius != PowerUsageWatcher.UNAVAILABLE) { val celsius = milliCelsiusToCelsius(milliCelsius) hottest = max(hottest, celsius) coldest = min(coldest, celsius) diff --git a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt index 8b96943750..e9121ff366 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -61,6 +61,7 @@ class SafeLineChart : LineChart { * Drawn here rather than by the caller because the chart owns the transformer that maps an * x value to a pixel, and that mapping changes with every zoom, pan and layout. */ + @Volatile var backgroundSpans: List = emptyList() set(value) { field = value @@ -80,11 +81,6 @@ class SafeLineChart : LineChart { val color: Int, ) - private val spanPaint = Paint(Paint.ANTI_ALIAS_FLAG) - - /** Reused by [drawBackgroundSpans]: two (x, y) pairs, transformed in place. */ - private val spanPoints = FloatArray(4) - /** * Draws the spans immediately after the grid background, which is an opaque fill of the plot: a * span painted before [onDraw] delegates upwards is covered by it and never reaches the screen. @@ -96,24 +92,33 @@ class SafeLineChart : LineChart { } private fun drawBackgroundSpans(canvas: Canvas) { - if (backgroundSpans.isEmpty()) { + val spans = backgroundSpans + if (spans.isEmpty()) { return } val content = viewPortHandler.contentRect val transformer = getTransformer(YAxis.AxisDependency.LEFT) ?: return - backgroundSpans.forEach { span -> - // A reused buffer through pointValuesToPixel, not two getPixelForValues calls: those - // hand back pooled MPPointD instances that have to be recycled, and this runs inside - // onDraw for every span on every frame of every pan and zoom. - spanPoints[0] = span.startX - spanPoints[1] = 0f - spanPoints[2] = span.endX - spanPoints[3] = 0f - transformer.pointValuesToPixel(spanPoints) - val left = spanPoints[0] - val right = spanPoints[2] + // Locals, not fields. This runs inside [onDraw], and the whole reason this class exists is + // that onDraw is entered from two threads at once -- Sentry Session Replay draws the + // hierarchy off the main thread. A scratch buffer and a Paint held as fields are a data + // race on exactly the hazard the class guards: the replay thread can overwrite all four + // slots, or the colour, between the main thread's write and its read, and the band is then + // painted at another span's coordinates or in another span's hue. One array and one Paint + // per draw is still far less churn than the pooled MPPointD instances this replaced, and + // it cannot be raced. + val points = FloatArray(4) + val paint = Paint(Paint.ANTI_ALIAS_FLAG) + + spans.forEach { span -> + points[0] = span.startX + points[1] = 0f + points[2] = span.endX + points[3] = 0f + transformer.pointValuesToPixel(points) + val left = points[0] + val right = points[2] // A span scrolled out of view still maps to a pixel, so clip to the plot. val clippedLeft = left.coerceAtLeast(content.left) val clippedRight = right.coerceAtMost(content.right) @@ -121,8 +126,8 @@ class SafeLineChart : LineChart { return@forEach } - spanPaint.color = span.color - canvas.drawRect(clippedLeft, content.top, clippedRight, content.bottom, spanPaint) + paint.color = span.color + canvas.drawRect(clippedLeft, content.top, clippedRight, content.bottom, paint) } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt index c05b5d8b33..512fe62b49 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/DevicePowerSource.kt @@ -23,6 +23,7 @@ import android.content.IntentFilter import android.os.BatteryManager import android.os.Build import android.os.PowerManager +import androidx.annotation.VisibleForTesting import androidx.core.content.getSystemService import com.itsaky.androidide.services.builder.ThermalInfo import com.itsaky.androidide.services.builder.ThermalState @@ -93,12 +94,36 @@ class DevicePowerSource( return PowerUsageWatcher.UNAVAILABLE } + return microWattsOrUnavailable(microAmps, milliVolts) + } + + /** + * Turns a current and a voltage into microwatts, or [PowerUsageWatcher.UNAVAILABLE]. + * + * Separated so the envelope can be asserted: [readPower] needs a BatteryManager and a sticky + * intent, and the part worth testing is arithmetic. + */ + @VisibleForTesting + internal fun microWattsOrUnavailable( + microAmps: Int, + milliVolts: Int, + ): Long { val microWatts = microAmps.toLong() * milliVolts.toLong() / NANOWATTS_PER_MICROWATT // The sign of CURRENT_NOW is documented and not always honoured; the unit is the same - // story. Several OEM kernels report milliamps, which makes a five-watt build read as five - // milliwatts -- indistinguishable from an idle device, with no error path at all. Outside - // a plausible envelope, report the reading as unavailable rather than as a believable lie. + // story. Several OEM kernels report milliamps, which divides the reading by a thousand: + // a five-watt build then reads as five milliwatts, with no error path at all. + // + // A single sample cannot tell that apart from a genuinely tiny draw -- both are 5,000 + // microwatts -- so this is a plausibility floor, not a detector. It is set to reject the + // range a misreported build actually lands in: a real draw of 0.01W to 100W misreported as + // milliwatts gives 10 to 100,000 microwatts, and a phone running a Gradle build draws + // watts, not milliwatts. The residual gaps are stated rather than papered over: a real + // draw below MIN_PLAUSIBLE_MICROWATTS is rejected as implausible, and a misreport of a + // draw above 10W would pass -- neither happens on a phone. + // + // The earlier comment here claimed the envelope caught the milliamp case at a 1,000 + // microwatt floor. It did not: five watts misreported is 5,000, comfortably inside it. val magnitude = abs(microWatts) return if (magnitude == 0L || magnitude in MIN_PLAUSIBLE_MICROWATTS..MAX_PLAUSIBLE_MICROWATTS) { microWatts @@ -162,8 +187,16 @@ class DevicePowerSource( /** Microamps times millivolts gives nanowatts; this scales the product to microwatts. */ const val NANOWATTS_PER_MICROWATT = 1_000L - /** A milliwatt: below this a non-zero reading is likelier a unit mismatch than a real draw. */ - const val MIN_PLAUSIBLE_MICROWATTS = 1_000L + /** + * Ten milliwatts. + * + * Below this, a non-zero reading is likelier a milliamp-for-microamp kernel than a real + * draw: it is a thousandth of the 10W ceiling a phone can actually reach, so any build + * misreported this way lands under it. A device deep in doze can draw single-digit + * milliwatts, which this would reject -- acceptable, because the chart exists to show what + * a build costs and a dozing device is not running one. + */ + const val MIN_PLAUSIBLE_MICROWATTS = 10_000L /** A hundred watts: no phone draws this, so that is a unit mismatch the other way. */ const val MAX_PLAUSIBLE_MICROWATTS = 100_000_000L diff --git a/app/src/main/java/com/itsaky/androidide/utils/MutableShiftedLongArray.kt b/app/src/main/java/com/itsaky/androidide/utils/MutableShiftedLongArray.kt index 3d3417646c..6add3e6902 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MutableShiftedLongArray.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MutableShiftedLongArray.kt @@ -63,11 +63,16 @@ class MutableShiftedLongArray( fun copy(): MutableShiftedLongArray = MutableShiftedLongArray(LongArray(size) { this[it] }) /** - * Resets every element to zero and returns the shift to its starting position, so the array reads - * as though nothing had ever been recorded. + * Fills every element with [fillWith] and returns the shift to its starting position, so the + * array reads as though nothing had ever been recorded. + * + * The fill value is a parameter because zero is a measurement for some series and an absence + * for others: a memory buffer of zeros means "no memory used", while a temperature buffer of + * zeros would plot a flat 0 C line and present it as a reading (ADFA-5499). */ - fun clear() { - array.fill(0L) + @JvmOverloads + fun clear(fillWith: Long = 0L) { + array.fill(fillWith) shift = 0 } diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 4753e9e730..94a9577249 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -75,8 +75,12 @@ class PowerUsageWatcher /** Guards the ring buffers: the sampler writes them, the UI thread snapshots them. */ private val historyLock = Any() - private val temperature = MutableShiftedLongArray(MAX_USAGE_ENTRIES) - private val power = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + // Filled with UNAVAILABLE, not zero. A slot that has never been sampled is an absence, and + // zero is a reading: a zero-filled prefix plotted a flat 0 C and 0 W line and presented it + // as measurement, which then forced applyAxisRanges to special-case `!= 0L` -- discarding + // a genuine freezing-battery sample along with the fake ones. + private val temperature = MutableShiftedLongArray(MAX_USAGE_ENTRIES) { UNAVAILABLE } + private val power = MutableShiftedLongArray(MAX_USAGE_ENTRIES) { UNAVAILABLE } /** * The thermal throttling level at each sample, or [THERMAL_UNKNOWN]. @@ -84,7 +88,7 @@ class PowerUsageWatcher * Kept per sample rather than as a separate timestamped log so the chart's shading lines up * with the sample grid exactly: a shaded span is just a run of equal values here. */ - private val thermal = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + private val thermal = MutableShiftedLongArray(MAX_USAGE_ENTRIES) { UNAVAILABLE } /** * Milliseconds between samples. Changing it clears the history, for the reason given on @@ -130,9 +134,9 @@ class PowerUsageWatcher fun clearHistory() { synchronized(historyLock) { - temperature.clear() - power.clear() - thermal.clear() + temperature.clear(UNAVAILABLE) + power.clear(UNAVAILABLE) + thermal.clear(UNAVAILABLE) } } diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index 880ca7526d..0ee4a56e20 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -326,21 +326,38 @@ class PowerUsageChartRendererTest { } @Test - fun `the temperature axis ignores the buffer's unsampled zeros`() { + fun `the temperature axis ignores the buffer's unsampled slots`() { // A real reading only in the newest slots; the rest of the buffer has never been written. - val temperature = LongArray(SAMPLES) + // Unsampled now means UNAVAILABLE rather than zero -- the watcher fills its buffers with + // it, because a zero-filled prefix plotted a flat 0 C line and presented it as a reading. + val temperature = LongArray(SAMPLES) { PowerUsageWatcher.UNAVAILABLE } for (index in SAMPLES - 10 until SAMPLES) { temperature[index] = 30_000L } val (_, chart) = rendererFor(usage(temperature = temperature)) laidOut(chart) - // Ranged over the zeros the 30C band is squeezed into the top tenth of the plot, with a - // negative gridline below it. + // Ranged over the unsampled slots the 30C band is squeezed into a corner of the plot. assertThat(chart.axisLeft.axisMinimum).isGreaterThan(20f) assertThat(chart.axisLeft.axisMaximum).isLessThan(40f) } + @Test + fun `a genuine zero degrees is a reading and is ranged over`() { + // The half the old workaround got wrong. Ignoring the unsampled prefix used to be done by + // discarding every zero, which also discarded a real freezing-battery sample -- so a phone + // left in a car overnight charted its own temperature as absent. + val temperature = LongArray(SAMPLES) { PowerUsageWatcher.UNAVAILABLE } + for (index in SAMPLES - 10 until SAMPLES) { + temperature[index] = 0L + } + val (_, chart) = rendererFor(usage(temperature = temperature)) + laidOut(chart) + + // The axis has to include it rather than falling back to its default band. + assertThat(chart.axisLeft.axisMinimum).isAtMost(0f) + } + @Test fun `the battery readout gets room, and gives it back`() { val (renderer, chart) = rendererFor(usage(temperature = LongArray(SAMPLES) { 30_000L })) diff --git a/app/src/test/java/com/itsaky/androidide/utils/DevicePowerEnvelopeTest.kt b/app/src/test/java/com/itsaky/androidide/utils/DevicePowerEnvelopeTest.kt new file mode 100644 index 0000000000..04ec17ed5d --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/DevicePowerEnvelopeTest.kt @@ -0,0 +1,74 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Which battery readings the power page will believe (ADFA-5499). + * + * A kernel that reports CURRENT_NOW in milliamps rather than microamps divides every reading by a + * thousand, and a single sample cannot tell that from a genuinely tiny draw. So the envelope is a + * plausibility floor rather than a detector, and what these cases pin is that the floor is placed + * where a misreported build actually lands. The earlier floor was 1,000uW and the comment claimed + * it caught the case; five watts misreported is 5,000uW, which sailed through. + */ +@RunWith(RobolectricTestRunner::class) +class DevicePowerEnvelopeTest { + private val source = DevicePowerSource(ApplicationProvider.getApplicationContext()) + + @Test + fun `a five-watt build misreported in milliamps is rejected`() { + // The case the old floor let through. 5W at 4V is 1.25A; a milliamp kernel reports 1250 + // where microamps would say 1_250_000, so the product comes out a thousand times small. + assertThat(source.microWattsOrUnavailable(microAmps = 1_250, milliVolts = 4_000)) + .isEqualTo(PowerUsageWatcher.UNAVAILABLE) + } + + @Test + fun `the same build reported correctly is believed`() { + assertThat(source.microWattsOrUnavailable(microAmps = 1_250_000, milliVolts = 4_000)) + .isEqualTo(5_000_000L) + } + + @Test + fun `a discharging reading keeps its sign`() { + // CURRENT_NOW is negative for current leaving the battery. The envelope tests the + // magnitude; the sign survives, because the chart decides for itself what to plot. + assertThat(source.microWattsOrUnavailable(microAmps = -1_250_000, milliVolts = 4_000)) + .isEqualTo(-5_000_000L) + } + + @Test + fun `an exactly-zero reading is a reading, not an absence`() { + // A device on mains with a full battery really does draw nothing through it. + assertThat(source.microWattsOrUnavailable(microAmps = 0, milliVolts = 4_000)).isEqualTo(0L) + } + + @Test + fun `an absurdly large reading is rejected the other way`() { + // The mismatch in the opposite direction: nanoamps read as microamps. No phone draws 400W. + assertThat(source.microWattsOrUnavailable(microAmps = 100_000_000, milliVolts = 4_000)) + .isEqualTo(PowerUsageWatcher.UNAVAILABLE) + } +} diff --git a/app/src/test/java/com/itsaky/androidide/viewmodel/MetricsViewModelTest.kt b/app/src/test/java/com/itsaky/androidide/viewmodel/MetricsViewModelTest.kt index a2eb18665d..e6b0b9c6ff 100644 --- a/app/src/test/java/com/itsaky/androidide/viewmodel/MetricsViewModelTest.kt +++ b/app/src/test/java/com/itsaky/androidide/viewmodel/MetricsViewModelTest.kt @@ -17,8 +17,10 @@ package com.itsaky.androidide.viewmodel +import android.app.Application import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.ViewModelStore +import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat import org.junit.Test import org.junit.runner.RunWith @@ -38,15 +40,33 @@ class MetricsViewModelTest { private val store = ViewModelStore() private fun viewModel(): MetricsViewModel { - val provider = ViewModelProvider(store, ViewModelProvider.NewInstanceFactory()) + // AndroidViewModelFactory, not NewInstanceFactory: MetricsViewModel became an + // AndroidViewModel when the power page needed a Context for the battery broadcast, and + // NewInstanceFactory reflects on a no-arg constructor that no longer exists. This class + // has been failing with "Cannot create an instance of class MetricsViewModel" ever since, + // which nothing noticed because the only CI job that runs unit tests runs them with + // ignoreFailures set (ADFA-5559). + val application = ApplicationProvider.getApplicationContext() + val provider = ViewModelProvider(store, ViewModelProvider.AndroidViewModelFactory(application)) return provider[MetricsViewModel::class.java] } @Test - fun `clearing the view model closes both watchers for good`() { + fun `clearing the view model closes every watcher for good`() { val model = viewModel() + // All three, not two. The power watcher was added later and left out of this case, so its + // close() -- and the sampling thread it owns -- was unasserted. Spelled out rather than + // looped: the three watchers share no supertype that exposes isWatching. model.memoryUsageWatcher.startWatching() model.networkUsageWatcher.startWatching() + model.powerUsageWatcher.startWatching() + + // Only the memory watcher is asserted to have started. The other two refuse when the + // platform cannot supply their metric -- TrafficStats and the battery properties are both + // unsupported off a device -- so requiring them to start here would pin the test + // environment rather than the teardown. What the terminal property needs is that clear() + // stops whatever was running and that nothing restarts afterwards, which is asserted for + // all three below. assertThat(model.memoryUsageWatcher.isWatching).isTrue() cleared() @@ -55,11 +75,14 @@ class MetricsViewModelTest { // to restart, which is what makes this the terminal teardown rather than a pause. assertThat(model.memoryUsageWatcher.isWatching).isFalse() assertThat(model.networkUsageWatcher.isWatching).isFalse() + assertThat(model.powerUsageWatcher.isWatching).isFalse() model.memoryUsageWatcher.startWatching() model.networkUsageWatcher.startWatching() + model.powerUsageWatcher.startWatching() assertThat(model.memoryUsageWatcher.isWatching).isFalse() assertThat(model.networkUsageWatcher.isWatching).isFalse() + assertThat(model.powerUsageWatcher.isWatching).isFalse() } @Test @@ -70,6 +93,7 @@ class MetricsViewModelTest { // back a new watcher per read would quietly defeat that. assertThat(model.memoryUsageWatcher).isSameInstanceAs(model.memoryUsageWatcher) assertThat(model.networkUsageWatcher).isSameInstanceAs(model.networkUsageWatcher) + assertThat(model.powerUsageWatcher).isSameInstanceAs(model.powerUsageWatcher) assertThat(model.annotations).isSameInstanceAs(model.annotations) } }