-
Notifications
You must be signed in to change notification settings - Fork 0
Android: opt-in removal snapshot (snapshotOnRemoval) + interactive edge-swipe-back #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: readwise
Are you sure you want to change the base?
Changes from 8 commits
f5dcc56
5879b16
16b8ea4
84db105
9ff816c
f5a1e8a
165ac0e
cc9ab08
d085779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,340 @@ | ||
| package com.swmansion.rnscreens | ||
|
|
||
| import android.animation.Animator | ||
| import android.animation.AnimatorListenerAdapter | ||
| import android.animation.ValueAnimator | ||
| import android.util.Log | ||
| import android.view.MotionEvent | ||
| import android.view.VelocityTracker | ||
| import android.view.ViewConfiguration | ||
| import android.view.animation.DecelerateInterpolator | ||
| import com.swmansion.rnscreens.utils.RNSLog | ||
| import kotlin.math.abs | ||
|
|
||
| /** | ||
| * Readwise: native interactive edge-swipe-back for Android, mirroring the iOS | ||
| * interactive pop (RNSScreenStack.mm). | ||
| * | ||
| * Detection: DOWN within the edge band (50dp — master's reader gesture | ||
| * hitSlop) on a screen whose [Screen.isGestureEnabled] is set (native-stack | ||
| * maps androidEdgeSwipeBack/gestureEnabled/usePreventRemove into it), | ||
| * activation after a platform-touch-slop pull (~8dp) of horizontal-dominant | ||
| * travel. | ||
| * Tracking: top screen translationX 1:1; screen below parallaxes from | ||
| * -0.3*width to 0 (RNSScreenStackAnimator.mm:542). | ||
| * Commit: translation + 0.3*velocity > width/2 (RNSScreenStack.mm:1139-1140), | ||
| * then notifyTopDetached() -> ScreenDismissedEvent -> JS StackActions.pop(). | ||
| * Cancel: settle back, detachBelowTop(), transforms reset. | ||
| */ | ||
| internal class EdgeSwipeBackController( | ||
| private val stack: ScreenStack, | ||
| ) { | ||
| private enum class State { IDLE, ARMED, DRAGGING, SETTLING, AWAITING_REMOVAL } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation opinion anchor — Keep the dedicated controller as the touch/state ownership boundary, but make it participate in the existing transition event protocol and bind ARMED/AWAITING_REMOVAL to an exact screen identity. |
||
|
|
||
| private var state = State.IDLE | ||
| private var downX = 0f | ||
| private var downY = 0f | ||
| private var velocityTracker: VelocityTracker? = null | ||
| private var topScreenView: Screen? = null | ||
| private var belowScreenView: Screen? = null | ||
| private var settleAnimator: ValueAnimator? = null | ||
|
|
||
| // Force-release AWAITING_REMOVAL if JS never reconciles a committed pop, so | ||
| // the gesture can't wedge input permanently. Cancelled in the normal path | ||
| // by onStackChildrenChanged(). | ||
| private val releaseAwaitingRemoval = | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convention audit: minor divergence — Name this rollback for what it does. releaseAwaitingRemoval resets transforms and detaches the below-top fragment; it performs a full rollback, not a release. Rename it to rollbackAwaitingRemoval and update the force-release comment so future lifecycle edits do not mistake this for state-only cleanup. |
||
| Runnable { | ||
| if (state == State.AWAITING_REMOVAL) { | ||
| Log.w(TAG, "awaiting-removal safeguard fired; JS never reconciled the pop") | ||
| state = State.IDLE | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH — BLOCKING] This safeguard releases the state machine without restoring the native stack. On the committed path, Retain the committed pair until reconciliation. On timeout, either roll back fully (restore the top transform, detach the exact below fragment, clear refs/tracker, then return to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d085779, and verified the way this finding deserved. The committed (top, below) pair is now retained through AWAITING_REMOVAL (finish(true) no longer clears the refs); on timeout the rollback restores the top screen's transform, detaches the below fragment, clears the refs, and returns to IDLE. Reconciliation (onStackChildrenChanged) drops the retained pair without touching transforms, preserving the no-flash asymmetry. Verification: I froze the app's JS thread via the Hermes debugger (CDP Debugger.pause through Metro) and performed a committed edge swipe — exactly the "JS never reconciles" case. Logcat shows the rollback firing at 1s, and the screen shows the Reader restored at identity while JS is still frozen; on resume, the queued ScreenDismissedEvent pops normally. Notably, the first cut of this fix (rollback without retaining the pair) produced a black screen in this very test — the refs were already nulled so the restore no-oped — which is empirical confirmation that pair retention is the load-bearing part of your prescription. On the coverage ask: the fork's Android tree still has no test source sets/dependencies, so this remains harness-limited to the live-device procedure above (scripted and repeatable: freeze_js.js via Metro's inspector endpoint). |
||
| } | ||
| } | ||
|
|
||
| private val density = stack.resources.displayMetrics.density | ||
| private val edgeRegionPx = EDGE_REGION_DP * density | ||
|
|
||
| // Activation distance: the platform drag threshold (~8dp), matching how | ||
| // eagerly UIKit's pan hysteresis (~10pt) engages on iOS. The | ||
| // horizontal-dominance check below keeps scroll intents safe. | ||
| private val touchSlopPx = ViewConfiguration.get(stack.context).scaledTouchSlop.toFloat() | ||
| private val activationPx = touchSlopPx | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convention audit: redundant — Remove the semantic alias that is permanently equal to touchSlopPx. activationPx suggests an independently tuned activation threshold, but it is only touchSlopPx under a second name. Compare against touchSlopPx directly, or introduce a genuinely independent activationThresholdPx if separate tuning is intended. |
||
|
|
||
| val isInteracting: Boolean | ||
| get() = state == State.DRAGGING || state == State.SETTLING | ||
|
|
||
| fun onInterceptTouchEvent(ev: MotionEvent): Boolean { | ||
| when (ev.actionMasked) { | ||
| MotionEvent.ACTION_DOWN -> { | ||
| if (state == State.SETTLING) { | ||
| // Eat taps while the settle animation finishes. | ||
| return true | ||
| } | ||
| if (state == State.AWAITING_REMOVAL) { | ||
| // A committed dismissal is awaiting JS reconciliation of the | ||
| // pop. Don't arm a new gesture (a second swipe here would | ||
| // over-pop), but let the below screen keep receiving touches. | ||
| // Released from onStackChildrenChanged() when the pop lands. | ||
| return false | ||
| } | ||
| state = State.IDLE | ||
| // Cheapest-first rejection after the state guards above: nearly | ||
| // every touch is outside the band, and pair resolution (a filter | ||
| // pass over screenWrappers) must stay dead last so disabled | ||
| // screens and mid-screen touches never pay for it. | ||
| if (ev.x > edgeRegionPx) { | ||
| return false | ||
| } | ||
| val top = stack.topScreen | ||
| // isGestureEnabled is the arming signal: native-stack maps | ||
| // androidEdgeSwipeBack + gestureEnabled + usePreventRemove into it. | ||
| if (top?.isGestureEnabled != true) { | ||
| RNSLog.d(TAG, "down in band but not armed: gesture disabled") | ||
| return false | ||
| } | ||
| if (top.stackPresentation != Screen.StackPresentation.PUSH) { | ||
| RNSLog.d(TAG, "down in band but not armed: non-push presentation") | ||
| return false | ||
| } | ||
| if (stack.resolveEdgeSwipeScreenPair() == null) { | ||
| RNSLog.d(TAG, "down in band but not armed: active pair unresolvable") | ||
| return false | ||
| } | ||
| state = State.ARMED | ||
| downX = ev.x | ||
| downY = ev.y | ||
| velocityTracker?.recycle() | ||
| velocityTracker = VelocityTracker.obtain() | ||
| velocityTracker?.addMovement(ev) | ||
| RNSLog.d(TAG, "armed at x=${ev.x} (edge=${edgeRegionPx}px)") | ||
| } | ||
| MotionEvent.ACTION_MOVE -> { | ||
| velocityTracker?.addMovement(ev) | ||
| if (state == State.ARMED) { | ||
| val dx = ev.x - downX | ||
| val dy = ev.y - downY | ||
| if (dx >= activationPx && dx > abs(dy)) { | ||
| // Deliberate horizontal pull from the edge — steal the | ||
| // stream (children get ACTION_CANCEL, like UIKit). | ||
| startDrag() | ||
| } else if (abs(dy) > touchSlopPx && abs(dy) > dx) { | ||
| // Vertical intent — the content scroll wins. | ||
| RNSLog.d(TAG, "vertical bail dx=$dx dy=$dy") | ||
| disarm() | ||
| } | ||
| } | ||
| } | ||
| MotionEvent.ACTION_UP, MotionEvent.ACTION_CANCEL -> | ||
| if (state == State.ARMED) { | ||
| // UP = lifted without an activating pull; CANCEL = an ancestor | ||
| // or the system claimed the stream before we activated. | ||
| RNSLog.d( | ||
| TAG, | ||
| "armed released without activation (${if (ev.actionMasked == MotionEvent.ACTION_CANCEL) "cancelled" else "lifted"})", | ||
| ) | ||
| disarm() | ||
| } | ||
| } | ||
| return state == State.DRAGGING | ||
| } | ||
|
|
||
| fun onTouchEvent(ev: MotionEvent): Boolean { | ||
| if (state != State.DRAGGING) { | ||
| return state == State.SETTLING | ||
| } | ||
| velocityTracker?.addMovement(ev) | ||
| when (ev.actionMasked) { | ||
| MotionEvent.ACTION_MOVE -> track(ev.x - downX) | ||
| MotionEvent.ACTION_UP -> { | ||
| velocityTracker?.computeCurrentVelocity(1000) | ||
| val vx = velocityTracker?.xVelocity ?: 0f | ||
| val dx = (ev.x - downX).coerceIn(0f, stack.width.toFloat()) | ||
| // iOS commit rule — RNSScreenStack.mm:1139-1140. | ||
| val commit = dx + 0.3f * vx > stack.width / 2f | ||
| RNSLog.d(TAG, "release dx=$dx vx=$vx commit=$commit") | ||
| settle(dx, commit) | ||
| } | ||
| MotionEvent.ACTION_CANCEL -> { | ||
| RNSLog.d(TAG, "cancelled by ancestor/system mid-drag") | ||
| settle((ev.x - downX).coerceIn(0f, stack.width.toFloat()), false) | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| private fun startDrag() { | ||
| // Resolving the pair also revalidates the positional invariant | ||
| // attachBelowTop() relies on (it may have changed since ARMED). | ||
| val (top, below) = | ||
| stack.resolveEdgeSwipeScreenPair() ?: run { | ||
| RNSLog.d(TAG, "active pair unresolvable at activation; aborting drag") | ||
| disarm() | ||
| return | ||
| } | ||
| try { | ||
| stack.attachBelowTop() | ||
| } catch (e: RuntimeException) { | ||
| Log.w(TAG, "attachBelowTop failed; aborting drag", e) | ||
| disarm() | ||
| return | ||
| } | ||
| // The Screen views persist across attachBelowTop's fragment re-add | ||
| // (view recycling), so the pair resolved above stays valid. | ||
| topScreenView = top | ||
| belowScreenView = below | ||
| state = State.DRAGGING | ||
| // Keep ancestors (incl. the RNGH root) from intercepting mid-drag. | ||
| stack.parent?.requestDisallowInterceptTouchEvent(true) | ||
| track(0f) | ||
| RNSLog.d(TAG, "drag started top=${topScreenView?.id} below=${belowScreenView?.id}") | ||
| } | ||
|
|
||
| private fun recycleTracker() { | ||
| velocityTracker?.recycle() | ||
| velocityTracker = null | ||
| } | ||
|
|
||
| /** | ||
| * Reset an ARMED-but-not-yet-dragging gesture back to idle, recycling the | ||
| * tracker. Every ARMED abort (vertical bail, UP/CANCEL, activation failure) | ||
| * routes through here so the pooled VelocityTracker is never leaked — DOWN | ||
| * accumulates into it before the state check, so a bare `state = IDLE` would | ||
| * strand it until the next arm. | ||
| */ | ||
| private fun disarm() { | ||
| recycleTracker() | ||
| state = State.IDLE | ||
| } | ||
|
|
||
| private fun track(rawDx: Float) { | ||
| val width = stack.width.toFloat() | ||
| val dx = rawDx.coerceIn(0f, width) | ||
| topScreenView?.translationX = dx | ||
| // Below screen parallax — RNSScreenStackAnimator.mm:542. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Publish transition progress and cancellation for this manual interactive transition. This path only mutates translationX, so it bypasses the existing fragment animator path that emits ScreenTransitionProgressEvent. As a result, useTransitionProgress/Reanimated consumers stay frozen during the drag, and a cancelled swipe emits no gestureCancel signal even though NativeStackView exposes that lifecycle. The existing Android interactive-transition implementation in ScreensModule.updateTransition dispatches progress on every update. Dispatch normalized progress through the existing screen/fragment event path while tracking, and emit the appropriate terminal/cancel lifecycle signal when settling completes. |
||
| belowScreenView?.translationX = -PARALLAX_FACTOR * (width - dx) | ||
| } | ||
|
|
||
| private fun settle( | ||
| fromDx: Float, | ||
| commit: Boolean, | ||
| ) { | ||
| state = State.SETTLING | ||
| val width = stack.width.toFloat() | ||
| val target = if (commit) width else 0f | ||
| // Base 0.5s (RNSDefaultTransitionDuration), proportional to remaining distance. | ||
| val remaining = abs(target - fromDx) / width | ||
| val duration = (BASE_DURATION_MS * remaining).toLong().coerceIn(80L, BASE_DURATION_MS.toLong()) | ||
| settleAnimator?.cancel() | ||
| settleAnimator = | ||
| ValueAnimator.ofFloat(fromDx, target).apply { | ||
| this.duration = duration | ||
| // Approximates iOS's overdamped nav spring (ratio ~4.56). | ||
| interpolator = DecelerateInterpolator(1.5f) | ||
| addUpdateListener { track(it.animatedValue as Float) } | ||
| addListener( | ||
| object : AnimatorListenerAdapter() { | ||
| // cancel() still delivers onAnimationEnd; the abort path | ||
| // (onStackChildrenChanged) does its own cleanup and must | ||
| // not double-dispatch finish(): a committed finish would | ||
| // fire a second ScreenDismissedEvent against a stack | ||
| // React is already mutating. | ||
| private var cancelled = false | ||
|
|
||
| override fun onAnimationCancel(animation: Animator) { | ||
| cancelled = true | ||
| } | ||
|
|
||
| override fun onAnimationEnd(animation: Animator) { | ||
| if (!cancelled) { | ||
| finish(commit) | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| start() | ||
| } | ||
| } | ||
|
|
||
| private fun finish(commit: Boolean) { | ||
| RNSLog.d(TAG, "finish commit=$commit") | ||
| recycleTracker() | ||
| settleAnimator = null | ||
| if (commit) { | ||
| // ScreenDismissedEvent -> native-stack onDismissed -> StackActions.pop(). | ||
| // The JS pop removes the top fragment; reset the below screen so it | ||
| // sits at identity when it becomes top. | ||
| stack.notifyTopDetached() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the architectural handoff to protect: after dispatching dismissal, retain enough native state to either confirm reconciliation or reverse this commit completely. Clearing the pair before that outcome is known makes the timeout incapable of restoring the invariant. |
||
| belowScreenView?.translationX = 0f | ||
| } else { | ||
| try { | ||
| stack.detachBelowTop() | ||
| } catch (e: RuntimeException) { | ||
| Log.w(TAG, "detachBelowTop failed on cancel", e) | ||
| } | ||
| topScreenView?.translationX = 0f | ||
| belowScreenView?.translationX = 0f | ||
| } | ||
| topScreenView = null | ||
| belowScreenView = null | ||
| if (commit) { | ||
| // The pop is dispatched to JS, but the top fragment isn't gone until | ||
| // JS reconciles. Hold ownership in AWAITING_REMOVAL so a second | ||
| // swipe in that window can't begin an overlapping dismissal (which | ||
| // would over-pop). Released from onStackChildrenChanged() when the | ||
| // stack updates; the delayed safeguard covers a pop that never lands. | ||
| state = State.AWAITING_REMOVAL | ||
| stack.removeCallbacks(releaseAwaitingRemoval) | ||
| stack.postDelayed(releaseAwaitingRemoval, AWAITING_REMOVAL_TIMEOUT_MS) | ||
| } else { | ||
| state = State.IDLE | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The stack's children changed under an active gesture (e.g. hardware back | ||
| * popped mid-drag). Abort and restore consistent transforms. | ||
| */ | ||
| fun onStackChildrenChanged() { | ||
| if (state == State.AWAITING_REMOVAL) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Do not treat every container update as proof that the committed pop reconciled. ScreenStack.onUpdate calls this method unconditionally, including updates caused by activity-state or other child changes. In AWAITING_REMOVAL this branch clears the retained pair, cancels the rollback timeout, and returns to IDLE without checking that the committed top was actually removed. An unrelated update can therefore leave the logical top translated fully offscreen, allow another swipe to over-pop, and permanently disable the safeguard. Keep AWAITING_REMOVAL until the retained top is no longer the logical/contained top. Pass the current stack identity into this transition or query it here; only then cancel the timeout and clear the pair. |
||
| // JS reconciled the committed pop; release ownership so the next | ||
| // gesture is accepted. Deliberately does NOT reset translations — | ||
| // the dismissed screen is off-window and restoring it here would | ||
| // flash it back onscreen for a frame before removal. | ||
| stack.removeCallbacks(releaseAwaitingRemoval) | ||
| state = State.IDLE | ||
| return | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Abort ARMED when the stack changes too. A stack update between ACTION_DOWN and activation currently leaves the old tracker/down coordinates armed. The next MOVE calls startDrag, which resolves the new pair but does not re-check the new top gestureEnabled or presentation, so a stream armed for the previous route can steal and dismiss a newly installed or disabled route. Include State.ARMED in this invalidation path and call disarm(). |
||
| if (state == State.DRAGGING || state == State.SETTLING) { | ||
| RNSLog.d(TAG, "stack changed mid-gesture; aborting") | ||
| settleAnimator?.cancel() | ||
| settleAnimator = null | ||
| topScreenView?.translationX = 0f | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Consolidate the repeated reset-to-idle cleanup. The same translation reset, retained-view clearing, and IDLE transition now appears in the timeout rollback, cancel finish, and stack-change abort paths. This has reached the rule-of-three threshold, and cleanup drift is especially risky in a state machine. Extract a resetViewsAndIdle helper, optionally recycling the tracker, while keeping path-specific detach/logging at each caller. |
||
| belowScreenView?.translationX = 0f | ||
| topScreenView = null | ||
| belowScreenView = null | ||
| recycleTracker() | ||
| state = State.IDLE | ||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| private const val TAG = "[EdgeSwipeBack]" | ||
|
|
||
| // Master's reader gesture band: useNavigationGestures hitSlop | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convention audit: minor divergence — Describe this as the controller's 50dp Android edge region (or link a durable shared contract). Otherwise a later Reader hit-slop change can make this comment silently false even though the constant remains unchanged.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d085779 — the constant is now described as the controller's own Android edge region (system-back bezel note retained), with no app/branch-specific terminology. |
||
| // {left: 0, width: 50}. The Android system back gesture owns the | ||
| // outermost ~30dp of this on gesture-nav devices (by design — we | ||
| // don't fight it; a bezel swipe does a plain system back, which | ||
| // also pops); the strip beyond it is the interactive drag. | ||
| private const val EDGE_REGION_DP = 50f | ||
|
|
||
| // RNSScreenStackAnimator.mm:542. | ||
| private const val PARALLAX_FACTOR = 0.3f | ||
|
|
||
| // RNSDefaultTransitionDuration — RNSScreenStackAnimator.mm:11. | ||
| private const val BASE_DURATION_MS = 500f | ||
|
|
||
| // Safety valve for AWAITING_REMOVAL: a committed pop normally reconciles | ||
| // within a frame or two. If it never does, force-release so input can't | ||
| // wedge permanently. | ||
| private const val AWAITING_REMOVAL_TIMEOUT_MS = 1000L | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convention audit: coupling — This controller's arming contract depends on an
androidEdgeSwipeBack/usePreventRemovemapping that is not present in this repository.The current native-stack source still forces
gestureEnabled={false}on Android, while the PR description explains that Reader supplies a separate patch-level mapping. That is intentional fork integration, but it makes this native code unsafe to consume independently: the prevent-remove guarantee exists only in the downstream patch. Please make that dependency durable—either add the typed mapping here or link/name the downstream patch contract explicitly in code and keep the two changes pinned together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d085779 — the controller header now names the downstream contract durably: it states that upstream native-stack forces gestureEnabled=false on Android (so the controller ships disarmed for independent consumers), and points at readwiseio/rekindled's patches/@react-navigation__native-stack@7.12.0.patch with the exact arming expression, explicitly noting the prevent-remove guarantee lives in that patch. The two sides stay pinned together by the app's commit-pinned dependency on this fork.