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

Add support for gobject closures #173

Merged
merged 1 commit into from May 27, 2017

Conversation

Projects
None yet
4 participants
@antoyo
Member

antoyo commented May 23, 2017

Hi.
I started to create the Closure type so that we can generate the code of methods taking a GClosure as a parameter.
This is a work in progress, but I open this pull request right now, which is working (even thought it contains a hack) so that you can comment on it.
I'll have to fix the hack for size_of by writing a C function to get the size of GClosure, unless someone has a better idea.

I think that marshallers support should be done in another pull request since I believe that doing something both safe and efficient will be non trivial.

@antoyo antoyo referenced this pull request May 23, 2017

Closed

Capturing mouse clicks #2

Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 23, 2017

Member

Just added minor tips, as I don't fully understand closures and don't sure how to test it in wild ;)

Member

EPashkin commented May 23, 2017

Just added minor tips, as I don't fully understand closures and don't sure how to test it in wild ;)

Show outdated Hide outdated src/value.rs Outdated
Show outdated Hide outdated src/value.rs Outdated
Show outdated Hide outdated src/value.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 24, 2017

Member

I think I fixed all mentionned issues (thanks for your review).
I'll use the work of pull request #172 when it's merged.

Member

antoyo commented May 24, 2017

I think I fixed all mentionned issues (thanks for your review).
I'll use the work of pull request #172 when it's merged.

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 24, 2017

Member

I made a quick test with @sdroege Value implementation and my closure implementation with webkit2gtk-webextension and it does not seem to work well:
the first time, the callback is called, but then, the webview freeze and obviously, the callback is not called anymore after that.
Perhaps it would be easier to debug with a function from gobject/glib.

Member

antoyo commented May 24, 2017

I made a quick test with @sdroege Value implementation and my closure implementation with webkit2gtk-webextension and it does not seem to work well:
the first time, the callback is called, but then, the webview freeze and obviously, the callback is not called anymore after that.
Perhaps it would be easier to debug with a function from gobject/glib.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

@antoyo How does it hang? Do you have a simple testcase for reproducing it, or at least a backtrace of all threads when that happens?

It also would make sense to fix up the GValue conversion functions (or just merging my commits in here), as those will definitely cause problems.

Member

sdroege commented May 24, 2017

@antoyo How does it hang? Do you have a simple testcase for reproducing it, or at least a backtrace of all threads when that happens?

It also would make sense to fix up the GValue conversion functions (or just merging my commits in here), as those will definitely cause problems.

Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 24, 2017

Member

@sdroege You can test the main example in the webkit2gtk-rs repository with cargo run --example --features v2_6.
It requires that the example in this repository (branch feature/closure)) to be built in debug mode (be sure to put both repositories at the same directory level so that this path works).
This will make the webview freeze after you clicked on the page: no more scroll or click working after that.

Since I used your ToGlibPtr implementations, this freeze cannot be caused by my implementation being unsound.

Member

antoyo commented May 24, 2017

@sdroege You can test the main example in the webkit2gtk-rs repository with cargo run --example --features v2_6.
It requires that the example in this repository (branch feature/closure)) to be built in debug mode (be sure to put both repositories at the same directory level so that this path works).
This will make the webview freeze after you clicked on the page: no more scroll or click working after that.

Since I used your ToGlibPtr implementations, this freeze cannot be caused by my implementation being unsound.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

Seems to work fine for me, no freezes or anything. Oh no, I spoke to fast. It freezes here too :)

Member

sdroege commented May 24, 2017

Seems to work fine for me, no freezes or anything. Oh no, I spoke to fast. It freezes here too :)

Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
unsafe {
let size = 4 + 4 + 3 * mem::size_of::<*mut c_void>() as u32;
let closure = gobject_ffi::g_closure_new_simple(size, ptr::null_mut());

This comment has been minimized.

@sdroege

sdroege May 24, 2017

Member

And you want to call g_closure_ref() and g_closure_sink() after this. Floating references \o/ (fixes crashes, testcase coming in a minute)

@sdroege

sdroege May 24, 2017

Member

And you want to call g_closure_ref() and g_closure_sink() after this. Floating references \o/ (fixes crashes, testcase coming in a minute)

This comment has been minimized.

@antoyo

antoyo May 25, 2017

Member

I think I need to remove these calls, now that I use glib_wrapper, right?

@antoyo

antoyo May 25, 2017

Member

I think I need to remove these calls, now that I use glib_wrapper, right?

This comment has been minimized.

@sdroege

sdroege May 25, 2017

Member

Yes

@sdroege

sdroege May 25, 2017

Member

Yes

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

Standalone testcase here, with fixed up (hopefully) Closure bindings: https://gist.github.com/sdroege/d47508b39f55bf454f90673150ccef20
Is still missing the optimizations to prevent unneeded copying though

Member

sdroege commented May 24, 2017

Standalone testcase here, with fixed up (hopefully) Closure bindings: https://gist.github.com/sdroege/d47508b39f55bf454f90673150ccef20
Is still missing the optimizations to prevent unneeded copying though

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

Maybe use the glib_wrapper!() macro here with the ref/unref functions, it would automatically give you most of the trait impls. And instead of calling g_closure_ref() & g_closure_sink only in the constructor, the best would be to make that the ref function. I.e.

glib_wrapper! {
     pub struct Closure(Shared<ffi::Closure>);

     match fn {
         ref => |ptr| {
             g_closure_ref(ptr);
             g_closure_sink(ptr);
         },
         unref => |ptr| ffi::g_closure_unref(ptr),
     }
 }

And then just use from_glib_none() in the new function (yes, the none variant because floating references)

Member

sdroege commented May 24, 2017

Maybe use the glib_wrapper!() macro here with the ref/unref functions, it would automatically give you most of the trait impls. And instead of calling g_closure_ref() & g_closure_sink only in the constructor, the best would be to make that the ref function. I.e.

glib_wrapper! {
     pub struct Closure(Shared<ffi::Closure>);

     match fn {
         ref => |ptr| {
             g_closure_ref(ptr);
             g_closure_sink(ptr);
         },
         unref => |ptr| ffi::g_closure_unref(ptr),
     }
 }

And then just use from_glib_none() in the new function (yes, the none variant because floating references)

Show outdated Hide outdated src/closure.rs Outdated
@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 24, 2017

Member

I've just tested the webkit example with the latest version of this pull request and it now works fine.

Member

antoyo commented May 24, 2017

I've just tested the webkit example with the latest version of this pull request and it now works fine.

Show outdated Hide outdated Cargo.toml Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
Show outdated Hide outdated src/closure.rs Outdated
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 25, 2017

Member

Generally looks good to me now, just some further comments

Member

sdroege commented May 25, 2017

Generally looks good to me now, just some further comments

@@ -0,0 +1,134 @@
// TODO: support marshaller.

This comment has been minimized.

@EPashkin

EPashkin May 25, 2017

Member

Please, don't forget add standard header

@EPashkin

EPashkin May 25, 2017

Member

Please, don't forget add standard header

Show outdated Hide outdated src/closure.rs Outdated
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 25, 2017

Member

No further comments from me 👍

Member

sdroege commented May 25, 2017

No further comments from me 👍

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 26, 2017

Member

All the tests passed, is there any other comment you want to make?

Member

antoyo commented May 26, 2017

All the tests passed, is there any other comment you want to make?

pub fn invoke(&self, values: &[&ToValue]) -> Option<Value> {
let mut result = unsafe { Value::uninitialized() };
let mut values: Vec<_> = values.iter()

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez May 26, 2017

Member

Ii's more of a style issue, so not mandatory, but I'd prefer it to be like this:

let mut values: Vec<_> = values.iter()
                                .map(|v| v.to_value())
                                .collect();
@GuillaumeGomez

GuillaumeGomez May 26, 2017

Member

Ii's more of a style issue, so not mandatory, but I'd prefer it to be like this:

let mut values: Vec<_> = values.iter()
                                .map(|v| v.to_value())
                                .collect();

This comment has been minimized.

@antoyo

antoyo May 26, 2017

Member

I believe it is best to be consistent, whatever style we choose.
For instance, I used the same style as this.

@antoyo

antoyo May 26, 2017

Member

I believe it is best to be consistent, whatever style we choose.
For instance, I used the same style as this.

Show outdated Hide outdated src/closure.rs Outdated
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 26, 2017

Member

I made some but otherwise seems all good to me.

Member

GuillaumeGomez commented May 26, 2017

I made some but otherwise seems all good to me.

Some(result) => *return_value = result.into_raw(),
None => {
let result = Value::uninitialized();
*return_value = result.into_raw();

This comment has been minimized.

@EPashkin

EPashkin May 26, 2017

Member

Why don't call g_value_init(return_value)?

@EPashkin

EPashkin May 26, 2017

Member

Why don't call g_value_init(return_value)?

This comment has been minimized.

@sdroege

sdroege May 26, 2017

Member

There's no type it could be initialized with here. We just need to fill the return value with something here, just in case. And that something is a GValue initialized with all zeroes (i.e. G_TYPE_INVALID), something we can detect later and handle appropriately.

@sdroege

sdroege May 26, 2017

Member

There's no type it could be initialized with here. We just need to fill the return value with something here, just in case. And that something is a GValue initialized with all zeroes (i.e. G_TYPE_INVALID), something we can detect later and handle appropriately.

This comment has been minimized.

@EPashkin

EPashkin May 26, 2017

Member

You right, Type::Invalid don't accepted by g_value_init

@EPashkin

EPashkin May 26, 2017

Member

You right, Type::Invalid don't accepted by g_value_init

This comment has been minimized.

@sdroege

sdroege May 26, 2017

Member

Value::uninitialized() is a bit misleading IMHO, it's not uninitialized but all-zeroes :)

@sdroege

sdroege May 26, 2017

Member

Value::uninitialized() is a bit misleading IMHO, it's not uninitialized but all-zeroes :)

Show outdated Hide outdated src/closure.rs Outdated
// See the COPYRIGHT file at the top-level directory of this distribution.
// Licensed under the MIT license, see the LICENSE file or <http://opensource.org/licenses/MIT>
// TODO: support marshaller.

This comment has been minimized.

@sdroege

sdroege May 27, 2017

Member

What does TODO: support marshaller mean? What do you want to support with marshallers?

@sdroege

sdroege May 27, 2017

Member

What does TODO: support marshaller mean? What do you want to support with marshallers?

This comment has been minimized.

@antoyo

antoyo May 27, 2017

Member

I would like to have something like GCClosure, i.e. being able to use a function with Rust types (instead of getting Values).
I believe it should be done in another pull request since it looks non-trivial to do something both safe and efficient.

@antoyo

antoyo May 27, 2017

Member

I would like to have something like GCClosure, i.e. being able to use a function with Rust types (instead of getting Values).
I believe it should be done in another pull request since it looks non-trivial to do something both safe and efficient.

This comment has been minimized.

@sdroege

sdroege May 27, 2017

Member

That would indeed be very interesting and is also on my list. For dynamically using signals/action signals
Separate pull request, yes

@sdroege

sdroege May 27, 2017

Member

That would indeed be very interesting and is also on my list. For dynamically using signals/action signals
Separate pull request, yes

@antoyo antoyo changed the title from [WIP] Add support for gobject closures to Add support for gobject closures May 27, 2017

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 27, 2017

Member

Ok, it seems good to me.

@EPashkin @sdroege: For you?

Member

GuillaumeGomez commented May 27, 2017

Ok, it seems good to me.

@EPashkin @sdroege: For you?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 27, 2017

Member

👍 Thanks, @antoyo
And thanks @sdroege too for good ideas

Member

EPashkin commented May 27, 2017

👍 Thanks, @antoyo
And thanks @sdroege too for good ideas

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 27, 2017

Member

Let's merge then. Thanks to everyone for your work in here! :)

Member

GuillaumeGomez commented May 27, 2017

Let's merge then. Thanks to everyone for your work in here! :)

@GuillaumeGomez GuillaumeGomez merged commit e1a4a44 into gtk-rs:master May 27, 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