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

Use a newtype wrapper around the signal handler IDs #231

Merged
merged 2 commits into from Sep 9, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Sep 7, 2017

commit b6ba3c6 (HEAD -> signal-handler-newtype)
Author: Sebastian Dröge sebastian@centricular.com
Date: Thu Sep 7 19:55:40 2017 +0300

Don't implement Copy/Clone for SignalHandlerId and SourceId

These can only really be used once.

commit 3bfe752
Author: Sebastian Dröge sebastian@centricular.com
Date: Thu Sep 7 19:53:11 2017 +0300

Use a newtype wrapper around the signal handler IDs

Fixes https://github.com/gtk-rs/glib/issues/229
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 8, 2017

Member

@GuillaumeGomez @EPashkin Any comments on this? Once this and the gir change landed, we need to regenerate all bindings that come with signals.

Member

sdroege commented Sep 8, 2017

@GuillaumeGomez @EPashkin Any comments on this? Once this and the gir change landed, we need to regenerate all bindings that come with signals.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 8, 2017

Member

Ah sorry, I said it on IRC but I repeat here as well: I like it. All good for me!

Member

GuillaumeGomez commented Sep 8, 2017

Ah sorry, I said it on IRC but I repeat here as well: I like it. All good for me!

@@ -702,7 +703,7 @@ impl<T: IsA<Object> + SetValue> ObjectExt for T {
if handler == 0 {
Err(BoolError("Failed to connect to signal"))
} else {
Ok(handler as u64)
Ok(from_glib(handler))

This comment has been minimized.

@EPashkin

EPashkin Sep 9, 2017

Member

handler as u64?

@EPashkin

EPashkin Sep 9, 2017

Member

handler as u64?

This comment has been minimized.

@EPashkin

EPashkin Sep 9, 2017

Member

I meant: you lost conversion.

@EPashkin

EPashkin Sep 9, 2017

Member

I meant: you lost conversion.

This comment has been minimized.

@sdroege

sdroege Sep 9, 2017

Member

No, it's internally all based on c_ulong now again, like the FFI functions. No cast is needed here unless you meant something else.

@sdroege

sdroege Sep 9, 2017

Member

No, it's internally all based on c_ulong now again, like the FFI functions. No cast is needed here unless you meant something else.

This comment has been minimized.

@EPashkin

EPashkin Sep 9, 2017

Member

Seems this not fully so:

error[E0277]: the trait bound `signal::SignalHandlerId: translate::FromGlib<u64>` is not satisfied
  --> src\signal.rs:57:5
   |
57 |     from_glib(handle)
   |     ^^^^^^^^^ the trait `translate::FromGlib<u64>` is not implemented for `signal::SignalHandlerId`
   |
   = help: the following implementations were found:
             <signal::SignalHandlerId as translate::FromGlib<u32>>
   = note: required by `translate::from_glib`
@EPashkin

EPashkin Sep 9, 2017

Member

Seems this not fully so:

error[E0277]: the trait bound `signal::SignalHandlerId: translate::FromGlib<u64>` is not satisfied
  --> src\signal.rs:57:5
   |
57 |     from_glib(handle)
   |     ^^^^^^^^^ the trait `translate::FromGlib<u64>` is not implemented for `signal::SignalHandlerId`
   |
   = help: the following implementations were found:
             <signal::SignalHandlerId as translate::FromGlib<u32>>
   = note: required by `translate::from_glib`

This comment has been minimized.

@sdroege

sdroege Sep 9, 2017

Member

There was a cast right in the line above, which would have to be removed. Done! Thanks for noticing

@sdroege

sdroege Sep 9, 2017

Member

There was a cast right in the line above, which would have to be removed. Done! Thanks for noticing

This comment has been minimized.

@sdroege

sdroege Sep 9, 2017

Member

FWIW, this would've only failed on Windows (32/64 bit) and 32 bit variants of other platforms. Elsewhere c_ulong and u64 are equivalent.

@sdroege

sdroege Sep 9, 2017

Member

FWIW, this would've only failed on Windows (32/64 bit) and 32 bit variants of other platforms. Elsewhere c_ulong and u64 are equivalent.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 9, 2017

Member

👍 for merge.

Member

EPashkin commented Sep 9, 2017

👍 for merge.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 9, 2017

Member

Or it need regen with gtk-rs/gir#454?

Member

EPashkin commented Sep 9, 2017

Or it need regen with gtk-rs/gir#454?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 9, 2017

Member

It needs regen with gtk-rs/gir#453

Member

sdroege commented Sep 9, 2017

It needs regen with gtk-rs/gir#453

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 9, 2017

Member

Let's wait for the regen then! :)

Member

GuillaumeGomez commented Sep 9, 2017

Let's wait for the regen then! :)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 9, 2017

Member

The regens won't compile without having this GLib change merged first.

Member

sdroege commented Sep 9, 2017

The regens won't compile without having this GLib change merged first.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 9, 2017

Member

@GuillaumeGomez Or do you want all regens first, not compiling, and then merge it all in one go?

Member

sdroege commented Sep 9, 2017

@GuillaumeGomez Or do you want all regens first, not compiling, and then merge it all in one go?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 9, 2017

Member

It's always so annoying... Let's merge this one first!

Member

GuillaumeGomez commented Sep 9, 2017

It's always so annoying... Let's merge this one first!

@GuillaumeGomez GuillaumeGomez merged commit d17431e into gtk-rs:master Sep 9, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 9, 2017

Member

It definitely is, yes. Once you get gtk-rs/gir#454 and #232 in, I'll send regens for everything. Otherwise there would have to be regens twice

Member

sdroege commented Sep 9, 2017

It definitely is, yes. Once you get gtk-rs/gir#454 and #232 in, I'll send regens for everything. Otherwise there would have to be regens twice

@EPashkin EPashkin referenced this pull request Sep 29, 2017

Merged

Dox #237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment