-
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 6 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,8 @@ 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 | ||
|
|
||
| @SuppressLint("ViewConstructor") // Only we construct this view, it is never inflated. | ||
| class Screen( | ||
|
|
@@ -459,12 +470,158 @@ 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 removalSnapshotDrawable: BitmapDrawable? = null | ||
|
|
||
| // One capture attempt per removal: a snapshot is only valid if taken before the removal | ||
| // transaction starts animating. Whether the attempt fails outright or merely completes | ||
| // asynchronously (the bounded wait elapsing does not fail it — the same PixelCopy stays | ||
| // live and may install from its posted continuation), later entry points must not issue | ||
| // another capture: a retry could photograph an already-translated, mid-exit window. | ||
| // | ||
| // Attempt state is owned by the MAIN thread and claimed only at actual capture time, so the | ||
|
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 — Make the one-attempt comment match when the attempt is consumed. The code sets removalSnapshotAttempted before attachment, size, activity, and window prerequisites, while this comment says it is claimed only at actual capture time. Document that entering the main-thread routine consumes the attempt and a prerequisite failure is final, or move the assignment after prerequisites if retry is intended. |
||
| // synchronous pre-transaction call in ScreenStack.onUpdate() always wins over queued off-main | ||
| // requests: a queued request that runs after it no-ops on the claimed attempt. | ||
| 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. |
||
|
|
||
| // Incremented (on main) whenever the snapshot lifecycle resets. An off-main request carries | ||
| // the generation it was posted under and no-ops if its removal has already been cleaned up, | ||
| // so a delayed post can never install an overlay into a later lifecycle. | ||
| @Volatile | ||
| private var removalSnapshotGeneration: Int = 0 | ||
|
|
||
| fun captureScreenSnapshotForRemoval() { | ||
| if (!snapshotOnRemoval) return | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return | ||
| if (Looper.myLooper() == Looper.getMainLooper()) { | ||
| captureScreenSnapshotOnMain() | ||
| } else { | ||
| val requestGeneration = removalSnapshotGeneration | ||
| post { | ||
| if (requestGeneration == removalSnapshotGeneration) { | ||
| 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 (removalSnapshotAttempted) return | ||
| removalSnapshotAttempted = true | ||
| 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 | ||
| } | ||
|
|
||
| // The copy is requested before the removal transaction is created, so it reads the | ||
| // pre-transition presented frame. The main thread waits briefly for it (measured | ||
| // completion is 6-16ms, so the typical install is synchronous — which matters: the | ||
| // overlay must be up before the next frame can present the app's own teardown blank, | ||
| // or one black frame slips through). If the copy is slower, the main thread moves on | ||
| // and the completion posts the install instead: an in-flight successful copy is never | ||
| // abandoned merely because the synchronous wait elapsed. (The posted continuation still | ||
| // drops the bitmap on copy failure, generation change, or detach.) | ||
| // | ||
| // The install/drop decision runs exactly once, always on the main thread (`handled` is | ||
| // only touched there): the synchronous path claims it when the copy beats the bounded | ||
| // wait; otherwise the posted continuation claims it. Install only if this removal is | ||
| // still live (generation) and the screen attached; otherwise the bitmap is dropped. | ||
| val requestGeneration = removalSnapshotGeneration | ||
| val handled = booleanArrayOf(false) | ||
| val latch = CountDownLatch(1) | ||
| val pixelCopyResult = intArrayOf(PixelCopy.ERROR_UNKNOWN) | ||
| try { | ||
| PixelCopy.request(window, srcRect, bitmap, { copyResult -> | ||
| pixelCopyResult[0] = copyResult | ||
| latch.countDown() | ||
| mainHandler.post { | ||
| if (!handled[0]) { | ||
| handled[0] = true | ||
| installOrDropRemovalSnapshot(copyResult, bitmap, requestGeneration) | ||
| } | ||
| } | ||
| }, snapshotHandler) | ||
| } catch (e: IllegalArgumentException) { | ||
| bitmap.recycle() | ||
| return | ||
| } | ||
|
|
||
| val completed = | ||
| try { | ||
| latch.await(SNAPSHOT_SYNC_WAIT_MS, TimeUnit.MILLISECONDS) | ||
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() | ||
| false | ||
| } | ||
| if (completed && !handled[0]) { | ||
| handled[0] = true | ||
| installOrDropRemovalSnapshot(pixelCopyResult[0], bitmap, requestGeneration) | ||
| } | ||
| } | ||
|
|
||
| private fun installOrDropRemovalSnapshot( | ||
| copyResult: Int, | ||
| bitmap: Bitmap, | ||
| requestGeneration: Int, | ||
| ) { | ||
| if (copyResult == PixelCopy.SUCCESS && | ||
| requestGeneration == removalSnapshotGeneration && | ||
| isAttachedToWindow | ||
| ) { | ||
| val drawable = BitmapDrawable(resources, bitmap) | ||
| drawable.setBounds(0, 0, width, height) | ||
| removalSnapshotDrawable = drawable | ||
| overlay.add(drawable) | ||
| invalidate() | ||
| } else { | ||
| bitmap.recycle() | ||
| } | ||
| } | ||
|
|
||
| fun releaseRemovalSnapshot() { | ||
| removalSnapshotGeneration++ | ||
| removalSnapshotDrawable?.let { overlay.remove(it) } | ||
| removalSnapshotDrawable = null | ||
| removalSnapshotAttempted = false | ||
| } | ||
| // --- end screen-level removal snapshot ---------------------------------------------------- | ||
|
|
||
| fun endRemovalTransition() { | ||
| releaseRemovalSnapshot() | ||
| if (!isBeingRemoved) { | ||
| return | ||
| } | ||
|
|
@@ -563,6 +720,11 @@ class Screen( | |
| } | ||
| } | ||
|
|
||
| override fun onDetachedFromWindow() { | ||
| releaseRemovalSnapshot() | ||
| super.onDetachedFromWindow() | ||
| } | ||
|
|
||
| private fun dispatchSheetDetentChanged( | ||
| detentIndex: Int, | ||
| isStable: Boolean, | ||
|
|
@@ -650,6 +812,19 @@ 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 | ||
|
|
||
| // Bounded synchronous wait for the PixelCopy before handing the install to the async | ||
| // continuation. ~One frame at 60Hz (more than one at 90/120Hz refresh rates). | ||
| private const val SNAPSHOT_SYNC_WAIT_MS = 17L | ||
|
|
||
| // Shared thread PixelCopy callbacks land on; one idle thread for the process lifetime. | ||
| private val snapshotHandler: Handler by lazy { | ||
| Handler(HandlerThread("RNSScreenSnapshot").also { it.start() }.looper) | ||
| } | ||
|
|
||
| // Main-thread continuation for snapshot installs. A plain handler (not View.post) so the | ||
| // decision always runs even if the view detaches while the copy is in flight. | ||
| private val mainHandler: Handler by lazy { Handler(Looper.getMainLooper()) } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| 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.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.