Skip to content

fix: use JoystickId for joystick/gamepad event which fields#322

Open
revmischa wants to merge 1 commit into
masterfrom
fix/joystick-id-newtype
Open

fix: use JoystickId for joystick/gamepad event which fields#322
revmischa wants to merge 1 commit into
masterfrom
fix/joystick-id-newtype

Conversation

@revmischa

Copy link
Copy Markdown
Member

Converts JoystickId from a type alias to a proper newtype struct with Debug, Display, and From trait implementations.

  • JoystickId is now a newtype wrapping SDL_JoystickID
  • Implements Debug, Display, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash
  • Adds From and Into for easy conversion
  • Updates all joystick/gamepad events to use JoystickId for their which field

Breaking change: Event which fields now require JoystickId instead of u32. Migration is straightforward using the From/Into impls:

// Before
Event::JoyButtonDown { which: 0, .. }

// After
Event::JoyButtonDown { which: JoystickId::new(0), .. }
// or
Event::JoyButtonDown { which: 0.into(), .. }

Part 1 of 2 for #319. The second PR will rename Controller* events to Gamepad* to match SDL3 terminology.

@maia-s

maia-s commented Jan 25, 2026

Copy link
Copy Markdown
Collaborator

I can add a new associated function to all the id types in sdl3-sys (and satellites) to fix the tuple struct alias init issue, and Display impls too (behind the display-impls feature). They already implement the rest (Debug via debug-impls)

Debug would say it's SDL_JoystickId instead of just JoystickId, but with this you could keep using the alias instead of making your own newtype for it.

The event fields should still be updated to use the id type though so that'd still be a breaking change

@revmischa

Copy link
Copy Markdown
Member Author

I can add a new associated function to all the id types in sdl3-sys (and satellites) to fix the tuple struct alias init issue, and Display impls too (behind the display-impls feature). They already implement the rest (Debug via debug-impls)

Debug would say it's SDL_JoystickId instead of just JoystickId, but with this you could keep using the alias instead of making your own newtype for it.

The event fields should still be updated to use the id type though so that'd still be a breaking change

How should we proceed?

@maia-s

maia-s commented May 3, 2026

Copy link
Copy Markdown
Collaborator

I added the new functions and the display-impls feature in sdl3-sys 0.6.1

revmischa added a commit that referenced this pull request May 4, 2026
Replaces the previous custom newtype approach with the simpler one maia-s
suggested in #322: keep JoystickId as a type alias for sdl3-sys's
SDL_JoystickID and rely on sdl3-sys 0.6.5's display-impls/debug-impls
features and the new `SDL_JoystickID::new(...)` constructor.

The breaking change is unchanged: event `which` fields go from u32 to
JoystickId. Migration is `JoystickId::new(0)`.
@revmischa
revmischa force-pushed the fix/joystick-id-newtype branch from 6e0465d to 54fa191 Compare May 4, 2026 00:03
@revmischa

Copy link
Copy Markdown
Member Author

Reworked per @maia-s's suggestion: JoystickId is back to a type alias for SDL_JoystickID, with display-impls/debug-impls enabled on sdl3-sys to get Display/Debug and the new SDL_JoystickID::new(...) constructor for ergonomic init. Diff is now ~65/57 instead of carrying a custom newtype.

Stacked on #379 which bumps sdl3-sys to 0.6.5.

@revmischa revmischa changed the title fix: convert JoystickId to newtype with Debug/Display traits fix: use JoystickId for joystick/gamepad event which fields May 8, 2026
revmischa added a commit that referenced this pull request Jun 9, 2026
Replaces the previous custom newtype approach with the simpler one maia-s
suggested in #322: keep JoystickId as a type alias for sdl3-sys's
SDL_JoystickID and rely on sdl3-sys 0.6.5's display-impls/debug-impls
features and the new `SDL_JoystickID::new(...)` constructor.

The breaking change is unchanged: event `which` fields go from u32 to
JoystickId. Migration is `JoystickId::new(0)`.
@revmischa
revmischa force-pushed the fix/joystick-id-newtype branch from 54fa191 to 8065da9 Compare June 9, 2026 20:57
@revmischa
revmischa requested a review from Copilot June 9, 2026 21:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the public API surface for joystick/gamepad IDs by switching event which fields and related getters to use JoystickId (SDL’s joystick instance ID type) instead of u32, aligning event payloads with the rest of the joystick/gamepad APIs.

Changes:

  • Update Joystick::id() and Gamepad::id() to return JoystickId and adjust error checks accordingly.
  • Change joystick/gamepad-related Event::*::which fields from u32 to JoystickId, including test updates and doctest examples.
  • Enable debug-impls and display-impls features for sdl3-sys and adjust example output formatting to use Display.

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
src/sdl3/joystick.rs Returns JoystickId from joystick APIs and simplifies passing IDs to sys calls.
src/sdl3/gamepad.rs Returns JoystickId from Gamepad::id() with updated error checks.
src/sdl3/event.rs Updates all joystick/controller event which fields to JoystickId and updates tests/docs.
examples/gamepad.rs Prints gamepad IDs via Display instead of accessing .0.
Cargo.toml Enables debug-impls/display-impls on sdl3-sys to support formatting traits.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/sdl3/joystick.rs
Comment thread src/sdl3/event.rs
Comment thread src/sdl3/event.rs
Replaces the previous custom newtype approach with the simpler one maia-s
suggested in #322: keep JoystickId as a type alias for sdl3-sys's
SDL_JoystickID and rely on sdl3-sys 0.6.5's display-impls/debug-impls
features and the new `SDL_JoystickID::new(...)` constructor.

The breaking change is unchanged: event `which` fields go from u32 to
JoystickId. Migration is `JoystickId::new(0)`.
@revmischa
revmischa force-pushed the fix/joystick-id-newtype branch from 8065da9 to 3fcd4e2 Compare June 9, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants