ADFA-3218: use correct window context for debug overlay#1293
Open
itsaky-adfa wants to merge 2 commits into
Open
ADFA-3218: use correct window context for debug overlay#1293itsaky-adfa wants to merge 2 commits into
itsaky-adfa wants to merge 2 commits into
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
📝 WalkthroughWalkthroughThis PR enables the debugger overlay to follow the application across multiple displays. DebugOverlayManager now accepts a display ID and creates a themed context for that specific display. DebuggerService dynamically creates overlay managers on-demand and adds a public method to relocate overlays. BaseEditorActivity starts the service with the current display ID and triggers relocation on resume. UI Designer result handling is refactored for explicit error flow and logging. ChangesDisplay-aware debugger overlay management
Sequence Diagram(s)sequenceDiagram
participant Activity as BaseEditorActivity
participant Service as DebuggerService
participant Manager as DebugOverlayManager
Activity->>Service: startService(intent + displayId)
Service->>Service: onBind(intent)
Service->>Service: createOverlayManagerIfNeeded(displayId)
Service->>Manager: create(ctx, displayId)
Manager-->>Service: overlay manager
Service-->>Activity: service binder
Activity->>Service: maybeMoveOverlayToDisplay(newDisplayId)
Service->>Service: hide current overlay
Service->>Service: createOverlayManagerIfNeeded(newDisplayId)
Service->>Manager: create(ctx, newDisplayId)
Manager-->>Service: new overlay manager
Service->>Service: show overlay on new display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/src/main/java/com/itsaky/androidide/services/debug/DebuggerService.kt (2)
124-130: ⚡ Quick winSimplify overlay relocation logic.
The
maybeMoveOverlayToDisplaymethod hides the overlay before callingcreateOverlayManagerIfNeeded, which also hides the overlay if the displayId changed (lines 134). This results inhideOverlay()being called twice when relocating to a new display.♻️ Simplify to avoid double hide
fun maybeMoveOverlayToDisplay(displayId: Int) { if (overlayManager?.attachedDisplayId == displayId) return - hideOverlay() createOverlayManagerIfNeeded(displayId) showOverlay() }The
createOverlayManagerIfNeededmethod already handles hiding when needed (line 134).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/services/debug/DebuggerService.kt` around lines 124 - 130, The method maybeMoveOverlayToDisplay redundantly calls hideOverlay() before createOverlayManagerIfNeeded, causing hideOverlay to run twice when the display actually changes; remove the initial hideOverlay() call in maybeMoveOverlayToDisplay and rely on createOverlayManagerIfNeeded(displayId) (which already hides the overlay when the displayId differs from overlayManager?.attachedDisplayId) and then call showOverlay() as before so the overlay is only hidden once during relocation.
132-145: ⚡ Quick winRedundant null check in overlay manager recreation.
Lines 133-136 check if the displayId differs and hide/null the manager, but line 138 immediately checks
if (overlayManager == null). The second check is redundant since we just set it to null. Consider restructuring for clarity.♻️ Simplify the logic
private fun createOverlayManagerIfNeeded(displayId: Int) { if (overlayManager?.attachedDisplayId != displayId) { hideOverlay() - overlayManager = null - } - - if (overlayManager == null) { overlayManager = DebugOverlayManager.create( ctx = this, displayId = displayId ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/services/debug/DebuggerService.kt` around lines 132 - 145, The createOverlayManagerIfNeeded function contains a redundant null check: instead of first checking overlayManager?.attachedDisplayId != displayId then setting overlayManager = null and later checking if (overlayManager == null), simplify the flow by checking whether overlayManager exists and has a matching attachedDisplayId; if it doesn't, call hideOverlay(), set overlayManager = null, and then unconditionally create a new manager via DebugOverlayManager.create with ctx = this and displayId; update references to overlayManager, attachedDisplayId, hideOverlay, and DebugOverlayManager.create in the createOverlayManagerIfNeeded method to reflect this simplified logic.app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (1)
899-904: ⚡ Quick winConsider more specific exception handling for overlay relocation.
The
runCatchingblock (line 900) catches all exceptions when moving the debugger overlay. While this prevents crashes, it may mask specific issues likeSecurityExceptionif overlay permissions change, orIllegalStateExceptionif the service is not bound. Consider catching specific exception types to aid debugging.🛡️ More specific exception handling
-runCatching { - debuggerService?.maybeMoveOverlayToDisplay(displayId) -}.onFailure { err -> - log.warn("Unable to move debugger overlay to display {}", displayId, err) -} +try { + debuggerService?.maybeMoveOverlayToDisplay(displayId) +} catch (e: SecurityException) { + log.error("Overlay permission denied when moving to display {}", displayId, e) +} catch (e: IllegalStateException) { + log.warn("Unable to move debugger overlay to display {} (service state issue)", displayId, e) +} catch (e: Exception) { + log.warn("Unable to move debugger overlay to display {}", displayId, e) +}Based on learnings, prefer narrow exception handling that catches specific exception types instead of broad catch-all blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt` around lines 899 - 904, The runCatching that wraps debuggerService?.maybeMoveOverlayToDisplay(displayId) is too broad; replace it with explicit try/catch around the call in BaseEditorActivity (the block using ContextCompat.getDisplayOrDefault and maybeMoveOverlayToDisplay) and handle expected exceptions separately — e.g., catch SecurityException (log permission-specific message and suggestion), IllegalStateException (log service binding/state info), and optionally RemoteException/RuntimeException if the debuggerService is remote — logging the displayId and full error for each; rethrow or propagate only truly unexpected exceptions if appropriate.app/src/main/java/com/itsaky/androidide/services/debug/DebugOverlayManager.kt (1)
173-189: ⚡ Quick winConsider handling display not found more explicitly.
When
displayManager.getDisplay(displayId)returns null (line 182), the code falls back to the base context. While this provides safe fallback behavior, the log message at line 180 claims to be "trying to get window context" but doesn't indicate whether the display was actually found. Consider logging when the display is not found to aid debugging.📝 Suggested logging improvement
val displayManager = DisplayManagerCompat.getInstance(ctx) val targetDisplay = displayManager.getDisplay(displayId) +if (targetDisplay == null) { + logger.warn("Display {} not found, falling back to default display", displayId) +} val displayContext = if (targetDisplay != null) ctx.createDisplayContext(targetDisplay) else ctx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/services/debug/DebugOverlayManager.kt` around lines 173 - 189, The create function's windowContext resolution logs an attempt to get a display but never logs whether DisplayManagerCompat.getInstance(ctx).getDisplay(displayId) actually returned a display; update the logic around targetDisplay/getDisplay to emit an explicit logger.debug or logger.warn when targetDisplay is null (e.g., "display not found for id={}" with displayId) and when it is found (e.g., "using display id={}"), so developers can see if the fallback to ctx occurred; keep the existing fallback to ctx/createDisplayContext and retain the isAtLeastR() branch for createWindowContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt`:
- Around line 899-904: The runCatching that wraps
debuggerService?.maybeMoveOverlayToDisplay(displayId) is too broad; replace it
with explicit try/catch around the call in BaseEditorActivity (the block using
ContextCompat.getDisplayOrDefault and maybeMoveOverlayToDisplay) and handle
expected exceptions separately — e.g., catch SecurityException (log
permission-specific message and suggestion), IllegalStateException (log service
binding/state info), and optionally RemoteException/RuntimeException if the
debuggerService is remote — logging the displayId and full error for each;
rethrow or propagate only truly unexpected exceptions if appropriate.
In `@app/src/main/java/com/itsaky/androidide/services/debug/DebuggerService.kt`:
- Around line 124-130: The method maybeMoveOverlayToDisplay redundantly calls
hideOverlay() before createOverlayManagerIfNeeded, causing hideOverlay to run
twice when the display actually changes; remove the initial hideOverlay() call
in maybeMoveOverlayToDisplay and rely on createOverlayManagerIfNeeded(displayId)
(which already hides the overlay when the displayId differs from
overlayManager?.attachedDisplayId) and then call showOverlay() as before so the
overlay is only hidden once during relocation.
- Around line 132-145: The createOverlayManagerIfNeeded function contains a
redundant null check: instead of first checking
overlayManager?.attachedDisplayId != displayId then setting overlayManager =
null and later checking if (overlayManager == null), simplify the flow by
checking whether overlayManager exists and has a matching attachedDisplayId; if
it doesn't, call hideOverlay(), set overlayManager = null, and then
unconditionally create a new manager via DebugOverlayManager.create with ctx =
this and displayId; update references to overlayManager, attachedDisplayId,
hideOverlay, and DebugOverlayManager.create in the createOverlayManagerIfNeeded
method to reflect this simplified logic.
In
`@app/src/main/java/com/itsaky/androidide/services/debug/DebugOverlayManager.kt`:
- Around line 173-189: The create function's windowContext resolution logs an
attempt to get a display but never logs whether
DisplayManagerCompat.getInstance(ctx).getDisplay(displayId) actually returned a
display; update the logic around targetDisplay/getDisplay to emit an explicit
logger.debug or logger.warn when targetDisplay is null (e.g., "display not found
for id={}" with displayId) and when it is found (e.g., "using display id={}"),
so developers can see if the fallback to ctx occurred; keep the existing
fallback to ctx/createDisplayContext and retain the isAtLeastR() branch for
createWindowContext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d30844e1-1710-43c9-9bc0-14f776b7e38c
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/services/debug/DebugOverlayManager.ktapp/src/main/java/com/itsaky/androidide/services/debug/DebuggerService.kt
dara-abijo-adfa
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See ADFA-3218 for more details.