fix/292 : Fix multiwindow#299
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
695dbb4 to
450c28a
Compare
kolkov
left a comment
There was a problem hiding this comment.
@lkmavi Enterprise code review of PR #299 (multiwindow fix). Great work — 4 real bugs fixed.
Architecture: ✅ Sound
Independent Wayland connections per secondary window (secondaryWaylandConn) is the right approach. Each window gets its own wl_display + wl_surface + xdg_toplevel. Input stays on primary seat (comment at line 184). This matches how GLFW handles multi-window Wayland.
Per-proxy callback routing: ✅ Correct
Replacing global xdgCallbackHandle / inputCallbackHandle with per-proxy maps (xdgSurfaceHndls, xdgWmBaseHndls, toplevelHandles) keyed by C proxy pointer — clean solution. Mutex-protected, unregistered in Close().
Thread panic propagation: ✅ Nice catch
threadResult with recover() in Call/CallVoid re-panics on caller. Prevents silent goroutine death on render thread.
macOS GetPoint buffer: ✅ Correct
[2]float64 → [4]float64 for goffi HFA return — matches Go 1.26 checkptr with -race.
Show() in NewWindow: ✅ Our regression fixed
platWindow.Show() added in window_manager.go:354 — fixes the hidden-then-show regression from v0.41.5 where we added Show() for primary window but forgot secondary.
Items to address before merge:
-
replacedirective in go.mod (line 8) —replace github.com/gogpu/wgpu => github.com/lkmavi/wgpu ...must be removed. Please rebase onmainwhich now has wgpu v0.29.13 + naga v0.17.14 (v0.41.8 just released). -
PR body says "remove replay before merge" — is there debug/replay code still in the PR that needs cleanup?
-
No CSD for secondary windows —
createSecondaryConnpassesdecorName/decorVersiontoOpenLibwaylandfor SSD. Without SSD, secondary windows have no decorations. This is a reasonable first implementation, but worth a brief comment increateSecondaryConnnoting that CSD for secondaries is deferred (separate subsurface + pointer routing per window is complex). -
Defensive:
WaitEventssecondary FD iteration (line 2566) —secs := p.secondariescopies slice header under RLock, then iterates after RUnlock. Currently safe (main thread is single-threaded), but a brief comment noting the safety assumption would help future readers. -
naga v0.17.13 in go.sum — resolves after rebase on v0.41.8.
Summary
Solid multiwindow implementation. Main blocker: remove replace directive and rebase on v0.41.8 (wgpu v0.29.13 + naga v0.17.14). Rest are minor suggestions. CI 9/9 pass.
fix/292 : Fix golint fix(gles): re-add wgpu-local replace for GLES MakeCurrent fix Restore the local wgpu replace directive to pick up the fix for glFenceSync OOM + eglSwapBuffers EGL_BAD_SURFACE on Wayland GLES 3.0. The wgpu-local commit b8b98f3 adds MakeCurrent in Submit() and MakeCurrentSurface in Present(), matching Rust wgpu-hal's explicit EGL context management pattern. This fix must land in wgpu-local before it can be published as wgpu v0.29.13. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> fix/292 : Return objc format Add deep copy slice fix/292 : Add temporary replay # Conflicts: # go.sum
450c28a to
3635f0f
Compare
|
@kolkov fixed on after rewiew |
platWindow.Show(). The v0.41.5 "hidden-then-show" refactor moved Show() out of CreateWindow(), but only wired it up in the
primary window path (app.go). Fixed: platWindow.Show() is now called after GPU surface configuration in NewWindow().
(xdgCallbackHandle, inputCallbackHandle) were overwritten when the second window's LibwaylandHandle was created, so the
first window's xdg configure acks were sent to the wrong surface — the compositor never mapped the first surface. Fixed:
replaced both globals with per-proxy maps (xdgSurfaceHndls, xdgWmBaseHndls, toplevelHandles) keyed by C proxy pointer.
window's ID on the second CreateWindow() call, and ShouldClose() shared state across windows. Fixed: secondary windows get
independent Wayland connections via createSecondaryConn(), with their own shouldClose flag; ShouldClose() and Destroy()
dispatch to the secondary's own state when w.secondary != nil.
primary connection's FD. Fixed: secondary connection FDs are appended to the unix.Poll set so the main loop unblocks for
any window's compositor events.
@kolkov remove replay before merge