Embed native OpenGL surfaces on Linux and Windows - #110
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughAdds embedded OpenGL platform APIs and rendering support, terminal response suppression, safer Windows ConPTY I/O teardown, standardized internal library artifacts, and a Windows Electron demo with native WGL/N-API integration, diagnostics, software-renderer deployment, and stress testing. ChangesOpenGL embedding
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 437064019e
ℹ️ 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".
| host->wgl_choose_pixel_format = reinterpret_cast<WglChoosePixelFormatFn>( | ||
| load("wglChoosePixelFormat")); |
There was a problem hiding this comment.
When this runs against the normal Windows OpenGL stack, these lookups leave wgl_choose_pixel_format/wgl_set_pixel_format/wgl_swap_buffers null because the Win32 pixel-format APIs are exposed as ChoosePixelFormat, SetPixelFormat, and SwapBuffers from wingdi/GDI, not as wgl* exports. Since LoadWglApi treats any missing pointer as fatal, create() throws before the Electron example can create any terminal surface.
Useful? React with 👍 / 👎.
| errdefer leaveEmbedded(); | ||
| try prepareContext(&embeddedGetProcAddress); |
There was a problem hiding this comment.
This errdefer is scoped only to surfaceInit, but Surface.init keeps the embedded context current until finalizeSurfaceInit after several later fallible steps such as font-grid setup and renderer construction. If one of those steps fails, ghostty_surface_new unwinds without calling finalizeSurfaceInit, so embedders see make_current succeed without a matching clear_current, which can leave their context lock/current context stuck after a failed surface creation.
Useful? React with 👍 / 👎.
![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
🤖 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 `@example/electron-embed-windows/scripts/start-software-renderer.cmd`:
- Around line 1-18: Convert start-software-renderer.cmd from LF-only to CRLF
line endings, preserving its existing commands and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba76671e-01ad-4189-9db8-8cd47a0d90a9
⛔ Files ignored due to path filters (1)
example/electron-embed-windows/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
build.zigexample/electron-embed-windows/.gitignoreexample/electron-embed-windows/README.mdexample/electron-embed-windows/binding.gypexample/electron-embed-windows/main.jsexample/electron-embed-windows/package.jsonexample/electron-embed-windows/renderer.htmlexample/electron-embed-windows/scripts/build-ghostty.ps1example/electron-embed-windows/scripts/deploy-mesa.ps1example/electron-embed-windows/scripts/start-software-renderer.cmdexample/electron-embed-windows/scripts/stress-cycles.mjsexample/electron-embed-windows/src/libghostty_embed_win.ccinclude/ghostty.hpkg/opengl/glad.zigsrc/Surface.zigsrc/apprt/embedded.zigsrc/build/GhosttyLib.zigsrc/build/SharedDeps.zigsrc/renderer/OpenGL.zigsrc/renderer/Thread.zigsrc/termio/Thread.zig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186ce2b0b3
ℹ️ 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".
| ghostty_surface_mouse_pos(host->surface, GET_X_LPARAM(lparam), | ||
| GET_Y_LPARAM(lparam), CurrentModifiers(host)); |
There was a problem hiding this comment.
On high-DPI displays, the mouse lparam coordinates for this child HWND are already client/framebuffer pixels, while ghostty_surface_mouse_pos goes through embedded.Surface.cursorPosCallback, which multiplies the supplied coordinates by the surface content scale. Because UpdateSurfaceMetrics also reports the scaled framebuffer size and content scale, passing GET_X/Y_LPARAM here makes clicks and selections land too far down/right at scales like 150%; convert back to DIPs before calling the C API.
Useful? React with 👍 / 👎.
| if (IsTextProducingKey(wparam) && !HasCommandModifier(host)) | ||
| return 0; |
There was a problem hiding this comment.
When no Ctrl/Alt/Super modifier is held, this returns before SendKey for all printable keys, including the numpad, and the later WM_CHAR path calls ghostty_surface_text_input, whose implementation explicitly bypasses keyboard protocol encoding. That means terminal modes that depend on physical key identity, such as application keypad or enhanced keyboard protocols, receive plain text instead of a key event; for example numpad 1 cannot be encoded as a keypad key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/termio/stream_handler.zig (1)
19-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the side-effecting nature of the suppression function.
Because this function acts as a destructor for
.write_allocmessages by freeingreq.data, its predicate-style name (suppressTerminalResponse) obscures the fact that it consumes the message.Consider renaming it to
dropTerminalResponseorconsumeSuppressedResponseto clearly signal its side effects, protecting against accidental use-after-free or memory leaks if misused in future conditionals or logging.♻️ Proposed refactor
-fn suppressTerminalResponse(enabled: bool, msg: termio.Message) bool { +fn dropTerminalResponse(enabled: bool, msg: termio.Message) bool { if (!enabled) return false; switch (msg) { .write_small, .write_stable, .color_scheme_report, .size_report, .focused, => return true, .write_alloc => |req| { req.alloc.free(req.data); return true; }, else => return false, } } -test "terminal response suppression only drops parser writes" { +test "dropTerminalResponse only drops parser writes" { const testing = std.testing; const small_bytes: []const u8 = "reply"; const small = try termio.Message.writeReq(testing.allocator, small_bytes); - try testing.expect(suppressTerminalResponse(true, small)); + try testing.expect(dropTerminalResponse(true, small)); const large_bytes = [_]u8{'x'} ** 80; const large_slice: []const u8 = &large_bytes; const large = try termio.Message.writeReq(testing.allocator, large_slice); - try testing.expect(suppressTerminalResponse(true, large)); + try testing.expect(dropTerminalResponse(true, large)); - try testing.expect(!suppressTerminalResponse( + try testing.expect(!dropTerminalResponse( false, .{ .write_stable = "reply" }, )); - try testing.expect(!suppressTerminalResponse( + try testing.expect(!dropTerminalResponse( true, .{ .linefeed_mode = true }, )); - try testing.expect(suppressTerminalResponse( + try testing.expect(dropTerminalResponse( true, .{ .size_report = .csi_18_t }, )); }Apply the rename at the call site on line 187 as well:
- if (suppressTerminalResponse(self.suppress_terminal_responses, msg)) + if (dropTerminalResponse(self.suppress_terminal_responses, msg))🤖 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 `@src/termio/stream_handler.zig` around lines 19 - 61, Rename the side-effecting suppressTerminalResponse function to dropTerminalResponse, or an equivalent consume-prefixed name, so callers understand that .write_alloc messages are freed. Update the call site around the indicated handler logic and adjust the related test name/references while preserving the existing suppression behavior.src/pty.zig (1)
391-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a helper for the pipe-pair creation.
in_pipe/in_pipe_ptyandout_pipe/out_pipe_ptycreation are near-duplicates (CreateNamedPipeW + CreateFileW, differing only by path/access direction). A small helper parameterized on direction would reduce the chance of the two paths drifting apart on future edits.🤖 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 `@src/pty.zig` around lines 391 - 451, Extract the duplicated CreateNamedPipeW/CreateFileW logic from the Windows PTY setup into a helper that accepts the pipe path and direction-specific access flags, then use it for both in_pipe/in_pipe_pty and out_pipe/out_pipe_pty. Preserve the existing overlapped app-side handles, synchronous ConPTY client handles, security attributes, buffer settings, and error propagation.
🤖 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.
Nitpick comments:
In `@src/pty.zig`:
- Around line 391-451: Extract the duplicated CreateNamedPipeW/CreateFileW logic
from the Windows PTY setup into a helper that accepts the pipe path and
direction-specific access flags, then use it for both in_pipe/in_pipe_pty and
out_pipe/out_pipe_pty. Preserve the existing overlapped app-side handles,
synchronous ConPTY client handles, security attributes, buffer settings, and
error propagation.
In `@src/termio/stream_handler.zig`:
- Around line 19-61: Rename the side-effecting suppressTerminalResponse function
to dropTerminalResponse, or an equivalent consume-prefixed name, so callers
understand that .write_alloc messages are freed. Update the call site around the
indicated handler logic and adjust the related test name/references while
preserving the existing suppression behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3127f4ae-c394-430b-8e37-a7e5cbc44105
📒 Files selected for processing (12)
example/electron-embed-windows/README.mdexample/electron-embed-windows/package.jsonexample/electron-embed-windows/src/libghostty_embed_win.ccinclude/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zigsrc/os/windows.zigsrc/pty.zigsrc/termio/Exec.zigsrc/termio/Options.zigsrc/termio/Termio.zigsrc/termio/stream_handler.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- example/electron-embed-windows/package.json
- include/ghostty.h
- example/electron-embed-windows/src/libghostty_embed_win.cc
1 participant
Adds an embedder-owned OpenGL surface ABI for libghostty, Linux shared-library packaging, and a native HWND/WGL Electron example. The X11/GLX Electron host lives in the private
manaflow-ai/electron-ghostty-demorepository.The renderer updates the GL viewport from the live embedded surface size before each frame. GLAD thread-local contexts are zero-initialized before reload, preventing invalid
dlcloseduring rapid teardown. Renderer and IO stop watchers are armed before embedded surface creation returns.Validation against Ghostty commit
186ce2b:5a9c6e1ff3ae39ead368ecf023a170105979dc1f3c23495f92f34ec763f1f2ab.Windows cloud validation used Mesa llvmpipe because the RDP adapter exposed OpenGL 1.1. Hardware WGL remains a required follow-up.
Summary by CodeRabbit
New Features
Bug Fixes
Build Improvements