fix: screenshot from multi display android (like Pro Fold 9) #113
Conversation
WalkthroughEnhanced screenshot capture with multi-display support. Added display detection utilities to query SurfaceFlinger for display count and identify the first active display. Modified Changes
Sequence DiagramsequenceDiagram
participant Caller
participant TakeScreenshot
participant getDisplayCount
participant SurfaceFlinger as SurfaceFlinger (adb)
participant getFirstDisplayId
participant Parser
participant captureScreenshot
Caller->>TakeScreenshot: Take screenshot
TakeScreenshot->>getDisplayCount: Query display count
getDisplayCount->>SurfaceFlinger: adb shell getprop | grep display
SurfaceFlinger-->>getDisplayCount: Display info
getDisplayCount-->>TakeScreenshot: Count (1 or more)
alt Single Display
TakeScreenshot->>captureScreenshot: capture(empty)
captureScreenshot->>SurfaceFlinger: adb shell screencap
SurfaceFlinger-->>captureScreenshot: Screenshot data
else Multiple Displays
TakeScreenshot->>getFirstDisplayId: Get first active display
getFirstDisplayId->>SurfaceFlinger: cmd display get-displays / dumpsys display
SurfaceFlinger-->>getFirstDisplayId: Display output
getFirstDisplayId->>Parser: Parse display ID
Parser-->>getFirstDisplayId: Display ID
getFirstDisplayId-->>TakeScreenshot: Display ID (or empty)
TakeScreenshot->>captureScreenshot: capture(displayID)
captureScreenshot->>SurfaceFlinger: adb shell screencap -d displayID
SurfaceFlinger-->>captureScreenshot: Screenshot data
end
captureScreenshot-->>TakeScreenshot: []byte or error
TakeScreenshot-->>Caller: Screenshot result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
devices/android.go (1)
208-216: Viewport parser is precise and appropriately scopedThe
DisplayViewport{type=INTERNAL ... isActive=true ... uniqueId='...'}regex is nicely constrained to the internal active viewport, and trimming thelocal:prefix matches how IDs are used later. If this becomes hot, consider hoisting the compiled regexp to a package‑level var, but that’s not required now.devices/android_display_test.go (2)
5-38: Align cmd-display test fixtures with actual adb outputThe tests cover single, multi, and invalid cases nicely, but the
outputstrings startDisplay id 0: ...at column 0, whereas realadb shell cmd display get-displaysoutput is typically indented (" Display id 0: ...") before the “Display id” token.Once
parseDisplayIdFromCmdDisplayis updated toTrimSpaceeach line, consider also updating at least one test case to include the leading spaces so the tests reflect real-world output and guard against regressions.
40-72: Viewport parser tests give good coverage of expected scenariosThese cases (single active, two viewports with first active, invalid input) match the regex’s intent and should catch most regressions in
parseDisplayIdFromDumpsysViewport. If you later need to support external displays, you could add a case withtype=EXTERNALas a negative test to ensure INTERNAL-only behavior remains intentional, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devices/android.go(1 hunks)devices/android_display_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (6)
devices/android.go (5)
172-188: Display count heuristic is reasonable and fails safeUsing
dumpsys SurfaceFlinger --display-idand falling back to1on error is a pragmatic choice; worst case you behave like a single‑display device and rely onscreencap’s default. This looks good as-is.
218-226: State parser is a reasonable heuristic for legacy dumpsys formatsUsing
Display Id=(\d+)[\s\S]*?Display State=ONto derive the first ON display ID is a pragmatic last‑resort fallback whenuniqueIdisn’t present. It won’t be perfect if dumpsys formats change, but given it’s only used when higher‑fidelity parsers fail, this trade‑off seems acceptable.
228-253:getFirstDisplayIdfallback chain is solid, but relies on screencap’s-dsemanticsThe order (cmd display → dumpsys viewport → dumpsys state) is sensible and gives you multiple chances to find an active internal display before returning
"".One thing to double‑check on your target devices/Android versions is that the IDs you extract here (trimmed
uniqueIdvalues or numeric “Display Id=…”) are always valid arguments toscreencap -d, as some docs/examples derive IDs directly fromdumpsys SurfaceFlinger --display-id / --displays. Given the diversity of multi‑display setups (physical vs virtual), it’s worth validating this empirically on the devices you care about.
255-265:captureScreenshotcomposition and error handling look correctBuilding the adb args via
exec-out screencap -pand conditionally appending-d <displayID>is correct, and wrapping failures asfmt.Errorf("failed to take screenshot: %w", err)gives callers useful context. The use ofrunAdbCommandkeeps the-s <device>handling consistent with the rest of the code.
268-285: Multi-display screenshot behavior is well-fallbacked
TakeScreenshotnow:
- Treats
displayCount <= 1as legacy/single‑display and uses the defaultscreencap.- For multi‑display, tries to locate the first active display via
getFirstDisplayId, falling back to defaultscreencapif parsing fails.This gives foldables and multi‑display emulators better behavior without regressing older devices, and the “hope for the best” default path is a sensible last resort.
devices/android_display_test.go (1)
74-110: State parser tests are concise and sufficientThe tests exercise the core contract of
parseDisplayIdFromDumpsysState: pick the first ON display ID and return""for garbage input. Given this function is already a fallback, this level of coverage is appropriate.
| // look for lines like "Display id X, ... state ON, ... uniqueId "..." | ||
| if strings.HasPrefix(line, "Display id ") && | ||
| strings.Contains(line, ", state ON,") && | ||
| strings.Contains(line, ", uniqueId ") { | ||
| re := regexp.MustCompile(`uniqueId "([^"]+)"`) | ||
| matches := re.FindStringSubmatch(line) | ||
| if len(matches) == 2 { | ||
| return strings.TrimPrefix(matches[1], "local:") | ||
| } | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // parseDisplayIdFromDumpsysViewport extracts display ID from dumpsys DisplayViewport entries | ||
| func parseDisplayIdFromDumpsysViewport(dumpsys string) string { | ||
| re := regexp.MustCompile(`DisplayViewport\{type=INTERNAL[^}]*isActive=true[^}]*uniqueId='([^']+)'`) |
There was a problem hiding this comment.
parseDisplayIdFromCmdDisplay likely misses real output due to indentation
cmd display get-displays output is typically indented (" Display id 0: ...") rather than starting at column 0, so strings.HasPrefix(line, "Display id ") will not match and this parser will always return "", forcing the dumpsys fallback.
Suggest trimming leading/trailing whitespace before the prefix check, and updating tests to reflect the actual output shape:
func parseDisplayIdFromCmdDisplay(output string) string {
lines := strings.Split(output, "\n")
for _, line := range lines {
+ line = strings.TrimSpace(line)
// look for lines like "Display id X, ... state ON, ... uniqueId "..."
if strings.HasPrefix(line, "Display id ") &&
strings.Contains(line, ", state ON,") &&
strings.Contains(line, ", uniqueId ") {
re := regexp.MustCompile(`uniqueId "([^"]+)"`)You may also want a test case whose output has indented Display id lines so this behavior is locked in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // parseDisplayIdFromCmdDisplay extracts display ID from "cmd display get-displays" output (Android 11+) | |
| func parseDisplayIdFromCmdDisplay(output string) string { | |
| lines := strings.Split(output, "\n") | |
| for _, line := range lines { | |
| // look for lines like "Display id X, ... state ON, ... uniqueId "..." | |
| if strings.HasPrefix(line, "Display id ") && | |
| strings.Contains(line, ", state ON,") && | |
| strings.Contains(line, ", uniqueId ") { | |
| re := regexp.MustCompile(`uniqueId "([^"]+)"`) | |
| matches := re.FindStringSubmatch(line) | |
| if len(matches) == 2 { | |
| return strings.TrimPrefix(matches[1], "local:") | |
| } | |
| } | |
| } | |
| return "" | |
| } | |
| // parseDisplayIdFromCmdDisplay extracts display ID from "cmd display get-displays" output (Android 11+) | |
| func parseDisplayIdFromCmdDisplay(output string) string { | |
| lines := strings.Split(output, "\n") | |
| for _, line := range lines { | |
| line = strings.TrimSpace(line) | |
| // look for lines like "Display id X, ... state ON, ... uniqueId "..." | |
| if strings.HasPrefix(line, "Display id ") && | |
| strings.Contains(line, ", state ON,") && | |
| strings.Contains(line, ", uniqueId ") { | |
| re := regexp.MustCompile(`uniqueId "([^"]+)"`) | |
| matches := re.FindStringSubmatch(line) | |
| if len(matches) == 2 { | |
| return strings.TrimPrefix(matches[1], "local:") | |
| } | |
| } | |
| } | |
| return "" | |
| } |
🤖 Prompt for AI Agents
In devices/android.go around lines 190 to 206, the parser ignores indented lines
because it uses strings.HasPrefix(line, "Display id ") on raw lines; trim
leading/trailing whitespace on each line (e.g., call strings.TrimSpace or
TrimLeft) before doing the HasPrefix and subsequent Contains checks so indented
" Display id ..." matches, keep the regex logic the same, and add/update unit
tests to include an indented Display id example to ensure the behavior is locked
in.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.