Skin designer: safe-area insets are points, and a notch lives in the display - #5626
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a00a7f653
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a5d93872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
shai-almog
force-pushed
the
fix/skin-designer-safe-area-units
branch
from
August 29, 2026 12:20
b9a5d93 to
d4204f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4204f0978
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47cfd5c2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c109ef90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 955e310615
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…display Reported in discussion #5580 -- a skin built for an iPhone 13 Pro Max does not reserve the safe area the hardware does. Two independent causes. Wrong unit. The device catalog stores safeTop/safeBottom in the units the vendors publish -- iOS points, Android dp: 47/34 for a notch, 59/34 for a Dynamic Island, 24/24 for a tablet, 20/0 otherwise. buildProperties scaled them by resolutionW / 320, the designer's drawing viewbox, which has nothing to do with the device's density and only coincides with it on a 320pt-wide phone. Every device was wrong: the 13 Pro Max reserved 189px where the hardware reserves 141, an iPad Pro 12.9 reserved 154px where the hardware reserves 48. The catalog now carries a `scale` per device (devices.json version 3, written by density_scale() in the generator), and the two conversions use it. The Info tab's fields say "pt" instead of "px" to match, and the max against the in-screen cutout extent -- which really is in viewbox units -- now happens after both sides are in pixels rather than between two different units. Notch placement. Notches were rendered into a frame extension ABOVE the screen rect, which made the skin taller than the device, pushed displayY down by the notch height, and left the status bar band conspicuously empty -- and, because nothing covered the screen, notches were excluded from the safe-area top. On an iPhone X through 13 the notch is inside the display: the panel is 1284x2778 and the notch eats into the top of it. It is now painted on the screen rect like every other cutout, anchored to the top edge with a rounded bottom, and counted in the safe-area top. This is also what the wizard's own live preview always drew, so the editor and the exported skin finally agree. Also clears roundedSkin / safeAreaPortrait / safeAreaLandscape at the top of JavaSEPort's skin load. They were only ever assigned on the branches that had a value for them, so loading a legacy skin after a skin designer one kept the previous device's safe area in force and kept painting the skin over the UI. Verified against the real private raster and property code for four devices: displayX/Y equal the bezel with no extension, display size equals the device resolution, the safe insets equal the vendor values times the device scale, a cutout is painted inside the screen rect exactly when the device has one, and no cutout material reaches below the safe area. For the reported device safePortraitY is now 141 (47pt x 3) instead of 189, with the notch bottom at row 119, inside the reserved band. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing as iOS Review feedback on the density scale, plus a catalog bug the same check uncovered, plus the copyright headers the CI gate wants on the three skin designer files this branch touches. Downsampled panels. Rounding ppi/163 to an integer is right for every Apple panel except the iPhone 6/6s/7/8 Plus, which lays out a 414pt-wide surface, renders it at 3x into a 1242x2208 buffer and downsamples that onto the 1080x1920 panel. Its scale is 1080/414, neither 2 nor 3, and the integral rule turned the 20pt status-bar inset into 40px where the panel wants about 52. DOWNSAMPLED_IOS_PANELS records the logical width, keyed on the panel geometry because that is the physical fact identifying it rather than a model name, and check_density_scales() now refuses to write a catalog in which any iOS record's implied point density leaves the band Apple actually ships (120-170 ppi per point). Emptying the table makes the check report exactly the six Plus records, so it is not vacuous; a future downsampled panel fails the generator instead of silently producing a wrong safe area. Platform detection. `"ios" in os_str` also matches Tecno's HIOS and Meizu's Flyme AIOS, so 113 Android phones were on the iOS path: iOS theme, SF Pro, iOS override names, iOS notch/home-indicator inference, and now an integral iOS density scale. Detection matches on a word boundary and lets an explicit "Android" win, since a Tecno record names both. The catalog is migrated in place for exactly those 113 records and nothing is dropped -- it carries entries from scrapes whose platform rules predate the current one, and re-running today's normalise() over all of them would have deleted 143 real devices. The fields that follow from the platform move into derive_platform_fields() so the migration re-derives them through the generator's own rules instead of a second implementation. iPhone 8 Plus now reports safePortraitY=52 against a 2.6087 scale, and the four devices checked before are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…as dp Two review findings on the notch change. Landscape safe area. skin_l.png is skin.png through Image.rotate90Degrees, which maps source (x, y) to (height - 1 - y, x) -- the portrait TOP row becomes the landscape RIGHT column, so the notch is drawn on the right. buildProperties wrote safeTopPx as safeLandscapeX, reserving the LEFT edge, which was harmless while the notch lived in a frame extension outside the display and is not harmless now that it is painted on the screen: app content rendered underneath the opaque shape on the right while an unused band was reserved on the left. snapToSafeAreaInternal derives the right margin as surfaceWidth - width - x, so writing the BOTTOM inset into X leaves exactly safeTopPx on the right, where the notch is. The reasoning, including the pixel mapping, is now a comment at the site so the mapping cannot be re-derived backwards. Unit label. The Info tab's safe-area fields said "pt" for every device. The catalog and the conversion are in points on iOS and dp on Android, so an Android skin told the editor the wrong unit and invited converting a documented dp inset as though it were typographic points. The label follows device.platformName. Verified on the rotated raster rather than asserted: for an iPhone 13 Pro Max the landscape insets are left=102 (34pt bottom) and right=141 (47pt top), all 86132 notch pixels land on the right, none on the left, and none falls outside an inset. Restoring the old mapping makes the check report notch pixels at x=2673 outside both insets, so it is not vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Painting the notch on the display, which is where it sits on the hardware, also put it on the simulator's input surface. JavaSEPort's round-screen path accepts every coordinate inside the display rect, so a click on the opaque notch reached whatever app content was hidden under it. Before this branch the notch lived in a frame extension outside the display and the click fell out of bounds, so this is a regression this branch introduced and not pre-existing behaviour. The skin file had no way to say "these pixels are not display", so the designer now writes a `cutouts` property: display-relative x,y,w,h rectangles, one per cutout. JavaSEPort parses it and drops a pointer press inside one, along with the drag and release that follow -- both of those deliver on `mouseDown` alone, so suppressing only the press would hand the app a gesture it never saw start. skin_map.png deliberately does NOT stand in for the new property, which is the obvious cheap alternative and is wrong: every shipped round skin (iPhoneX, the watch skins) already carries a map marking only the screen, so hit-testing against it would also reject the screen's rounded CORNERS. Those are opaque skin material but real touch surface on the hardware, so that trade swaps a small fidelity gap for a new one. The same argument rules out testing the skin image's alpha. A skin that declares no cutouts behaves exactly as before, which covers every skin written until now. applyCutouts and buildProperties now share cutoutRectPx() so the painted shape and the declared hit area are the same rectangle by construction. JavaSEPortSkinCutoutTest covers the parse, the malformed-entry fallback, the landscape rotation and the bounds test; breaking the rotation makes it report "expected 2658 but was 0", so it is not vacuous. The generator-side harness additionally confirms, per device, that each declared rectangle contains every painted cutout pixel and none outside it, and lies inside the safe area. Also records in density_scale() why Android dp is converted from the physical panel ppi rather than DisplayMetrics.densityDpi, which was the other review question: the simulator has no densityDpi, it derives pixelMilliRatio from the skin's own ppi, and one dp there is exactly ppi/160 px -- so this is the conversion that keeps the safe area in step with the UI drawn beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build-linux-jdk8 went red on the previous commit. The new test passed; CodenameOneExtensionTest failed two unrelated assertions -- @LargerText read back 1.0 instead of 1.6, @orientation(PORTRAIT) read back false. JavaSEPort's constructor assigns the static `instance`, and the test built one to reach an instance method. CodenameOneExtension applies @LargerText and @orientation through JavaSEPort.instance while the assertions read them back through Display, which still holds the original implementation, so the extension configured the orphaned port and Display reported its defaults. isPointerOnCutout takes its rectangle list explicitly and is static, with a one-line instance overload for the pointer path, so the test never needs a port. The reason is a comment on the method, because "make this static" is exactly the kind of thing a later refactor undoes. Reproduced locally in one JVM in CI's order -- a Display-initialising test, then a class that constructs a throwaway port, then CodenameOneExtensionTest -- which fails those same two assertions; the committed test in that slot gives 21/21. The earlier local run missed it because Display had not been initialised yet when the throwaway port was built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more review findings. The documented landscape example was reversed. skin_l.png is skin.png rotated clockwise, so the portrait top inset lands on the landscape RIGHT edge and safeLandscapeX carries the BOTTOM inset. The snippet still showed 185 there from before that fix, which with width 2335 hands the island side a 102px margin and the other side 185 -- the wrong way round, and the sample is what readers copy. It is 102 now, with a line saying why. A hole escaped its own rectangle. applyCutouts drew a circle of diameter cw centred in the box, ignoring c.h, so any hole a user made non-square painted opaque pixels outside the rectangle written into the cutouts property -- and that rectangle is what the simulator hit-tests, so those pixels stayed clickable. It is an ellipse filling the box now, which is also what DevicePreview has always drawn (g.fillArc with the cutout's w and h), so the preview and the exported skin agree. The safe-area floor now measures the same rectangles, rather than recomputing the extent from viewbox units: round(y) + round(h) and round(y + h) differ by a pixel often enough, and the difference would put a cutout edge one pixel below the reserved band. Hit testing stays rectangular, and there is a comment at the property saying why: safeTopPx is floored at the lowest cutout edge, so every declared rectangle lies inside the reserved band where app content does not render, and the corners of a box that a pill or ellipse does not fill cost nothing. Carrying the shape instead would put a second copy of the rasteriser's geometry in JavaSEPort, free to drift from the code that paints the pixels, which is what cutoutRectPx exists to prevent. What has to hold is that the paint never leaves the box, and now it does not. Checked with three deliberately non-square, off-centre cutouts across five devices: every painted pixel falls inside some declared rectangle, every rectangle has paint in it, and every rectangle sits inside the safe area. Restoring the circle makes that report 10918 escaped pixels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more review findings. The persisted wizard state survives a reload, and SkinModel.load() read safeTop/safeBottom back verbatim. Those numbers were entered as preview viewbox units and are now read as points or dp, so a session left by the older build silently regenerates a different skin: a hand-tuned 40 on a 1080-wide 400ppi Android device meant 135px then and would mean 100px now. The persisted model carries a schema version, and a session below the current one takes its insets fresh from the device catalog. There is nothing to convert back to -- a value tuned by eye was tuned against the scaling this branch fixes -- and an untouched session got the same numbers from the catalog anyway. A session with no device selected goes back to the device step, where resetForDevice refills both values on the way through. The migration is written back once rather than repeated on every reload. The documented property example omitted the `cutouts` key the wizard now writes. A skin rebuilt from that example would paint the opaque island and give JavaSEPort.parseCutouts() nothing, leaving pointer events free to reach the content underneath -- exactly the bug the key exists to close. The example carries the real value for its device (377,53,452,132, checked against the generator), and the guide's property list and cutouts section mention it. Also drops this branch's own fix for the signup-funnel test: master fixed it in #5628 with an attribute parser rather than a looser regex, which is the better of the two, so the rebase takes master's version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix/skin-designer-safe-area-units
branch
from
August 29, 2026 17:44
955e310 to
896ea3a
Compare
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
1 participant
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.
Fixes the skin designer half of discussion #5580 — a skin built for an iPhone 13 Pro Max doesn't reserve the safe area the hardware does. Two independent causes.
The insets were scaled by the wrong number
The device catalog stores
safeTop/safeBottomin the units Apple and Google publish — iOS points, Android dp.build_devices_json.pywrites the canonical values: 47/34 for a notch, 59/34 for a Dynamic Island, 24/24 for a tablet, 20/0 otherwise.buildPropertiesconverted them withresolutionW / DevicePreview.VB_W. That 320-wide figure is the designer's drawing viewbox; it has nothing to do with the device's density and only coincides with it on a 320pt-wide phone, which is why nobody caught it. The error ispointWidth / 320, so every device was wrong:The catalog now carries a
scaleper device —devices.jsonmoves to version 3, written by a newdensity_scale()in the generator (iOS roundsppi/163because Apple only ships integral @2x/@3x panels; Android keepsppi/160continuous).DeviceDatabaseexposes it asdensityScale, with a fallback that mirrors the same rule for a pre-v3 catalog.The
Math.maxagainst the in-screen cutout extent was mixing units too — comparing points against viewbox units. Both sides are converted to pixels first now.The Info tab's fields say
ptinstead ofpxto match, and the developer guide's "in viewbox pixels" claim is corrected.A notch belongs inside the display
Notches were rendered into a frame extension above the screen rect. That made the skin taller than the device, pushed
displayYdown by the notch height, and left the status-bar band conspicuously empty — and, since nothing covered the screen, notches were deliberately excluded from the safe-area top.On an iPhone X through 13 the notch is inside the display: the panel is 1284×2778 and the notch eats into the top of it. It's now painted on the screen rect like every other cutout — anchored to the top edge, square at the top, rounded at the bottom — and counted in the safe-area top.
applyTopFrameCutoutsandapplyInScreenCutoutscollapse into oneapplyCutouts;computeTopCutoutPxis gone.This is also what the wizard's own live preview has always drawn, so the editor and the exported skin finally agree, and the existing editor screenshots in the guide stay accurate.
Simulator-side state leak
JavaSEPortnever clearedroundedSkin,safeAreaPortraitorsafeAreaLandscape— they were only ever assigned on the branches that had a value for them. Loading a legacy skin after a skin designer one kept the previous device's safe area in force and kept painting the skin over the UI. They're cleared at the top of the skin load now.Verification
Drove the real private raster and property code through reflection for four devices (13 Pro Max / 16 Pro / SE 2022 / iPad Pro 12.9) and asserted:
displayXanddisplayYequal the bezel — no frame extension above the screenAll pass. For the reported device
safePortraitYis 141 with the notch bottom at row 119, inside the reserved band.Also:
scripts/skindesigner/commoncompiles clean;JavaSEPortcompiles clean on JDK 8; Vale reports 0/0/0 atsuggestionand LanguageToolstatus: ok, total: 0on the changed guide chapter (dpadded to the accept list).Not changed
The default notch is 180 of 320 viewbox units — calibrated for an iPhone X, where the notch really is 56% of the screen width. It's too wide for a 13 Pro Max (38%), but it's a user-draggable default and purely cosmetic now that the safe area is computed from the catalog rather than from the drawn shape.
🤖 Generated with Claude Code