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

Implement ObjectExt::connect() for connecting to arbitrary signals #191

Merged
merged 2 commits into from Jul 6, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jul 6, 2017

No description provided.

Show outdated Hide outdated src/object.rs
Show outdated Hide outdated src/object.rs
Show outdated Hide outdated src/object.rs
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 6, 2017

Member

Maybe comment on call g_object_set_property can be removed already?

Member

EPashkin commented Jul 6, 2017

Maybe comment on call g_object_set_property can be removed already?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 6, 2017

Member

Maybe comment on call g_object_set_property can be removed already?

Which?

Member

sdroege commented Jul 6, 2017

Maybe comment on call g_object_set_property can be removed already?

Which?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 6, 2017

Member

There only one call of g_object_set_property 😉
As you added type check beforehand this comment IMHO already unneeded.

// Using property names that don't exist or using a GValue of a wrong type
// causes a warning printed at runtime by GObject and is considered a
// programming error. It is however memory-safe to do so
Member

EPashkin commented Jul 6, 2017

There only one call of g_object_set_property 😉
As you added type check beforehand this comment IMHO already unneeded.

// Using property names that don't exist or using a GValue of a wrong type
// causes a warning printed at runtime by GObject and is considered a
// programming error. It is however memory-safe to do so
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 6, 2017

Member

Ah, that's unrelated to this pull request though :) Can do, sure

Member

sdroege commented Jul 6, 2017

Ah, that's unrelated to this pull request though :) Can do, sure

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 6, 2017

Member

👍
Big thanks, @sdroege ,now we have generic signals and will have way to emit it ;)

Member

EPashkin commented Jul 6, 2017

👍
Big thanks, @sdroege ,now we have generic signals and will have way to emit it ;)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 6, 2017

Member

Now we only need to be able to put enums/flags and all that into GValues, but that comes next then (I actually need that working for GStreamer too).

Once that is there, we could in theory rewrite the connect/trampoline logic in gir around this. Not sure if that's worth it though, doesn't really make any difference.

Member

sdroege commented Jul 6, 2017

Now we only need to be able to put enums/flags and all that into GValues, but that comes next then (I actually need that working for GStreamer too).

Once that is there, we could in theory rewrite the connect/trampoline logic in gir around this. Not sure if that's worth it though, doesn't really make any difference.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 6, 2017

Member

I just have a small question/concern: shouldn't the handler be put inside a type so users don't confuse it with something else?

Member

GuillaumeGomez commented Jul 6, 2017

I just have a small question/concern: shouldn't the handler be put inside a type so users don't confuse it with something else?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 6, 2017

Member

I just have a small question/concern: shouldn't the handler be put inside a type so users don't confuse it with something else?

It's the same as with all the other signal related functions, like the ones generated from gir :) I agree with you though, but let's do that in another step.

Member

sdroege commented Jul 6, 2017

I just have a small question/concern: shouldn't the handler be put inside a type so users don't confuse it with something else?

It's the same as with all the other signal related functions, like the ones generated from gir :) I agree with you though, but let's do that in another step.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 6, 2017

Member

And not just type but a newtype wrapper imho (struct SignalId(u64))

Member

sdroege commented Jul 6, 2017

And not just type but a newtype wrapper imho (struct SignalId(u64))

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 6, 2017

Member

That's what I meant when I wrote "put inside a type". But you're right: I should have said newtype, my bad.

Except for this point, everything seems good to me. @EPashkin Good for you too?

Member

GuillaumeGomez commented Jul 6, 2017

That's what I meant when I wrote "put inside a type". But you're right: I should have said newtype, my bad.

Except for this point, everything seems good to me. @EPashkin Good for you too?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 6, 2017

Member

Yes.

Member

EPashkin commented Jul 6, 2017

Yes.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 6, 2017

Member

Then I merge. Thanks!

Member

GuillaumeGomez commented Jul 6, 2017

Then I merge. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 53ed524 into gtk-rs:master Jul 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment