Skip to content
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

Allow chaining expressions with Closure objects #714

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

jf2048
Copy link
Member

@jf2048 jf2048 commented Nov 14, 2021

I had to change ClosureExpression::with_closure to expand out the generic parameters, it could be changed back depending on this: rust-lang/rust#83701

Also includes some docs fixes.

@SeaDve
Copy link
Contributor

SeaDve commented Nov 14, 2021

I think chain_gclosure may be clearer

@jf2048
Copy link
Member Author

jf2048 commented Nov 15, 2021

That may not be straightforward as the macro was changed to just closure!. Maybe it could be chain_with_closure, but that also seems confusing.

@sdroege sdroege added this to the 0.4 milestone Nov 15, 2021
@bilelmoussaoui
Copy link
Member

Maybe rename the first chain_closure to chain_callback & keep this one as chain_closure?

@SeaDve
Copy link
Contributor

SeaDve commented Nov 15, 2021

I just realized that it was named chain_closure before since it chains a ClosureExpression, but something like chain_closure_with_closure is a weird name

@bilelmoussaoui
Copy link
Member

I just realized that it was named chain_closure before since it chains a ClosureExpression, but something like chain_closure_with_closure is a weird name

Well the ClosureExpression should only take a glib::Closure but we provide a helper constructor that takes a callback. So it's fine to not name it chain_closure if it's documented properly. The double chain_closure_with_closure is definitely not great

@SeaDve
Copy link
Contributor

SeaDve commented Nov 15, 2021

I see, maybe chain_closure and chain_closure_with_callback ?

@bilelmoussaoui
Copy link
Member

I see, maybe chain_closure and chain_closure_with_callback ?

No strong opinions here, @sdroege ?:)

@sdroege
Copy link
Member

sdroege commented Nov 15, 2021

Seems fine either way. Ask your favorite random number generator and go with that :)

@bilelmoussaoui
Copy link
Member

I see, maybe chain_closure and chain_closure_with_callback ?

Let's go with this one then

@SeaDve
Copy link
Contributor

SeaDve commented Nov 16, 2021

semi related to pr, maybe the ClosureExpression
's constructors could also be renamed to new and with_callback for coherence

gdk4/src/event.rs Outdated Show resolved Hide resolved
@jf2048
Copy link
Member Author

jf2048 commented Dec 13, 2021

This is updated with the name changes, but it's a bit awkward with the recent addition of RustClosure because now it always has to be closure!( ... ).into().

@@ -23,16 +23,13 @@ define_expression!(

impl ClosureExpression {
#[doc(alias = "gtk_closure_expression_new")]
pub fn new<F, R>(params: impl IntoIterator<Item = impl AsRef<Expression>>, callback: F) -> Self
pub fn new<R, I, E>(params: I, closure: glib::Closure) -> Self
Copy link
Member

Choose a reason for hiding this comment

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

Make it take a RustClosure ?

Copy link
Member

Choose a reason for hiding this comment

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

It probably has to for type-safety reasons anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to RustClosure, if someone wants to add another function that takes just Closure it should probably be unsafe anyway.

@bilelmoussaoui
Copy link
Member

other than the cargo fmt, lgtm

@bilelmoussaoui bilelmoussaoui merged commit d3a859b into gtk-rs:master Dec 13, 2021
@bilelmoussaoui
Copy link
Member

Thanks!

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.

4 participants