From 0d475fca7d533200dff32f709d71e63b4f56dce4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 16:17:46 -0700 Subject: [PATCH 1/6] ADFA-5554: hold, rather than brush, to get help Help was appearing at the platform's long-press timeout -- 400ms, a brisk tap, and shorter still on a device where the accessibility "touch and hold delay" has been lowered -- so the carousel's buttons answered with a tooltip instead of doing their job. The hold is now max(platform * 2, 800ms). The timing had to become ours rather than the framework's, and that is the whole difficulty. setOnLongClickListener fires at the platform timeout, and returning true from it sets mHasPerformedLongPress, which cancels the click. Simply deferring the tooltip would therefore leave a 500ms press doing nothing at all: no help, and no button either -- worse than the complaint. So performOnHold takes the touch over and performs the click itself, only when no hold completed. Never shorter than the platform's own value. That setting exists as an accessibility control and someone who lengthened it meant to. The long-click listener stays installed for accessibility. Touch never reaches View.onTouchEvent, so the framework cannot fire it from a finger; TalkBack's long press calls performLongClick directly, and that path still shows help at once, as it should -- it is already deliberate. The chart needs its own handling because MPAndroidChart's GestureDetector has already suppressed the tap by the time onChartLongPressed arrives. Its help is deferred for the rest of the hold and cancelled on gesture end -- and when the finger lifts early the axis-band tap is invoked directly, because the detector ate the one that would have opened the sampling-rate chooser. Without that a 500ms press on the axis would do nothing. Two bugs my own tests caught while writing them: View.postDelayed parks work in a HandlerActionQueue that only drains on attach, so the hold never timed out for a detached view -- an explicit Handler now; and a press that wandered off the control still clicked, where the framework would have done neither. performOnHold is split out from displayTooltipOnLongPress so the timing can be tested at all: TooltipManager reads the docs database from device storage in its static initialiser and cannot load off-device. Seven tests. The first is the 500ms case, and it fails if the click is suppressed the way the framework would. Verified on a Pixel 6 Pro: a 500ms press on the next arrow pages Memory to Network; a 1200ms hold opens the tooltip and does not page. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MetricsChartRenderer.kt | 50 +++++- .../utils/LongPressHelpExtensions.kt | 4 + .../androidide/ui/LongPressHelpTimingTest.kt | 169 ++++++++++++++++++ .../com/itsaky/androidide/utils/ViewUtils.kt | 115 +++++++++++- 4 files changed, 328 insertions(+), 10 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index ce4526ec53..fb5b0bf3fd 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -23,6 +23,7 @@ import android.os.SystemClock import android.util.TypedValue import android.view.MotionEvent import android.view.View +import android.view.ViewConfiguration import androidx.annotation.CallSuper import androidx.annotation.UiThread import androidx.annotation.VisibleForTesting @@ -38,6 +39,7 @@ import com.github.mikephil.charting.listener.OnChartGestureListener import com.itsaky.androidide.R import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.utils.MetricsAnnotationStore +import com.itsaky.androidide.utils.longPressHelpTimeoutMillis import com.itsaky.androidide.utils.resolveAttr import com.itsaky.androidide.utils.showIdeCategoryTooltipIfPresent import kotlin.math.ceil @@ -395,6 +397,15 @@ abstract class MetricsChartRenderer( private inner class XAxisTapListener( private val chart: SafeLineChart, ) : OnChartGestureListener { + /** The deferred half of a long press, waiting out the rest of the hold. */ + private var pendingHelp: Runnable? = null + + /** Whether this gesture already showed help, so its lift must not also count as a tap. */ + private var helpShown = false + + /** Whether the press that became a long press had started on the axis band. */ + private var pendingTapOnAxis = false + override fun onChartSingleTapped(me: MotionEvent?) { val y = me?.y ?: return if (isOnAxisBand(y)) { @@ -410,16 +421,43 @@ abstract class MetricsChartRenderer( override fun onChartGestureEnd( me: MotionEvent?, lastPerformedGesture: ChartTouchListener.ChartGesture?, - ) = Unit + ) { + pendingHelp?.let(chart::removeCallbacks) + pendingHelp = null + // Lifted before the hold completed: the detector ate the tap, so stand in for it. + if (!helpShown && pendingTapOnAxis) { + onXAxisTap?.invoke() + } + helpShown = false + pendingTapOnAxis = false + } override fun onChartLongPressed(me: MotionEvent?) { val y = me?.y ?: return val tag = helpTagAt(y) ?: return - // Haptic feedback left at its default, unlike every view-based help site, which - // passes false. Those rely on View.performLongClick buzzing for them; - // BarLineChartBase.onTouchEvent never calls super, so the framework's long press -- - // and its feedback -- never runs here and this is the only thing that provides it. - showIdeCategoryTooltipIfPresent(chart.context, chart, tag) + + // This arrives at the platform's own timeout -- 400ms by default, a brisk tap -- and + // help at that speed is what ADFA-5554 is about. Wait out the rest of the hold and + // show it only if the finger is still down; [onChartGestureEnd] cancels otherwise. + pendingHelp?.let(chart::removeCallbacks) + val onAxisBand = isOnAxisBand(y) + pendingHelp = + Runnable { + pendingHelp = null + helpShown = true + // Haptic feedback left at its default, unlike every view-based help site, + // which passes false. Those rely on View.performLongClick buzzing for them; + // BarLineChartBase.onTouchEvent never calls super, so the framework's long + // press -- and its feedback -- never runs here and this is the only thing + // that provides it. + showIdeCategoryTooltipIfPresent(chart.context, chart, tag) + }.also { chart.postDelayed(it, longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout()) } + + // GestureDetector has already decided this gesture is a long press, so it will not + // report the tap that would have opened the sampling-rate chooser. Remember whether + // this one was headed there, so a finger lifted before the hold completes still gets + // the tap it asked for rather than nothing at all. + pendingTapOnAxis = onAxisBand } override fun onChartDoubleTapped(me: MotionEvent?) = Unit diff --git a/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt b/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt index 43e3b76b00..97e8765212 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt @@ -31,4 +31,8 @@ import android.view.View fun View.clearLongPressHelp() { setOnLongClickListener(null) isLongClickable = false + // The hold is timed by a touch listener rather than the framework (ADFA-5554), so leaving that + // installed would keep the view swallowing every touch -- and performing its own clicks -- for + // help it no longer offers. + setOnTouchListener(null) } diff --git a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt new file mode 100644 index 0000000000..60a17e6014 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt @@ -0,0 +1,169 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.os.Looper +import android.view.MotionEvent +import android.view.View +import android.view.ViewConfiguration +import android.widget.Button +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.clearLongPressHelp +import com.itsaky.androidide.utils.longPressHelpTimeoutMillis +import com.itsaky.androidide.utils.performOnHold +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import java.util.concurrent.TimeUnit + +/** + * How long a press has to last before help replaces the control (ADFA-5554). + * + * The platform fires a long press at 400ms, which is a brisk tap, so the carousel's buttons were + * answering with a tooltip instead of doing their job. The interesting case is neither the long + * press nor the short one -- it is the press in between. At 500ms the framework has already + * decided the gesture is a long press and cancelled the click, so a fix that merely defers the + * tooltip leaves that press doing nothing whatsoever: no help, and no button either. That is the + * first test here, and it is why the timing is this code's rather than the framework's. + * + * The hold's payload is a lambda rather than a real tooltip because `TooltipManager` reads the + * docs database from device storage in its static initialiser and cannot be loaded off-device -- + * the same reason the renderer separates deciding a help tag from showing one. + */ +@RunWith(RobolectricTestRunner::class) +class LongPressHelpTimingTest { + private val context = ApplicationProvider.getApplicationContext() + + private var holds = 0 + + private var clicks = 0 + + private fun target(): Button = + Button(context).apply { + setOnClickListener { clicks++ } + performOnHold { holds++ } + } + + private fun send( + view: View, + action: Int, + x: Float = 0f, + ) { + val event = MotionEvent.obtain(0L, 0L, action, x, 0f, 0) + view.dispatchTouchEvent(event) + event.recycle() + } + + /** Runs the main looper forward by [millis] of virtual time. */ + private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) + + @Test + fun `a press past the platform timeout but short of the hold still clicks`() { + // The regression the obvious fix introduces, and the reason this class exists. The + // framework's long press is 400ms and the hold is 800ms; everything between the two would + // otherwise be dead. + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(ViewConfiguration.getLongPressTimeout() + 100L) + send(view, MotionEvent.ACTION_UP) + + assertThat(clicks).isEqualTo(1) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `a quick tap clicks`() { + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_UP) + + assertThat(clicks).isEqualTo(1) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `a press held past the hold shows help and does not click`() { + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(longPressHelpTimeoutMillis() + 50L) + send(view, MotionEvent.ACTION_UP) + + assertThat(holds).isEqualTo(1) + assertThat(clicks).isEqualTo(0) + } + + @Test + fun `a press that wanders off the control does neither`() { + val view = target() + val slop = ViewConfiguration.get(context).scaledTouchSlop + + send(view, MotionEvent.ACTION_DOWN) + elapse(100L) + send(view, MotionEvent.ACTION_MOVE, x = slop + 10f) + elapse(longPressHelpTimeoutMillis()) + send(view, MotionEvent.ACTION_UP) + + // The framework treats a drag out of a view as neither, so taking the touch over means + // saying so rather than inventing a third behaviour. + assertThat(clicks).isEqualTo(0) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `a cancelled gesture does neither`() { + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(100L) + send(view, MotionEvent.ACTION_CANCEL) + elapse(longPressHelpTimeoutMillis()) + + assertThat(clicks).isEqualTo(0) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `the hold is longer than the platform's, and never shorter`() { + // The floor matters: the platform value is exposed as an accessibility "touch and hold + // delay", and someone who lengthened it meant to. + assertThat(longPressHelpTimeoutMillis()).isAtLeast(800L) + assertThat(longPressHelpTimeoutMillis()).isAtLeast(ViewConfiguration.getLongPressTimeout().toLong()) + } + + @Test + fun `clearing the help stops the timing`() { + val view = target() + view.clearLongPressHelp() + + send(view, MotionEvent.ACTION_DOWN) + elapse(longPressHelpTimeoutMillis() + 50L) + send(view, MotionEvent.ACTION_UP) + + // Left installed, the listener would go on timing holds -- and swallowing every touch -- + // for help the view no longer offers. + assertThat(holds).isEqualTo(0) + assertThat(view.isLongClickable).isFalse() + } +} diff --git a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt index 02213fd571..24fb584b7b 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt @@ -1,10 +1,15 @@ package com.itsaky.androidide.utils import android.content.Context +import android.os.Handler +import android.os.Looper import android.view.HapticFeedbackConstants +import android.view.MotionEvent import android.view.View +import android.view.ViewConfiguration import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager +import kotlin.math.abs /** * Shows [tag]'s tooltip (under [category]) anchored to [anchor], or does nothing if [tag] is @@ -40,17 +45,119 @@ fun showIdeCategoryTooltipIfPresent( ) = showTooltipIfPresent(context, anchor, TooltipCategory.CATEGORY_IDE, tag, playHapticFeedback) /** - * Installs a long-click listener on this view that consumes the click and shows [tooltipTag]'s - * tooltip (under [tooltipCategory]) anchored to this view, or does nothing if [tooltipTag] is - * blank. See [showTooltipIfPresent] - no manual haptic feedback here for the same reason. + * How long a press has to be held before help appears, in milliseconds. + * + * Twice the platform's own long-press timeout, floored at 800ms. The platform default is 400ms, + * which is a brisk tap, so help was appearing instead of the control activating (ADFA-5554). + * + * Never *shorter* than the platform's value: that setting is exposed as an accessibility + * "touch and hold delay", and someone who has lengthened it did so deliberately. + */ +fun longPressHelpTimeoutMillis(): Long = maxOf(ViewConfiguration.getLongPressTimeout() * 2L, 800L) + +/** + * Shows [tooltipTag]'s tooltip (under [tooltipCategory]) when this view is held for + * [holdMillis], and lets a shorter press through as an ordinary click. + * + * The timing is this function's rather than the framework's, and that is the whole point. + * `setOnLongClickListener` fires at [ViewConfiguration.getLongPressTimeout] -- 400ms by default -- + * and returning `true` from it sets `mHasPerformedLongPress`, which cancels the click. So simply + * deferring the tooltip would leave a 500ms press doing nothing at all: no help, and no button + * press either. Instead the touch is taken over outright, and the click is performed here only + * when no tooltip was shown. + * + * The long-click listener stays installed for accessibility. Touch never reaches + * [View.onTouchEvent], so the framework cannot fire it from a finger; TalkBack's own long-press + * calls [View.performLongClick] directly, and that path shows help immediately, as it should -- + * it is already a deliberate gesture. + * + * On a [android.view.ViewGroup] this only sees touches its children did not take, which is what + * makes it safe to install on a container for the gaps between its controls. */ fun View.displayTooltipOnLongPress( context: Context, tooltipTag: String, tooltipCategory: String = TooltipCategory.CATEGORY_IDE, + holdMillis: Long = longPressHelpTimeoutMillis(), ) { - this.setOnLongClickListener { + if (tooltipTag.isBlank()) { + return + } + + setOnLongClickListener { showTooltipIfPresent(context, this, tooltipCategory, tooltipTag, playHapticFeedback = false) true } + + // Haptic feedback on, unlike the long-click path above: nothing else buzzes here, because the + // framework's own long press never runs for this view. + performOnHold(holdMillis) { showTooltipIfPresent(context, this, tooltipCategory, tooltipTag) } +} + +/** + * Runs [onHold] when this view is held for [holdMillis], and lets a shorter press through as an + * ordinary click. + * + * Separated from [displayTooltipOnLongPress] so the timing can be tested: `TooltipManager` reads + * the docs database from device storage in its static initialiser and cannot be loaded off-device, + * so a test that showed a real tooltip could not run at all. + */ +fun View.performOnHold( + holdMillis: Long = longPressHelpTimeoutMillis(), + onHold: () -> Unit, +) { + val slop = ViewConfiguration.get(context).scaledTouchSlop + // An explicit handler, not View.postDelayed: a view not attached to a window parks posted work + // in its HandlerActionQueue and only runs it on attach, so the hold would never time out. + val handler = Handler(Looper.getMainLooper()) + var held = false + var holding = false + var downX = 0f + var downY = 0f + val fire = + Runnable { + held = true + isPressed = false + onHold() + } + + setOnTouchListener { view, event -> + when (event.actionMasked) { + MotionEvent.ACTION_DOWN -> { + held = false + holding = true + downX = event.x + downY = event.y + view.isPressed = true + handler.postDelayed(fire, holdMillis) + } + + MotionEvent.ACTION_MOVE -> { + if (holding && (abs(event.x - downX) > slop || abs(event.y - downY) > slop)) { + // Wandered off the control: neither a click nor help, which is how the + // framework treats a drag out of a view. Taking the touch over means saying so. + holding = false + handler.removeCallbacks(fire) + view.isPressed = false + } + } + + MotionEvent.ACTION_UP -> { + handler.removeCallbacks(fire) + view.isPressed = false + // The click belongs to a press that stayed put and did not become a hold. + if (holding && !held) { + view.performClick() + } + holding = false + } + + MotionEvent.ACTION_CANCEL -> { + holding = false + handler.removeCallbacks(fire) + view.isPressed = false + } + } + true + } } From ac03b125882726a51afacca9e8cd6ec4882f10e6 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 17:24:09 -0700 Subject: [PATCH 2/6] ADFA-5554: fix the review findings across the chart and the hold Twelve findings from the xhigh review. The chart half had none of its own tests, which is why most of them are there. A press that became a pan opened the sampling-rate chooser. The detector calls a press a long press at 400ms and a drag can start from it, so onChartGestureEnd stood in for a tap that was never a tap -- and picking a rate in that chooser clears every sample buffer, the history loss the tap band's lower bound exists to prevent, reached by another route. The stand-in now requires the gesture to still be a long press when it ended, and a translate or a scale gives it up as the gesture escalates. The deferred help had the same shape of problem one step along: it was cancelled at the end of a gesture but not when the gesture turned into something else, so the tooltip opened over a chart the user was in the middle of panning. Same fix, same place. detach did not cancel a hold that was counting down, and attach installs a fresh listener whose own pendingHelp is null -- so nothing could ever have cancelled the old one. It fired the outgoing page's help against whatever replaced it. The renderer holds the listener now and drops the hold with the chart. The chart timed its hold with View.postDelayed, which is the HandlerActionQueue trap this PR's own message says it found and replaced in performOnHold. It worked only because a chart that receives a long press happens to be attached. Explicit Handler, as on the other side, which is also what makes any of the above testable. Seven tests for that, in a class of their own. Help is a seam rather than a real tooltip: TooltipManager reads the docs database from device storage in its static initialiser and cannot load off-device. They live in MetricsChartHoldHelpTest and not beside the axis-tap tests because the two sets together exhaust the test JVM -- unbounded, not large; 4GB fails the same way -- while either alone is fine and no pair of them reproduces it. That is a Robolectric interaction, not a product defect, and splitting the class is where it belongs anyway. On the view side: performOnHold cancelled the click on any movement of one touch slop from the down point. The framework's rule is leaving the view grown by the slop, which the comment beside it already claimed. Measured from the down point, an ordinary thumb tap on a large target rolls far enough to cancel its own click without leaving the control, and these targets are large -- the carousel strip is the full width of the editor. It now uses the framework's rule, and a test rolls a slop and ten pixels across a 400px-wide button and expects a click. A hold already counting down could not be cancelled: the handler and the runnable were captured in a closure, so a teardown could only stop the next hold, not the one running. The listener is an object now, kept in a keyed view tag, and the teardown reaches through it. That also answers the complaint that the teardown nulled any touch listener at all: it removes one only where the tag says performOnHold installed it, which is one of the six views it is called on. The teardown moved to :idetooltips beside performOnHold, in two halves -- clearLongPressHelp for both, clearOnHold for the hold alone -- so a module that installed only a hold can undo only a hold. The name is unchanged, so call sites keep their import. The click ran inside touch dispatch. View.onTouchEvent posts it so the pressed state is drawn first, and these actions open dialogs and re-page the carousel from inside the dispatch of the event that triggered them. Posted now, through the same handler rather than View.post, which parks work on an unattached view and returns true having run nothing. The ripple also had no hotspot, so every one of these controls rippled from the centre of its drawable rather than the finger. A blank tooltip tag returned before installing anything, which left the previous tag's listeners in place. It now clears them: a blank tag says this view offers no help, and that has to replace what was wired before. The timeout test was vacuous. maxOf(x * 2, 800) is at least 800 and at least x for every x by construction, so both assertions held with the whole rule deleted. The platform timeout is a parameter now, which is the only way to name a value that separates the doubling from the floor, and there is a case for each. Sibling sweep: the six EditorBottomSheet action buttons -- share, clear, search, filter, word wrap, view options -- were still wired to the framework's 400ms long click and still returning true from it, so a 450ms press on "clear output" showed a tooltip and cleared nothing. That is the defect this ticket exists to fix, in a file this PR already touches. All six converted, and generateTooltipListener has no callers left. Deliberately left alone: the TabLayout tab long-press in the same file, because taking over a TabView's touch stream is a different risk and not one this can check off-device, and ActionMenuUtils, which the PR body already named as out of scope. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../itsaky/androidide/ui/EditorBottomSheet.kt | 25 +-- .../androidide/ui/MetricsChartRenderer.kt | 77 ++++++- .../utils/LongPressHelpExtensions.kt | 38 ---- .../androidide/ui/LongPressHelpTimingTest.kt | 135 ++++++++++- .../androidide/ui/MetricsChartHoldHelpTest.kt | 210 ++++++++++++++++++ .../com/itsaky/androidide/utils/ViewUtils.kt | 147 ++++++++++-- idetooltips/src/main/res/values/ids.xml | 8 + 7 files changed, 546 insertions(+), 94 deletions(-) delete mode 100644 app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt create mode 100644 idetooltips/src/main/res/values/ids.xml diff --git a/app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt b/app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt index 7f464dbd2f..bc078478d4 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.kt @@ -68,6 +68,7 @@ import com.itsaky.androidide.utils.DiagnosticsFormatter import com.itsaky.androidide.utils.IntentUtils.shareFile import com.itsaky.androidide.utils.Symbols.forFile import com.itsaky.androidide.utils.clearLongPressHelp +import com.itsaky.androidide.utils.displayTooltipOnLongPress import com.itsaky.androidide.utils.dpToPx import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess @@ -252,7 +253,7 @@ class EditorBottomSheet } } } - binding.shareOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_SHARE_EXTERNAL)) + binding.shareOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_SHARE_EXTERNAL) binding.clearOutputAction.setOnClickListener { val fragment = @@ -263,7 +264,7 @@ class EditorBottomSheet } (fragment as ShareableOutputFragment).clearOutput() } - binding.clearOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_CLEAR)) + binding.clearOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_CLEAR) binding.copyDiagnosticsFab.setOnClickListener { copyDiagnosticsToClipboard() @@ -279,7 +280,7 @@ class EditorBottomSheet viewModel.setSheetState(sheetState = BottomSheetBehavior.STATE_EXPANDED) fragment.beginSearch() } - binding.searchOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_SEARCH)) + binding.searchOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_SEARCH) binding.filterOutputAction.setOnClickListener { val fragment = pagerAdapter.getFragmentAtIndex(binding.tabs.selectedTabPosition) @@ -290,7 +291,7 @@ class EditorBottomSheet viewModel.setSheetState(sheetState = BottomSheetBehavior.STATE_EXPANDED) fragment.toggleFilterBar() } - binding.filterOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_FILTER)) + binding.filterOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_FILTER) updateWordWrapButtonState(EditorPreferences.outputWordWrap) binding.wordWrapOutputAction.setOnClickListener { @@ -298,7 +299,7 @@ class EditorBottomSheet EditorPreferences.outputWordWrap = newState updateWordWrapButtonState(newState) } - binding.wordWrapOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_WORD_WRAP)) + binding.wordWrapOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_WORD_WRAP) binding.viewOptionsOutputAction.setOnClickListener { val fragment = pagerAdapter.getFragmentAtIndex(binding.tabs.selectedTabPosition) @@ -306,7 +307,7 @@ class EditorBottomSheet fragment.showViewOptions(it) } } - binding.viewOptionsOutputAction.setOnLongClickListener(generateTooltipListener(TooltipTag.OUTPUT_VIEW_OPTIONS)) + binding.viewOptionsOutputAction.displayTooltipOnLongPress(context, TooltipTag.OUTPUT_VIEW_OPTIONS) binding.headerContainer.setOnClickListener { viewModel.setSheetState(sheetState = BottomSheetBehavior.STATE_EXPANDED) @@ -388,18 +389,6 @@ class EditorBottomSheet } } - private fun generateTooltipListener(tooltipTag: String): OnLongClickListener = - OnLongClickListener { view: View -> - TooltipManager.showIdeCategoryTooltip( - context = context, - anchorView = view, - tag = tooltipTag, - ) - - // A long-click listener must return true to indicate it has consumed the event. - true - } - fun setCurrentTab( @BottomSheetViewModel.TabDef tabIndex: Int, ) { 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 e6f4eedd04..15b494d0a5 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -19,6 +19,8 @@ package com.itsaky.androidide.ui import android.content.Context import android.graphics.Bitmap +import android.os.Handler +import android.os.Looper import android.os.SystemClock import android.util.TypedValue import android.view.MotionEvent @@ -145,6 +147,23 @@ abstract class MetricsChartRenderer( */ private var appliedTextScale = Float.NaN + /** + * The gesture listener installed on the attached chart, kept so [detach] can reach its + * pending hold. Nothing else can: it lives on the chart, and a rebind installs a new one. + */ + private var axisTapListener: XAxisTapListener? = null + + /** + * How the chart's hold shows its help. + * + * A seam, not a setting: `TooltipManager` reads the docs database from device storage in its + * static initialiser and cannot be loaded off-device, so without this the whole deferred-help + * path -- when it fires, when it is given up -- could not be tested at all. + */ + @VisibleForTesting + internal var showHelp: (Context, SafeLineChart, String) -> Unit = + { context, anchor, tag -> showIdeCategoryTooltipIfPresent(context, anchor, tag) } + /** * The attached chart, or `null` when no carousel page is bound to this renderer. */ @@ -210,6 +229,12 @@ abstract class MetricsChartRenderer( open fun detach() { userHasZoomed = false appliedTextScale = Float.NaN + // A hold counting down survives the chart it was started on: the timer is on the main + // thread's queue. Left running it shows the outgoing page's help over whatever replaced + // it, and the replacement's listener -- a new object with its own null pendingHelp -- + // could never have cancelled it. + axisTapListener?.cancelPendingHelp() + axisTapListener = null chart?.removeOnLayoutChangeListener(newestWindowOnLayout) chart = null } @@ -303,7 +328,7 @@ abstract class MetricsChartRenderer( // leaving it NaN would silently adopt the library's 3f the day anyone chooses LINE. legend.formLineWidth = 1f - onChartGestureListener = XAxisTapListener(this) + onChartGestureListener = XAxisTapListener(this).also { axisTapListener = it } xAxis.valueFormatter = ElapsedTimeFormatter(sampleIntervalMillis) // One label per 15 samples keeps the window readable without crowding. @@ -408,6 +433,12 @@ abstract class MetricsChartRenderer( private inner class XAxisTapListener( private val chart: SafeLineChart, ) : OnChartGestureListener { + // An explicit handler, not View.postDelayed, which parks work on an unattached view's + // HandlerActionQueue until it attaches. The chart that receives a long press is attached, + // so that would happen to work -- but only by accident, and it puts the hold out of reach + // of a test. The same handler [performOnHold] uses, for the same reason. + private val handler = Handler(Looper.getMainLooper()) + /** The deferred half of a long press, waiting out the rest of the hold. */ private var pendingHelp: Runnable? = null @@ -433,16 +464,33 @@ abstract class MetricsChartRenderer( me: MotionEvent?, lastPerformedGesture: ChartTouchListener.ChartGesture?, ) { - pendingHelp?.let(chart::removeCallbacks) - pendingHelp = null + cancelPendingHelp() // Lifted before the hold completed: the detector ate the tap, so stand in for it. - if (!helpShown && pendingTapOnAxis) { + // + // Only for a gesture that was still a long press when it ended. A press that became a + // pan or a pinch is not a tap by any reading, and standing in for one there opened the + // sampling-rate chooser from a drag -- which clears every sample buffer, the exact + // history loss [isOnAxisBand] was narrowed to prevent. [onChartTranslate] and + // [onChartScale] give up the stand-in as the gesture escalates; this is the check for + // an escalation neither of them reports. + if (!helpShown && pendingTapOnAxis && lastPerformedGesture == ChartTouchListener.ChartGesture.LONG_PRESS) { onXAxisTap?.invoke() } helpShown = false pendingTapOnAxis = false } + /** + * Drops a hold that has not fired and the tap it was standing in for. + * + * For the end of a gesture, for a gesture that turns into something else, and for + * [detach], which is the one caller outside the touch stream. + */ + fun cancelPendingHelp() { + pendingHelp?.let(handler::removeCallbacks) + pendingHelp = null + } + override fun onChartLongPressed(me: MotionEvent?) { val y = me?.y ?: return val tag = helpTagAt(y) ?: return @@ -450,7 +498,7 @@ abstract class MetricsChartRenderer( // This arrives at the platform's own timeout -- 400ms by default, a brisk tap -- and // help at that speed is what ADFA-5554 is about. Wait out the rest of the hold and // show it only if the finger is still down; [onChartGestureEnd] cancels otherwise. - pendingHelp?.let(chart::removeCallbacks) + cancelPendingHelp() val onAxisBand = isOnAxisBand(y) pendingHelp = Runnable { @@ -461,8 +509,8 @@ abstract class MetricsChartRenderer( // BarLineChartBase.onTouchEvent never calls super, so the framework's long // press -- and its feedback -- never runs here and this is the only thing // that provides it. - showIdeCategoryTooltipIfPresent(chart.context, chart, tag) - }.also { chart.postDelayed(it, longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout()) } + showHelp(chart.context, chart, tag) + }.also { handler.postDelayed(it, longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout()) } // GestureDetector has already decided this gesture is a long press, so it will not // report the tap that would have opened the sampling-rate chooser. Remember whether @@ -486,6 +534,7 @@ abstract class MetricsChartRenderer( scaleY: Float, ) { userHasZoomed = true + abandonGesture() } override fun onChartTranslate( @@ -497,6 +546,20 @@ abstract class MetricsChartRenderer( // showNewestWindow dragged them back to the newest samples on the next tick -- once a // second -- so panning a zoomed chart appeared not to work at all. userHasZoomed = true + abandonGesture() + } + + /** + * Gives up the deferred help and the stand-in tap, because this gesture has become + * something neither is meant for. + * + * A drag or a pinch can begin from a press the detector already called a long press, and + * the finger is then still down: the hold would go on to open a tooltip over a chart the + * user is in the middle of panning, and the lift would open the sampling-rate chooser. + */ + private fun abandonGesture() { + cancelPendingHelp() + pendingTapOnAxis = false } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt b/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt deleted file mode 100644 index 97e8765212..0000000000 --- a/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt +++ /dev/null @@ -1,38 +0,0 @@ -/* - * This file is part of AndroidIDE. - * - * AndroidIDE is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * AndroidIDE is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with AndroidIDE. If not, see . - */ - -package com.itsaky.androidide.utils - -import android.view.View - -/** - * Stops this view answering a long press. - * - * `setOnLongClickListener(null)` alone is not enough: [View.setOnLongClickListener] sets - * `isLongClickable` when it installs a listener but does not unset it when the listener is - * removed, so the view goes on consuming long presses -- and showing the system's own - * "performLongClick" feedback -- for help it no longer offers. Every teardown that clears a - * long-press help listener wants both halves, so it is one call. - */ -fun View.clearLongPressHelp() { - setOnLongClickListener(null) - isLongClickable = false - // The hold is timed by a touch listener rather than the framework (ADFA-5554), so leaving that - // installed would keep the view swallowing every touch -- and performing its own clicks -- for - // help it no longer offers. - setOnTouchListener(null) -} diff --git a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt index 60a17e6014..4e3a41a07b 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt @@ -26,6 +26,7 @@ import android.widget.Button import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.utils.clearLongPressHelp +import com.itsaky.androidide.utils.displayTooltipOnLongPress import com.itsaky.androidide.utils.longPressHelpTimeoutMillis import com.itsaky.androidide.utils.performOnHold import org.junit.Test @@ -56,8 +57,14 @@ class LongPressHelpTimingTest { private var clicks = 0 + /** + * A control with a real size, which the move cases need: whether a touch is still on the view + * is measured against the view's bounds, so an unmeasured one collapses every position onto + * the same answer. + */ private fun target(): Button = Button(context).apply { + layout(0, 0, WIDTH, HEIGHT) setOnClickListener { clicks++ } performOnHold { holds++ } } @@ -65,9 +72,10 @@ class LongPressHelpTimingTest { private fun send( view: View, action: Int, - x: Float = 0f, + x: Float = CENTRE_X, + y: Float = CENTRE_Y, ) { - val event = MotionEvent.obtain(0L, 0L, action, x, 0f, 0) + val event = MotionEvent.obtain(0L, 0L, action, x, y, 0) view.dispatchTouchEvent(event) event.recycle() } @@ -75,6 +83,14 @@ class LongPressHelpTimingTest { /** Runs the main looper forward by [millis] of virtual time. */ private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) + /** + * Runs whatever is already due on the main looper without advancing the clock. + * + * The click is posted rather than performed inside the touch dispatch, as the framework does + * it, so nothing has clicked until the looper turns. + */ + private fun drain() = shadowOf(Looper.getMainLooper()).idle() + @Test fun `a press past the platform timeout but short of the hold still clicks`() { // The regression the obvious fix introduces, and the reason this class exists. The @@ -85,6 +101,7 @@ class LongPressHelpTimingTest { send(view, MotionEvent.ACTION_DOWN) elapse(ViewConfiguration.getLongPressTimeout() + 100L) send(view, MotionEvent.ACTION_UP) + drain() assertThat(clicks).isEqualTo(1) assertThat(holds).isEqualTo(0) @@ -97,6 +114,7 @@ class LongPressHelpTimingTest { send(view, MotionEvent.ACTION_DOWN) elapse(50L) send(view, MotionEvent.ACTION_UP) + drain() assertThat(clicks).isEqualTo(1) assertThat(holds).isEqualTo(0) @@ -109,21 +127,59 @@ class LongPressHelpTimingTest { send(view, MotionEvent.ACTION_DOWN) elapse(longPressHelpTimeoutMillis() + 50L) send(view, MotionEvent.ACTION_UP) + drain() assertThat(holds).isEqualTo(1) assertThat(clicks).isEqualTo(0) } @Test - fun `a press that wanders off the control does neither`() { + fun `the click is posted, not run inside the touch that ended it`() { + // View.onTouchEvent posts its click so the pressed state is drawn before the action runs, + // and these actions open dialogs and re-page the carousel from inside the dispatch of the + // event that triggered them. Taking the touch over means taking that over too. + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_UP) + + assertThat(clicks).isEqualTo(0) + drain() + assertThat(clicks).isEqualTo(1) + } + + @Test + fun `a press that rolls but stays on the control still clicks`() { + // The framework gives up on a press when the finger leaves the view grown by the slop -- + // not when it has travelled slop from where it went down. Measured from the down point + // instead, an ordinary thumb tap on a large target rolls far enough to cancel its own + // click without ever leaving the control, and every one of these targets is large: the + // carousel strip is the full width of the editor. + val view = target() + val slop = ViewConfiguration.get(context).scaledTouchSlop + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_MOVE, x = CENTRE_X + slop + 10f) + send(view, MotionEvent.ACTION_UP) + drain() + + assertThat(clicks).isEqualTo(1) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `a press that leaves the control does neither`() { val view = target() val slop = ViewConfiguration.get(context).scaledTouchSlop send(view, MotionEvent.ACTION_DOWN) elapse(100L) - send(view, MotionEvent.ACTION_MOVE, x = slop + 10f) + send(view, MotionEvent.ACTION_MOVE, x = WIDTH + slop + 10f) elapse(longPressHelpTimeoutMillis()) send(view, MotionEvent.ACTION_UP) + drain() // The framework treats a drag out of a view as neither, so taking the touch over means // saying so rather than inventing a third behaviour. @@ -145,11 +201,28 @@ class LongPressHelpTimingTest { } @Test - fun `the hold is longer than the platform's, and never shorter`() { - // The floor matters: the platform value is exposed as an accessibility "touch and hold - // delay", and someone who lengthened it meant to. - assertThat(longPressHelpTimeoutMillis()).isAtLeast(800L) - assertThat(longPressHelpTimeoutMillis()).isAtLeast(ViewConfiguration.getLongPressTimeout().toLong()) + fun `a lengthened touch-and-hold delay is doubled, not ignored`() { + // Asserting isAtLeast against the live platform value pins nothing: maxOf(x * 2, 800) is + // at least 800 and at least x for every x by construction, so the whole rule could be + // deleted and such a test would still pass. Named values, and each of the two terms + // decides one of them. + // + // The delay is exposed as an accessibility setting, and someone who lengthened it meant + // to -- so the hold has to grow with it rather than staying at the floor. + assertThat(longPressHelpTimeoutMillis(platformTimeoutMillis = 1_000L)).isEqualTo(2_000L) + } + + @Test + fun `a shortened touch-and-hold delay still gets the floor`() { + // Doubling alone would put help back inside a brisk tap, which is the defect. + assertThat(longPressHelpTimeoutMillis(platformTimeoutMillis = 100L)).isEqualTo(800L) + } + + @Test + fun `the platform default lands on the floor`() { + // 400ms doubled is exactly the floor, so the two terms agree at the value almost every + // device reports -- which is why neither can be tested at it. + assertThat(longPressHelpTimeoutMillis(platformTimeoutMillis = 400L)).isEqualTo(800L) } @Test @@ -166,4 +239,48 @@ class LongPressHelpTimingTest { assertThat(holds).isEqualTo(0) assertThat(view.isLongClickable).isFalse() } + + @Test + fun `clearing the help cancels a hold already counting down`() { + // The teardown runs while a finger is down -- the carousel unbinds, the strip is replaced, + // the sheet is torn down. The timer is on the main thread's queue rather than on the view, + // so clearing the listeners does not reach it: held in a closure it was unreachable + // altogether, and the tooltip appeared over a control that had just been unwired. + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(100L) + view.clearLongPressHelp() + elapse(longPressHelpTimeoutMillis()) + + assertThat(holds).isEqualTo(0) + } + + @Test + fun `re-wiring with a blank tag takes the previous tag's help away`() { + // A blank tag says this view offers no help, which has to replace whatever was wired here + // before. Returning early instead left the previous listeners in place, still timing holds + // and still swallowing every touch. + val view = target() + view.displayTooltipOnLongPress(context, tooltipTag = "") + + send(view, MotionEvent.ACTION_DOWN) + elapse(longPressHelpTimeoutMillis() + 50L) + send(view, MotionEvent.ACTION_UP) + drain() + + assertThat(holds).isEqualTo(0) + assertThat(view.isLongClickable).isFalse() + } + + private companion object { + /** Big enough that a roll of one touch slop is still well inside it. */ + const val WIDTH = 400 + + const val HEIGHT = 200 + + const val CENTRE_X = WIDTH / 2f + + const val CENTRE_Y = HEIGHT / 2f + } } diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt new file mode 100644 index 0000000000..019a406191 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt @@ -0,0 +1,210 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.os.Looper +import android.view.MotionEvent +import android.view.ViewConfiguration +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.listener.ChartTouchListener +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.NetworkUsageWatcher +import com.itsaky.androidide.utils.longPressHelpTimeoutMillis +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import java.util.concurrent.TimeUnit + +/** + * When the chart answers a hold with help, and when it gives that help up (ADFA-5554). + * + * The platform reports its long press at 400ms, which is a brisk tap, so the chart waits out the + * rest of the hold before showing anything. Two things have to be true of that wait: it happens, + * and it is abandoned when the gesture turns into something a hold is not -- a pan, a pinch, or a + * page being unbound underneath it. + * + * The help itself is a seam rather than a real tooltip. `TooltipManager` reads the docs database + * from device storage in its static initialiser and cannot be loaded off-device, which is the same + * reason the renderer separates deciding a help tag from showing one. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsChartHoldHelpTest { + private val context = ApplicationProvider.getApplicationContext() + + private var taps = 0 + + private var helps = 0 + + private lateinit var renderer: NetworkUsageChartRenderer + + private fun laidOutChart(): SafeLineChart { + val chart = SafeLineChart(context) + // Any concrete renderer will do -- the hold is the base class's, and every page wires it + // the same way. + renderer = + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + }, + ) + renderer.attach(chart) + renderer.onXAxisTap = { taps++ } + renderer.showHelp = { _, _, _ -> helps++ } + + chart.layOutAndDraw() + return chart + } + + private fun eventAt(y: Float) = MotionEvent.obtain(0L, 0L, MotionEvent.ACTION_MOVE, 10f, y, 0) + + /** The platform's own long press, which is where the chart's hold starts counting from. */ + private fun longPressAt( + chart: SafeLineChart, + y: Float, + ) { + val event = eventAt(y) + chart.onChartGestureListener.onChartLongPressed(event) + event.recycle() + } + + private fun panBy( + chart: SafeLineChart, + dx: Float, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartTranslate(event, dx, 0f) + event.recycle() + } + + private fun endGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartGestureEnd(event, gesture) + event.recycle() + } + + /** Runs the main looper forward by [millis] of virtual time. */ + private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) + + /** The rest of the hold, after the platform's long press has already been reported. */ + private fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L + + /** A y inside the plot, where a hold means help for the page rather than for the axis. */ + private fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f + + @Test + fun `a press held past the hold shows help`() { + val chart = laidOutChart() + + longPressAt(chart, insidePlot(chart)) + elapse(remainderOfHold()) + + // The deferral is the point of ADFA-5554: the platform reports its long press at 400ms, + // which is a brisk tap, and help at that speed is what the ticket is about. + assertThat(helps).isEqualTo(1) + } + + @Test + fun `a press lifted before the hold completes shows no help`() { + val chart = laidOutChart() + + longPressAt(chart, insidePlot(chart)) + endGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + @Test + fun `a press on the axis lifted before the hold still opens the chooser`() { + val chart = laidOutChart() + + // The detector has already called this a long press, so it will not report the tap. The + // stand-in is what keeps a brisk press on the axis doing what it always did. + longPressAt(chart, chart.viewPortHandler.contentBottom() + 1f) + endGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + + assertThat(taps).isEqualTo(1) + assertThat(helps).isEqualTo(0) + } + + @Test + fun `a press on the axis that becomes a pan does not open the chooser`() { + val chart = laidOutChart() + + // A drag begins from a press the detector has already called a long press, so the + // stand-in fired for it: panning the chart opened the sampling-rate chooser, and picking + // a rate there clears every buffer -- the history loss the band's lower bound exists to + // prevent, reached by another route. + longPressAt(chart, chart.viewPortHandler.contentBottom() + 1f) + panBy(chart, -50f) + endGesture(chart, ChartTouchListener.ChartGesture.DRAG) + + assertThat(taps).isEqualTo(0) + } + + @Test + fun `a press that becomes a pan shows no help either`() { + val chart = laidOutChart() + + // The finger is still down and still dragging when the hold would come due, so the + // tooltip opened over a chart the user was in the middle of panning. + longPressAt(chart, insidePlot(chart)) + panBy(chart, -50f) + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + @Test + fun `a press that becomes a pinch shows no help`() { + val chart = laidOutChart() + + longPressAt(chart, insidePlot(chart)) + val event = eventAt(0f) + chart.onChartGestureListener.onChartScale(event, 1.2f, 1.2f) + event.recycle() + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + @Test + fun `detaching cancels a hold already counting down`() { + val chart = laidOutChart() + + // The timer is on the main thread's queue, not on the chart, so unbinding the page does + // not reach it. Worse, the rebind installs a fresh listener whose own pending hold is + // null -- so nobody could have cancelled the old one, and it fired the outgoing page's + // help over whatever replaced it. + longPressAt(chart, insidePlot(chart)) + renderer.detach() + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + private companion object { + /** Longer than the chart's visible window, matching the axis-tap tests' fixture. */ + const val SAMPLES = 200 + } +} diff --git a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt index 24fb584b7b..49e826ce92 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt @@ -7,9 +7,9 @@ import android.view.HapticFeedbackConstants import android.view.MotionEvent import android.view.View import android.view.ViewConfiguration +import com.itsaky.androidide.idetooltips.R import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager -import kotlin.math.abs /** * Shows [tag]'s tooltip (under [category]) anchored to [anchor], or does nothing if [tag] is @@ -52,8 +52,19 @@ fun showIdeCategoryTooltipIfPresent( * * Never *shorter* than the platform's value: that setting is exposed as an accessibility * "touch and hold delay", and someone who has lengthened it did so deliberately. + * + * [platformTimeoutMillis] is a parameter only so a test can name one. Asserted against the live + * value, both the doubling and the floor are implied by the expression itself and a test of them + * pins nothing. */ -fun longPressHelpTimeoutMillis(): Long = maxOf(ViewConfiguration.getLongPressTimeout() * 2L, 800L) +fun longPressHelpTimeoutMillis(platformTimeoutMillis: Long = ViewConfiguration.getLongPressTimeout().toLong()): Long = + maxOf(platformTimeoutMillis * PLATFORM_TIMEOUT_MULTIPLE, MIN_HOLD_MILLIS) + +/** The shortest hold that will ever be asked for, whatever the platform's own timeout. */ +private const val MIN_HOLD_MILLIS = 800L + +/** How much longer than the platform's long press a hold is, above the floor. */ +private const val PLATFORM_TIMEOUT_MULTIPLE = 2L /** * Shows [tooltipTag]'s tooltip (under [tooltipCategory]) when this view is held for @@ -81,6 +92,10 @@ fun View.displayTooltipOnLongPress( holdMillis: Long = longPressHelpTimeoutMillis(), ) { if (tooltipTag.isBlank()) { + // Not a no-op. This call replaces whatever help was wired here before, and a blank tag + // says there is none now; returning early would leave the previous tag's listeners + // answering holds -- and swallowing every touch -- for help this view no longer offers. + clearLongPressHelp() return } @@ -106,48 +121,136 @@ fun View.performOnHold( holdMillis: Long = longPressHelpTimeoutMillis(), onHold: () -> Unit, ) { - val slop = ViewConfiguration.get(context).scaledTouchSlop + // The hold only. [displayTooltipOnLongPress] installs its long-click listener first and this + // second, so clearing that here would take away what the caller had just wired. + clearOnHold() + val listener = HoldTouchListener(this, holdMillis, onHold) + // Tagged so [clearLongPressHelp] can tell that the listener it is about to remove is this one. + setTag(R.id.tooltip_hold_listener, listener) + setOnTouchListener(listener) +} + +/** + * Stops this view answering a hold or a long press with help, and cancels one already timing. + * + * `setOnLongClickListener(null)` alone is not enough: [View.setOnLongClickListener] sets + * `isLongClickable` when it installs a listener but does not unset it when the listener is + * removed, so the view goes on consuming long presses -- and showing the system's own + * "performLongClick" feedback -- for help it no longer offers. The hold half is [clearOnHold]. + */ +fun View.clearLongPressHelp() { + setOnLongClickListener(null) + isLongClickable = false + clearOnHold() +} + +/** + * Stops this view timing a hold, and cancels one already counting down. + * + * The half of [clearLongPressHelp] that undoes [performOnHold], separately callable because a + * caller that installed only a hold should be able to undo only a hold. + * + * The touch listener is removed only when [performOnHold] is the one that installed it, which the + * tag says. Five of the six views [clearLongPressHelp] is called on are wired through the + * framework's long click and never had one, and a blanket `setOnTouchListener(null)` there would + * silently take away an unrelated listener the next contributor adds. + */ +fun View.clearOnHold() { + val hold = getTag(R.id.tooltip_hold_listener) as? HoldTouchListener ?: return + // A hold already counting down outlives its listener: the timer is on the main thread's + // queue, not on the view. Left running it fires against a control that has just been unwired + // -- or a carousel page that has just been replaced (ADFA-5554). + hold.cancel() + setTag(R.id.tooltip_hold_listener, null) + setOnTouchListener(null) +} + +/** + * Times a hold on [view] and stands in for the framework's own press handling while it does. + * + * A class rather than a lambda so the pending hold can be cancelled from outside the touch stream; + * captured in a closure it was unreachable, and a teardown could only stop the *next* hold. + */ +private class HoldTouchListener( + private val view: View, + private val holdMillis: Long, + private val onHold: () -> Unit, +) : View.OnTouchListener { // An explicit handler, not View.postDelayed: a view not attached to a window parks posted work // in its HandlerActionQueue and only runs it on attach, so the hold would never time out. - val handler = Handler(Looper.getMainLooper()) - var held = false - var holding = false - var downX = 0f - var downY = 0f - val fire = + private val handler = Handler(Looper.getMainLooper()) + + private val slop = ViewConfiguration.get(view.context).scaledTouchSlop + + private var held = false + + private var holding = false + + private val fire = Runnable { held = true - isPressed = false + view.isPressed = false onHold() } - setOnTouchListener { view, event -> + fun cancel() { + holding = false + handler.removeCallbacks(fire) + view.isPressed = false + } + + /** + * Whether a touch at ([x], [y]) is still on the view, by the framework's rule. + * + * `View.onTouchEvent` gives up on a press when `!pointInView(x, y, mTouchSlop)` -- when the + * finger leaves the view's bounds grown by the slop, not when it has travelled slop from + * where it went down. Measured from the down point instead, an ordinary thumb tap on a large + * target rolls far enough to cancel its own click without ever leaving the control, and the + * carousel strip is the full width of the editor. + */ + private fun isInside( + x: Float, + y: Float, + ): Boolean = x >= -slop && y >= -slop && x < view.width + slop && y < view.height + slop + + override fun onTouch( + v: View, + event: MotionEvent, + ): Boolean { when (event.actionMasked) { MotionEvent.ACTION_DOWN -> { held = false holding = true - downX = event.x - downY = event.y - view.isPressed = true + v.isPressed = true + // The framework starts the ripple from the touch point. Without this every ripple + // on these controls begins at the centre of the drawable instead. + v.drawableHotspotChanged(event.x, event.y) handler.postDelayed(fire, holdMillis) } MotionEvent.ACTION_MOVE -> { - if (holding && (abs(event.x - downX) > slop || abs(event.y - downY) > slop)) { - // Wandered off the control: neither a click nor help, which is how the - // framework treats a drag out of a view. Taking the touch over means saying so. + if (holding && !isInside(event.x, event.y)) { + // Left the control: neither a click nor help, which is how the framework + // treats a drag out of a view. Taking the touch over means saying so. holding = false handler.removeCallbacks(fire) - view.isPressed = false + v.isPressed = false } } MotionEvent.ACTION_UP -> { handler.removeCallbacks(fire) - view.isPressed = false + v.isPressed = false // The click belongs to a press that stayed put and did not become a hold. if (holding && !held) { - view.performClick() + // Posted rather than called here, as View.onTouchEvent does, so the pressed + // state is drawn before the action runs -- these open dialogs and re-page the + // carousel from inside the dispatch of the event that triggered them. + // + // Through this handler and not View.post, which parks work on an unattached + // view's HandlerActionQueue and returns true having run nothing. Same trap as + // the hold timer, one method along. + handler.post { v.performClick() } } holding = false } @@ -155,9 +258,9 @@ fun View.performOnHold( MotionEvent.ACTION_CANCEL -> { holding = false handler.removeCallbacks(fire) - view.isPressed = false + v.isPressed = false } } - true + return true } } diff --git a/idetooltips/src/main/res/values/ids.xml b/idetooltips/src/main/res/values/ids.xml new file mode 100644 index 0000000000..53b7a6f9d8 --- /dev/null +++ b/idetooltips/src/main/res/values/ids.xml @@ -0,0 +1,8 @@ + + + + + From be353d9bb2f2dc4b0e42b560cca3fff288000ed8 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 18:58:54 -0700 Subject: [PATCH 3/6] ADFA-5554: fix the second review round, and the heap that hid it Nine findings from the xhigh review, and one thing I got wrong twice. A two-finger tap on the axis band still opened the sampling-rate chooser. The gate added last round asks whether the gesture ended as a LONG_PRESS, and ChartTouchListener never assigns its mLastGesture from ACTION_POINTER_DOWN -- only from a drag, a zoom, a long press, a tap or a fling -- so a second finger that lands and lifts without moving leaves the label untouched and nothing reports a move. The chooser clears every sample buffer, and the carousel undocks on a two-finger tap, so one gesture undocked the strip and threw away the history it was showing. SafeLineChart now reports the second pointer, because MPAndroidChart's listener cannot, and the renderer gives the gesture up on it. attach() skipped the teardown when handed the chart it already had, so a rebind of a bound holder installed a second gesture listener while the first stayed queued with a hold nothing could cancel, and added a second layout listener that one removal cannot undo. It tears down unconditionally now. detach() also left the chart holding the listener, which is an inner class holding the renderer -- so a detached chart kept the whole renderer alive and answered a later press through a listener whose own chart reference was null. The remaining hold was computed by subtracting the platform timeout from the total, which assumes GestureDetector reports a long press exactly that long after the finger landed. It does not: below Q it adds TAP_TIMEOUT, and it caches the timeout in a static read at class-load, so someone who lengthens the accessibility touch-and-hold delay moves the buttons' hold and not the chart's. minSdk here is 28. It is measured from the event's own downTime now, and the test drives a press reported 700ms late. The stand-in tap ran inside the chart's touch dispatch while the click in performOnHold, added in the same PR, was posted for exactly the reason that is wrong -- it opens a dialog. One PR, two paths, opposite rules. Posted now. On the view side: the click ignored isClickable, which is how the carousel dims the arrow at either end while keeping it able to answer a hold, so a tap on a dimmed arrow played the click sound and announced a click for a control the screen reader is told is unavailable. cancel() removed the hold but not the posted click, which is unheld and so unreachable, so a teardown between the lift and the looper's next turn still clicked a control it had just unwired. And a second finger was not noticed at all, so the undock gesture also paged the carousel. One finding is deliberately not fixed. View.CheckForLongPress refuses to fire once the view's window has gone, and reproducing that guard here was tried and backed out: a Robolectric view is never window-attached, so it turned every timing test into a no-op, and attaching one needs the activity harness that takes this JVM down. What it protects is already protected where it matters -- TooltipManager re-checks isAttachedToWindow, and clearLongPressHelp runs from every teardown in this module. The reasoning is in the code, not just here. The heap, which is the thing I got wrong. Adding these tests killed the test JVM: exit 3, no failure recorded, and the tests that had not run reported zeroes that read as assertion failures. I diagnosed that twice as a mysterious Robolectric interaction -- "unbounded, since 4g fails too" -- and split test classes around it. It was heap the whole time. The 4g experiment set maxHeapSize in :app's own testOptions, and the root subprojects block overwrites that, so the run was never at 4g. It is 2g in the root now, where the setting actually takes effect, with a note saying why. Robolectric builds a sandbox per distinct @Config and :app now has enough of them. The two splits those wrong diagnoses produced are kept: they group sensibly either way, and re-merging them is churn for no gain. But they were not necessary, and the comment in one of them said so wrongly -- that is corrected. Every fix above has a test that fails without it, except the two noted: the window-attach guard, which is not implemented, and detach() releasing the gesture listener, which is a leak rather than a behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MetricsChartRenderer.kt | 37 +++- .../com/itsaky/androidide/ui/SafeLineChart.kt | 19 ++ .../androidide/ui/LongPressHelpTimingTest.kt | 68 +++++++ .../ui/MetricsChartGestureTeardownTest.kt | 190 ++++++++++++++++++ .../androidide/ui/MetricsChartHoldHelpTest.kt | 63 ++++-- build.gradle.kts | 9 +- .../com/itsaky/androidide/utils/ViewUtils.kt | 45 ++++- 7 files changed, 408 insertions(+), 23 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 15b494d0a5..f7be8f449f 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -25,7 +25,6 @@ import android.os.SystemClock import android.util.TypedValue import android.view.MotionEvent import android.view.View -import android.view.ViewConfiguration import androidx.annotation.CallSuper import androidx.annotation.UiThread import androidx.annotation.VisibleForTesting @@ -214,7 +213,12 @@ abstract class MetricsChartRenderer( // oldest-samples symptom this ticket was filed for -- reachable only after a pan, which is // why the resume paths reproduce it and a fresh chart never does. Subclasses clear their own // per-chart state through the same override. - this.chart?.let { outgoing -> if (outgoing !== chart) detach() } + // + // Unconditionally, including when the same chart is handed back. Skipping the teardown + // there let [configure] install a second gesture listener while the first stayed queued on + // the main thread with a hold nothing could reach, and added a second layout listener that + // one removeOnLayoutChangeListener cannot undo. + detach() this.chart = chart configure(chart) chart.addOnLayoutChangeListener(newestWindowOnLayout) @@ -235,6 +239,11 @@ abstract class MetricsChartRenderer( // could never have cancelled it. axisTapListener?.cancelPendingHelp() axisTapListener = null + // The chart holds the listener, and the listener is an inner class holding this renderer, + // so a detached chart left with it keeps the whole renderer alive -- and answers a later + // press through a listener whose own chart reference is now null. + chart?.onChartGestureListener = null + chart?.onSecondPointerDown = null chart?.removeOnLayoutChangeListener(newestWindowOnLayout) chart = null } @@ -329,6 +338,10 @@ abstract class MetricsChartRenderer( legend.formLineWidth = 1f onChartGestureListener = XAxisTapListener(this).also { axisTapListener = it } + // A two-finger tap is the carousel's undock gesture, and it starts as a press like any + // other. Without this the stand-in tap fired for it and opened the sampling-rate + // chooser -- so one gesture both undocked the strip and cleared every buffer. + onSecondPointerDown = { axisTapListener?.abandonGesture() } xAxis.valueFormatter = ElapsedTimeFormatter(sampleIntervalMillis) // One label per 15 samples keeps the window readable without crowding. @@ -474,7 +487,11 @@ abstract class MetricsChartRenderer( // [onChartScale] give up the stand-in as the gesture escalates; this is the check for // an escalation neither of them reports. if (!helpShown && pendingTapOnAxis && lastPerformedGesture == ChartTouchListener.ChartGesture.LONG_PRESS) { - onXAxisTap?.invoke() + // Posted, not called here. This runs inside the chart's onTouchEvent, and the tap + // opens a dialog; showing one mid-dispatch leaves the chart's touch state and its + // velocity tracker part-way through a gesture. performOnHold posts its click for + // the same reason, and the two paths should not disagree. + handler.post { onXAxisTap?.invoke() } } helpShown = false pendingTapOnAxis = false @@ -492,7 +509,8 @@ abstract class MetricsChartRenderer( } override fun onChartLongPressed(me: MotionEvent?) { - val y = me?.y ?: return + val event = me ?: return + val y = event.y val tag = helpTagAt(y) ?: return // This arrives at the platform's own timeout -- 400ms by default, a brisk tap -- and @@ -500,6 +518,13 @@ abstract class MetricsChartRenderer( // show it only if the finger is still down; [onChartGestureEnd] cancels otherwise. cancelPendingHelp() val onAxisBand = isOnAxisBand(y) + // From the event's own downTime, not by subtracting the platform timeout from the hold. + // GestureDetector does not report a long press exactly getLongPressTimeout() after the + // finger landed: below Android Q it adds TAP_TIMEOUT, and it caches LONGPRESS_TIMEOUT + // in a static read once at class-load, so a user who lengthens the accessibility + // touch-and-hold delay moves the buttons' hold and not this one. minSdk here is 28. + val elapsed = SystemClock.uptimeMillis() - event.downTime + val remaining = (longPressHelpTimeoutMillis() - elapsed).coerceAtLeast(0L) pendingHelp = Runnable { pendingHelp = null @@ -510,7 +535,7 @@ abstract class MetricsChartRenderer( // press -- and its feedback -- never runs here and this is the only thing // that provides it. showHelp(chart.context, chart, tag) - }.also { handler.postDelayed(it, longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout()) } + }.also { handler.postDelayed(it, remaining) } // GestureDetector has already decided this gesture is a long press, so it will not // report the tap that would have opened the sampling-rate chooser. Remember whether @@ -557,7 +582,7 @@ abstract class MetricsChartRenderer( * the finger is then still down: the hold would go on to open a tooltip over a chart the * user is in the middle of panning, and the lift would open the sampling-rate chooser. */ - private fun abandonGesture() { + fun abandonGesture() { cancelPendingHelp() pendingTapOnAxis = false } 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 b5acd0f684..f7916f9dce 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -21,6 +21,7 @@ import android.content.Context import android.graphics.Canvas import android.graphics.Paint import android.util.AttributeSet +import android.view.MotionEvent import com.github.mikephil.charting.charts.LineChart import com.github.mikephil.charting.components.YAxis import org.slf4j.LoggerFactory @@ -84,6 +85,24 @@ class SafeLineChart : LineChart { /** Reused by [drawBackgroundSpans]: two (x, y) pairs, transformed in place. */ private val spanPoints = FloatArray(4) + /** + * Called when a second finger lands, which ends whatever one-finger gesture was in progress. + * + * MPAndroidChart's gesture listener cannot report this. `ChartTouchListener` assigns its + * `mLastGesture` only from a drag, a zoom, a long press, a tap or a fling -- never from + * ACTION_POINTER_DOWN -- so a second finger that lands and lifts without moving leaves the + * gesture still labelled LONG_PRESS, and the listener cannot tell that from a finger simply + * being lifted (ADFA-5554). + */ + var onSecondPointerDown: (() -> Unit)? = null + + override fun onTouchEvent(event: MotionEvent): Boolean { + if (event.actionMasked == MotionEvent.ACTION_POINTER_DOWN) { + onSecondPointerDown?.invoke() + } + return super.onTouchEvent(event) + } + /** * Draws the spans immediately after the grid background, which is an opaque fill of the plot: a * span painted before [onDraw] delegates upwards is covered by it and never reaches the screen. diff --git a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt index 4e3a41a07b..7b81c07063 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt @@ -273,6 +273,74 @@ class LongPressHelpTimingTest { assertThat(view.isLongClickable).isFalse() } + @Test + fun `a control that does not answer taps is not clicked`() { + // View.onTouchEvent performs a click only for a clickable view, and taking the touch over + // means taking that test over too. The carousel dims the arrow at either end by clearing + // isClickable rather than isEnabled -- deliberately, so it still answers a hold -- so + // without this a tap on the dimmed arrow played the click sound and announced a click for + // a control the screen reader is being told is unavailable. + val view = target() + view.isClickable = false + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_UP) + drain() + + assertThat(clicks).isEqualTo(0) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `a control that does not answer taps still answers a hold`() { + // The other half, and the reason isClickable was chosen over isEnabled in the first place. + val view = target() + view.isClickable = false + + send(view, MotionEvent.ACTION_DOWN) + elapse(longPressHelpTimeoutMillis() + 50L) + send(view, MotionEvent.ACTION_UP) + drain() + + assertThat(holds).isEqualTo(1) + assertThat(clicks).isEqualTo(0) + } + + @Test + fun `a second finger gives up the press`() { + // The carousel undocks on a two-finger tap anywhere in the strip, and one of those fingers + // lands on a control. Counting it as a press meant the gesture both undocked the strip and + // paged it, or held long enough to open that button's help over a strip on its way out. + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_POINTER_DOWN) + elapse(longPressHelpTimeoutMillis()) + send(view, MotionEvent.ACTION_UP) + drain() + + assertThat(clicks).isEqualTo(0) + assertThat(holds).isEqualTo(0) + } + + @Test + fun `clearing the help takes back a click that has not run yet`() { + // The click is posted, so there is a turn of the looper between the finger lifting and the + // action running. A teardown landing in it -- the sheet detaching, the carousel unbinding + // -- would otherwise still click a control it has just unwired. + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + elapse(50L) + send(view, MotionEvent.ACTION_UP) + view.clearLongPressHelp() + drain() + + assertThat(clicks).isEqualTo(0) + } + private companion object { /** Big enough that a roll of one touch slop is still well inside it. */ const val WIDTH = 400 diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt new file mode 100644 index 0000000000..2243484f4e --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt @@ -0,0 +1,190 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.os.Looper +import android.os.SystemClock +import android.view.MotionEvent +import android.view.ViewConfiguration +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.listener.ChartTouchListener +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.NetworkUsageWatcher +import com.itsaky.androidide.utils.longPressHelpTimeoutMillis +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import java.util.concurrent.TimeUnit + +/** + * What happens to a hold in progress when the gesture or the chart under it goes away (ADFA-5554). + * + * A hold is a timer on the main thread's queue, not state on the view, so it outlives whatever + * started it: a second finger landing, the page being rebound, the renderer letting the chart go. + * Each of those has to reach the timer, and none of them can once the listener holding it has been + * replaced. + * + * Split from [MetricsChartHoldHelpTest] only because these three are about teardown rather than + * timing. An earlier version of this comment blamed a Robolectric interaction: the suite was + * killing the test JVM as cases were added, and splitting appeared to help. It was heap -- + * Robolectric builds a sandbox per distinct `@Config` and `:app` had outgrown the 1g in the root + * build file. The split is kept because it reads better, not because it fixes anything. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsChartGestureTeardownTest { + private val context = ApplicationProvider.getApplicationContext() + + private var taps = 0 + + private var helps = 0 + + private lateinit var renderer: NetworkUsageChartRenderer + + private fun laidOutChart(): SafeLineChart { + val chart = SafeLineChart(context) + // Any concrete renderer will do -- the hold is the base class's, and every page wires it + // the same way. + renderer = + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + }, + ) + renderer.attach(chart) + renderer.onXAxisTap = { taps++ } + renderer.showHelp = { _, _, _ -> helps++ } + + chart.layOutAndDraw() + return chart + } + + /** + * An event whose finger landed [sincePressMillis] ago. + * + * The down time is what the chart measures its remaining hold from, so it has to be real here. + * Defaults to the platform's long-press timeout, which is when a detector on a current device + * reports one. + */ + private fun eventAt( + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ): MotionEvent { + val now = SystemClock.uptimeMillis() + return MotionEvent.obtain(now - sincePressMillis, now, MotionEvent.ACTION_MOVE, 10f, y, 0) + } + + /** The platform's own long press, which is where the chart's hold started counting from. */ + private fun longPressAt( + chart: SafeLineChart, + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ) { + val event = eventAt(y, sincePressMillis) + chart.onChartGestureListener.onChartLongPressed(event) + event.recycle() + } + + private fun panBy( + chart: SafeLineChart, + dx: Float, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartTranslate(event, dx, 0f) + event.recycle() + } + + private fun endGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartGestureEnd(event, gesture) + event.recycle() + } + + /** Runs the main looper forward by [millis] of virtual time. */ + private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) + + /** + * Runs what is already due on the main looper without advancing the clock. + * + * The stand-in tap is posted rather than invoked inside the chart's touch dispatch, so nothing + * has been tapped until the looper turns. + */ + private fun drain() = shadowOf(Looper.getMainLooper()).idle() + + /** The rest of the hold, after a long press reported at the platform's own timeout. */ + private fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L + + /** A y inside the plot, where a hold means help for the page rather than for the axis. */ + private fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f + + @Test + fun `a second finger gives up the gesture, even without a move`() { + val chart = laidOutChart() + + // The carousel undocks on a two-finger tap, and that starts as a press like any other. + // MPAndroidChart cannot report it -- ACTION_POINTER_DOWN never touches its mLastGesture -- + // so the gesture still ends labelled LONG_PRESS and the stand-in tap fired, opening the + // sampling-rate chooser. Picking a rate there clears every buffer, so one gesture both + // undocked the strip and threw away the history it was showing. + longPressAt(chart, chart.viewPortHandler.contentBottom() + 1f) + chart.onSecondPointerDown?.invoke() + endGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + elapse(remainderOfHold()) + drain() + + assertThat(taps).isEqualTo(0) + assertThat(helps).isEqualTo(0) + } + + @Test + fun `re-attaching the same chart leaves no second listener behind`() { + val chart = laidOutChart() + + // attach() used to skip the teardown when handed the chart it already had, so configure() + // installed a second gesture listener while the first stayed queued with a hold nothing + // could reach. A rebind of a bound holder does exactly that. + longPressAt(chart, insidePlot(chart)) + renderer.attach(chart) + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + @Test + fun `detaching cancels a hold already counting down`() { + val chart = laidOutChart() + + // The timer is on the main thread's queue, not on the chart, so unbinding the page does + // not reach it. Worse, the rebind installs a fresh listener whose own pending hold is + // null -- so nobody could have cancelled the old one, and it fired the outgoing page's + // help over whatever replaced it. + longPressAt(chart, insidePlot(chart)) + renderer.detach() + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + private companion object { + const val SAMPLES = 200 + } +} diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt index 019a406191..7e799a6823 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.ui import android.content.Context import android.os.Looper +import android.os.SystemClock import android.view.MotionEvent import android.view.ViewConfiguration import androidx.test.core.app.ApplicationProvider @@ -72,14 +73,28 @@ class MetricsChartHoldHelpTest { return chart } - private fun eventAt(y: Float) = MotionEvent.obtain(0L, 0L, MotionEvent.ACTION_MOVE, 10f, y, 0) + /** + * An event whose finger landed [sincePressMillis] ago. + * + * The down time is what the chart measures its remaining hold from, so it has to be real here. + * Defaults to the platform's long-press timeout, which is when a detector on a current device + * reports one. + */ + private fun eventAt( + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ): MotionEvent { + val now = SystemClock.uptimeMillis() + return MotionEvent.obtain(now - sincePressMillis, now, MotionEvent.ACTION_MOVE, 10f, y, 0) + } - /** The platform's own long press, which is where the chart's hold starts counting from. */ + /** The platform's own long press, which is where the chart's hold started counting from. */ private fun longPressAt( chart: SafeLineChart, y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), ) { - val event = eventAt(y) + val event = eventAt(y, sincePressMillis) chart.onChartGestureListener.onChartLongPressed(event) event.recycle() } @@ -105,7 +120,15 @@ class MetricsChartHoldHelpTest { /** Runs the main looper forward by [millis] of virtual time. */ private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) - /** The rest of the hold, after the platform's long press has already been reported. */ + /** + * Runs what is already due on the main looper without advancing the clock. + * + * The stand-in tap is posted rather than invoked inside the chart's touch dispatch, so nothing + * has been tapped until the looper turns. + */ + private fun drain() = shadowOf(Looper.getMainLooper()).idle() + + /** The rest of the hold, after a long press reported at the platform's own timeout. */ private fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L /** A y inside the plot, where a hold means help for the page rather than for the axis. */ @@ -143,6 +166,11 @@ class MetricsChartHoldHelpTest { longPressAt(chart, chart.viewPortHandler.contentBottom() + 1f) endGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + // Nothing has been tapped inside the dispatch itself: the tap opens a dialog, and doing + // that mid-gesture leaves the chart's touch state part-way through one. + assertThat(taps).isEqualTo(0) + drain() + assertThat(taps).isEqualTo(1) assertThat(helps).isEqualTo(0) } @@ -158,6 +186,7 @@ class MetricsChartHoldHelpTest { longPressAt(chart, chart.viewPortHandler.contentBottom() + 1f) panBy(chart, -50f) endGesture(chart, ChartTouchListener.ChartGesture.DRAG) + drain() assertThat(taps).isEqualTo(0) } @@ -189,22 +218,30 @@ class MetricsChartHoldHelpTest { } @Test - fun `detaching cancels a hold already counting down`() { + fun `the hold is measured from the finger landing, not from when the press was reported`() { val chart = laidOutChart() - // The timer is on the main thread's queue, not on the chart, so unbinding the page does - // not reach it. Worse, the rebind installs a fresh listener whose own pending hold is - // null -- so nobody could have cancelled the old one, and it fired the outgoing page's - // help over whatever replaced it. - longPressAt(chart, insidePlot(chart)) - renderer.detach() - elapse(remainderOfHold()) + // GestureDetector does not report a long press exactly getLongPressTimeout() after the + // finger lands: below Q it adds TAP_TIMEOUT, and it caches the timeout in a static read at + // class-load, so a lengthened accessibility touch-and-hold delay moves the buttons' hold + // and not the detector's. Subtracting the platform timeout from the total assumed + // otherwise, and stretched the chart's hold by however far the detector was late. + longPressAt(chart, insidePlot(chart), sincePressMillis = LATE_REPORT_MILLIS) + elapse(longPressHelpTimeoutMillis() - LATE_REPORT_MILLIS + 50L) - assertThat(helps).isEqualTo(0) + assertThat(helps).isEqualTo(1) } private companion object { /** Longer than the chart's visible window, matching the axis-tap tests' fixture. */ const val SAMPLES = 200 + + /** + * A long press reported well after the finger landed. + * + * Comfortably past the platform timeout, so the two ways of computing the remaining hold + * give different answers and the test can tell them apart. + */ + const val LATE_REPORT_MILLIS = 700L } } diff --git a/build.gradle.kts b/build.gradle.kts index 60208faa6a..ba897ec108 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -93,7 +93,14 @@ subprojects { // Gradle's default test-worker heap is 512m, too small for the Robolectric + // Kotlin Analysis API suites (:lsp:kotlin peaks near 240m and keeps growing). // Keep it explicit so the suites fail on a real regression, not on the default. - maxHeapSize = "1g" + // + // 1g -> 2g (ADFA-5554): Robolectric builds a separate sandbox per distinct @Config, each + // loading the framework again, and :app now has enough of them that 1g died mid-run -- + // exit code 3, no failure recorded, and whichever tests had not run yet reported zeroes + // that look like assertion failures. Raising it here rather than in :app because this is + // the block that wins: a maxHeapSize set in a module's own testOptions is overwritten by + // this one, which is why an experiment that appeared to rule heap out did not. + maxHeapSize = "2g" // Backstop: kill any individual Test task that runs longer than 10 minutes. // Prevents a single hung test JVM (e.g. the Tooling API child) from burning diff --git a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt index 49e826ce92..f7638a16fb 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt @@ -180,12 +180,24 @@ private class HoldTouchListener( // in its HandlerActionQueue and only runs it on attach, so the hold would never time out. private val handler = Handler(Looper.getMainLooper()) + // Deliberately without View.CheckForLongPress's window-attach test. The framework refuses to + // fire a long press for a view whose window has gone, and reproducing that here was tried and + // backed out: a Robolectric view is never window-attached, so the guard turned every timing + // test into a no-op, and attaching one needs the activity harness that takes this JVM down. + // The exposure it covers is already covered where it matters -- TooltipManager re-checks + // isAttachedToWindow before showing, and [clearLongPressHelp] is called from every teardown + // this module has. A caller of [performOnHold] doing something else with the callback would + // not be covered, and there is no such caller today. + private val slop = ViewConfiguration.get(view.context).scaledTouchSlop private var held = false private var holding = false + /** The click waiting for the next turn of the looper, so a teardown can still take it back. */ + private var pendingClick: Runnable? = null + private val fire = Runnable { held = true @@ -196,6 +208,11 @@ private class HoldTouchListener( fun cancel() { holding = false handler.removeCallbacks(fire) + // The click too. It is posted rather than run inline, so a teardown landing between the + // finger lifting and the looper's next turn would otherwise still click a control it has + // just unwired -- the same defect the hold timer has, one method along. + pendingClick?.let(handler::removeCallbacks) + pendingClick = null view.isPressed = false } @@ -228,6 +245,16 @@ private class HoldTouchListener( handler.postDelayed(fire, holdMillis) } + MotionEvent.ACTION_POINTER_DOWN -> { + // A second finger means this is no longer the single-finger press this listener + // times. The carousel undocks on a two-finger tap anywhere in the strip, and + // without this the finger that started on a button also clicked it, or held long + // enough to open that button's help over a strip that was undocking. + holding = false + handler.removeCallbacks(fire) + v.isPressed = false + } + MotionEvent.ACTION_MOVE -> { if (holding && !isInside(event.x, event.y)) { // Left the control: neither a click nor help, which is how the framework @@ -241,8 +268,14 @@ private class HoldTouchListener( MotionEvent.ACTION_UP -> { handler.removeCallbacks(fire) v.isPressed = false - // The click belongs to a press that stayed put and did not become a hold. - if (holding && !held) { + // The click belongs to a press that stayed put, did not become a hold, and landed + // on something that answers taps. That last test is View.onTouchEvent's, and + // taking the touch over means taking it over too: the carousel dims the arrow at + // either end by clearing isClickable rather than isEnabled, precisely so it still + // answers a hold, and without this it went back to answering taps -- playing the + // click sound and announcing a click for a control a screen reader is being told + // is unavailable. + if (holding && !held && v.isClickable) { // Posted rather than called here, as View.onTouchEvent does, so the pressed // state is drawn before the action runs -- these open dialogs and re-page the // carousel from inside the dispatch of the event that triggered them. @@ -250,7 +283,13 @@ private class HoldTouchListener( // Through this handler and not View.post, which parks work on an unattached // view's HandlerActionQueue and returns true having run nothing. Same trap as // the hold timer, one method along. - handler.post { v.performClick() } + val click = + Runnable { + pendingClick = null + v.performClick() + } + pendingClick = click + handler.post(click) } holding = false } From b7d9b697dc11104a77de65ee59742dcf80589e3d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 10:38:31 -0700 Subject: [PATCH 4/6] ADFA-5554: a cancelled gesture is not a lift, and the heap raise was wrong A press an ancestor steals opened the sampling-rate chooser. ChartTouchListener.endAction runs for ACTION_CANCEL as well as ACTION_UP -- case 3 and case 1 of one tableswitch, both reaching it with the original event -- and it reports mLastGesture untouched, because startAction never resets it. So the reveal layout, the bottom sheet or the pager taking the stream mid-press looked exactly like a finger lifting early, and the stand-in tap fired. The chooser clears every sample buffer. The listener now asks the event whether this was a lift. The stand-in tap was posted and then forgotten, so cancelPendingHelp could not take it back although its own KDoc says it does -- the asymmetry the comment two lines up forbids. A detach landing between the lift and the looper's next turn opened the chooser for a chart the renderer no longer had. It is held now, like performOnHold's click. attach()'s unconditional teardown, added last round to stop a second gesture listener being installed, also cleared userHasZoomed. Rebinding an already-bound holder therefore threw away the user's pan and the next tick scrolled the chart back to the newest samples underneath them. The teardown stays; the viewport survives it. A control did not wait out the tap timeout before lighting up. View.onTouchEvent delays the pressed state for a view in a scrolling container so that a flick starting on a button scrolls without flashing it, and this listener lit up on DOWN regardless -- for the bottom sheet's own output-action buttons, which this ticket wired for help and which sit in a HorizontalScrollView. Note ViewGroup defaults shouldDelayChildPressedState to true but FrameLayout and LinearLayout both override it to false, so the container has to be one that scrolls. The long-press haptic fired before anything checked whether a tooltip could appear, so a hold completing after its window had gone still buzzed for help that never showed -- which was the stated reason for backing out the window-attach guard. canShowPopup is asked first now. Untested: TooltipManager reads the docs database in its static initialiser and cannot be loaded off-device, which is why no test in this module reaches it. Three comments described things that are not so. The isClickable gate was attributed to View.onTouchEvent, which reads `clickable` once as CLICKABLE || LONG_CLICKABLE || CONTEXT_CLICKABLE and never re-tests it -- our gate is deliberately stricter, and the comment now says why. clearOnHold counted "five of the six" views without a touch listener, which stopped being true in the PR that wrote it. And the two-finger test now says plainly that it drives the callback directly and cannot reach the gesture that matters, because a ViewGroup rewrites ACTION_POINTER_DOWN to ACTION_MOVE for the child holding the first pointer; driving that from MetricsCarouselLayout is its own change. The 2g heap raise is reverted, and my reasoning for it was wrong twice over. The three test classes this PR adds contribute one @Config between them, so they add no Robolectric sandbox -- :app has ten either way -- and the full suite runs green at 1g from scratch. Worse, the symptom I was chasing is not heap at all: the app installs an uncaught exception handler that calls exitProcess, so any background throw inside the test JVM kills it with no failure recorded. That is exit 1; the exit 3 I saw is ExitOnOutOfMemoryError arriving while the handler chain runs. Running this class alone still kills the JVM with every change here reverted, and the same full-suite command failed once and passed on rerun. None of that is this ticket's, and doubling heap for every subproject at workers.max=30 to hide it was the wrong trade. It belongs to ADFA-5559. The pressed-state test asserts only that the control is dark on DOWN. That it lights up after the timeout could not be asserted: running the delayed press from the looper trips the instability above. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- REVIEW.md | 2 +- .../androidide/ui/MetricsChartRenderer.kt | 42 +++++++- .../androidide/ui/LongPressHelpTimingTest.kt | 52 ++++++++++ .../androidide/ui/MetricsChartAxisTapTest.kt | 23 +++++ .../ui/MetricsChartGestureTeardownTest.kt | 57 +++++++++++ build.gradle.kts | 9 +- .../androidide/idetooltips/ToolTipManager.kt | 10 +- .../com/itsaky/androidide/utils/ViewUtils.kt | 99 +++++++++++++++---- 8 files changed, 261 insertions(+), 33 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index dec26df2a1..036de3a0bc 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -176,7 +176,7 @@ Help in CoGo is reached by **long-press**, anywhere: a progressive three-tier ex - **Wire up help on new interactive elements.** Anything tappable — buttons, icon controls, menu items, list rows, toolbar actions — gets long-press help. A new actionable view with no tooltip is as incomplete as a missing `contentDescription`. - **Cover new screens and panels too.** Even where pixels aren't interactive, a new screen/panel/dialog needs a top-level help entry so help is always reachable. - **The affordance is the requirement, not finished copy.** Tooltip content may still be in authoring — fine — but the long-press must be wired and routed into the tier system. Don't ship UI that can never surface help. -- **Reuse the system.** Wire help through `idetooltips` — today the `View.displayTooltipOnLongPress(context, anchorView, category, tag)` extension (`setOnLongClickListener` → `TooltipManager.showTooltip`) — not a one-off popup. +- **Reuse the system.** Wire help through `idetooltips` — today the `View.displayTooltipOnLongPress(context, tooltipTag, tooltipCategory, holdMillis)` extension (a long-click listener for the framework's own gesture, plus a touch listener that times the longer hold ADFA-5554 asks for, both reaching `TooltipManager.showTooltip`) — not a one-off popup. - **Compose has no native entry point yet** (tracked by **ADFA-4381**). The helper is View-based (needs an `anchorView`), so until `idetooltips` grows a Compose API, a composable wires help via `AndroidView` interop. Flag it in review rather than skipping help, and build the reusable `Modifier`/wrapper once instead of copy-pasting interop. ## 10. Architecture alignment 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 f7be8f449f..3caceafd64 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -218,7 +218,16 @@ abstract class MetricsChartRenderer( // there let [configure] install a second gesture listener while the first stayed queued on // the main thread with a hold nothing could reach, and added a second layout listener that // one removeOnLayoutChangeListener cannot undo. + // + // The one thing that must survive it is the user's own viewport. detach() clears + // userHasZoomed, which is what turns the auto-follow window back on, so a rebind of an + // already-bound holder would snap a chart the user had panned back to the newest samples. + val sameChart = this.chart === chart + val hadZoomed = userHasZoomed detach() + if (sameChart) { + userHasZoomed = hadZoomed + } this.chart = chart configure(chart) chart.addOnLayoutChangeListener(newestWindowOnLayout) @@ -455,6 +464,16 @@ abstract class MetricsChartRenderer( /** The deferred half of a long press, waiting out the rest of the hold. */ private var pendingHelp: Runnable? = null + /** + * The stand-in tap waiting for the next turn of the looper. + * + * Held for the same reason [performOnHold] holds its click: posted rather than run inline, + * it outlives the dispatch that queued it, so a [detach] landing in between would otherwise + * still open the sampling-rate chooser for a chart the renderer no longer has -- and that + * chooser clears every sample buffer. + */ + private var pendingTap: Runnable? = null + /** Whether this gesture already showed help, so its lift must not also count as a tap. */ private var helpShown = false @@ -486,12 +505,29 @@ abstract class MetricsChartRenderer( // history loss [isOnAxisBand] was narrowed to prevent. [onChartTranslate] and // [onChartScale] give up the stand-in as the gesture escalates; this is the check for // an escalation neither of them reports. - if (!helpShown && pendingTapOnAxis && lastPerformedGesture == ChartTouchListener.ChartGesture.LONG_PRESS) { + // A cancel is not a lift. ChartTouchListener.endAction runs for ACTION_CANCEL as well + // as ACTION_UP -- case 3 and case 1 of the same tableswitch, both reaching it with the + // original event -- and it reports mLastGesture untouched, because startAction never + // resets it. So a press an ancestor steals mid-gesture (the reveal layout, the bottom + // sheet, the pager) arrived here looking exactly like a finger lifted early, and stood + // in for a tap the user never completed. The chooser it opens clears every buffer. + val lifted = me?.actionMasked != MotionEvent.ACTION_CANCEL + if (lifted && + !helpShown && + pendingTapOnAxis && + lastPerformedGesture == ChartTouchListener.ChartGesture.LONG_PRESS + ) { // Posted, not called here. This runs inside the chart's onTouchEvent, and the tap // opens a dialog; showing one mid-dispatch leaves the chart's touch state and its // velocity tracker part-way through a gesture. performOnHold posts its click for // the same reason, and the two paths should not disagree. - handler.post { onXAxisTap?.invoke() } + val tap = + Runnable { + pendingTap = null + onXAxisTap?.invoke() + } + pendingTap = tap + handler.post(tap) } helpShown = false pendingTapOnAxis = false @@ -506,6 +542,8 @@ abstract class MetricsChartRenderer( fun cancelPendingHelp() { pendingHelp?.let(handler::removeCallbacks) pendingHelp = null + pendingTap?.let(handler::removeCallbacks) + pendingTap = null } override fun onChartLongPressed(me: MotionEvent?) { diff --git a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt index 7b81c07063..30e722393c 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt @@ -23,6 +23,7 @@ import android.view.MotionEvent import android.view.View import android.view.ViewConfiguration import android.widget.Button +import android.widget.HorizontalScrollView import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.utils.clearLongPressHelp @@ -69,6 +70,21 @@ class LongPressHelpTimingTest { performOnHold { holds++ } } + /** + * The same control inside a container that delays its children's pressed state. + * + * A `HorizontalScrollView` because that is the real case: the bottom sheet's output-action + * buttons, which this ticket wired for help, sit in one. Not any container -- `ViewGroup` + * defaults to true but `FrameLayout` and `LinearLayout` both override it to false, so the + * choice here has to be a container that actually scrolls. + */ + private fun targetInScrollingContainer(): Button { + val button = target() + HorizontalScrollView(context).addView(button) + button.layout(0, 0, WIDTH, HEIGHT) + return button + } + private fun send( view: View, action: Int, @@ -91,6 +107,42 @@ class LongPressHelpTimingTest { */ private fun drain() = shadowOf(Looper.getMainLooper()).idle() + @Test + fun `a control with no scrolling ancestor lights up the moment the finger lands`() { + val view = target() + + send(view, MotionEvent.ACTION_DOWN) + + assertThat(view.isPressed).isTrue() + } + + @Test + fun `a control inside a scrolling container waits out the tap timeout first`() { + val view = targetInScrollingContainer() + + send(view, MotionEvent.ACTION_DOWN) + + // View.onTouchEvent does not light a control up straight away when it can be scrolled: + // it waits a tap timeout, so a flick that happens to start on a button scrolls without + // flashing it. Taking the touch over means taking that over too, and this listener did + // not -- every drag off one of these controls blinked it first. + assertThat(view.isPressed).isFalse() + } + + @Test + fun `a flick off a control in a scrolling container never lights it up`() { + val view = targetInScrollingContainer() + + send(view, MotionEvent.ACTION_DOWN) + send(view, MotionEvent.ACTION_MOVE, x = WIDTH * 4f, y = HEIGHT * 4f) + elapse(ViewConfiguration.getTapTimeout().toLong()) + + // The pressed state is on the queue when the finger leaves, so dropping the hold is not + // enough: the flash arrives after the gesture that cancelled it. + assertThat(view.isPressed).isFalse() + assertThat(holds).isEqualTo(0) + } + @Test fun `a press past the platform timeout but short of the hold still clicks`() { // The regression the obvious fix introduces, and the reason this class exists. The diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt index c9713c73f9..e27603e510 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt @@ -105,6 +105,29 @@ class MetricsChartAxisTapTest { .isLessThan(SAMPLES - 1) } + @Test + fun `re-attaching the same chart keeps the viewport the user drove`() { + val chart = laidOutChart() + drawOnce(chart) + chart.setVisibleXRangeMaximum(VISIBLE_WINDOW.toFloat()) + chart.moveViewToXNow(0f) + drawOnce(chart) + + val event = MotionEvent.obtain(0L, 0L, MotionEvent.ACTION_MOVE, 10f, 10f, 0) + chart.onChartGestureListener.onChartTranslate(event, -50f, 0f) + event.recycle() + assertThat(attachedRenderer.visibleSampleRange(chart, SAMPLES).last).isLessThan(SAMPLES - 1) + + // A rebind of an already-bound holder. The teardown it runs is what stops a second gesture + // listener being installed, so it has to happen -- but it also cleared the flag that says + // the user has driven the viewport, and the next tick then scrolled the chart back to the + // newest samples underneath them. + attachedRenderer.attach(chart) + drawOnce(chart) + + assertThat(attachedRenderer.visibleSampleRange(chart, SAMPLES).last).isLessThan(SAMPLES - 1) + } + /** MPAndroidChart runs its viewport jobs during a draw, so a pan is not real until one. */ private fun drawOnce(chart: SafeLineChart) { chart.draw(Canvas(Bitmap.createBitmap(CHART_WIDTH, CHART_HEIGHT, Bitmap.Config.ARGB_8888))) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt index 2243484f4e..0b870553a9 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt @@ -119,6 +119,26 @@ class MetricsChartGestureTeardownTest { event.recycle() } + /** + * The end of a gesture an ancestor took away. + * + * ChartTouchListener.endAction is reached from ACTION_CANCEL as well as ACTION_UP, with the + * original event and with mLastGesture untouched, so this is what the listener actually sees + * when the reveal layout, the bottom sheet or the pager claims the stream mid-press. + */ + private fun cancelGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) { + val now = SystemClock.uptimeMillis() + val event = MotionEvent.obtain(now, now, MotionEvent.ACTION_CANCEL, 10f, 0f, 0) + chart.onChartGestureListener.onChartGestureEnd(event, gesture) + event.recycle() + } + + /** A y on the axis band, where a tap opens the sampling-rate chooser. */ + private fun onAxisBand(chart: SafeLineChart) = chart.viewPortHandler.contentBottom() + 1f + /** Runs the main looper forward by [millis] of virtual time. */ private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) @@ -136,10 +156,47 @@ class MetricsChartGestureTeardownTest { /** A y inside the plot, where a hold means help for the page rather than for the axis. */ private fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f + @Test + fun `a gesture an ancestor cancels does not stand in for a tap`() { + val chart = laidOutChart() + + longPressAt(chart, onAxisBand(chart)) + cancelGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + drain() + + // A cancel is not a lift. The chooser this would open clears every sample buffer, so a + // press the sheet or the pager steals mid-gesture must not be read as a finger lifting + // early -- which is exactly what it looked like, because endAction reports the same + // LONG_PRESS for both. + assertThat(taps).isEqualTo(0) + } + + @Test + fun `detaching takes back a stand-in tap that has been posted`() { + val chart = laidOutChart() + + longPressAt(chart, onAxisBand(chart)) + endGesture(chart, ChartTouchListener.ChartGesture.LONG_PRESS) + // The tap is on the looper now, not yet run. Letting the chart go in that window used to + // leave it there: it opened the chooser, and cleared every buffer, for a chart this + // renderer no longer had. + renderer.detach() + drain() + + assertThat(taps).isEqualTo(0) + } + @Test fun `a second finger gives up the gesture, even without a move`() { val chart = laidOutChart() + // This tests what the renderer does when the second pointer is reported, by calling the + // callback directly. It does NOT test that the callback fires for the gesture that matters, + // and it cannot: a ViewGroup rewrites ACTION_POINTER_DOWN to ACTION_MOVE for the child + // already holding the first pointer, so on the realistic undock -- one finger on an arrow, + // one on the strip -- SafeLineChart.onTouchEvent never sees a pointer-down at all. Driving + // this from MetricsCarouselLayout.dispatchTouchEvent, which does see it, is its own change. + // // The carousel undocks on a two-finger tap, and that starts as a press like any other. // MPAndroidChart cannot report it -- ACTION_POINTER_DOWN never touches its mLastGesture -- // so the gesture still ends labelled LONG_PRESS and the stand-in tap fired, opening the diff --git a/build.gradle.kts b/build.gradle.kts index ba897ec108..60208faa6a 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -93,14 +93,7 @@ subprojects { // Gradle's default test-worker heap is 512m, too small for the Robolectric + // Kotlin Analysis API suites (:lsp:kotlin peaks near 240m and keeps growing). // Keep it explicit so the suites fail on a real regression, not on the default. - // - // 1g -> 2g (ADFA-5554): Robolectric builds a separate sandbox per distinct @Config, each - // loading the framework again, and :app now has enough of them that 1g died mid-run -- - // exit code 3, no failure recorded, and whichever tests had not run yet reported zeroes - // that look like assertion failures. Raising it here rather than in :app because this is - // the block that wins: a maxHeapSize set in a module's own testOptions is overwritten by - // this one, which is why an experiment that appeared to rule heap out did not. - maxHeapSize = "2g" + maxHeapSize = "1g" // Backstop: kill any individual Test task that runs longer than 10 minutes. // Prevents a single hung test JVM (e.g. the Tooling API child) from burning diff --git a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt index 92d5221b68..6e2e4de7b3 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt @@ -252,7 +252,15 @@ object TooltipManager { ) } - private fun canShowPopup(context: Context, view: View): Boolean { + /** + * Whether a popup anchored to [view] can actually be shown right now. + * + * Internal so [com.itsaky.androidide.utils.showTooltipIfPresent] can ask before it plays the + * long-press haptic. Asking after is too late: the buzz is the user's signal that help arrived, + * and a hold that completes 800ms after its window has gone fired it for a tooltip that never + * appeared. + */ + internal fun canShowPopup(context: Context, view: View): Boolean { tailrec fun Context.findActivity(): Activity? { return when (this) { is Activity -> this diff --git a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt index f7638a16fb..304270eac2 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt @@ -7,6 +7,7 @@ import android.view.HapticFeedbackConstants import android.view.MotionEvent import android.view.View import android.view.ViewConfiguration +import android.view.ViewGroup import com.itsaky.androidide.idetooltips.R import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager @@ -28,12 +29,16 @@ fun showTooltipIfPresent( tag: String, playHapticFeedback: Boolean = true, ) { - if (tag.isNotBlank()) { - if (playHapticFeedback) { - anchor.performHapticFeedback(HapticFeedbackConstants.LONG_PRESS) - } - TooltipManager.showTooltip(context, anchor, category, tag) + if (tag.isBlank() || !TooltipManager.canShowPopup(context, anchor)) { + // Asked before the haptic, not after. The buzz is what tells the user help has arrived, and + // showTooltip declines silently for a detached anchor -- so a hold completing after its + // window has gone used to buzz for a tooltip that never appeared. + return } + if (playHapticFeedback) { + anchor.performHapticFeedback(HapticFeedbackConstants.LONG_PRESS) + } + TooltipManager.showTooltip(context, anchor, category, tag) } /** Shows [tag]'s IDE-category tooltip anchored to [anchor]. See [showTooltipIfPresent]. */ @@ -151,9 +156,11 @@ fun View.clearLongPressHelp() { * caller that installed only a hold should be able to undo only a hold. * * The touch listener is removed only when [performOnHold] is the one that installed it, which the - * tag says. Five of the six views [clearLongPressHelp] is called on are wired through the - * framework's long click and never had one, and a blanket `setOnTouchListener(null)` there would - * silently take away an unrelated listener the next contributor adds. + * tag says. Most views [clearLongPressHelp] is called on are wired through the framework's long + * click and never had one, and a blanket `setOnTouchListener(null)` there would silently take away + * an unrelated listener the next contributor adds. (An earlier version of this line counted them -- + * "five of the six" -- which stopped being true in the same PR that wrote it, when the bottom + * sheet's buttons were converted.) */ fun View.clearOnHold() { val hold = getTag(R.id.tooltip_hold_listener) as? HoldTouchListener ?: return @@ -195,27 +202,61 @@ private class HoldTouchListener( private var holding = false + /** + * The pressed state waiting out [ViewConfiguration.getTapTimeout], or `null` when there is none. + * + * `View.onTouchEvent` does not light a control up the instant a finger lands on it when the + * control sits in a scrolling container: it waits a tap timeout first, so that a flick which + * happens to start on a button scrolls without flashing it. Taking the touch over means taking + * that over too. The bottom sheet's output-action buttons, which ADFA-5554 wired for help, sit + * in a HorizontalScrollView, so this is a real case here and not a hypothetical one. + */ + private var pendingPress: Runnable? = null + /** The click waiting for the next turn of the looper, so a teardown can still take it back. */ private var pendingClick: Runnable? = null private val fire = Runnable { held = true - view.isPressed = false + releasePress() onHold() } fun cancel() { holding = false handler.removeCallbacks(fire) + releasePress() // The click too. It is posted rather than run inline, so a teardown landing between the // finger lifting and the looper's next turn would otherwise still click a control it has // just unwired -- the same defect the hold timer has, one method along. pendingClick?.let(handler::removeCallbacks) pendingClick = null + } + + /** Drops a press that has not been drawn yet, and any that has. */ + private fun releasePress() { + pendingPress?.let(handler::removeCallbacks) + pendingPress = null view.isPressed = false } + /** + * Whether any ancestor delays the pressed state of its children, which is what + * `View.isInScrollingContainer` asks. That method is not in the public SDK; the question it + * answers is, one `ViewGroup` at a time. + */ + private fun isInScrollingContainer(): Boolean { + var parent = view.parent + while (parent is ViewGroup) { + if (parent.shouldDelayChildPressedState()) { + return true + } + parent = parent.parent + } + return false + } + /** * Whether a touch at ([x], [y]) is still on the view, by the framework's rule. * @@ -238,10 +279,22 @@ private class HoldTouchListener( MotionEvent.ACTION_DOWN -> { held = false holding = true - v.isPressed = true // The framework starts the ripple from the touch point. Without this every ripple // on these controls begins at the centre of the drawable instead. - v.drawableHotspotChanged(event.x, event.y) + val x = event.x + val y = event.y + val press = + Runnable { + pendingPress = null + v.isPressed = true + v.drawableHotspotChanged(x, y) + } + if (isInScrollingContainer()) { + pendingPress = press + handler.postDelayed(press, ViewConfiguration.getTapTimeout().toLong()) + } else { + press.run() + } handler.postDelayed(fire, holdMillis) } @@ -252,7 +305,7 @@ private class HoldTouchListener( // enough to open that button's help over a strip that was undocking. holding = false handler.removeCallbacks(fire) - v.isPressed = false + releasePress() } MotionEvent.ACTION_MOVE -> { @@ -261,20 +314,24 @@ private class HoldTouchListener( // treats a drag out of a view. Taking the touch over means saying so. holding = false handler.removeCallbacks(fire) - v.isPressed = false + releasePress() } } MotionEvent.ACTION_UP -> { handler.removeCallbacks(fire) - v.isPressed = false + releasePress() // The click belongs to a press that stayed put, did not become a hold, and landed - // on something that answers taps. That last test is View.onTouchEvent's, and - // taking the touch over means taking it over too: the carousel dims the arrow at - // either end by clearing isClickable rather than isEnabled, precisely so it still - // answers a hold, and without this it went back to answering taps -- playing the - // click sound and announcing a click for a control a screen reader is being told - // is unavailable. + // on something that answers taps. + // + // That last test is stricter than the framework's, deliberately. View.onTouchEvent + // reads `clickable` once at the top, as CLICKABLE || LONG_CLICKABLE || + // CONTEXT_CLICKABLE, and never re-tests isClickable before performing the click -- + // so a long-clickable view still clicks there. The carousel dims the arrow at + // either end by clearing isClickable rather than isEnabled, precisely so it keeps + // answering a hold, and matching the framework here would have it answer taps too: + // playing the click sound and announcing a click for a control a screen reader is + // being told is unavailable. if (holding && !held && v.isClickable) { // Posted rather than called here, as View.onTouchEvent does, so the pressed // state is drawn before the action runs -- these open dialogs and re-page the @@ -297,7 +354,7 @@ private class HoldTouchListener( MotionEvent.ACTION_CANCEL -> { holding = false handler.removeCallbacks(fire) - v.isPressed = false + releasePress() } } return true From 349db0ed0e66a999abfc88c0a3a18e51c4a017db Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 11:36:01 -0700 Subject: [PATCH 5/6] ADFA-5554: reindent the fixtures the merge resolution left Formatting only. The sampleTimes argument added while resolving the merge from ADFA-5553 was indented to the old call's continuation rather than to ktlint's. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MetricsChartGestureTeardownTest.kt | 8 ++++---- .../com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt index 93888ea9c9..8b77a43037 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt @@ -65,10 +65,10 @@ class MetricsChartGestureTeardownTest { NetworkUsageChartRenderer( usageProvider = { NetworkUsageWatcher.NetworkUsage( - LongArray(SAMPLES) { 1_000L }, - LongArray(SAMPLES) { 500L }, - LongArray(SAMPLES), - ) + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ) renderer.attach(chart) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt index f857052e93..02e80bf619 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt @@ -63,10 +63,10 @@ class MetricsChartHoldHelpTest { NetworkUsageChartRenderer( usageProvider = { NetworkUsageWatcher.NetworkUsage( - LongArray(SAMPLES) { 1_000L }, - LongArray(SAMPLES) { 500L }, - LongArray(SAMPLES), - ) + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ) renderer.attach(chart) From 01ffa5813de11b6f8c2fad25905f89e3a681f489 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 13:47:48 -0700 Subject: [PATCH 6/6] ADFA-5554: one harness for the two chart-gesture test classes The class that tests when help fires and the class that tests what happens to a hold when the gesture or the chart goes away had grown byte-identical copies of their setup: 74 lines each, down to the comments, covering the laid-out chart, the renderer, the tap and help counters, and every gesture helper. `diff` on the two regions reported no differences at all. One copy had a `panBy` that nothing called, which is the usual end of a duplicated fixture -- a helper is copied for symmetry and then only one side grows a test for it. Both delegate to ChartGestureHarness now. The one test that reached past its helpers to build a MotionEvent by hand, for a pinch, gets a `scaleBy` alongside the existing `panBy` instead, so no test handles raw events any more. `elapse`, `drain` and `remainderOfHold` are top-level: they are about the looper and the timeout rather than about a chart. Fifteen imports went dead with the extraction and are gone. Spotless does not flag those, which is worth knowing. No behaviour change and no new coverage: every test still asserts exactly what it did before, and the suite is green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/ChartGestureHarness.kt | 171 ++++++++++++++++++ .../ui/MetricsChartGestureTeardownTest.kt | 110 ++--------- .../androidide/ui/MetricsChartHoldHelpTest.kt | 103 ++--------- 3 files changed, 202 insertions(+), 182 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/ChartGestureHarness.kt diff --git a/app/src/test/java/com/itsaky/androidide/ui/ChartGestureHarness.kt b/app/src/test/java/com/itsaky/androidide/ui/ChartGestureHarness.kt new file mode 100644 index 0000000000..5790369e71 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/ChartGestureHarness.kt @@ -0,0 +1,171 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import android.os.Looper +import android.os.SystemClock +import android.view.MotionEvent +import android.view.ViewConfiguration +import com.github.mikephil.charting.listener.ChartTouchListener +import com.itsaky.androidide.utils.NetworkUsageWatcher +import com.itsaky.androidide.utils.longPressHelpTimeoutMillis +import org.robolectric.Shadows.shadowOf +import java.util.concurrent.TimeUnit + +/** Samples a harnessed chart is given; more than a window's worth, so a pan has somewhere to go. */ +const val HARNESS_SAMPLES = 200 + +/** + * A laid-out chart with a renderer attached, and the gestures to drive it. + * + * The two classes that test the chart's hold -- when help fires, and what happens to a hold when + * the gesture or the chart goes away -- had grown byte-identical copies of all of this, 74 lines + * each, down to the comments. One of the copies had a `panBy` nothing called. + * + * The gesture helpers all go through `chart.onChartGestureListener` rather than dispatching real + * touches, because that is the seam the renderer actually listens on: MPAndroidChart's own + * detector is what decides a press is a long press, and standing that up would be testing the + * library rather than the renderer. + */ +class ChartGestureHarness( + private val context: Context, +) { + /** Times [MetricsChartRenderer.onXAxisTap] fired -- the sampling-rate chooser opening. */ + var taps = 0 + private set + + /** Times the renderer asked for help to be shown. */ + var helps = 0 + private set + + lateinit var renderer: NetworkUsageChartRenderer + private set + + fun laidOutChart(): SafeLineChart { + val chart = SafeLineChart(context) + // Any concrete renderer will do -- the hold is the base class's, and every page wires it + // the same way. + renderer = + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage( + LongArray(HARNESS_SAMPLES) { 1_000L }, + LongArray(HARNESS_SAMPLES) { 500L }, + LongArray(HARNESS_SAMPLES), + ) + }, + ) + renderer.attach(chart) + renderer.onXAxisTap = { taps++ } + renderer.showHelp = { _, _, _ -> helps++ } + + chart.layOutAndDraw() + return chart + } + + /** + * An event whose finger landed [sincePressMillis] ago. + * + * The down time is what the chart measures its remaining hold from, so it has to be real here. + * Defaults to the platform's long-press timeout, which is when a detector on a current device + * reports one. + */ + fun eventAt( + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ): MotionEvent { + val now = SystemClock.uptimeMillis() + return MotionEvent.obtain(now - sincePressMillis, now, MotionEvent.ACTION_MOVE, 10f, y, 0) + } + + /** The platform's own long press, which is where the chart's hold started counting from. */ + fun longPressAt( + chart: SafeLineChart, + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ) { + val event = eventAt(y, sincePressMillis) + chart.onChartGestureListener.onChartLongPressed(event) + event.recycle() + } + + fun panBy( + chart: SafeLineChart, + dx: Float, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartTranslate(event, dx, 0f) + event.recycle() + } + + fun scaleBy( + chart: SafeLineChart, + factor: Float, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartScale(event, factor, factor) + event.recycle() + } + + fun endGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) { + val event = eventAt(0f) + chart.onChartGestureListener.onChartGestureEnd(event, gesture) + event.recycle() + } + + /** + * The end of a gesture an ancestor took away. + * + * ChartTouchListener.endAction is reached from ACTION_CANCEL as well as ACTION_UP, with the + * original event and with mLastGesture untouched, so this is what the listener actually sees + * when the reveal layout, the bottom sheet or the pager claims the stream mid-press. + */ + fun cancelGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) { + val now = SystemClock.uptimeMillis() + val event = MotionEvent.obtain(now, now, MotionEvent.ACTION_CANCEL, 10f, 0f, 0) + chart.onChartGestureListener.onChartGestureEnd(event, gesture) + event.recycle() + } + + /** A y on the axis band, where a tap opens the sampling-rate chooser. */ + fun onAxisBand(chart: SafeLineChart) = chart.viewPortHandler.contentBottom() + 1f + + /** A y inside the plot, where a hold means help for the page rather than for the axis. */ + fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f +} + +/** Runs the main looper forward by [millis] of virtual time. */ +fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) + +/** + * Runs what is already due on the main looper without advancing the clock. + * + * The stand-in tap and the stand-in click are posted rather than run inside the touch dispatch, so + * nothing has been tapped until the looper turns. + */ +fun drain() = shadowOf(Looper.getMainLooper()).idle() + +/** The rest of the hold, after a long press reported at the platform's own timeout. */ +fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt index 8b77a43037..9723b88c74 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt @@ -17,21 +17,13 @@ package com.itsaky.androidide.ui -import android.content.Context -import android.os.Looper -import android.os.SystemClock -import android.view.MotionEvent import android.view.ViewConfiguration import androidx.test.core.app.ApplicationProvider import com.github.mikephil.charting.listener.ChartTouchListener import com.google.common.truth.Truth.assertThat -import com.itsaky.androidide.utils.NetworkUsageWatcher -import com.itsaky.androidide.utils.longPressHelpTimeoutMillis import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -import org.robolectric.Shadows.shadowOf -import java.util.concurrent.TimeUnit /** * What happens to a hold in progress when the gesture or the chart under it goes away (ADFA-5554). @@ -49,116 +41,40 @@ import java.util.concurrent.TimeUnit */ @RunWith(RobolectricTestRunner::class) class MetricsChartGestureTeardownTest { - private val context = ApplicationProvider.getApplicationContext() - - private var taps = 0 - - private var helps = 0 - - private lateinit var renderer: NetworkUsageChartRenderer - - private fun laidOutChart(): SafeLineChart { - val chart = SafeLineChart(context) - // Any concrete renderer will do -- the hold is the base class's, and every page wires it - // the same way. - renderer = - NetworkUsageChartRenderer( - usageProvider = { - NetworkUsageWatcher.NetworkUsage( - LongArray(SAMPLES) { 1_000L }, - LongArray(SAMPLES) { 500L }, - LongArray(SAMPLES), - ) - }, - ) - renderer.attach(chart) - renderer.onXAxisTap = { taps++ } - renderer.showHelp = { _, _, _ -> helps++ } + private val harness = ChartGestureHarness(ApplicationProvider.getApplicationContext()) - chart.layOutAndDraw() - return chart - } + private val taps get() = harness.taps - /** - * An event whose finger landed [sincePressMillis] ago. - * - * The down time is what the chart measures its remaining hold from, so it has to be real here. - * Defaults to the platform's long-press timeout, which is when a detector on a current device - * reports one. - */ - private fun eventAt( - y: Float, - sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), - ): MotionEvent { - val now = SystemClock.uptimeMillis() - return MotionEvent.obtain(now - sincePressMillis, now, MotionEvent.ACTION_MOVE, 10f, y, 0) - } + private val helps get() = harness.helps + + private val renderer get() = harness.renderer + + private fun laidOutChart() = harness.laidOutChart() - /** The platform's own long press, which is where the chart's hold started counting from. */ private fun longPressAt( chart: SafeLineChart, y: Float, sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), - ) { - val event = eventAt(y, sincePressMillis) - chart.onChartGestureListener.onChartLongPressed(event) - event.recycle() - } + ) = harness.longPressAt(chart, y, sincePressMillis) private fun panBy( chart: SafeLineChart, dx: Float, - ) { - val event = eventAt(0f) - chart.onChartGestureListener.onChartTranslate(event, dx, 0f) - event.recycle() - } + ) = harness.panBy(chart, dx) private fun endGesture( chart: SafeLineChart, gesture: ChartTouchListener.ChartGesture, - ) { - val event = eventAt(0f) - chart.onChartGestureListener.onChartGestureEnd(event, gesture) - event.recycle() - } + ) = harness.endGesture(chart, gesture) - /** - * The end of a gesture an ancestor took away. - * - * ChartTouchListener.endAction is reached from ACTION_CANCEL as well as ACTION_UP, with the - * original event and with mLastGesture untouched, so this is what the listener actually sees - * when the reveal layout, the bottom sheet or the pager claims the stream mid-press. - */ private fun cancelGesture( chart: SafeLineChart, gesture: ChartTouchListener.ChartGesture, - ) { - val now = SystemClock.uptimeMillis() - val event = MotionEvent.obtain(now, now, MotionEvent.ACTION_CANCEL, 10f, 0f, 0) - chart.onChartGestureListener.onChartGestureEnd(event, gesture) - event.recycle() - } - - /** A y on the axis band, where a tap opens the sampling-rate chooser. */ - private fun onAxisBand(chart: SafeLineChart) = chart.viewPortHandler.contentBottom() + 1f - - /** Runs the main looper forward by [millis] of virtual time. */ - private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) - - /** - * Runs what is already due on the main looper without advancing the clock. - * - * The stand-in tap is posted rather than invoked inside the chart's touch dispatch, so nothing - * has been tapped until the looper turns. - */ - private fun drain() = shadowOf(Looper.getMainLooper()).idle() + ) = harness.cancelGesture(chart, gesture) - /** The rest of the hold, after a long press reported at the platform's own timeout. */ - private fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L + private fun onAxisBand(chart: SafeLineChart) = harness.onAxisBand(chart) - /** A y inside the plot, where a hold means help for the page rather than for the axis. */ - private fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f + private fun insidePlot(chart: SafeLineChart) = harness.insidePlot(chart) @Test fun `a gesture an ancestor cancels does not stand in for a tap`() { diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt index 02e80bf619..99851457d5 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt @@ -17,21 +17,14 @@ package com.itsaky.androidide.ui -import android.content.Context -import android.os.Looper -import android.os.SystemClock -import android.view.MotionEvent import android.view.ViewConfiguration import androidx.test.core.app.ApplicationProvider import com.github.mikephil.charting.listener.ChartTouchListener import com.google.common.truth.Truth.assertThat -import com.itsaky.androidide.utils.NetworkUsageWatcher import com.itsaky.androidide.utils.longPressHelpTimeoutMillis import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -import org.robolectric.Shadows.shadowOf -import java.util.concurrent.TimeUnit /** * When the chart answers a hold with help, and when it gives that help up (ADFA-5554). @@ -47,96 +40,38 @@ import java.util.concurrent.TimeUnit */ @RunWith(RobolectricTestRunner::class) class MetricsChartHoldHelpTest { - private val context = ApplicationProvider.getApplicationContext() - - private var taps = 0 - - private var helps = 0 - - private lateinit var renderer: NetworkUsageChartRenderer - - private fun laidOutChart(): SafeLineChart { - val chart = SafeLineChart(context) - // Any concrete renderer will do -- the hold is the base class's, and every page wires it - // the same way. - renderer = - NetworkUsageChartRenderer( - usageProvider = { - NetworkUsageWatcher.NetworkUsage( - LongArray(SAMPLES) { 1_000L }, - LongArray(SAMPLES) { 500L }, - LongArray(SAMPLES), - ) - }, - ) - renderer.attach(chart) - renderer.onXAxisTap = { taps++ } - renderer.showHelp = { _, _, _ -> helps++ } - - chart.layOutAndDraw() - return chart - } + private val harness = ChartGestureHarness(ApplicationProvider.getApplicationContext()) - /** - * An event whose finger landed [sincePressMillis] ago. - * - * The down time is what the chart measures its remaining hold from, so it has to be real here. - * Defaults to the platform's long-press timeout, which is when a detector on a current device - * reports one. - */ - private fun eventAt( - y: Float, - sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), - ): MotionEvent { - val now = SystemClock.uptimeMillis() - return MotionEvent.obtain(now - sincePressMillis, now, MotionEvent.ACTION_MOVE, 10f, y, 0) - } + private val taps get() = harness.taps + + private val helps get() = harness.helps + + private val renderer get() = harness.renderer + + private fun laidOutChart() = harness.laidOutChart() - /** The platform's own long press, which is where the chart's hold started counting from. */ private fun longPressAt( chart: SafeLineChart, y: Float, sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), - ) { - val event = eventAt(y, sincePressMillis) - chart.onChartGestureListener.onChartLongPressed(event) - event.recycle() - } + ) = harness.longPressAt(chart, y, sincePressMillis) private fun panBy( chart: SafeLineChart, dx: Float, - ) { - val event = eventAt(0f) - chart.onChartGestureListener.onChartTranslate(event, dx, 0f) - event.recycle() - } + ) = harness.panBy(chart, dx) + + private fun scaleBy( + chart: SafeLineChart, + factor: Float, + ) = harness.scaleBy(chart, factor) private fun endGesture( chart: SafeLineChart, gesture: ChartTouchListener.ChartGesture, - ) { - val event = eventAt(0f) - chart.onChartGestureListener.onChartGestureEnd(event, gesture) - event.recycle() - } - - /** Runs the main looper forward by [millis] of virtual time. */ - private fun elapse(millis: Long) = shadowOf(Looper.getMainLooper()).idleFor(millis, TimeUnit.MILLISECONDS) - - /** - * Runs what is already due on the main looper without advancing the clock. - * - * The stand-in tap is posted rather than invoked inside the chart's touch dispatch, so nothing - * has been tapped until the looper turns. - */ - private fun drain() = shadowOf(Looper.getMainLooper()).idle() - - /** The rest of the hold, after a long press reported at the platform's own timeout. */ - private fun remainderOfHold() = longPressHelpTimeoutMillis() - ViewConfiguration.getLongPressTimeout() + 50L + ) = harness.endGesture(chart, gesture) - /** A y inside the plot, where a hold means help for the page rather than for the axis. */ - private fun insidePlot(chart: SafeLineChart) = (chart.viewPortHandler.contentTop() + chart.viewPortHandler.contentBottom()) / 2f + private fun insidePlot(chart: SafeLineChart) = harness.insidePlot(chart) @Test fun `a press held past the hold shows help`() { @@ -213,9 +148,7 @@ class MetricsChartHoldHelpTest { val chart = laidOutChart() longPressAt(chart, insidePlot(chart)) - val event = eventAt(0f) - chart.onChartGestureListener.onChartScale(event, 1.2f, 1.2f) - event.recycle() + scaleBy(chart, 1.2f) elapse(remainderOfHold()) assertThat(helps).isEqualTo(0)