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

Implement Builder::connect_signals_full #852

Merged
merged 2 commits into from Jul 23, 2019

Conversation

@sfanxiang
Copy link
Member

sfanxiang commented Jul 18, 2019

This is a safe way to implement builder signal connection, as discussed in #707.

Closes #707.

cc @sdroege

Gir.toml Outdated Show resolved Hide resolved
let func = (*callback)(&builder, handler_name.as_str());
object
.connect(signal_name.as_str(), flags & gobject_sys::G_CONNECT_AFTER != 0, move |args| func(args))
.expect("Failed to connect to builder signal");

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jul 18, 2019

Member

Instead of panicking, shouldn't we return a Result<(), String>? I really don't like panics.

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 18, 2019

Member

To where would you return this Result? There's nobody there who can handle errors

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jul 18, 2019

Member

Isn't this function called by users directly?

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 18, 2019

Member

It's a callback that is called from GTK

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jul 18, 2019

Member

Oh damn you're absolutely right! I didn't pay attention that we were in the callback... Nevermind then!

let callback: *mut P = user_data as *const _ as usize as *mut P;
let func = (*callback)(&builder, handler_name.as_str());
object
.connect(signal_name.as_str(), flags & gobject_sys::G_CONNECT_AFTER != 0, move |args| func(args))

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 18, 2019

Member

flags.contains(...) might be easier to read

This comment has been minimized.

Copy link
@sfanxiang

sfanxiang Jul 18, 2019

Author Member

@sdroege Actually, flags is u32 here, and I don't think we should introduce ConnectFlags which is not used in public API.

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 18, 2019

Member

Indeed, sorry.

@@ -52,4 +55,27 @@ impl<O: IsA<Builder>> BuilderExtManual for O {
}
}
}

fn connect_signals_full<P: FnMut(&Builder, &str) -> Box<dyn Fn(&[glib::Value]) -> Option<glib::Value> + Send + Sync + 'static>>(&self, func: P) {

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 18, 2019

Member

I already see people complaining about the trait bounds on the closure :) Send and Sync are not needed here as this is all for GTK objects. Remove those bounds, and if necessary use some unsafe code to cast the bounds in there again for object.connect() :)

@sfanxiang sfanxiang force-pushed the sfanxiang:connect-signals-full branch 2 times, most recently from 95e3344 to d85e517 Jul 18, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 18, 2019

Looks all good to me

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jul 18, 2019

@sfanxiang Thanks, please, update generated files

@sfanxiang sfanxiang force-pushed the sfanxiang:connect-signals-full branch from 3f10186 to af97ea2 Jul 19, 2019
@@ -24,6 +26,12 @@ impl Builder {
pub trait BuilderExtManual: 'static {
fn get_object<T: IsA<Object>>(&self, name: &str) -> Option<T>;
fn add_from_file<T: AsRef<Path>>(&self, file_path: T) -> Result<(), Error>;
fn connect_signals_full<

This comment has been minimized.

Copy link
@sdroege

sdroege Jul 19, 2019

Member

I wouldn't call it connect_signals_full but only connect_signals. We can never have the other variant :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 19, 2019

Good to go for me otherwise. I assume you tested that it actually works and can be used? :) Maybe worth adding an example to the examples repo?

@sfanxiang sfanxiang force-pushed the sfanxiang:connect-signals-full branch from af97ea2 to a96e7ea Jul 19, 2019
@sfanxiang

This comment has been minimized.

Copy link
Member Author

sfanxiang commented Jul 20, 2019

@sdroege

I assume you tested that it actually works and can be used? :) Maybe worth adding an example to the examples repo?

This example builds and works. Does it suffice?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 20, 2019

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 23, 2019

@GuillaumeGomez let's get this in?

@GuillaumeGomez GuillaumeGomez merged commit 201acd9 into gtk-rs:master Jul 23, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
alex179ohm pushed a commit to alex179ohm/gtk that referenced this pull request Oct 21, 2019
Remove some duplicate clone calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.