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/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 0795375801..a2202c154a 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 @@ -38,6 +40,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 @@ -149,6 +152,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 top inset last reserved, so an unchanged value costs nothing. * @@ -215,7 +235,21 @@ 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. + // + // 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) @@ -230,6 +264,17 @@ 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 + // 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 } @@ -325,7 +370,11 @@ abstract class MetricsChartRenderer( // while the form is a circle, and load-bearing only if anyone chooses LINE. legend.formLineWidth = 1f - onChartGestureListener = XAxisTapListener(this) + 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. @@ -430,6 +479,31 @@ 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 + + /** + * 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 + + /** 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)) { @@ -445,16 +519,91 @@ abstract class MetricsChartRenderer( override fun onChartGestureEnd( me: MotionEvent?, lastPerformedGesture: ChartTouchListener.ChartGesture?, - ) = Unit + ) { + cancelPendingHelp() + // Lifted before the hold completed: the detector ate the tap, so stand in for it. + // + // 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. + // 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. + val tap = + Runnable { + pendingTap = null + onXAxisTap?.invoke() + } + pendingTap = tap + handler.post(tap) + } + 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 + pendingTap?.let(handler::removeCallbacks) + pendingTap = null + } override fun onChartLongPressed(me: MotionEvent?) { - val y = me?.y ?: return + val event = me ?: return + val y = event.y 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. + 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 + 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. + showHelp(chart.context, chart, tag) + }.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 + // 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 @@ -472,6 +621,7 @@ abstract class MetricsChartRenderer( scaleY: Float, ) { userHasZoomed = true + abandonGesture() } override fun onChartTranslate( @@ -483,6 +633,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. + */ + 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 030dd3954f..f116dc2945 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 @@ -80,6 +81,24 @@ class SafeLineChart : LineChart { val color: Int, ) + /** + * 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/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt b/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt deleted file mode 100644 index 43e3b76b00..0000000000 --- a/app/src/main/java/com/itsaky/androidide/utils/LongPressHelpExtensions.kt +++ /dev/null @@ -1,34 +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 -} 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/LongPressHelpTimingTest.kt b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt new file mode 100644 index 0000000000..30e722393c --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/LongPressHelpTimingTest.kt @@ -0,0 +1,406 @@ +/* + * 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 android.widget.HorizontalScrollView +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 +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 + + /** + * 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++ } + } + + /** + * 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, + x: Float = CENTRE_X, + y: Float = CENTRE_Y, + ) { + val event = MotionEvent.obtain(0L, 0L, action, x, y, 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) + + /** + * 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 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 + // 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) + drain() + + 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) + drain() + + 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) + drain() + + assertThat(holds).isEqualTo(1) + assertThat(clicks).isEqualTo(0) + } + + @Test + 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 = 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. + 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 `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 + 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() + } + + @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() + } + + @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 + + 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/MetricsChartAxisTapTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt index b56f2805d3..672c5430f9 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt @@ -106,6 +106,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 new file mode 100644 index 0000000000..9723b88c74 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartGestureTeardownTest.kt @@ -0,0 +1,167 @@ +/* + * 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.view.ViewConfiguration +import androidx.test.core.app.ApplicationProvider +import com.github.mikephil.charting.listener.ChartTouchListener +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * 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 harness = ChartGestureHarness(ApplicationProvider.getApplicationContext()) + + private val taps get() = harness.taps + + private val helps get() = harness.helps + + private val renderer get() = harness.renderer + + private fun laidOutChart() = harness.laidOutChart() + + private fun longPressAt( + chart: SafeLineChart, + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ) = harness.longPressAt(chart, y, sincePressMillis) + + private fun panBy( + chart: SafeLineChart, + dx: Float, + ) = harness.panBy(chart, dx) + + private fun endGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) = harness.endGesture(chart, gesture) + + private fun cancelGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) = harness.cancelGesture(chart, gesture) + + private fun onAxisBand(chart: SafeLineChart) = harness.onAxisBand(chart) + + private fun insidePlot(chart: SafeLineChart) = harness.insidePlot(chart) + + @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 + // 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 new file mode 100644 index 0000000000..99851457d5 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartHoldHelpTest.kt @@ -0,0 +1,184 @@ +/* + * 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.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.longPressHelpTimeoutMillis +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * 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 harness = ChartGestureHarness(ApplicationProvider.getApplicationContext()) + + private val taps get() = harness.taps + + private val helps get() = harness.helps + + private val renderer get() = harness.renderer + + private fun laidOutChart() = harness.laidOutChart() + + private fun longPressAt( + chart: SafeLineChart, + y: Float, + sincePressMillis: Long = ViewConfiguration.getLongPressTimeout().toLong(), + ) = harness.longPressAt(chart, y, sincePressMillis) + + private fun panBy( + chart: SafeLineChart, + dx: Float, + ) = harness.panBy(chart, dx) + + private fun scaleBy( + chart: SafeLineChart, + factor: Float, + ) = harness.scaleBy(chart, factor) + + private fun endGesture( + chart: SafeLineChart, + gesture: ChartTouchListener.ChartGesture, + ) = harness.endGesture(chart, gesture) + + private fun insidePlot(chart: SafeLineChart) = harness.insidePlot(chart) + + @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) + + // 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) + } + + @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) + drain() + + 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)) + scaleBy(chart, 1.2f) + elapse(remainderOfHold()) + + assertThat(helps).isEqualTo(0) + } + + @Test + fun `the hold is measured from the finger landing, not from when the press was reported`() { + val chart = laidOutChart() + + // 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(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/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 02213fd571..304270eac2 100644 --- a/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt +++ b/idetooltips/src/main/java/com/itsaky/androidide/utils/ViewUtils.kt @@ -1,8 +1,14 @@ 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 android.view.ViewGroup +import com.itsaky.androidide.idetooltips.R import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager @@ -23,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]. */ @@ -40,17 +50,313 @@ 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. + * + * [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(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 + * [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()) { + // 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 + } + + 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, +) { + // 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. 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 + // 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. + 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 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 + 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. + * + * `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 + // The framework starts the ripple from the touch point. Without this every ripple + // on these controls begins at the centre of the drawable instead. + 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) + } + + 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) + releasePress() + } + + MotionEvent.ACTION_MOVE -> { + 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) + releasePress() + } + } + + MotionEvent.ACTION_UP -> { + handler.removeCallbacks(fire) + 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 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 + // 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. + val click = + Runnable { + pendingClick = null + v.performClick() + } + pendingClick = click + handler.post(click) + } + holding = false + } + + MotionEvent.ACTION_CANCEL -> { + holding = false + handler.removeCallbacks(fire) + releasePress() + } + } + 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 @@ + + + + +