mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
172e4d3a64
Part of #609. ## What was wrong On a rotated Android device `screenshot` returned a portrait-framed image with the content lying sideways. `describe` frames and gesture coordinates were **already upright** — uiautomator measures against the rotated display — so the capture was the one surface out of step. An agent reading that image saw geometry that disagreed with every coordinate it was given. Measured on a landscape Pixel_9 (API 36, `mCurrentOrientation=1`): | | space | |---|---| | `describe` | upright — `"Search Settings"` centre `(0.530, 0.234)` | | taps | upright — tapping describe's `"Display & touch"` centre opens it | | `screenshot` (default) | **portrait-native, sideways** | ## The fix simulator-server already applies `rotation` correctly; nothing was telling it which one. The rotation is now queried from the device per capture, so `screenshot`, `screenshot-diff`'s live captures and the auto-attached "Screen after action" screenshot all land in `describe`/tap space. **Nothing is tracked.** A cached orientation could only be staler than the ~40 ms probe, and serving a stale one is exactly the failure being fixed. When the rotation cannot be read the request is left byte-identical to before rather than guessing — a wrong orientation is a confidently wrong image, which is worse than a sideways one. **iOS is deliberately untouched.** There the whole surface is consistently portrait-native, so rotating only the capture would *break* an agreement rather than restore one. The iOS half of #609 needs a different change and is not in this PR. ### Also fixed: the legacy uiautomator describe path Same root cause, worse symptom. `wm size` reports the **unrotated** size — on the landscape Pixel_9 it still answered `Physical size: 1080x2424`. Dividing rotated bounds by that does not merely squash frames: `isVisibleRect` prunes nodes whose left edge is past the screen width, so **the right-hand half of a landscape screen vanished from the tree**. The dump carries `<hierarchy rotation="N">`, so the divisor is oriented from bytes already in hand — no extra round-trip on the path that runs precisely when the helper could not be reached. ## On the rotation-name table Pinned by measurement, never by what the names mean — the reporter of #609 found the same names are 180° inverted on iOS. `adb exec-out screencap` is rotation-aware, so it is ground truth; each candidate was scored as mean absolute difference against it both as-is and rotated 180°, so an inverted mapping could not pass: ``` rotation 1 LandscapeLeft 2.19 (vs 6.54 for its 180° twin) rotation 3 LandscapeRight 2.19 (vs 6.34) rotation 2 PortraitUpsideDown 2.67 (vs 13.13) rotation 0 Portrait identity ``` This table is the **inverse** of simulator-server's own `90° → LandscapeRight` convention, because it compensates for a rotation that layer applies while decoding the scrcpy stream. That makes it a property of a particular simulator-server build rather than of Android — which is why a table test alone is not enough, and the delivered PNG's aspect is checked against the rotation requested, falling back to an unrotated capture if they disagree. ## Verified live Branch tool-server against a real emulator: - landscape, default capture → **2424×1080 upright**, MAD 2.19 vs ground truth (6.54 vs its 180° twin) - cropping `describe`'s frame for the toolbar out of that default capture shows exactly the "Display & touch" toolbar — the two spaces agree - rotation 0 → 1080×2424, MAD 2.82, unchanged - rotated **iPad** → still 2064×2752 sideways, i.e. iOS provably untouched Plus 27 new unit tests, including one that asserts the legacy path *drops* the right-hand node before the fix and keeps it after. Full suite 3125 passing; lint, prettier, `typecheck:tests` and the tool-description gate all clean. ## Not in scope - The iOS half of #609. - `rotate` on Android silently no-ops when auto-rotate is enabled (it returned `{orientation:"LandscapeLeft"}` while `dumpsys` still read `cur=1080x2424`). Filing separately. - `screen-recording` still records sideways — same root cause, different pipeline. Worth noting a session's screenshots are now upright while its video is not. - Existing visual baselines captured on a rotated Android device will stop matching, since the baseline key embeds capture dimensions and those now swap. This fails loudly as a missing baseline rather than silently comparing the wrong thing — and those baselines were sideways captures of an upright UI. --- > **#673 and #658 are both stacked on this branch** — each conflicts with this one (`screenshot/index.ts` and `screenshot-diff/index.ts` respectively) but not with each other, so both were rebased onto this branch as siblings rather than chained. Merge this first; GitHub then retargets both to `main`. --------- Co-authored-by: Filip131311 <f.kaminski2000@gmail.com>
74 lines
3.0 KiB
TypeScript
74 lines
3.0 KiB
TypeScript
import { describe, it, expect, vi } from "vitest";
|
|
import {
|
|
ServiceNotFoundError,
|
|
ServiceState,
|
|
type DeviceInfo,
|
|
type Registry,
|
|
} from "@argent/registry";
|
|
import { androidDevtoolsRotationPeek } from "../src/utils/android-devtools-rotation-peek";
|
|
|
|
const ANDROID: DeviceInfo = { id: "emulator-5554", platform: "android", kind: "emulator" };
|
|
const IOS: DeviceInfo = { id: "BC0026C7-AAE0-490E", platform: "ios", kind: "simulator" };
|
|
|
|
/** A registry that knows one android-devtools service in a given state. */
|
|
function fakeRegistry(
|
|
state: ServiceState | "never-created",
|
|
api?: { isReady?: () => boolean; getScreenSize?: () => Promise<unknown> }
|
|
) {
|
|
const resolveService = vi.fn(async () => ({
|
|
isReady: () => true,
|
|
getScreenSize: async () => ({ width: 2424, height: 1080, rotation: 1 }),
|
|
...api,
|
|
}));
|
|
const getServiceState = vi.fn((urn: string) => {
|
|
if (state === "never-created") throw new ServiceNotFoundError(urn);
|
|
return state;
|
|
});
|
|
return { registry: { resolveService, getServiceState } as unknown as Registry, resolveService };
|
|
}
|
|
|
|
describe("androidDevtoolsRotationPeek", () => {
|
|
it("answers from a running, ready helper", async () => {
|
|
const { registry, resolveService } = fakeRegistry(ServiceState.RUNNING);
|
|
expect(await androidDevtoolsRotationPeek(registry, ANDROID)()).toBe(1);
|
|
expect(resolveService).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("never creates the service when it has not been started", async () => {
|
|
// This is the property that makes the peek safe inside `screenshot`:
|
|
// resolving would install an APK and run `am instrument`.
|
|
const { registry, resolveService } = fakeRegistry("never-created");
|
|
expect(await androidDevtoolsRotationPeek(registry, ANDROID)()).toBeNull();
|
|
expect(resolveService).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it.each([ServiceState.IDLE, ServiceState.STARTING, ServiceState.TERMINATING, ServiceState.ERROR])(
|
|
"does not resolve a service in state %s",
|
|
async (state) => {
|
|
const { registry, resolveService } = fakeRegistry(state);
|
|
expect(await androidDevtoolsRotationPeek(registry, ANDROID)()).toBeNull();
|
|
expect(resolveService).not.toHaveBeenCalled();
|
|
}
|
|
);
|
|
|
|
it("has no answer for a helper that is running but not yet ready", async () => {
|
|
const { registry } = fakeRegistry(ServiceState.RUNNING, { isReady: () => false });
|
|
expect(await androidDevtoolsRotationPeek(registry, ANDROID)()).toBeNull();
|
|
});
|
|
|
|
it("has no answer when the helper RPC fails", async () => {
|
|
const { registry } = fakeRegistry(ServiceState.RUNNING, {
|
|
getScreenSize: async () => {
|
|
throw new Error("socket closed");
|
|
},
|
|
});
|
|
expect(await androidDevtoolsRotationPeek(registry, ANDROID)()).toBeNull();
|
|
});
|
|
|
|
it("ignores non-Android devices without touching the registry", async () => {
|
|
const { registry, resolveService } = fakeRegistry(ServiceState.RUNNING);
|
|
expect(await androidDevtoolsRotationPeek(registry, IOS)()).toBeNull();
|
|
expect(resolveService).not.toHaveBeenCalled();
|
|
});
|
|
});
|