feat(v3): make DMG icon layout configurable - #5864
Open
leaanthony wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughDMG packaging now supports automatic or manual icon layouts. Manual layouts accept named Finder-window coordinates. The implementation validates layout modes, coordinates, required DMG items, and unknown items. Task configuration, flags, tests, and macOS documentation expose the new settings. ChangesDMG icon layout
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ToolPackage
participant createDmgTask
participant buildDMGOptions
participant DMGTool
ToolPackage->>createDmgTask: DMG layout and position settings
createDmgTask->>buildDMGOptions: Layout arguments
buildDMGOptions->>buildDMGOptions: Validate and parse configuration
buildDMGOptions->>DMGTool: Generated icon options
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
github-actions
Bot
added
Documentation
leaanthony
marked this pull request as ready for review
August 1, 2026 03:43
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes DMG Finder icon placement configurable for wails3 tool package, replacing the previously hard-coded layout with either library-driven auto layout or explicit, validated manual coordinates. It also propagates the new configuration into the Darwin Taskfile template and documentation, with test coverage for the new behavior.
Changes:
- Added
--icon-layout auto|manualand--icon-positionsflags, with parsing/validation for manual positioning. - Updated the Darwin Taskfile template to pass
DMG_ICON_LAYOUT/DMG_ICON_POSITIONSthrough to the packager. - Updated macOS DMG documentation and expanded unit tests around layout defaults and invalid input.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/internal/flags/package.go | Adds new CLI flags for DMG icon layout and manual icon positioning. |
| v3/internal/commands/tool_package.go | Removes hard-coded icon positions and introduces layout selection + parsing/validation helpers. |
| v3/internal/commands/tool_package_test.go | Adds/updates unit tests for auto default behavior, manual layout, and invalid inputs. |
| v3/internal/commands/build_assets/darwin/Taskfile.yml | Wires new DMG layout/position variables into the generated DMG packaging task. |
| docs/src/content/docs/guides/build/macos.mdx | Documents new DMG icon layout modes and related Taskfile variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+155
| for name := range iconPositions { | ||
| if name != "Applications" { | ||
| if _, ok := opts.Files[name]; !ok { | ||
| return fmt.Errorf("DMG icon position references unknown item %q", name) | ||
| } | ||
| } | ||
| } |
Comment on lines
+188
to
+200
| xy := strings.Split(coordinates, ",") | ||
| if len(xy) != 2 { | ||
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | ||
| } | ||
| x, err := strconv.Atoi(strings.TrimSpace(xy[0])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DMG icon X coordinate in %q: %w", item, err) | ||
| } | ||
| y, err := strconv.Atoi(strings.TrimSpace(xy[1])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DMG icon Y coordinate in %q: %w", item, err) | ||
| } | ||
| positions[strings.TrimSpace(name)] = dmg.IconPosition{X: x, Y: y} |
Comment on lines
+125
to
+128
| # auto lets the DMG library distribute every icon. For manual placement, | ||
| # use `manual` and specify every position as name=x,y;name=x,y. | ||
| DMG_ICON_LAYOUT: auto | ||
| DMG_ICON_POSITIONS: "" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@v3/internal/commands/tool_package.go`:
- Around line 173-206: Update parseDMGIconPositions to detect when the trimmed
icon name already exists in positions before assigning it, and return an error
matching the existing duplicate-entry pattern instead of overwriting the prior
position. Preserve the current coordinate validation and successful parsing
behavior for unique names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d6d7924-acff-4151-828a-cd749c4aa985
📒 Files selected for processing (5)
docs/src/content/docs/guides/build/macos.mdxv3/internal/commands/build_assets/darwin/Taskfile.ymlv3/internal/commands/tool_package.gov3/internal/commands/tool_package_test.gov3/internal/flags/package.go
Comment on lines
+173
to
+206
| func parseDMGIconPositions(value string) (map[string]dmg.IconPosition, error) { | ||
| if strings.TrimSpace(value) == "" { | ||
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | ||
| } | ||
|
|
||
| positions := make(map[string]dmg.IconPosition) | ||
| for _, item := range strings.Split(value, ";") { | ||
| item = strings.TrimSpace(item) | ||
| if item == "" { | ||
| continue | ||
| } | ||
| name, coordinates, ok := strings.Cut(item, "=") | ||
| if !ok || strings.TrimSpace(name) == "" { | ||
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | ||
| } | ||
| xy := strings.Split(coordinates, ",") | ||
| if len(xy) != 2 { | ||
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | ||
| } | ||
| x, err := strconv.Atoi(strings.TrimSpace(xy[0])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DMG icon X coordinate in %q: %w", item, err) | ||
| } | ||
| y, err := strconv.Atoi(strings.TrimSpace(xy[1])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DMG icon Y coordinate in %q: %w", item, err) | ||
| } | ||
| positions[strings.TrimSpace(name)] = dmg.IconPosition{X: x, Y: y} | ||
| } | ||
| if len(positions) == 0 { | ||
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | ||
| } | ||
| return positions, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject duplicate names in manual icon positions.
parseDMGIconPositions silently overwrites an earlier entry when the same name appears twice in the ;-separated list. addDMGFiles, in the same file, explicitly rejects a duplicate name with "DMG file %q conflicts with an existing entry". Apply the same fail-loud pattern here so a typo does not silently discard a position.
🛠️ Proposed fix to reject duplicate position names
name, coordinates, ok := strings.Cut(item, "=")
if !ok || strings.TrimSpace(name) == "" {
return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item)
}
+ name = strings.TrimSpace(name)
+ if _, exists := positions[name]; exists {
+ return nil, fmt.Errorf("duplicate DMG icon position for %q", name)
+ }
xy := strings.Split(coordinates, ",")
if len(xy) != 2 {
return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item)
}
x, err := strconv.Atoi(strings.TrimSpace(xy[0]))
if err != nil {
return nil, fmt.Errorf("invalid DMG icon X coordinate in %q: %w", item, err)
}
y, err := strconv.Atoi(strings.TrimSpace(xy[1]))
if err != nil {
return nil, fmt.Errorf("invalid DMG icon Y coordinate in %q: %w", item, err)
}
- positions[strings.TrimSpace(name)] = dmg.IconPosition{X: x, Y: y}
+ positions[name] = dmg.IconPosition{X: x, Y: y}📝 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.
Suggested change
| func parseDMGIconPositions(value string) (map[string]dmg.IconPosition, error) { | |
| if strings.TrimSpace(value) == "" { | |
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | |
| } | |
| positions := make(map[string]dmg.IconPosition) | |
| for _, item := range strings.Split(value, ";") { | |
| item = strings.TrimSpace(item) | |
| if item == "" { | |
| continue | |
| } | |
| name, coordinates, ok := strings.Cut(item, "=") | |
| if !ok || strings.TrimSpace(name) == "" { | |
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | |
| } | |
| xy := strings.Split(coordinates, ",") | |
| if len(xy) != 2 { | |
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | |
| } | |
| x, err := strconv.Atoi(strings.TrimSpace(xy[0])) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid DMG icon X coordinate in %q: %w", item, err) | |
| } | |
| y, err := strconv.Atoi(strings.TrimSpace(xy[1])) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid DMG icon Y coordinate in %q: %w", item, err) | |
| } | |
| positions[strings.TrimSpace(name)] = dmg.IconPosition{X: x, Y: y} | |
| } | |
| if len(positions) == 0 { | |
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | |
| } | |
| return positions, nil | |
| } | |
| func parseDMGIconPositions(value string) (map[string]dmg.IconPosition, error) { | |
| if strings.TrimSpace(value) == "" { | |
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | |
| } | |
| positions := make(map[string]dmg.IconPosition) | |
| for _, item := range strings.Split(value, ";") { | |
| item = strings.TrimSpace(item) | |
| if item == "" { | |
| continue | |
| } | |
| name, coordinates, ok := strings.Cut(item, "=") | |
| if !ok || strings.TrimSpace(name) == "" { | |
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | |
| } | |
| name = strings.TrimSpace(name) | |
| if _, exists := positions[name]; exists { | |
| return nil, fmt.Errorf("duplicate DMG icon position for %q", name) | |
| } | |
| xy := strings.Split(coordinates, ",") | |
| if len(xy) != 2 { | |
| return nil, fmt.Errorf("invalid DMG icon position %q: expected name=x,y", item) | |
| } | |
| x, err := strconv.Atoi(strings.TrimSpace(xy[0])) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid DMG icon X coordinate in %q: %w", item, err) | |
| } | |
| y, err := strconv.Atoi(strings.TrimSpace(xy[1])) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid DMG icon Y coordinate in %q: %w", item, err) | |
| } | |
| positions[name] = dmg.IconPosition{X: x, Y: y} | |
| } | |
| if len(positions) == 0 { | |
| return nil, fmt.Errorf("manual DMG icon layout requires icon positions") | |
| } | |
| return positions, nil | |
| } |
🤖 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 `@v3/internal/commands/tool_package.go` around lines 173 - 206, Update
parseDMGIconPositions to detect when the trimmed icon name already exists in
positions before assigning it, and return an error matching the existing
duplicate-entry pattern instead of overwriting the prior position. Preserve the
current coordinate validation and successful parsing behavior for unique names.
leaanthony
force-pushed
the
feature/dmg-icon-layout
branch
from
August 1, 2026 03:52
29ba819 to
b642b11
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
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.
Summary
Makes Finder icon placement in Wails DMG packages configurable instead of hard-coded.
--icon-layout auto|manualtowails3 tool package.--icon-positions 'name=x,y;name=x,y'for manual Finder-window pixel centres.DMG_ICON_LAYOUTandDMG_ICON_POSITIONSto generated Darwin Taskfiles.Validation
go test ./internal/commands ./internal/flags ./internal/templateswails3 tool package --help.Summary by CodeRabbit
New Features
Documentation