Skip to content

Fix removal re-entry, drag alpha and touch listener ownership - #71

Merged
75py merged 3 commits into
release/3.0.0from
fix/3.0.0/core-lifecycle-review
Nov 1, 2026
Merged

75py merged 3 commits into
release/3.0.0from
fix/3.0.0/core-lifecycle-review

Conversation

@75py

@75py 75py commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Problem

The review of #66 found lifecycle and drag defects in core, and some stale documentation. This PR addresses items 1, 2, A1, A2, B and the design-document part of A4. No public signature or enum value changes.

Changes

  • Removal re-entry (1). removeViewImmediate runs detach callbacks synchronously, so a dispose() from one could remove the view twice, and the outer hide() then overwrote DISPOSED with CONFIGURED. A re-entered dispose() now releases at once without a second removal, and the outer call keeps DISPOSED. A re-entered hide() is a no-op. Forced Activity disposal follows the same path, and an ordinary failed removal still keeps the retry.
  • Drag alpha (2). The drag's translucent feedback now belongs to the window. spec.alpha keeps the caller's value throughout a drag, so leaving DRAGGABLE no longer strands 0.6 and an explicit alpha is no longer overwritten on UP or CANCEL. The feedback also ends when the window is removed, and a rejected restore is reapplied by the next update or gesture.
  • Host touch listener (A1). The library calls setOnTouchListener only to install or remove its own drag listener. DRAGGABLE replacing a host listener is documented in KDoc and the design document.
  • Disposal reason (A2). show and update after Activity destruction report ACTIVITY_DESTROYED, and an ordinary dispose keeps WINDOW_MANAGER_REJECTED. Each OverlayFailure constant has KDoc, and the design state table is updated.
  • Cleanup (B). Remove the dead >= M and >= JELLY_BEAN_MR1 checks and the per-MOVE log, explain the AOSP basis of isNotAttached, and add full license headers to OverlayPermission.kt and OverlayViewManager.kt.
  • Design document (A4). Remove the current-threading claim about DebugOverlayTree, and mark the remaining Timber mentions as historical.

Behavior to review

  • overlay.spec.alpha no longer reports the temporary value during a drag. This is the intended separation; the window alone carries it.
  • If a removal fails after a re-entered dispose() already released the handle, the outer call reports the failure, but the handle stays DISPOSED with no retry. The view has already been detached at that point, and this matches the forced-disposal contract.
  • If a host replaces the library's listener while DRAGGABLE, leaving the mode clears the host's listener too, because a View's listener cannot be read back without reflection.

Tests

Regression tests are in OverlayViewStateMachineTest, ActivityLifecycleCleanupTest and DraggableOnTouchListenerTest. Existing drag tests now assert the window's alpha separately from the spec.

Against the old implementation at b036288, these three classes ran 72 tests with 21 failures (Lifecycle 2, StateMachine 9, Drag 10), which reproduces the findings.

Validation

Claude could not run Gradle or device tests in its sandbox. The Codex coordinator ran everything below on head e344aa5.

ANDROID_HOME=/Users/ai-seb/Library/Android/sdk ./gradlew :core:assembleDebug :sample:assembleDebug :core:testDebugUnitTest :lint:test :sample:testDebugUnitTest :core:lintDebug :sample:lintDebug :core:assembleRelease :core:publishToMavenLocal :sample:assembleDebugAndroidTest -Dmaven.repo.local=/private/tmp/ovm-review-core-maven-20261101
ANDROID_HOME=/Users/ai-seb/Library/Android/sdk ./gradlew -p /private/tmp/ovm-review-core-consumer-20261101 :app:assembleRelease :app:lintRelease --refresh-dependencies
ANDROID_HOME=/Users/ai-seb/Library/Android/sdk ANDROID_SERIAL=emulator-5580 ./gradlew :sample:connectedDebugAndroidTest
Check Result
Build tasks, Release and test APK Passed
Unit tests 161 passed (core 145, lint 12, sample 4), none skipped or failed
Lint core 0 errors, 6 warnings; sample 0 errors, 55 warnings
Publishing Passed
Separate Java/Kotlin consumer with R8 Release and lint passed, 45 tasks
Public API javap found no signature change in 15 Java-callable public or protected classes, excluding the new internal synthetic helpers
API 36 emulator sample suite 31 of 31 passed including 2 freeform, none skipped or failed, 4m4s

Not verified:

  • The direct re-entry and drag alpha cases are reproduced in Robolectric only, not on a device.
  • This head was not run on physical devices or on API 23, 26, 31 or 35.
  • The JitPack service build remains a post-tag check.

This PR does not merge to main, tag or publish, and leaves the README unreleased wording and the CHANGELOG date unchanged.

🤖 Generated with Claude Code

75py and others added 3 commits November 1, 2026 17:48
Cover removal re-entry, transient drag alpha, host touch listeners and classification after Activity destruction. The transient alpha tests expect the spec to keep the caller's alpha during a drag, so these fail against the current implementation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dispose from a detach callback now releases once without a second removal, and the outer call keeps DISPOSED. The drag's translucent feedback lives on the window, so spec.alpha keeps the caller's value and leaving DRAGGABLE ends it.

The library now touches the View's listener only for its own drag listener, and show or update after Activity destruction reports ACTIVITY_DESTROYED. Also removes dead SDK checks and the per-move log, and documents these contracts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@75py

75py commented Nov 1, 2026

Copy link
Copy Markdown
Owner Author

APPROVE — Codex coordinator — PR #71 head e344aa5c05420b8c75be6a84b332090812c09ac4, base release/3.0.0.

Reviewed the implementation and regression tests against the user's findings. The removal guard prevents duplicate removals and terminal-state resurrection, preserves ordinary removal retries, and documents the reentrant-disposal exception. Drag feedback is separate from caller configuration, including mode changes, explicit alpha updates and failed restoration. Non-draggable operations retain host touch listeners, and Activity-destruction disposal has its own failure classification. The requested cleanup and design updates are included.

Review corrections are resolved: NOT_ATTACHED documentation covers disposal, equal updates can retry failed feedback restoration, and tests assert window alpha separately from spec alpha. The documented listener ownership restriction while DRAGGABLE remains explicit: replacing the library listener directly during that mode is not a supported way to preserve a host listener on teardown.

Codex validation on this exact head:

  • Baseline reproduction: test-only b036288 against old product code failed 21 of 72 targeted tests. The initial a231952 test set failed 18 of 70.
  • Required Debug builds, all unit tests, lint, :core:assembleRelease, :core:publishToMavenLocal and test APK build passed (176 tasks).
  • 161 unit tests passed: core 145, lint 12, sample 4; zero failures/errors/skips.
  • Lint: zero errors; core 6 warnings, sample 55 warnings.
  • Independent Java/Kotlin consumer with Release/R8 and lint passed (45 tasks).
  • API36 sample suite: 31/31 passed, zero failures/errors/skips, including both freeform cases (4m4s). Dedicated AVD settings restored and emulator stopped.
  • javap comparison of 15 public API classes found no changes to Java-callable public/protected signatures after excluding synthetic members; enum values are unchanged.
  • git diff --check passed. CI 41290698222 succeeded on this head.

The new callback/mode-switch reproductions are Robolectric tests, not physical-device reproductions. API23/26/31/35 and physical devices were not rerun in this pass. JitPack service verification remains post-tag. No blocking findings. This approval permits the task PR's integration into release/3.0.0 under the agreed gates; it does not authorize merging #66 to main, tagging or publication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant