Add low-level hook for SDL3 input events#2867
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the SdlEventListener interface to allow custom listeners to receive raw SDL events, and updates SdlJoystickInput, SdlKeyInput, and SdlMouseInput to implement it. LwjglWindow is updated to manage these listeners and dispatch events. The feedback suggests wrapping the custom listener event dispatch loop in a try-catch block to prevent any unhandled exceptions from interrupting the dispatching of events to subsequent listeners and internal input handlers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
deee7e0 to
dcc7875
Compare
| windowSizeListeners.remove(listener); | ||
| } | ||
|
|
||
| public void registerSdlEventListener(SdlEventListener listener) { |
There was a problem hiding this comment.
I wonder if registerSdlEventListener(...) could be renamed to addSdlEventListener(...) for consistency with removeSdlEventListener(...).
Using complementary method pairs (add/remove, get/set, etc.) is a well-established Java API convention and tends to make APIs more intuitive. Since the method simply adds the listener to an internal collection, addSdlEventListener() also seems to better reflect what it actually does.
There was a problem hiding this comment.
True. I went for consistency with existing API for WindowSizeListener: registerWindowSizeListener and removeWindowSizeListener. With that in mind, should I rename this to addSdlEventListener, or keep it as it is?
There was a problem hiding this comment.
You're right, and I owe you an apology for assuming your suggestion was inconsistent with the existing API. Your reply made it clear that you were actually following the current WindowSizeListener naming (registerWindowSizeListener / removeWindowSizeListener).
At this point, I see two possible approaches:
-
Adopt the
add/removeconvention for new APIs. In that case, this PR could introduceaddSdlEventListener()/removeSdlEventListener(), and a follow-up PR could deprecateregisterWindowSizeListener()in favor ofaddWindowSizeListener(), while keeping backward compatibility for the time being. -
Keep consistency with the existing API. That would mean naming the new methods
registerSdlEventListener()/removeSdlEventListener(), matching the currentWindowSizeListenerAPI even though theregister/removepairing is less consistent with common Java naming conventions.
That said, this is only my opinion. The final decision on API direction should rest with the engine maintainers.
@riccardobl
Resolves #2864