-
Notifications
You must be signed in to change notification settings - Fork 46
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
desktop/game_mode: Use i32
for pid
#234
desktop/game_mode: Use i32
for pid
#234
Conversation
@@ -154,7 +154,7 @@ impl<'a> GameMode<'a> { | |||
/// See also [`RegisterGame`](https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.GameMode.html#org-freedesktop-portal-gamemode-registergame). | |||
#[doc(alias = "RegisterGame")] | |||
pub async fn register(&self, pid: u32) -> Result<(), Error> { | |||
let status = self.0.call("RegisterGame", &(pid)).await?; | |||
let status = self.0.call("RegisterGame", &(pid as i32)).await?; |
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.
why not replace the function signature? is this in order to have a backportable variant?
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.
Originally, I had two reasons not to change the signature:
- Backward compatibility, in order not to break code that already uses
u32
. (Now writing this I realized that probably no one uses it as it doesn't work...) - Better interoperability with Rust, as Rust uses
u32
for pids: https://doc.rust-lang.org/std/process/fn.id.html, https://doc.rust-lang.org/std/process/struct.Child.html#method.id
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.
Right, in such case I would add
// Matches the type used in std
pub type Pid = u32;
and pass that to the functions instead.
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.
Sure thing, added the Pid
type alias and replaced most pids in API. There are instances where u64
is used though, but I haven't touched them.
Let me know if that's what you meant.
The Pid type alias ensures that the proper type is used for pids (u32), which is the same as used by std.
ef34450
to
46678ac
Compare
As per the documentation, methods which accept pids (e.g. RegisterGame, RegisterGameByPid) require the type of pid to be i (i32), not u (u32). Using u32 causes errors in runtime, making these methods unusable. This bug was most likely introduced by ae81250.
46678ac
to
536110a
Compare
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.
lgtm, thanks
Do you think we could get a release on |
sure, later this week |
Great, thanks! |
(a polite bump on this 😶 ) |
ah, sorry, completely forgot about it. let me do a release now |
No worries, thanks a lot! |
0.9.2 should be up |
Awesome, that was quick! ^^ |
As per the documentation, methods which accept pids (e.g.
RegisterGame
,RegisterGameByPid
) require the type of pid to bei
(i32
), notu
(u32
).Using
u32
causes errors in runtime, making these methods unusable. This bug was most likely introduced by ae81250.See https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.GameMode.html