From ec285377f7efa286cf336beb4553e69a8a885849 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 01:02:57 -0700 Subject: [PATCH 1/7] feat(metrics): annotate build start, finish and failure on the charts (ADFA-5509) Three markers, drawn in the theme's semantic colours: colorSuccess for a build starting and finishing, colorError for one that failed, and the existing colorOnSurface for the Gradle task markers around them. Both the line and its label take the colour; colouring only the line leaves the label unreadable against a coloured rule. The wiring is three calls into hooks that already existed -- prepareBuild, onBuildSuccessful and onBuildFailed -- but two things had to change first, and one of them is the whole point of the ticket. The throttle would have eaten them. The store keeps one annotation every five seconds, because Gradle emits dozens of task events a second, and it keeps the first of each quiet period. A build failure arriving two seconds after a task marker would therefore have been dropped -- the one annotation on the chart actually worth having. Annotations now carry a kind, and only task events are throttled. A build marker also restarts the window, so the next task marker waits its five seconds instead of landing a few pixels away and colliding. Colour had to become per-annotation. applyAnnotations resolved one colour and painted every limit line with it, so the kind is carried through to the renderer and mapped there. The labels are user-facing text, so they are string resources in :resources, unlike the task markers which are raw Gradle task display names. Verified on a Pixel 6 Pro against the sample project: a green dashed marker labelled "Build started" when a build begins, and -- with a deliberate syntax error pushed into MainActivity.kt -- a red marker beside it when the build fails. The source was restored and confirmed byte-identical afterwards. Not verified on the device: the "Build finished" marker. It shares its code path and colour with "Build started", and the tests pin that the two resolve to one colour while a failure resolves to another, but the device check was defeated by the install prompt that follows a successful build: returning from it parks the chart's viewport at the oldest samples in the buffer for a sample or two, and the marker had scrolled out of the window by the time the chart recovered. That viewport glitch is a pre-existing bug of the same family as the rotation and floating-window cases fixed under ADFA-5486, and is being reported separately rather than folded in here. Co-Authored-By: Claude Opus 5 --- .../activities/editor/BaseEditorActivity.kt | 26 +++++++- .../handlers/EditorBuildEventListener.kt | 9 ++- .../androidide/ui/MetricsChartRenderer.kt | 28 ++++++++- .../utils/MetricsAnnotationStore.kt | 44 ++++++++++++- .../ui/MetricsAnnotationRenderingTest.kt | 43 ++++++++++++- .../utils/MetricsAnnotationStoreTest.kt | 62 +++++++++++++++++++ resources/src/main/res/values/strings.xml | 4 ++ 7 files changed, 207 insertions(+), 9 deletions(-) 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 903e5f728f..de54574c05 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 @@ -129,6 +129,7 @@ import com.itsaky.androidide.utils.FlashType import com.itsaky.androidide.utils.InstallationResultHandler.onResult import com.itsaky.androidide.utils.IntentUtils import com.itsaky.androidide.utils.MemoryUsageWatcher +import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.StringsInjectionException import com.itsaky.androidide.utils.StringsXmlInjector import com.itsaky.androidide.utils.applyBottomSheetAnchorForOrientation @@ -209,8 +210,29 @@ abstract class BaseEditorActivity : } /** Records a significant event for the charts to annotate (ADFA-5486). */ - fun recordMetricsAnnotation(label: String) { - metricsViewModel.annotations.record(label) + fun recordMetricsAnnotation( + label: String, + kind: MetricsAnnotationStore.Kind = MetricsAnnotationStore.Kind.TASK, + ) { + metricsViewModel.annotations.record(label, kind) + } + + /** + * Marks a build outcome on the charts (ADFA-5509). + * + * Separate from [recordMetricsAnnotation] so the caller names the outcome rather than repeating + * the string lookup, and so these are never accidentally recorded as ordinary task markers -- + * which the throttle is allowed to drop. + */ + fun recordBuildAnnotation(kind: MetricsAnnotationStore.Kind) { + val label = + when (kind) { + MetricsAnnotationStore.Kind.BUILD_STARTED -> string.metrics_annotation_build_started + MetricsAnnotationStore.Kind.BUILD_FINISHED -> string.metrics_annotation_build_finished + MetricsAnnotationStore.Kind.BUILD_FAILED -> string.metrics_annotation_build_failed + MetricsAnnotationStore.Kind.TASK -> return + } + metricsViewModel.annotations.record(getString(label), kind) } private val fileManagerViewModel by viewModels() diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index 53d78988d5..946246d8cd 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -30,6 +30,7 @@ import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.configuration.ProjectConfigurationStartEvent import com.itsaky.androidide.tooling.events.task.TaskFinishEvent import com.itsaky.androidide.tooling.events.task.TaskStartEvent +import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.viewmodel.BuildOutputViewModel @@ -79,7 +80,9 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { - checkActivity("prepareBuild") ?: return + val prepared = checkActivity("prepareBuild") ?: return + + prepared.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) pluginBuildService?.setBuildInProgress(true) @@ -113,6 +116,8 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildSuccessful(tasks: List) { val act = checkActivity("onBuildSuccessful") ?: return + act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) + pluginBuildService?.notifyBuildFinished() analyzeCurrentFile() @@ -158,6 +163,8 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildFailed(tasks: List) { val act = checkActivity("onBuildFailed") ?: return + act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FAILED) + analyzeCurrentFile() GeneralPreferences.isFirstBuild = false act.editorViewModel.isBuildInProgress = false 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 15085b3b2e..abe2484e20 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -413,8 +413,6 @@ abstract class MetricsChartRenderer( val interval = sampleIntervalMillis() val bufferSpanMillis = (newestIndex.toLong() + 1L) * interval val now = nowMillis() - val markerColor = chart.context.resolveAttr(R.attr.colorOnSurface) - store.recentAnnotations(bufferSpanMillis).forEach { annotation -> val samplesAgo = (now - annotation.atMillis).toFloat() / interval val x = newestIndex - samplesAgo @@ -424,6 +422,7 @@ abstract class MetricsChartRenderer( chart.xAxis.addLimitLine( LimitLine(x, annotation.label).apply { + val markerColor = markerColorFor(chart, annotation.kind) lineWidth = ANNOTATION_LINE_WIDTH lineColor = markerColor textColor = markerColor @@ -437,6 +436,31 @@ abstract class MetricsChartRenderer( } } + /** + * The colour a marker is drawn in, from the kind of event it marks (ADFA-5509). + * + * Build outcomes are the events a user came to the chart for, so they get the theme's semantic + * colours -- success for a build starting or finishing, error for one that failed -- while the + * task markers that surround them stay in the ordinary text colour. Both the line and the label + * take it; colouring only the line would leave the label unreadable against a coloured rule. + */ + private fun markerColorFor( + chart: SafeLineChart, + kind: MetricsAnnotationStore.Kind, + ): Int { + val attr = + when (kind) { + MetricsAnnotationStore.Kind.BUILD_STARTED, + MetricsAnnotationStore.Kind.BUILD_FINISHED, + -> R.attr.colorSuccess + + MetricsAnnotationStore.Kind.BUILD_FAILED -> R.attr.colorError + + MetricsAnnotationStore.Kind.TASK -> R.attr.colorOnSurface + } + return chart.context.resolveAttr(attr) + } + /** * The row an annotation's label sits on, cycling so that neighbours never share one. */ 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 ae90870f42..8892058b80 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -46,6 +46,34 @@ class MetricsAnnotationStore( /** Hands each annotation its [Annotation.sequence]. */ private var nextSequence: Long = 0L + /** + * What kind of event an annotation marks, which decides both how it is drawn and whether the + * throttle applies to it (ADFA-5509). + */ + enum class Kind { + /** A Gradle task starting or finishing. Throttled: Gradle emits dozens a second. */ + TASK, + + /** A build beginning. */ + BUILD_STARTED, + + /** A build completing successfully. */ + BUILD_FINISHED, + + /** A build failing. */ + BUILD_FAILED, + ; + + /** + * Whether the throttle may drop this kind. + * + * Only task events. A build outcome dropped because a task marker happened to land two + * seconds earlier would be the one annotation on the chart worth having. + */ + val isThrottled: Boolean + get() = this == TASK + } + /** * An annotated moment. * @@ -65,23 +93,33 @@ class MetricsAnnotationStore( * makes consecutive annotations differ, which is when a collision is likeliest. */ val sequence: Long, + /** Decides the marker's colour, and whether the throttle could have dropped it. */ + val kind: Kind = Kind.TASK, ) /** * Records [label] unless another annotation was recorded within [THROTTLE_INTERVAL_MS]. * + * The throttle only applies to [Kind.TASK]; a build outcome is always kept. See + * [Kind.isThrottled]. + * * @return whether it was recorded. */ @Synchronized - fun record(label: String): Boolean { + fun record( + label: String, + kind: Kind = Kind.TASK, + ): Boolean { val now = nowMillis() val since = lastRecordedAt - if (since != null && now - since < THROTTLE_INTERVAL_MS) { + if (kind.isThrottled && since != null && now - since < THROTTLE_INTERVAL_MS) { return false } + // Set even for an unthrottled kind, so the next task marker waits its interval instead of + // landing a few pixels from a build marker and colliding with it. lastRecordedAt = now - annotations.addLast(Annotation(now, label, nextSequence++)) + annotations.addLast(Annotation(now, label, nextSequence++, kind)) while (annotations.size > MAX_ANNOTATIONS) { annotations.removeFirst() } diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt index 98ccf47ea7..9c6176c102 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.ui import android.content.Context +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 @@ -35,7 +36,13 @@ import org.robolectric.RobolectricTestRunner */ @RunWith(RobolectricTestRunner::class) class MetricsAnnotationRenderingTest { - private val context = ApplicationProvider.getApplicationContext() + // Themed: the marker colours come from theme attributes, and against a bare application + // context every one of them resolves to 0, so a colour test would pass by comparing nothing. + private val context: Context = + ContextThemeWrapper( + ApplicationProvider.getApplicationContext(), + com.itsaky.androidide.R.style.Theme_AndroidIDE, + ) /** A minimal renderer, so the placement is tested without a particular page's data. */ private class TestRenderer( @@ -144,6 +151,40 @@ class MetricsAnnotationRenderingTest { assertThat(rowsOf(chart).single()).isEqualTo(newestRowBefore) } + @Test + fun `a failed build is drawn in a different colour from a task marker`() { + val fixture = Fixture() + fixture.store.record("some task") + fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + fixture.store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + + val (_, chart) = render(fixture) + + val lines = chart.xAxis.limitLines + assertThat(lines).hasSize(2) + assertThat(lines[1].lineColor).isNotEqualTo(lines[0].lineColor) + // The label sits on the line, so colouring only the line would leave it unreadable. + assertThat(lines[1].textColor).isEqualTo(lines[1].lineColor) + } + + @Test + fun `a build starting and finishing share one colour, distinct from a failure`() { + val fixture = Fixture() + fixture.store.record("Build started", MetricsAnnotationStore.Kind.BUILD_STARTED) + fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + fixture.store.record("Build finished", MetricsAnnotationStore.Kind.BUILD_FINISHED) + fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + fixture.store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + + val (_, chart) = render(fixture) + + val lines = chart.xAxis.limitLines + assertThat(lines).hasSize(3) + // Started and finished are both outcomes worth seeing; only failure is bad news. + assertThat(lines[1].lineColor).isEqualTo(lines[0].lineColor) + assertThat(lines[2].lineColor).isNotEqualTo(lines[0].lineColor) + } + private companion object { const val SAMPLE_INTERVAL_MS = 1_000L const val SAMPLE_COUNT = 60 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 1e672ab2dd..0b1f0c921a 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt @@ -143,4 +143,66 @@ class MetricsAnnotationStoreTest { assertThat(store.recentAnnotations(60_000L).map { it.sequence }).containsExactly(0L) } + + @Test + fun `a build outcome is kept even inside the throttle window`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + store.record("some task") + // Well inside the window that drops a task marker. + now += 1_000L + store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + + // Dropped, this would be the one annotation on the chart worth having. + assertThat(store.recentAnnotations(60_000L).map { it.label }) + .containsExactly("some task", "Build failed") + .inOrder() + } + + @Test + fun `a task marker inside the window is still dropped`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + store.record("first task") + now += 1_000L + store.record("second task") + + // Guards the test above: the bypass must be for build outcomes only. + assertThat(store.recentAnnotations(60_000L).map { it.label }).containsExactly("first task") + } + + @Test + fun `a build outcome restarts the throttle window`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + store.record("Build started", MetricsAnnotationStore.Kind.BUILD_STARTED) + now += 1_000L + store.record("a task right behind it") + + // Otherwise the first task marker lands a few pixels from the build marker and collides. + assertThat(store.recentAnnotations(60_000L).map { it.label }).containsExactly("Build started") + } + + @Test + fun `the kind survives to the reader`() { + val store = MetricsAnnotationStore(nowMillis = { now }) + + store.record("Build started", MetricsAnnotationStore.Kind.BUILD_STARTED) + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + + // The renderer colours by kind, so it has to arrive intact. + assertThat(store.recentAnnotations(60_000L).map { it.kind }) + .containsExactly( + MetricsAnnotationStore.Kind.BUILD_STARTED, + MetricsAnnotationStore.Kind.BUILD_FAILED, + ).inOrder() + } + + @Test + fun `only task markers are throttled`() { + assertThat(MetricsAnnotationStore.Kind.TASK.isThrottled).isTrue() + assertThat(MetricsAnnotationStore.Kind.entries.filter { it.isThrottled }) + .containsExactly(MetricsAnnotationStore.Kind.TASK) + } } diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 5a4a57ae9e..6e77ed19ae 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1683,6 +1683,10 @@ Every %1$s %1$s (needs a 64-bit device) Temperature and power + + Build started + Build finished + Build failed Temperature and power chart Battery temp Power From c0a1c1606bb248ba37ac35fca21b36dd8218b090 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 04:53:18 -0700 Subject: [PATCH 2/7] fix(metrics): stop annotating syncs and cancels as builds (ADFA-5509) Five review findings, four of them behaviour a user would have seen. Opening a project drew a build. prepareBuild and onBuildSuccessful also fire for project initialization, which runs no tasks, so merely opening a project stamped a green "Build started" and "Build finished" pair on the charts -- and blamed the sync's own memory spike on a build nobody asked for. BuildInfo.tasks and the result's task list already distinguish the two; a sync now annotates nothing. A cancelled build was reported as a failure. The tooling API surfaces a cancel through onBuildFailed, so stopping a build yourself drew a red "Build failed" rule and left it on the chart for the next hour. GradleBuildService now tells the listener when a cancel is requested -- a defaulted interface method, since only a listener that cares needs it -- and a cancel gets its own kind, drawn in the ordinary text colour because it is neither good news nor bad. The colour tests could not have caught a mistake in the colours. All three asserted only that two resolved colours were equal or unequal, so swapping success and error passed every one of them while the chart told the user a failed build had succeeded. They now assert which attribute each kind resolves to, and were confirmed to fail with the two swapped. Labels moved onto the kind as string ids, which removes three problems at once: the label is resolved at draw time, so markers follow the system language rather than freezing the language they were recorded in -- the store lives in a ViewModel that outlives the activity; recordBuildAnnotation no longer needs a `when` with a silent `return` for the one kind it cannot label; and recordMetricsAnnotation loses the dead `kind` parameter that let any caller give a task name an unthrottled marker in the error colour. Marker colours are resolved once per redraw instead of once per annotation. applyAnnotations runs on every sampling tick across three charts and can hold MAX_ANNOTATIONS markers, and resolveAttr allocates a TypedValue per call. Also: prepareBuild now uses the checked local for its whole body rather than re-reading the WeakReference through the throwing `activity` property, which is what its four sibling handlers already do. Not changed: build start and finish still share one colour. A reviewer argued they should differ so the two ends of a build are distinguishable at a glance, which is a fair point, but green for both was an explicit product decision and the labels already differ. Co-Authored-By: Claude Opus 5 --- .../activities/editor/BaseEditorActivity.kt | 22 +++----- .../handlers/EditorBuildEventListener.kt | 50 +++++++++++++++---- .../services/builder/GradleBuildService.kt | 12 +++++ .../androidide/ui/MetricsChartRenderer.kt | 25 ++++++++-- .../utils/MetricsAnnotationStore.kt | 35 +++++++++++-- .../ui/MetricsAnnotationRenderingTest.kt | 43 ++++++++++++++-- resources/src/main/res/values/strings.xml | 1 + 7 files changed, 151 insertions(+), 37 deletions(-) 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 de54574c05..f34796e79e 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 @@ -210,29 +210,19 @@ abstract class BaseEditorActivity : } /** Records a significant event for the charts to annotate (ADFA-5486). */ - fun recordMetricsAnnotation( - label: String, - kind: MetricsAnnotationStore.Kind = MetricsAnnotationStore.Kind.TASK, - ) { - metricsViewModel.annotations.record(label, kind) + fun recordMetricsAnnotation(label: String) { + metricsViewModel.annotations.record(label) } /** * Marks a build outcome on the charts (ADFA-5509). * - * Separate from [recordMetricsAnnotation] so the caller names the outcome rather than repeating - * the string lookup, and so these are never accidentally recorded as ordinary task markers -- - * which the throttle is allowed to drop. + * Separate from [recordMetricsAnnotation] so a build outcome cannot be recorded as an ordinary + * task marker, which the throttle is allowed to drop -- and so a task name cannot be recorded + * as an outcome, which would give it an unthrottled marker in the error colour. */ fun recordBuildAnnotation(kind: MetricsAnnotationStore.Kind) { - val label = - when (kind) { - MetricsAnnotationStore.Kind.BUILD_STARTED -> string.metrics_annotation_build_started - MetricsAnnotationStore.Kind.BUILD_FINISHED -> string.metrics_annotation_build_finished - MetricsAnnotationStore.Kind.BUILD_FAILED -> string.metrics_annotation_build_failed - MetricsAnnotationStore.Kind.TASK -> return - } - metricsViewModel.annotations.record(getString(label), kind) + metricsViewModel.annotations.recordBuild(kind) } private val fileManagerViewModel by viewModels() diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index 946246d8cd..206918f423 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -48,6 +48,12 @@ class EditorBuildEventListener : GradleBuildService.EventListener { private var buildStartTimeMs: Long = System.currentTimeMillis() private var lastOutputTimeMs: Long = SystemClock.elapsedRealtime() + /** + * Set when the user asks for the running build to stop, so [onBuildFailed] can tell a cancel + * from a real failure. Cleared as each build is prepared. + */ + private var cancelRequested = false + private var enabled = true private var activityReference: WeakReference = WeakReference(null) @@ -80,30 +86,37 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { - val prepared = checkActivity("prepareBuild") ?: return + val act = checkActivity("prepareBuild") ?: return - prepared.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) + cancelRequested = false + + // A project sync runs through the same callbacks with no tasks, so annotating every + // prepareBuild put a "Build started" marker on the chart merely for opening a project -- + // and blamed the sync's own memory spike on a build the user never ran. + if (buildInfo.tasks.isNotEmpty()) { + act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) + } pluginBuildService?.setBuildInProgress(true) val isFirstBuild = GeneralPreferences.isFirstBuild - activity + act .setStatus( - activity.getString(if (isFirstBuild) string.preparing_first else string.preparing), + act.getString(if (isFirstBuild) string.preparing_first else string.preparing), ) if (isFirstBuild) { - activity.showFirstBuildNotice() + act.showFirstBuildNotice() } resetBuildTimers() - activity.editorViewModel.isBuildInProgress = true - activity.content.bottomSheet.clearBuildOutput() + act.editorViewModel.isBuildInProgress = true + act.content.bottomSheet.clearBuildOutput() if (buildInfo.tasks.isNotEmpty()) { onOutput( - activity.getString(R.string.title_run_tasks) + " : " + buildInfo.tasks, + act.getString(R.string.title_run_tasks) + " : " + buildInfo.tasks, ) } } @@ -116,7 +129,9 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildSuccessful(tasks: List) { val act = checkActivity("onBuildSuccessful") ?: return - act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) + if (tasks.isNotEmpty()) { + act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) + } pluginBuildService?.notifyBuildFinished() @@ -145,6 +160,10 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastStatusLine = "" } + override fun onBuildCancelRequested() { + cancelRequested = true + } + override fun onProgressEvent(event: ProgressEvent) { val act = checkActivity("onProgressEvent") ?: return @@ -163,7 +182,18 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildFailed(tasks: List) { val act = checkActivity("onBuildFailed") ?: return - act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FAILED) + if (tasks.isNotEmpty()) { + // A build the user stopped arrives through this same callback. Marking it as a failure + // would report their own deliberate action back to them in the error colour. + act.recordBuildAnnotation( + if (cancelRequested) { + MetricsAnnotationStore.Kind.BUILD_CANCELLED + } else { + MetricsAnnotationStore.Kind.BUILD_FAILED + }, + ) + } + cancelRequested = false analyzeCurrentFile() GeneralPreferences.isFirstBuild = false diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index 1182029806..c116e5b19a 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -632,6 +632,9 @@ class GradleBuildService : override fun cancelCurrentBuild(): CompletableFuture { checkServerStarted() + // Before delegating: the cancellation surfaces as a build failure, and the listener needs + // to know it was asked for rather than reporting the user's own action as an error. + eventListener?.onBuildCancelRequested() return server!!.cancelCurrentBuild() } @@ -807,6 +810,15 @@ class GradleBuildService : /** Handles events received from a Gradle build. */ interface EventListener { + /** + * Called when the user asks for the running build to stop. + * + * The tooling API reports a cancelled build through [onBuildFailed], so a listener that + * wants to tell the two apart has to be told here. Defaulted, because only a listener that + * cares about the distinction needs it. + */ + fun onBuildCancelRequested() = Unit + /** * Called just before a build is started. * 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 abe2484e20..6ecbb9a9d2 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -413,6 +413,11 @@ abstract class MetricsChartRenderer( val interval = sampleIntervalMillis() val bufferSpanMillis = (newestIndex.toLong() + 1L) * interval val now = nowMillis() + // Resolved once per redraw rather than once per annotation: applyAnnotations runs on every + // sampling tick, there can be MAX_ANNOTATIONS of them, and resolveAttr allocates a + // TypedValue per call. + val markerColors = MetricsAnnotationStore.Kind.entries.associateWith { markerColorFor(chart, it) } + store.recentAnnotations(bufferSpanMillis).forEach { annotation -> val samplesAgo = (now - annotation.atMillis).toFloat() / interval val x = newestIndex - samplesAgo @@ -421,8 +426,8 @@ abstract class MetricsChartRenderer( } chart.xAxis.addLimitLine( - LimitLine(x, annotation.label).apply { - val markerColor = markerColorFor(chart, annotation.kind) + LimitLine(x, labelFor(chart, annotation)).apply { + val markerColor = markerColors.getValue(annotation.kind) lineWidth = ANNOTATION_LINE_WIDTH lineColor = markerColor textColor = markerColor @@ -436,6 +441,17 @@ abstract class MetricsChartRenderer( } } + /** + * An annotation's label, resolved now rather than when it was recorded. + * + * A build outcome carries a string id instead of text, so its marker follows the system + * language even though the store holding it outlives the activity that recorded it. + */ + private fun labelFor( + chart: SafeLineChart, + annotation: MetricsAnnotationStore.Annotation, + ): String = annotation.kind.labelRes?.let(chart.context::getString) ?: annotation.label + /** * The colour a marker is drawn in, from the kind of event it marks (ADFA-5509). * @@ -456,7 +472,10 @@ abstract class MetricsChartRenderer( MetricsAnnotationStore.Kind.BUILD_FAILED -> R.attr.colorError - MetricsAnnotationStore.Kind.TASK -> R.attr.colorOnSurface + // A cancel is the user's own doing, so it is neither good news nor bad. + MetricsAnnotationStore.Kind.BUILD_CANCELLED, + MetricsAnnotationStore.Kind.TASK, + -> R.attr.colorOnSurface } return chart.context.resolveAttr(attr) } 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 8892058b80..77b220762c 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -18,6 +18,8 @@ package com.itsaky.androidide.utils import android.os.SystemClock +import androidx.annotation.StringRes +import com.itsaky.androidide.resources.R.string /** * Records significant events for the metrics charts to annotate (ADFA-5486). @@ -50,18 +52,34 @@ class MetricsAnnotationStore( * What kind of event an annotation marks, which decides both how it is drawn and whether the * throttle applies to it (ADFA-5509). */ - enum class Kind { + enum class Kind( + /** + * The label for this kind, or `null` for [TASK], whose label is the Gradle task's own name. + * + * A resource id rather than resolved text: the store lives in a ViewModel that outlives an + * activity, so a label resolved at record time would keep the old language after the system + * locale changes. Holding the id also removes the only reason a caller had to know which + * string went with which kind. + */ + @StringRes val labelRes: Int?, + ) { /** A Gradle task starting or finishing. Throttled: Gradle emits dozens a second. */ - TASK, + TASK(labelRes = null), /** A build beginning. */ - BUILD_STARTED, + BUILD_STARTED(string.metrics_annotation_build_started), /** A build completing successfully. */ - BUILD_FINISHED, + BUILD_FINISHED(string.metrics_annotation_build_finished), /** A build failing. */ - BUILD_FAILED, + BUILD_FAILED(string.metrics_annotation_build_failed), + + /** + * A build stopped by the user. Not a failure: the platform reports a cancel through the + * same failure callback, and painting a deliberate stop in the error colour misreports it. + */ + BUILD_CANCELLED(string.metrics_annotation_build_cancelled), ; /** @@ -97,6 +115,13 @@ class MetricsAnnotationStore( val kind: Kind = Kind.TASK, ) + /** + * Records a build outcome. Its label comes from [Kind.labelRes], so the caller names the + * outcome and nothing else. + */ + @Synchronized + fun recordBuild(kind: Kind): Boolean = record(label = "", kind = kind) + /** * Records [label] unless another annotation was recorded within [THROTTLE_INTERVAL_MS]. * diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt index 9c6176c102..308173aa2b 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt @@ -23,7 +23,10 @@ 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 com.itsaky.androidide.utils.MetricsAnnotationStore +import com.itsaky.androidide.utils.resolveAttr import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -162,7 +165,10 @@ class MetricsAnnotationRenderingTest { val lines = chart.xAxis.limitLines assertThat(lines).hasSize(2) - assertThat(lines[1].lineColor).isNotEqualTo(lines[0].lineColor) + // Which colour, not merely a different one: asserting inequality alone passes just as + // happily with the two attributes swapped, telling the user a failed build succeeded. + assertThat(lines[0].lineColor).isEqualTo(context.resolveAttr(R.attr.colorOnSurface)) + assertThat(lines[1].lineColor).isEqualTo(context.resolveAttr(R.attr.colorError)) // The label sits on the line, so colouring only the line would leave it unreadable. assertThat(lines[1].textColor).isEqualTo(lines[1].lineColor) } @@ -181,8 +187,39 @@ class MetricsAnnotationRenderingTest { val lines = chart.xAxis.limitLines assertThat(lines).hasSize(3) // Started and finished are both outcomes worth seeing; only failure is bad news. - assertThat(lines[1].lineColor).isEqualTo(lines[0].lineColor) - assertThat(lines[2].lineColor).isNotEqualTo(lines[0].lineColor) + assertThat(lines[0].lineColor).isEqualTo(context.resolveAttr(R.attr.colorSuccess)) + assertThat(lines[1].lineColor).isEqualTo(context.resolveAttr(R.attr.colorSuccess)) + assertThat(lines[2].lineColor).isEqualTo(context.resolveAttr(R.attr.colorError)) + } + + @Test + fun `a cancelled build is not drawn as a failure`() { + val fixture = Fixture() + fixture.store.recordBuild(MetricsAnnotationStore.Kind.BUILD_CANCELLED) + + val (_, chart) = render(fixture) + + // The user stopped the build themselves; reporting that back in the error colour reads as + // something having gone wrong. + val line = chart.xAxis.limitLines.single() + assertThat(line.lineColor).isNotEqualTo(context.resolveAttr(R.attr.colorError)) + assertThat(line.lineColor).isEqualTo(context.resolveAttr(R.attr.colorOnSurface)) + } + + @Test + fun `a build marker takes its label from its kind, not from the recorded text`() { + val fixture = Fixture() + fixture.store.recordBuild(MetricsAnnotationStore.Kind.BUILD_FAILED) + + val (_, chart) = render(fixture) + + // Resolved at draw time, so the marker follows the system language even though the store + // outlives the activity that recorded it. + assertThat( + chart.xAxis.limitLines + .single() + .label, + ).isEqualTo(context.getString(string.metrics_annotation_build_failed)) } private companion object { diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 6e77ed19ae..5941177d9e 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1687,6 +1687,7 @@ Build started Build finished Build failed + Build cancelled Temperature and power chart Battery temp Power From 531c0722962e9134201326b124fd484aab4f7826 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 09:58:55 -0700 Subject: [PATCH 3/7] ADFA-5509: address review findings on the build annotations Eviction now sacrifices task markers before build outcomes. Plain oldest-first eviction dropped a build's own "Build started" while the build was still running: 256 annotations at one per five seconds is about twenty minutes, which a clean build on a phone can exceed. That left an unpaired outcome on the chart and no way to see how long the build took -- which is most of the point of ADFA-5509. Task markers are the padding; the build's moments are the signal. A build marker can no longer come out invisible. resolveAttr discards resolveAttribute's result and hands back TypedValue.data, which for an attribute the theme does not carry is 0 -- fully transparent. colorSuccess is ours rather than Material's, and a floating window is built against a window context whose theme is not the activity's, so this is the same shape as the black-on-black axis labels: correct in every test, invisible on the device. It falls back to the axis text colour, which configure has already set to something legible. The "only task markers are throttled" test asserted isThrottled against its own definition, so it would have passed just as happily with record() ignoring the flag. It now records a task marker and then every build outcome inside the throttle window, and checks all of them survive. Plus both eviction branches, including the one where nothing but outcomes is left. Also refreshes the class KDoc, which still described the store as holding task starts and stops only. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsChartRenderer.kt | 21 +++++++- .../utils/MetricsAnnotationStore.kt | 33 +++++++++--- .../utils/MetricsAnnotationStoreTest.kt | 53 +++++++++++++++++-- 3 files changed, 96 insertions(+), 11 deletions(-) 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 050039879d..1c94736199 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -17,8 +17,10 @@ package com.itsaky.androidide.ui +import android.content.Context import android.graphics.Bitmap import android.os.SystemClock +import android.util.TypedValue import android.view.MotionEvent import androidx.annotation.CallSuper import androidx.annotation.UiThread @@ -508,7 +510,24 @@ abstract class MetricsChartRenderer( MetricsAnnotationStore.Kind.TASK, -> R.attr.colorOnSurface } - return chart.context.resolveAttr(attr) + // Not plain resolveAttr: it discards resolveAttribute's result and hands back TypedValue.data, + // which for an attribute the theme does not carry is 0 -- transparent. colorSuccess is + // ours rather than Material's, and a floating window is built against a window context + // whose theme is not the activity's, so a build marker could come out invisible. It falls + // back to the axis text colour, which configure has already set to something legible. + return chart.context.resolveColorAttr(attr, fallback = chart.xAxis.textColor) + } + + /** + * The colour [attr] names in this context's theme, or [fallback] if the theme has no such + * attribute. + */ + private fun Context.resolveColorAttr( + attr: Int, + fallback: Int, + ): Int { + val value = TypedValue() + return if (theme.resolveAttribute(attr, value, true)) value.data else fallback } /** 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 77b220762c..10394507be 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -24,10 +24,12 @@ import com.itsaky.androidide.resources.R.string /** * Records significant events for the metrics charts to annotate (ADFA-5486). * - * Significant means Gradle task starts and stops. A real build emits far too many of those to draw - * -- dozens a second during configuration -- so they are throttled to at most one every - * [THROTTLE_INTERVAL_MS]. The first event in a quiet period is the one kept, since the interesting - * moment is when work *began*, not an arbitrary one from the middle of a burst. + * Significant means Gradle task starts and stops, and a build's own start and outcome + * (ADFA-5509). A real build emits far too many task events to draw -- dozens a second during + * configuration -- so those are throttled to at most one every [THROTTLE_INTERVAL_MS]. The first + * event in a quiet period is the one kept, since the interesting moment is when work *began*, not + * an arbitrary one from the middle of a burst. Build outcomes are never throttled and are the last + * thing evicted; see [Kind.isThrottled] and [record]. * * Annotations are stored by wall-clock time rather than by sample position, because the charts hold * a ring buffer whose contents shift under them; a stored index would drift. The renderer converts @@ -145,10 +147,29 @@ class MetricsAnnotationStore( // landing a few pixels from a build marker and colliding with it. lastRecordedAt = now annotations.addLast(Annotation(now, label, nextSequence++, kind)) + evictToCapacity() + return true + } + + /** + * Drops the oldest annotations until the store is back within [MAX_ANNOTATIONS]. + * + * Task markers go first, whatever their age. Plain oldest-first eviction dropped a build's + * "Build started" while the build was still running -- 256 markers at one per + * [THROTTLE_INTERVAL_MS] is about twenty minutes, which a clean build on a phone can exceed -- + * leaving an unpaired outcome on the chart and no way to see how long the build took. Task + * markers are the padding here; the build's own moments are the point. + */ + private fun evictToCapacity() { while (annotations.size > MAX_ANNOTATIONS) { - annotations.removeFirst() + val oldestTask = annotations.indexOfFirst { it.kind.isThrottled } + if (oldestTask >= 0) { + annotations.removeAt(oldestTask) + } else { + // Nothing but build outcomes left, so the oldest of those has to go. + annotations.removeFirst() + } } - return true } /** 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 0b1f0c921a..26821a9256 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsAnnotationStoreTest.kt @@ -200,9 +200,54 @@ class MetricsAnnotationStoreTest { } @Test - fun `only task markers are throttled`() { - assertThat(MetricsAnnotationStore.Kind.TASK.isThrottled).isTrue() - assertThat(MetricsAnnotationStore.Kind.entries.filter { it.isThrottled }) - .containsExactly(MetricsAnnotationStore.Kind.TASK) + fun `a build outcome inside the throttle window is still recorded`() { + // Asserting isThrottled against its own definition, as this test used to, would pass just + // as happily with record() ignoring the flag altogether. + store.record("a task") + now += 1_000L + + MetricsAnnotationStore.Kind.entries + .filterNot { it == MetricsAnnotationStore.Kind.TASK } + .forEach { kind -> + assertThat(store.recordBuild(kind)).isTrue() + now += 1_000L + } + + // One task marker, then every build outcome, none of them dropped. + assertThat(store.recentAnnotations(60_000L).map { it.kind }) + .containsExactlyElementsIn( + listOf(MetricsAnnotationStore.Kind.TASK) + + MetricsAnnotationStore.Kind.entries.filterNot { it == MetricsAnnotationStore.Kind.TASK }, + ).inOrder() + } + + @Test + fun `a full store evicts task markers before build outcomes`() { + store.recordBuild(MetricsAnnotationStore.Kind.BUILD_STARTED) + // Enough task markers to overflow the store several times over. A build long enough to do + // that -- about twenty minutes at one marker every five seconds -- used to lose its own + // "Build started", leaving an unpaired outcome and no way to see how long it took. + repeat(MetricsAnnotationStore.MAX_ANNOTATIONS * 2) { + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + store.record("task $it") + } + store.recordBuild(MetricsAnnotationStore.Kind.BUILD_FINISHED) + + val kinds = store.recentAnnotations(Long.MAX_VALUE / 2).map { it.kind } + assertThat(kinds.first()).isEqualTo(MetricsAnnotationStore.Kind.BUILD_STARTED) + assertThat(kinds.last()).isEqualTo(MetricsAnnotationStore.Kind.BUILD_FINISHED) + assertThat(kinds).hasSize(MetricsAnnotationStore.MAX_ANNOTATIONS) + } + + @Test + fun `a store holding nothing but build outcomes still respects its bound`() { + // The fallback branch: with no task marker left to sacrifice, the oldest outcome goes. + repeat(MetricsAnnotationStore.MAX_ANNOTATIONS + 5) { + now += 1_000L + store.recordBuild(MetricsAnnotationStore.Kind.BUILD_FINISHED) + } + + assertThat(store.recentAnnotations(Long.MAX_VALUE / 2)) + .hasSize(MetricsAnnotationStore.MAX_ANNOTATIONS) } } From 963e37b7f821c00deca452116103b25800b328f5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 10:05:52 -0700 Subject: [PATCH 4/7] ADFA-5509: dim the end arrows rather than disabling them A disabled View still consumes a touch and then drops it, so a long press on the arrow at either end of the carousel showed no tooltip -- and that is exactly the arrow whose greyed-out state a user might want explained. The alpha was already there; the isEnabled = false beside it was doing nothing the clamp in step() did not already do, except swallow the help. Test fixture: record() now advances past the throttle window itself. Every caller wanted both halves and had to remember the second, and forgetting it made the store drop the next annotation -- leaving the test asserting against a chart with one fewer marker than it had asked for, which is a passing test for the wrong reason. Also removes a stray blank line between a KDoc and its declaration in three files, where it detaches the doc from what it documents. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MemoryUsageChartRenderer.kt | 1 - .../ui/MetricsCarouselController.kt | 12 ++--- .../ui/NetworkUsageChartRenderer.kt | 1 - .../com/itsaky/androidide/ui/SafeLineChart.kt | 1 - .../ui/MetricsAnnotationRenderingTest.kt | 45 ++++++++++++------- .../androidide/ui/MetricsCarouselHelpTest.kt | 16 +++++++ 6 files changed, 53 insertions(+), 23 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 0529f2f390..066d845ed0 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt @@ -77,7 +77,6 @@ class MemoryUsageChartRenderer( * process's complete [ProcessMemoryInfo.usageHistory]. Call when the set of watched processes * changes; [onUsagesChanged] calls it on its own when it detects such a change. */ - @UiThread override fun rebuild() { val chart = this.chart ?: return 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 b74d495321..56f5e068cc 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -312,10 +312,12 @@ class MetricsCarouselController( @UiThread private fun updateArrows(position: Int) { val binding = this.binding ?: return - binding.metricsPrevious.isEnabled = position > 0 - binding.metricsNext.isEnabled = position < pages.lastIndex - binding.metricsPrevious.alpha = if (position > 0) 1f else DISABLED_ARROW_ALPHA - binding.metricsNext.alpha = if (position < pages.lastIndex) 1f else DISABLED_ARROW_ALPHA + // Dimmed, not disabled. A disabled View still consumes a touch and simply drops it, so a + // long press on the arrow at either end of the carousel showed no tooltip -- and that is + // exactly the arrow whose greying-out a user might want explained. [step] already clamps, + // so a tap on a dimmed arrow does nothing either way. + binding.metricsPrevious.alpha = if (position > 0) 1f else DIMMED_ARROW_ALPHA + binding.metricsNext.alpha = if (position < pages.lastIndex) 1f else DIMMED_ARROW_ALPHA } /** @@ -571,7 +573,7 @@ class MetricsCarouselController( else -> null } - const val DISABLED_ARROW_ALPHA = 0.35f + const val DIMMED_ARROW_ALPHA = 0.35f /** Dims a rate this device cannot offer, so the list shows what the hardware costs. */ const val UNAVAILABLE_RATE_ALPHA = 0.4f 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 601ab3aeb6..1834f40d80 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt @@ -69,7 +69,6 @@ class NetworkUsageChartRenderer( /** * Rebuilds both series from the full sample history. */ - @UiThread override fun rebuild() { val chart = this.chart ?: return 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..8362a2565c 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -38,7 +38,6 @@ import org.slf4j.LoggerFactory * hierarchy on a background thread, which races the main-thread updates of the memory-usage chart. The * chart is a non-critical diagnostic view, so dropping the occasional frame is preferable to crashing the * whole IDE. The next `invalidate()` recovers cleanly. - * */ class SafeLineChart : LineChart { constructor(context: Context) : super(context) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt index a930e438be..a174c5ace3 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt @@ -71,12 +71,30 @@ class MetricsAnnotationRenderingTest { var now = 0L val store = MetricsAnnotationStore(nowMillis = { now }) - /** Records [count] annotations, spaced far enough apart to clear the store's throttle. */ + /** Records [count] task markers, 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 - } + repeat(count) { index -> record("task $index") } + } + + /** + * Records one annotation and advances past the throttle window. + * + * Every caller wanted both halves and had to remember the second one; forgetting it made + * the store drop the next annotation, and the test then asserted against a chart with one + * fewer marker than it had asked for. + */ + fun record( + label: String, + kind: MetricsAnnotationStore.Kind = MetricsAnnotationStore.Kind.TASK, + ) { + store.record(label, kind) + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS + } + + /** Records a build outcome, whose label comes from its kind, and advances the clock. */ + fun recordBuild(kind: MetricsAnnotationStore.Kind) { + store.recordBuild(kind) + now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS } } @@ -160,9 +178,8 @@ class MetricsAnnotationRenderingTest { @Test fun `a failed build is drawn in a different colour from a task marker`() { val fixture = Fixture() - fixture.store.record("some task") - fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS - fixture.store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + fixture.record("some task") + fixture.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) val (_, chart) = render(fixture) @@ -179,11 +196,9 @@ class MetricsAnnotationRenderingTest { @Test fun `a build starting and finishing share one colour, distinct from a failure`() { val fixture = Fixture() - fixture.store.record("Build started", MetricsAnnotationStore.Kind.BUILD_STARTED) - fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS - fixture.store.record("Build finished", MetricsAnnotationStore.Kind.BUILD_FINISHED) - fixture.now += MetricsAnnotationStore.THROTTLE_INTERVAL_MS - fixture.store.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + fixture.record("Build started", MetricsAnnotationStore.Kind.BUILD_STARTED) + fixture.record("Build finished", MetricsAnnotationStore.Kind.BUILD_FINISHED) + fixture.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) val (_, chart) = render(fixture) @@ -198,7 +213,7 @@ class MetricsAnnotationRenderingTest { @Test fun `a cancelled build is not drawn as a failure`() { val fixture = Fixture() - fixture.store.recordBuild(MetricsAnnotationStore.Kind.BUILD_CANCELLED) + fixture.recordBuild(MetricsAnnotationStore.Kind.BUILD_CANCELLED) val (_, chart) = render(fixture) @@ -212,7 +227,7 @@ class MetricsAnnotationRenderingTest { @Test fun `a build marker takes its label from its kind, not from the recorded text`() { val fixture = Fixture() - fixture.store.recordBuild(MetricsAnnotationStore.Kind.BUILD_FAILED) + fixture.recordBuild(MetricsAnnotationStore.Kind.BUILD_FAILED) val (_, chart) = render(fixture) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt index 177c1e4651..ba1f0699df 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt @@ -111,6 +111,22 @@ class MetricsCarouselHelpTest { assertThat(unwired).isEmpty() } + @Test + fun `the arrow at the end of the carousel is dimmed but still answers`() { + val binding = boundStrip() + + // On the first page there is nowhere to go back to. Disabling that arrow would leave it + // consuming the long press and dropping it, so the one arrow whose greyed-out state a + // user might want explained was the one with no explanation. + assertThat(binding.metricsPager.currentItem).isEqualTo(0) + assertThat(binding.metricsPrevious.alpha).isLessThan(1f) + assertThat(binding.metricsPrevious.isEnabled).isTrue() + assertThat(binding.metricsPrevious.isLongClickable).isTrue() + + // ...and the other end is at full strength, so the dimming means something. + assertThat(binding.metricsNext.alpha).isEqualTo(1f) + } + @Test fun `an unbound strip has no help wired`() { // Guards the test above: if inflation alone made these long-clickable, it would pass From 1573eeff62a29fedde22276cd97799dc106bfc37 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 11:29:46 -0700 Subject: [PATCH 5/7] ADFA-5509: move the end-arrow change to ADFA-5510, where it belongs 963e37b7f bundled three unrelated things because they were what happened to be in the tree. The arrow change is help behaviour -- it is what makes ADFA-5510's tooltip on a dimmed arrow actually fire -- so reviewing it here, on the build-annotations PR, hides it from the reviewer who cares about it. Backed out to exactly the merge base, so the version on ADFA-5510 applies cleanly; it also fixes an accessibility regression this one had. The other two thirds of that commit stay: the test fixture that advances its own clock, and the stray blank lines between a KDoc and what it documents. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../androidide/ui/MetricsCarouselController.kt | 12 +++++------- .../androidide/ui/MetricsCarouselHelpTest.kt | 16 ---------------- 2 files changed, 5 insertions(+), 23 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 56f5e068cc..b74d495321 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -312,12 +312,10 @@ class MetricsCarouselController( @UiThread private fun updateArrows(position: Int) { val binding = this.binding ?: return - // Dimmed, not disabled. A disabled View still consumes a touch and simply drops it, so a - // long press on the arrow at either end of the carousel showed no tooltip -- and that is - // exactly the arrow whose greying-out a user might want explained. [step] already clamps, - // so a tap on a dimmed arrow does nothing either way. - binding.metricsPrevious.alpha = if (position > 0) 1f else DIMMED_ARROW_ALPHA - binding.metricsNext.alpha = if (position < pages.lastIndex) 1f else DIMMED_ARROW_ALPHA + binding.metricsPrevious.isEnabled = position > 0 + binding.metricsNext.isEnabled = position < pages.lastIndex + binding.metricsPrevious.alpha = if (position > 0) 1f else DISABLED_ARROW_ALPHA + binding.metricsNext.alpha = if (position < pages.lastIndex) 1f else DISABLED_ARROW_ALPHA } /** @@ -573,7 +571,7 @@ class MetricsCarouselController( else -> null } - const val DIMMED_ARROW_ALPHA = 0.35f + const val DISABLED_ARROW_ALPHA = 0.35f /** Dims a rate this device cannot offer, so the list shows what the hardware costs. */ const val UNAVAILABLE_RATE_ALPHA = 0.4f diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt index ba1f0699df..177c1e4651 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt @@ -111,22 +111,6 @@ class MetricsCarouselHelpTest { assertThat(unwired).isEmpty() } - @Test - fun `the arrow at the end of the carousel is dimmed but still answers`() { - val binding = boundStrip() - - // On the first page there is nowhere to go back to. Disabling that arrow would leave it - // consuming the long press and dropping it, so the one arrow whose greyed-out state a - // user might want explained was the one with no explanation. - assertThat(binding.metricsPager.currentItem).isEqualTo(0) - assertThat(binding.metricsPrevious.alpha).isLessThan(1f) - assertThat(binding.metricsPrevious.isEnabled).isTrue() - assertThat(binding.metricsPrevious.isLongClickable).isTrue() - - // ...and the other end is at full strength, so the dimming means something. - assertThat(binding.metricsNext.alpha).isEqualTo(1f) - } - @Test fun `an unbound strip has no help wired`() { // Guards the test above: if inflation alone made these long-clickable, it would pass From 55c553d2d0920260de715be8b4eef4980f07c01b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 14:17:28 -0700 Subject: [PATCH 6/7] ADFA-5509: forward the cancel request, so BUILD_CANCELLED can happen The feature was unreachable. setEventListener stores wrap(listener), and cancelCurrentBuild calls onBuildCancelRequested on that wrapper -- but wrap overrode five of the interface's six callbacks and not this one, so the call landed on the interface default and stopped there. EditorBuildEventListener.cancelRequested therefore stayed false, and a build the user had stopped went on being annotated BUILD_FAILED: exactly what the kind was added to prevent. The comment at the call site described an intent the wiring did not deliver. The default is what allowed it. `= Unit` was there so that only listeners caring about the distinction had to implement it, and the wrapper then inherited silence instead of being asked to forward. It is now abstract: the compiler asks every implementor, wrapper included, and this class of omission stops being possible. My own test was why it survived. MetricsAnnotationStoreTest calls store.recordBuild(BUILD_CANCELLED) directly, so it proved the store handles the kind while nothing proved the kind could ever be produced -- a test on the destination with the path to it untested. Two tests now, and the first one I wrote was worthless: asserting that the wrapper "overrides every method the interface declares" cannot fail, because Kotlin emits a bridge method on the implementing class for an inherited default, so reflection sees an override that is really a no-op. It passed against the bug. What it checks instead is that no callback has a default implementation at all, which is the property that would have caught this; that one fails against the bug, as does a direct check that a cancel reaches the listener. wrap moved to the companion object -- it closes over nothing but its argument, and building the service under Robolectric to reach a private method brought up the whole tooling stack and crashed the test JVM. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../services/builder/GradleBuildService.kt | 70 ++++++++------- .../GradleBuildServiceListenerWrapperTest.kt | 88 +++++++++++++++++++ 2 files changed, 128 insertions(+), 30 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index c116e5b19a..437698d64d 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -24,6 +24,7 @@ import android.app.Service import android.content.Intent import android.os.IBinder import android.text.TextUtils +import androidx.annotation.VisibleForTesting import androidx.core.app.NotificationManagerCompat import com.itsaky.androidide.BuildConfig import com.itsaky.androidide.analytics.IAnalyticsManager @@ -174,6 +175,38 @@ class GradleBuildService : ) companion object { + @VisibleForTesting + internal fun wrap(listener: EventListener?): EventListener? = + if (listener == null) { + null + } else { + object : EventListener { + override fun onBuildCancelRequested() { + runOnUiThread { listener.onBuildCancelRequested() } + } + + override fun prepareBuild(buildInfo: BuildInfo) { + runOnUiThread { listener.prepareBuild(buildInfo) } + } + + override fun onBuildSuccessful(tasks: List) { + runOnUiThread { listener.onBuildSuccessful(tasks) } + } + + override fun onProgressEvent(event: ProgressEvent) { + runOnUiThread { listener.onProgressEvent(event) } + } + + override fun onBuildFailed(tasks: List) { + runOnUiThread { listener.onBuildFailed(tasks) } + } + + override fun onOutput(line: String?) { + runOnUiThread { listener.onOutput(line) } + } + } + } + private val log = LoggerFactory.getLogger(GradleBuildService::class.java) private val NOTIFICATION_ID = R.string.app_name private val SERVER_System_err = LoggerFactory.getLogger("ToolingApiErrorStream") @@ -750,33 +783,6 @@ class GradleBuildService : return this } - private fun wrap(listener: EventListener?): EventListener? = - if (listener == null) { - null - } else { - object : EventListener { - override fun prepareBuild(buildInfo: BuildInfo) { - runOnUiThread { listener.prepareBuild(buildInfo) } - } - - override fun onBuildSuccessful(tasks: List) { - runOnUiThread { listener.onBuildSuccessful(tasks) } - } - - override fun onProgressEvent(event: ProgressEvent) { - runOnUiThread { listener.onProgressEvent(event) } - } - - override fun onBuildFailed(tasks: List) { - runOnUiThread { listener.onBuildFailed(tasks) } - } - - override fun onOutput(line: String?) { - runOnUiThread { listener.onOutput(line) } - } - } - } - private fun startServerOutputReader(input: InputStream): Job { outputReaderJob?.let { job -> if (job.isActive) { @@ -814,10 +820,14 @@ class GradleBuildService : * Called when the user asks for the running build to stop. * * The tooling API reports a cancelled build through [onBuildFailed], so a listener that - * wants to tell the two apart has to be told here. Defaulted, because only a listener that - * cares about the distinction needs it. + * wants to tell the two apart has to be told here. + * + * Deliberately not defaulted. It was, and the forwarding wrapper in [GradleBuildService] + * then quietly inherited the no-op instead of passing it on -- so the cancel never reached + * the real listener, and a build the user stopped went on being annotated as a failure. A + * member with no default cannot be forgotten by a wrapper; the compiler asks for it. */ - fun onBuildCancelRequested() = Unit + fun onBuildCancelRequested() /** * Called just before a build is started. diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt new file mode 100644 index 0000000000..d515017ffa --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt @@ -0,0 +1,88 @@ +/* + * 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.services.builder + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.services.builder.GradleBuildService.EventListener +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.lang.reflect.Proxy + +/** + * Pins that the build service's listener wrapper forwards every callback it is given. + * + * It did not. `onBuildCancelRequested` was declared with a `= Unit` default so that only listeners + * that cared had to implement it; the wrapper then inherited that no-op rather than passing the + * call on, so the cancel never reached the real listener and a build the user had stopped went on + * being annotated as a failure -- which is what BUILD_CANCELLED exists to prevent. The feature was + * unreachable, and the store-level test for it passed the whole time, because it called the store + * directly and nothing exercised the path to it. + */ +@RunWith(RobolectricTestRunner::class) +class GradleBuildServiceListenerWrapperTest { + /** Records which interface method it was handed, so no call has to be spelled out here. */ + private class Recorder { + val calls = mutableListOf() + + val listener: EventListener = + Proxy.newProxyInstance( + EventListener::class.java.classLoader, + arrayOf(EventListener::class.java), + ) { _, method, _ -> + calls += method.name + null + } as EventListener + } + + @Test + fun `no callback on the interface has a default implementation`() { + // This is the invariant that would have caught the bug, and it is not the one you reach + // for first: asserting that the wrapper "overrides every declared method" looks right and + // cannot fail, because Kotlin emits a bridge method on the implementing class for an + // inherited default, so reflection sees an override that is really a no-op. + // + // A default is what let the wrapper inherit silence instead of being asked to forward. With + // none, the compiler demands an implementation from every implementor -- the wrapper + // included -- and this class of omission stops being possible. + val defaults = + EventListener::class.java.declaredClasses + .firstOrNull { it.simpleName == "DefaultImpls" } + ?.declaredMethods + .orEmpty() + .map { it.name } + + assertThat(defaults).isEmpty() + } + + @Test + fun `a cancel request reaches the listener`() { + val recorder = Recorder() + val wrapped = GradleBuildService.wrap(recorder.listener)!! + + wrapped.onBuildCancelRequested() + + // The one this went wrong on, kept as its own case so the reason is legible in a report. + assertThat(recorder.calls).containsExactly("onBuildCancelRequested") + } + + @Test + fun `wrapping nothing yields nothing`() { + assertThat(GradleBuildService.wrap(null)).isNull() + } +} From d5c67c5fd4a9780bc1cfcebc64257972d0cde95a Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 18:23:54 -0700 Subject: [PATCH 7/7] ADFA-5509: clear the build flags before the activity check Two review findings on this PR, both about state outliving the thing it described. cancelRequested was reset after prepareBuild's activity guard. This listener outlives any one activity, so a cancelled build whose onBuildFailed arrived with none attached left the flag set, and the next build to fail inherited it and was drawn as cancelled. The outcome callbacks decided for themselves whether to draw the second half of a marker pair, from the task list they are handed -- which is not the list prepareBuild sees. If those two ever disagreed the chart got a start with no finish, or a finish with no start, which is the one thing a pair exists to avoid. The build that started now decides, through a flag meaning "a start marker was drawn for the build now running", and the outcome follows it. Both flags are cleared before the guard and set only where the marker is actually drawn. Writing it the other way round -- recording the pairing from the task list before knowing whether a marker could be drawn -- would have swapped one asymmetry for its mirror image, a finish with no start, which is what the first attempt did and what the tests caught. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz --- .../handlers/EditorBuildEventListener.kt | 33 ++++++++++++++++--- .../EditorBuildEventListenerAnnotationTest.kt | 27 +++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index d388099ee3..c40e68d7b2 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -53,7 +53,19 @@ class EditorBuildEventListener : GradleBuildService.EventListener { * Set when the user asks for the running build to stop, so [onBuildFailed] can tell a cancel * from a real failure. Cleared as each build is prepared. */ - private var cancelRequested = false + @VisibleForTesting + internal var cancelRequested = false + + /** + * Whether the build now running drew a "Build started" marker. + * + * The outcome callbacks used to decide for themselves, from the task list they are handed -- + * a different list from the one prepareBuild sees. If those two ever disagreed the chart got + * a start with no finish, or a finish with no start, which is the one thing a pair of markers + * exists to avoid. The build that started decides, and its outcome follows. + */ + @VisibleForTesting + internal var annotatedBuild = false private var enabled = true private var activityReference: WeakReference = WeakReference(null) @@ -87,14 +99,23 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { - val act = checkActivity("prepareBuild") ?: return - + // Before the activity check, not after: this listener outlives any one activity, so a + // build whose outcome arrived with none attached would otherwise leave both flags set for + // the next build to inherit -- a stale cancel mislabelling a real failure, or a stale + // pairing drawing a finish for a build that never started. cancelRequested = false + annotatedBuild = false + + val act = checkActivity("prepareBuild") ?: return // A project sync runs through the same callbacks with no tasks, so annotating every // prepareBuild put a "Build started" marker on the chart merely for opening a project -- // and blamed the sync's own memory spike on a build the user never ran. + // + // The outcome callbacks are handed their own task list, which is not this one. Recorded + // here so the pair is decided once, by the build that started. if (buildInfo.tasks.isNotEmpty()) { + annotatedBuild = true act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_STARTED) } @@ -130,9 +151,10 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildSuccessful(tasks: List) { val act = checkActivity("onBuildSuccessful") ?: return - if (tasks.isNotEmpty()) { + if (annotatedBuild) { act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) } + annotatedBuild = false pluginBuildService?.notifyBuildFinished() @@ -193,7 +215,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildFailed(tasks: List) { val act = checkActivity("onBuildFailed") ?: return - if (tasks.isNotEmpty()) { + if (annotatedBuild) { // A build the user stopped arrives through this same callback. Marking it as a failure // would report their own deliberate action back to them in the error colour. act.recordBuildAnnotation( @@ -204,6 +226,7 @@ class EditorBuildEventListener : GradleBuildService.EventListener { }, ) } + annotatedBuild = false cancelRequested = false analyzeCurrentFile() diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index 8eb6db3252..5fe152155e 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -18,6 +18,8 @@ package com.itsaky.androidide.handlers import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.result.BuildInfo import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.internal.DefaultOperationDescriptor import com.itsaky.androidide.tooling.events.internal.DefaultProgressEvent @@ -71,6 +73,31 @@ class EditorBuildEventListenerAnnotationTest { descriptor = DefaultOperationDescriptor(name = "configure", displayName = "Configure"), ) + @Test + fun `preparing a build clears a stale cancel, even with no activity attached`() { + listener.cancelRequested = true + + // No activity is attached here, so prepareBuild returns early -- which is the point. This + // listener outlives any one activity, and a cancel whose onBuildFailed arrived without one + // would otherwise leave the flag set for the next build to inherit and be mislabelled. + listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) + + assertThat(listener.cancelRequested).isFalse() + } + + @Test + fun `preparing a build clears a stale pairing`() { + listener.annotatedBuild = true + + // The flag means "a start marker was drawn for the build now running", so a new build + // must not inherit it: the outcome callbacks read it to decide whether to draw the other + // half of the pair, and they are handed a different task list from this one. Cleared + // before the activity check for the same reason as the cancel flag. + listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) + + assertThat(listener.annotatedBuild).isFalse() + } + @Test fun `a task starting is annotated`() { assertThat(listener.isAnnotated(taskStart())).isTrue()