feat: configurable macOS terminal app via EditorPrefs#1117
Conversation
The macOS branch of TerminalLauncher.CreateTerminalProcessStartInfo
hardcoded `open -a Terminal`, which forces Terminal.app to spawn even
when the user prefers another terminal (iTerm2, Warp, etc.). macOS
LaunchServices resolves `-a Terminal` by name, so iTerm's
"Make iTerm2 default term" setting cannot redirect it.
Read the terminal app name from the new `MCPForUnity.MacOSTerminalApp`
EditorPref. Empty/unset falls back to "Terminal" so existing behavior
is preserved for users who never touch the setting.
The key is registered in EditorPrefsWindow's `knownPrefTypes` so it
shows up in the EditorPrefs window as a String entry.
Argument is now quoted (`-a "{app}"`) to handle names with spaces such
as "iTerm 2".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds user configuration for the macOS terminal application used in terminal launching. A new EditorPrefs key is introduced, integrated into the terminal launcher logic with a sensible default, registered in the EditorPrefs UI settings window, and covered by unit tests. ChangesmacOS Terminal App Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR makes the macOS terminal app used to launch the MCP server configurable via Unity EditorPrefs, avoiding a hardcoded dependency on Terminal.app while keeping the existing default behavior when unset.
Changes:
- Added a new
EditorPrefskey (MCPForUnity.MacOSTerminalApp) to choose which macOS terminal appopen -atargets (fallback remains"Terminal"). - Updated
TerminalLauncherto read and use the configured terminal app name (including quoting for names with spaces). - Registered the new key in
EditorPrefsWindowso it appears in the existing EditorPrefs UI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs | Registers the new macOS terminal app EditorPref key as a known string pref. |
| MCPForUnity/Editor/Services/Server/TerminalLauncher.cs | Reads the preferred macOS terminal app from EditorPrefs and uses it in the open -a invocation. |
| MCPForUnity/Editor/Constants/EditorPrefKeys.cs | Adds the MCPForUnity.MacOSTerminalApp EditorPref key constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string terminalApp = EditorPrefs.GetString(EditorPrefKeys.MacOSTerminalApp, "Terminal"); | ||
| if (string.IsNullOrWhiteSpace(terminalApp)) terminalApp = "Terminal"; | ||
| return new System.Diagnostics.ProcessStartInfo | ||
| { | ||
| FileName = "/usr/bin/open", | ||
| Arguments = $"-a Terminal \"{scriptPath}\"", | ||
| Arguments = $"-a \"{terminalApp}\" \"{scriptPath}\"", |
| string terminalApp = EditorPrefs.GetString(EditorPrefKeys.MacOSTerminalApp, "Terminal"); | ||
| if (string.IsNullOrWhiteSpace(terminalApp)) terminalApp = "Terminal"; | ||
| return new System.Diagnostics.ProcessStartInfo | ||
| { | ||
| FileName = "/usr/bin/open", | ||
| Arguments = $"-a Terminal \"{scriptPath}\"", | ||
| Arguments = $"-a \"{terminalApp}\" \"{scriptPath}\"", |
|
Hi thanks for the PR! Late to leave this review but do you think you can fulfill your test plan? Just want to double check before merging this. |
Extract the macOS terminal-app fallback and `open -a` argument building into pure internal helpers (ResolveMacTerminalApp, BuildMacOpenArguments) so the EditorPrefs-driven behavior is unit-testable without a live Editor. Adds EditMode tests covering the test plan logic: unset/empty/whitespace falls back to Terminal, custom apps are used, and quoting handles app names and script paths containing spaces (e.g. "iTerm 2"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the nudge! I've addressed the testability gap from my original notes. Rather than leaving the macOS logic untestable behind
The Added EditMode tests in
These are platform-agnostic pure functions, so the tests run on the Linux CI runner too — no macOS-only gating needed. The one thing automated tests can't assert is "an iTerm window visually appears," but that's purely Could you re-apply
|
Summary
TerminalLauncher.CreateTerminalProcessStartInfohardcodesopen -a Terminal, so Terminal.app spawns even when the user runs iTerm2 / Warp / Alacritty / etc. macOS LaunchServices resolves-a Terminalby app name, so iTerm's Make iTerm2 default term setting cannot redirect it — the package code itself has to expose the choice.MCPForUnity.MacOSTerminalApp). Unset/empty/whitespace → falls back to"Terminal", so existing users see no change.-aargument so values with spaces (e.g.iTerm 2) work.EditorPrefsWindow'sknownPrefTypes(String) so it shows up in the existing EditorPrefs UI — no new window/menu surface needed.Motivation (concrete case)
In my own project I had to embed this package and hand-edit the line to
-a iTermto stop Terminal.app from popping up on every server restart. That hand-edit is what this PR generalizes: same root change, but driven byEditorPrefsso it survives package upgrades and works for any macOS terminal app the user prefers.How to use
MCPForUnity.MacOSTerminalAppto the bundle name you want, e.g.iTerm,Warp,Alacritty. Same value you would pass toopen -a <name>.Compatibility
Terminal. Identical to current behavior.Test plan (not yet executed in this PR — happy to do whatever the maintainers want before merge)
MCPForUnity.MacOSTerminalApp = "iTerm", restart server → iTerm window opens with the MCP server log.iTerm 2) — quoting handles it.using UnityEditor;is fine — it already is in this codebase, so this is a formality).The only verification I have done so far is the equivalent hand-patched line (
-a iTermhardcoded) running in my own project against the current beta package, which confirmsopen -a <app>is the right control point. The EditorPrefs-driven path in this PR has not yet been exercised in a live Unity build.Notes for reviewers
knownPrefTypesis enough surface for now. Happy to wire a dropdown into the main MCP window if you'd prefer something more discoverable.#if UNITY_EDITOR_OSXand the new logic isEditorPrefs.GetString+ a fallback string check, which is hard to unit-test without abstracting EditorPrefs. If a small helper extraction is preferred for testability, let me know and I'll add it.🤖 Generated with Claude Code
Summary by CodeRabbit