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 24515c4782..eb4fb9bd35 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 @@ -213,6 +214,17 @@ abstract class BaseEditorActivity : metricsViewModel.annotations.record(label) } + /** + * Marks a build outcome on the charts (ADFA-5509). + * + * 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) { + metricsViewModel.annotations.recordBuild(kind) + } + private val fileManagerViewModel by viewModels() private var feedbackButtonManager: FeedbackButtonManager? = null private var fullscreenManager: FullscreenManager? = null 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 03671a8e12..c40e68d7b2 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -31,6 +31,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 @@ -48,6 +49,24 @@ 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. + */ + @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) @@ -80,28 +99,46 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } override fun prepareBuild(buildInfo: BuildInfo) { - 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) + } 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, ) } } @@ -114,6 +151,11 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildSuccessful(tasks: List) { val act = checkActivity("onBuildSuccessful") ?: return + if (annotatedBuild) { + act.recordBuildAnnotation(MetricsAnnotationStore.Kind.BUILD_FINISHED) + } + annotatedBuild = false + pluginBuildService?.notifyBuildFinished() analyzeCurrentFile() @@ -141,6 +183,10 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastStatusLine = "" } + override fun onBuildCancelRequested() { + cancelRequested = true + } + override fun onProgressEvent(event: ProgressEvent) { val act = checkActivity("onProgressEvent") ?: return @@ -169,6 +215,20 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun onBuildFailed(tasks: List) { val act = checkActivity("onBuildFailed") ?: return + 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( + if (cancelRequested) { + MetricsAnnotationStore.Kind.BUILD_CANCELLED + } else { + MetricsAnnotationStore.Kind.BUILD_FAILED + }, + ) + } + annotatedBuild = false + cancelRequested = false + analyzeCurrentFile() GeneralPreferences.isFirstBuild = false act.editorViewModel.isBuildInProgress = 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..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") @@ -632,6 +665,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() } @@ -747,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) { @@ -807,6 +816,19 @@ 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. + * + * 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() + /** * Called just before a build is started. * 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 6f41872940..1772089b39 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MemoryUsageChartRenderer.kt @@ -78,7 +78,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/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 81ae058415..60ad21c8aa 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,10 @@ abstract class MetricsChartRenderer( val oldestVisibleIndex = if (visible.isEmpty()) newestIndex else visible.first.toFloat() val spanMillis = ((newestIndex - oldestVisibleIndex).toLong() + 1L) * interval val now = nowMillis() - val markerColor = chart.context.resolveAttr(R.attr.colorOnSurface) + // 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(spanMillis).forEach { annotation -> val samplesAgo = (now - annotation.atMillis).toFloat() / interval @@ -518,7 +523,8 @@ abstract class MetricsChartRenderer( } chart.xAxis.addLimitLine( - LimitLine(x, annotation.label).apply { + LimitLine(x, labelFor(chart, annotation)).apply { + val markerColor = markerColors.getValue(annotation.kind) lineWidth = ANNOTATION_LINE_WIDTH lineColor = markerColor textColor = markerColor @@ -532,6 +538,62 @@ 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). + * + * 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 + + // 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 + } + // 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 + } + /** * 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/ui/NetworkUsageChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt index c3550c7c52..af4b093d9f 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/NetworkUsageChartRenderer.kt @@ -70,7 +70,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 e9121ff366..600ad00666 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/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt index 97e7d8e91e..fe70af6b2a 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -18,14 +18,18 @@ 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). * - * 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 @@ -46,6 +50,50 @@ 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( + /** + * 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(labelRes = null), + + /** A build beginning. */ + BUILD_STARTED(string.metrics_annotation_build_started), + + /** A build completing successfully. */ + BUILD_FINISHED(string.metrics_annotation_build_finished), + + /** A build failing. */ + 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), + ; + + /** + * 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,27 +113,63 @@ 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 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]. * + * 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)) + 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/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() 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() + } +} 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 c752cf9ece..a174c5ace3 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationRenderingTest.kt @@ -18,12 +18,16 @@ 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 import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.R import com.itsaky.androidide.idetooltips.TooltipTag +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 @@ -36,7 +40,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( @@ -61,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 } } @@ -147,6 +175,71 @@ 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.record("some task") + fixture.record("Build failed", MetricsAnnotationStore.Kind.BUILD_FAILED) + + val (_, chart) = render(fixture) + + val lines = chart.xAxis.limitLines + assertThat(lines).hasSize(2) + // 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) + } + + @Test + fun `a build starting and finishing share one colour, distinct from a failure`() { + val fixture = Fixture() + 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) + + val lines = chart.xAxis.limitLines + assertThat(lines).hasSize(3) + // Started and finished are both outcomes worth seeing; only failure is bad news. + 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.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.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 { 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..26821a9256 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,111 @@ 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 `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) + } } diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 203486d159..67a7e6af4c 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1690,6 +1690,11 @@ Every %1$s %1$s (needs a 64-bit device) Temperature and power + + Build started + Build finished + Build failed + Build cancelled Temperature and power chart Battery temp Power