-
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 2 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 |
|---|---|---|
|
|
@@ -2,10 +2,19 @@ package com.swmansion.rnscreens | |
|
|
||
| import android.annotation.SuppressLint | ||
| import android.content.pm.ActivityInfo | ||
| import android.graphics.Bitmap | ||
| import android.graphics.Paint | ||
| import android.graphics.Rect | ||
| import android.graphics.drawable.BitmapDrawable | ||
| import android.os.Build | ||
| import android.os.Handler | ||
| import android.os.HandlerThread | ||
| import android.os.Looper | ||
| import android.os.Parcelable | ||
| import android.util.Log | ||
| import android.util.SparseArray | ||
| import android.view.MotionEvent | ||
| import android.view.PixelCopy | ||
| import android.view.View | ||
| import android.view.ViewGroup | ||
| import android.view.WindowManager | ||
|
|
@@ -34,6 +43,9 @@ import com.swmansion.rnscreens.events.SheetDetentChangedEvent | |
| import com.swmansion.rnscreens.ext.asScreenStackFragment | ||
| import com.swmansion.rnscreens.ext.parentAsViewGroup | ||
| import com.swmansion.rnscreens.gamma.common.FragmentProviding | ||
| import java.util.concurrent.CountDownLatch | ||
| import java.util.concurrent.TimeUnit | ||
| import java.util.concurrent.atomic.AtomicBoolean | ||
|
|
||
| @SuppressLint("ViewConstructor") // Only we construct this view, it is never inflated. | ||
| class Screen( | ||
|
|
@@ -459,12 +471,136 @@ class Screen( | |
|
|
||
| fun startRemovalTransition() { | ||
| if (!isBeingRemoved) { | ||
| captureScreenSnapshotForRemoval() | ||
| isBeingRemoved = true | ||
| startTransitionRecursive(this) | ||
| } | ||
| } | ||
|
|
||
| // --- Screen-level removal snapshot ------------------------------------------------------- | ||
| // The exit animation renders this screen while surface-backed content (e.g. a mid-fling | ||
| // WebView) may stop producing pixels, which flashes the window background through. When a | ||
| // screen opts in via the `snapshotOnRemoval` prop, freeze its last presented frame into its | ||
| // ViewOverlay for the duration of the removal transition so the pop animates real pixels. | ||
| // Opt-in only: the capture reads the composited window, which is only correct for opaque, | ||
| // full-window screens (e.g. the Reader) — never enable it for translucent/partial screens. | ||
|
|
||
| /** Set from JS via the `snapshotOnRemoval` prop; default off. */ | ||
| var snapshotOnRemoval: Boolean = false | ||
|
|
||
| private var removalSnapshotBitmap: Bitmap? = null | ||
|
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] Remove this field and its two assignments; keep the drawable as the single owner tracked by the screen.
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 84db105 — the field was removed; the drawable is the single owner tracked by the screen. |
||
| private var removalSnapshotDrawable: BitmapDrawable? = null | ||
|
|
||
| // One capture attempt per removal: a snapshot is only valid if taken before the removal | ||
| // transaction starts animating. If the pre-transaction attempt fails (timeout etc.), a later | ||
|
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 — The timeout wording no longer matches the current async fallback. The 17ms wait expiring is not a failed attempt: the same PixelCopy remains live and may install from its posted continuation. Please distinguish actual failure from asynchronous completion, e.g. “If the attempt fails or completes asynchronously, later entry points must not issue another capture.”
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 f5a1e8a — the one-attempt comment now distinguishes outright failure from asynchronous completion, per your wording. |
||
| // entry point must NOT retry — it could capture an already-translated, mid-exit window. | ||
| // Reset alongside the snapshot itself in releaseRemovalSnapshot(). | ||
| @Volatile | ||
| private var removalSnapshotAttempted: Boolean = false | ||
|
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. Architectural seam: this single boolean currently conflates a queued request, an actual capture attempt, and membership in a particular removal lifecycle. I would replace it with main-thread-owned attempt state plus a removal generation/token; that makes the pre-transaction guarantee explicit rather than timing-dependent.
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. Addressed in 16b8ea4 per the blocking finding: scheduled/attempted/lifecycle are now distinct — attempt state is main-thread-owned and claimed at capture time, and queued requests carry a removal-generation token. The pre-transaction guarantee is structural rather than timing-dependent. |
||
|
|
||
| // Ordering note: Fabric may call startRemovalTransition() from its mounting thread, where the | ||
| // capture is posted to the main looper. The ordering guarantee (capture happens before the | ||
| // removal transaction's animation) does not rest on that post: ScreenStack.onUpdate() invokes | ||
| // captureScreenSnapshotForRemoval() synchronously on the main thread immediately before it | ||
| // creates the removal transaction, and whichever call runs first wins the single attempt. | ||
| fun captureScreenSnapshotForRemoval() { | ||
| if (!snapshotOnRemoval) return | ||
| if (removalSnapshotAttempted) return | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return | ||
| removalSnapshotAttempted = true | ||
|
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] The off-main caller claims the sole attempt before any capture happens. Fabric can enter this method off-main, set Claim the attempt on main at actual capture time, or distinguish scheduled vs attempted and attach a removal-generation token. The synchronous pre-transaction call must be allowed to win, and an older queued runnable must no-op. Please add a deterministic test for off-main
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 16b8ea4, following the prescribed design:
On the deterministic interleaving test: the Android tree currently has no unit-test infrastructure at all (no test source sets, no junit/robolectric dependencies), so a deterministic threading test means standing up a test framework in the fork — I'd rather not couple that to this fix. The redesign removes the interleaving class structurally (single-writer main-thread state) rather than defending against it, and the device re-verification (mid-fling back ×10, zero black frames, including the Fabric off-main |
||
| if (Looper.myLooper() == Looper.getMainLooper()) { | ||
| captureScreenSnapshotOnMain() | ||
| } else { | ||
| post { captureScreenSnapshotOnMain() } | ||
| } | ||
| } | ||
|
|
||
| private fun captureScreenSnapshotOnMain() { | ||
|
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: gap — this adds a new lifecycle/thread-race state machine without an automated regression case. The timeout/callback ownership handoff, cancellation, detach, repeated removal, API <26 behavior, and Paper/Fabric ordering are the load-bearing cases. Add a focused Android test seam around the capture result/timeout and at least an app regression case that exercises removal and cancellation; the existing stack test fixture is at
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. Acknowledged as a real gap. Two constraints shaped what I did instead: the fork's Android tree has no unit-test source sets or test dependencies, and the load-bearing behavior (PixelCopy timing vs presented frames) is only observable on a real compositor. The lifecycle cases named here — timeout, cancellation, detach, repeated removal, both entry-point orderings — are covered by on-device suites that ran against each commit (interrupted-transition ×6, rapid double-back, 15-cycle removal memory soak, and a per-frame dual-probe video scan of mid-fling pops that caught a real regression in the async variant and drove 9ff816c). I'd take the |
||
| // Recheck the opt-in: a posted capture can outlive a prop flip on a recycled screen. | ||
| if (!snapshotOnRemoval) return | ||
| if (removalSnapshotBitmap != null) return | ||
| if (!isAttachedToWindow || width <= 0 || height <= 0) return | ||
| val activity = reactContext.currentActivity ?: return | ||
| val window = activity.window | ||
| val decor = window.decorView | ||
|
|
||
| val location = IntArray(2) | ||
| getLocationInWindow(location) | ||
| val srcRect = Rect(location[0], location[1], location[0] + width, location[1] + height) | ||
| // Only capture a fully on-window screen; a clipped rect would be stretched over the | ||
| // screen's full bounds when displayed. | ||
| if (!srcRect.intersect(0, 0, decor.width, decor.height)) return | ||
| if (srcRect.width() != width || srcRect.height() != height) return | ||
|
|
||
| val bitmap = | ||
| try { | ||
| Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888) | ||
| } catch (e: OutOfMemoryError) { | ||
| Log.w(TAG, "removal snapshot allocation failed", e) | ||
| return | ||
| } | ||
|
|
||
| val latch = CountDownLatch(1) | ||
| val resultHolder = intArrayOf(PixelCopy.ERROR_UNKNOWN) | ||
|
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] Rename it 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. Renamed to |
||
| // Bitmap ownership on timeout: PixelCopy cannot be cancelled, so the copy may still be | ||
| // writing into the bitmap after the bounded wait expires. Whoever finishes second (the | ||
| // callback, or the timed-out waiter observing the callback already ran) recycles it; | ||
| // recycling from the waiter while the copy is in flight would be a native UAF. | ||
| val loserRecycles = AtomicBoolean(false) | ||
|
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] The flag records that one of two cleanup participants has finished; whichever arrives second recycles. Rename it around that actual invariant (for example
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. Renamed to |
||
| try { | ||
| PixelCopy.request(window, srcRect, bitmap, { copyResult -> | ||
| resultHolder[0] = copyResult | ||
| latch.countDown() | ||
| if (loserRecycles.getAndSet(true)) { | ||
| // The waiter already timed out and abandoned the bitmap — we own cleanup. | ||
| bitmap.recycle() | ||
| } | ||
| }, snapshotHandler()) | ||
| } catch (e: IllegalArgumentException) { | ||
| bitmap.recycle() | ||
| return | ||
| } | ||
|
|
||
| val completed = | ||
| try { | ||
| latch.await(SNAPSHOT_TIMEOUT_MS, TimeUnit.MILLISECONDS) | ||
|
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 seam I would change: PixelCopy may remain asynchronous, but the removal transaction should be its continuation rather than making the UI thread synchronously wait here.
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. See the thread on line 574 — the fully-async continuation was implemented (84db105) and measured: it guarantees 1–2 black frames per mid-fling pop because the app's React-commit blank presents inside the install gap; transaction deferral can't prevent that. 9ff816c bounds the synchronous wait to one frame period with an async fallback, which preserves the nonblocking property in the worst case while keeping the flash-free property in the typical case.
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: deviation — transition lifecycle work now synchronously waits on a worker callback from the main thread. The existing removal path is synchronous and lightweight, and nearby transition work is posted rather than waited on:
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] This blocks the Android main/UI thread for up to 50 ms immediately before starting the removal animation. Both entry paths deliberately converge on Please keep the main thread nonblocking: let PixelCopy completion (or the timeout fallback) continue/defer the removal transaction, or use a capture primitive that does not require waiting on another thread.
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. Addressed across 84db105 → 9ff816c, with an empirical result worth recording: I first shipped the fully-nonblocking variant (84db105): copy requested pre-transaction, main thread never waits, completion posts the install. On device it produced 1–2 fully black frames on every mid-fling pop (dual-probe per-frame video scan; the previous synchronous build showed zero such frames across 30+ cycles). The reason: the app's own teardown blank (React commit clearing the doc on So a synchronous install in the pre-transaction main-thread turn is load-bearing whenever the copy can make it. 9ff816c is the hybrid: the main thread waits at most one frame period (17 ms) — measured copies complete in 6–16 ms, so the typical install is synchronous and flash-free — and if the copy is slower it moves on, with the completion posting the install instead (single main-thread-owned install/drop decision, generation-guarded). Worst case is one frame of jank or one late frame; the 50 ms stall is gone, and the deterministic multi-frame jank concern no longer applies (bound ≤ 1 frame @60 Hz). Re-verified on device with the stricter dual-probe detector: 10/10 mid-fling pops, zero dual-black frames. (Final state verified against f5a1e8a, which also carries the round-4 comment-precision fixes: 10/10 mid-fling pops, dual-probe scan, zero black frames, probe values 190–201 throughout.) |
||
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() | ||
| false | ||
| } | ||
|
|
||
| if (!completed) { | ||
| // Abandon: if the callback already finished in the meantime we recycle, otherwise | ||
| // the callback sees the flag and recycles when the copy completes. | ||
| if (loserRecycles.getAndSet(true)) { | ||
| bitmap.recycle() | ||
| } | ||
| return | ||
| } | ||
| if (resultHolder[0] != PixelCopy.SUCCESS) { | ||
| // Copy finished (callback ran) but failed; the bitmap is no longer being written. | ||
| bitmap.recycle() | ||
| return | ||
| } | ||
|
|
||
| val drawable = BitmapDrawable(resources, bitmap) | ||
| drawable.setBounds(0, 0, width, height) | ||
| removalSnapshotBitmap = bitmap | ||
| removalSnapshotDrawable = drawable | ||
| overlay.add(drawable) | ||
| invalidate() | ||
| } | ||
|
|
||
| fun releaseRemovalSnapshot() { | ||
| removalSnapshotDrawable?.let { overlay.remove(it) } | ||
| removalSnapshotDrawable = null | ||
| removalSnapshotBitmap = null | ||
| removalSnapshotAttempted = false | ||
| } | ||
| // --- end screen-level removal snapshot ---------------------------------------------------- | ||
|
|
||
| fun endRemovalTransition() { | ||
| releaseRemovalSnapshot() | ||
| if (!isBeingRemoved) { | ||
| return | ||
| } | ||
|
|
@@ -563,6 +699,11 @@ class Screen( | |
| } | ||
| } | ||
|
|
||
| override fun onDetachedFromWindow() { | ||
| releaseRemovalSnapshot() | ||
| super.onDetachedFromWindow() | ||
| } | ||
|
|
||
| private fun dispatchSheetDetentChanged( | ||
| detentIndex: Int, | ||
| isStable: Boolean, | ||
|
|
@@ -650,6 +791,21 @@ class Screen( | |
| * This value describes value in sheet detents array that will be treated as `fitToContents` option. | ||
| */ | ||
| const val SHEET_FIT_TO_CONTENTS = -1.0 | ||
|
|
||
| private const val SNAPSHOT_TIMEOUT_MS = 50L | ||
|
|
||
| private var sSnapshotThread: HandlerThread? = null | ||
| private var sSnapshotHandler: Handler? = null | ||
|
|
||
| @JvmStatic | ||
| private fun snapshotHandler(): Handler { | ||
|
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] It has one call site (line 558), seven lines of thin initialization/caching orchestration, and two backing fields that exist only for it. Prefer a lazy companion property (or initialize at the call site) so the handler/thread ownership is visible without a one-off method.
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. Done in 16b8ea4 — replaced the one-use method and its two backing fields with a lazy companion property; thread/handler ownership is now visible at the declaration. |
||
| sSnapshotHandler?.let { return it } | ||
| val thread = HandlerThread("RNSScreenSnapshot").also { it.start() } | ||
| val handler = Handler(thread.looper) | ||
| sSnapshotThread = thread | ||
| sSnapshotHandler = handler | ||
| return handler | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,6 +220,13 @@ class ScreenStack( | |
| } | ||
| } | ||
|
|
||
| if (!shouldUseOpenAnimation && topScreenWillChange) { | ||
| // Close animation: the current top screen is outgoing. Freeze its last presented | ||
| // frame before the removal transaction starts animating it (covers dismissal paths | ||
| // where Fabric's removeViewAt — and thus startRemovalTransition — runs later). | ||
| topScreenWrapper?.screen?.captureScreenSnapshotForRemoval() | ||
|
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 right architectural seam: capture the explicitly opted-in outgoing top screen immediately before constructing the close transaction. The important invariant is that this remains the only point at which a new snapshot attempt can succeed; a later fallback attempt after failure can capture the transition itself.
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. The invariant is now enforced in code (5879b16): a per-removal attempted flag means no entry point can start a second capture attempt after the first — whether it succeeded or failed — until the removal's cleanup resets it. |
||
| } | ||
|
|
||
| createTransaction().let { transaction -> | ||
| if (stackAnimation != null) { | ||
| transaction.setTweenAnimations(stackAnimation, shouldUseOpenAnimation) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,6 +162,14 @@ open class ScreenViewManager : | |
| view.isGestureEnabled = gestureEnabled | ||
| } | ||
|
|
||
| @ReactProp(name = "snapshotOnRemoval", defaultBoolean = false) | ||
| override fun setSnapshotOnRemoval( | ||
|
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] The new prop was not added to the checked-in Paper codegen. Run
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 5879b16 — ran |
||
| view: Screen, | ||
| snapshotOnRemoval: Boolean, | ||
| ) { | ||
| view.snapshotOnRemoval = snapshotOnRemoval | ||
| } | ||
|
|
||
| @ReactProp(name = "replaceAnimation") | ||
| override fun setReplaceAnimation( | ||
| view: Screen, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,6 +103,7 @@ export interface NativeProps extends ViewProps { | |
| homeIndicatorHidden?: boolean; | ||
| preventNativeDismiss?: boolean; | ||
| gestureEnabled?: WithDefault<boolean, true>; | ||
| snapshotOnRemoval?: WithDefault<boolean, false>; | ||
|
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: no precedent — This is the repository’s first screen-removal PixelCopy/ViewOverlay mechanism. The positive opt-in/default-false prop matches existing native-prop conventions and appropriately prevents normal stack screens from paying its cost. The generated Paper surfaces still need to be synchronized, as noted in the blocking review.
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] Regenerate the shipped Fabric declaration for this new native prop
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. Done in f5a1e8a — ran the normal bob build; the regeneration touched exactly the declarations/maps downstream of the spec change (lib/typescript/fabric/ScreenNativeComponent.d.ts + .map, lib/typescript/types.d.ts + .map, and the module/commonjs source maps), all committed. |
||
| statusBarColor?: ColorValue; | ||
| statusBarHidden?: boolean; | ||
| screenOrientation?: string; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,15 @@ export interface ScreenProps extends ViewProps { | |
| * @platform ios | ||
| */ | ||
| gestureEnabled?: boolean; | ||
| /** | ||
| * When `true`, the screen freezes its last presented frame into a native overlay at the start | ||
| * of its removal transition, so the exit animation shows real pixels even if surface-backed | ||
|
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 — This public contract currently reads as an unconditional, start-of-transition freeze, but the implementation explicitly allows the 17 ms wait to elapse and installs from an async continuation; PixelCopy failure also drops the bitmap. Please describe this as a best-effort capture requested at removal start (and mirror the wording in the generated declaration via the normal build), so consumers do not rely on a guarantee the native path cannot make.
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 prop doc (src/types.tsx and the bob-regenerated shipped declarations) now describes a best-effort capture requested at removal start: asynchronous completion shortly after the transition begins is possible, and the capture is dropped on PixelCopy failure or when the view has left the window. |
||
| * content (e.g. a WebView) stops rendering mid-transition. Only enable for opaque, full-window | ||
| * screens — the capture reads the composited window. Defaults to `false`. | ||
| * | ||
| * @platform android | ||
| */ | ||
| snapshotOnRemoval?: boolean; | ||
| /** | ||
| * Use it to restrict the distance from the edges of screen in which the gesture should be recognized. To be used alongside `fullScreenSwipeEnabled`. | ||
| * | ||
|
|
||
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.
Implementation opinion anchor — This screen-level opt-in is the right boundary for the removal snapshot. I would preserve this design while fixing the edge-swipe lifecycle seams described in the review body.