Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust fixes #18
base: stable
Are you sure you want to change the base?
Rust fixes #18
Changes from 24 commits
28d3fe7
639cecc
f450266
f812746
d216136
264fe97
da41b6b
aa3f1fd
41314dc
e3e74fe
ca650aa
388c0ef
529aef8
72a2409
f212f56
69e23fe
eeb318c
a774277
78a6928
2d9a599
e890aa5
59eed9e
c257012
8cc65ca
d6e4876
8aa977d
5071c6f
2e20492
7a89e04
fdac019
d7dd8ca
d5f87f7
38765f1
2890983
4e5d1eb
99a4008
d14e93e
a2fc59e
d9506ce
f4d63e0
a80770e
a7ca191
090204e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public functions and types should have docstrings.
Also, you can use
impl AsRef<str>
andimpl Into<u16>
to give callers more flexibility. For example, that would allow you to use theevdev
enums without converting to a raw u16 first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of unwrapping, you should return
Result<Self, Something>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repr(u8)
makes no sense here, and I'm surprised it even compiles.I think a public trait would be more idiomatic than an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah haha it doesn't make sense indeed. It is a leftover from me trying to combine
JoypadKind
andJoypad
in one, but ultimately decided not to.I was considering a trait vs enum, and chose an enum currently. The reason is that you likely want to store a list of any type of joypad. With a trait you would need to store a
Vec
ofBox
with a bunch of traits. On top of that, you might want to do some specific logic if a joypad is a PS5 controller (like LED settings), for example. With an enum this becomes a simplematch
statement.I'm curious what your thoughts are on this, as I'm not 100% sure my reasoning is correct :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the trait vs enum consideration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the radio silence. I think it comes down to how much unique behavior each of the gamepad types has.
If each gamepad struct has unique functionality, but share some common behavior, that should be a trait. If all the gamepad types have the same methods, then I would say there should be a
pub struct Gamepad
with avendor
method or field, returning theGamepad
enum, rather than a separatestruct XoneGamepad
, etc. (From a quick skim, this seems to be the case.)I wouldn't really export trait enum like this - it just feels unidiomatic. The stdlib very rarely uses enums for API-central receiver objects like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's only the DualSense controller that is unlike the others. This could be a separate impl for
DualSense
.I do like the
trait
implementation the more I think of it. At first I wasn't sure how to handleDualSense
specific features if you are dealing with aBox<dyn Joypad>
, butJoypad
can implement something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work as I had hoped, because
on_rumble_fn
takes aon_rumble_fn: impl FnMut(i32, i32)
, which means the trait isn't eligible for dynamic dispatching. The rust compiler suggests to use anenum
xDAny recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, right.
I understand your point about matching the C++ API, but I still think a single
Joypad
struct is the best option. Future Joypads could also support LEDs, so I don't see a problem with having that method be a noop if the Joypad has none.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the callbacks are not very rusty. It would be better IMO to expose a pollable handle and not invert control. But that may be departing quite a bit from the C++ interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a minimum, the error type should be
pub struct InputtinoError(String)
. Better would be an enum usingthiserror
or similar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that each of these types has the exact same methods, but that is not enforced by the compiler, indicates that they should just be one type, imo - either a trait with many impls or just one
Joypad
type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a trait with many impls is closer to the C API. Without changing the C/C++ API too, I think that makes more sense.