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 #129: use wrapper callbacks to convert arguments #238

Merged
merged 4 commits into from Sep 6, 2018

Conversation

Projects
None yet
4 participants
@dhardy
Copy link
Contributor

dhardy commented Sep 4, 2018

Since there is no existing use of callbacks in GDK I had to improvise. I'm not certain if this is the ideal approach (there's the overhead of another function call which probably won't be optimised away, at least without full LTO), and it's not as simple as I'd like, but it may still be the best we can do.

Alternatively we could simplify by dropping the data and possibly notify arguments; I'm not sure how useful they are, though presumably a binding lib should just follow the underlying API as closely as possible.

I haven't tested it.

data: Option<*mut T>,
notify: Option<&'static Fn(Option<*mut T>)>)
{
let data = data.unwrap_or(ptr::null_mut()) as *mut c_void;

This comment has been minimized.

@EPashkin

EPashkin Sep 4, 2018

Member

This "static" function need macro like assert_initialized_main_thread!(); in first line

This comment has been minimized.

@dhardy

dhardy Sep 4, 2018

Author Contributor

I don't think it does (though of course if you initialise GTK afterwards, then its callback will override the func passed here).

/// Should another event handler be set in the future, `notify` will be
/// called before doing so, if not `None`.
pub fn set_handler<T>(
func: Option<&'static Fn(Event, Option<*mut T>)>,

This comment has been minimized.

@EPashkin

EPashkin Sep 4, 2018

Member

Second parameter in closure IMHO wrong, and don't need in rust closures at all, better always pass data=null_ptr

This comment has been minimized.

@dhardy

dhardy Sep 4, 2018

Author Contributor

Sure, I can remove the data pointer.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Sep 4, 2018

Also docs says "Note that GTK+ uses this to install its own event handler, so it is usually not useful for GTK+ applications. (Although an application can call this function then call gtk_main_do_event() to pass events to GTK+.)"
so this function need comment about unusable in gtk application
and maybe need marked unsafe

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Sep 4, 2018

Also usually we use destroy to "safe" free closure memory,
see https://github.com/gtk-rs/glib/blob/master/src/signal.rs#L51 as example.

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 4, 2018

Thanks for the fast feedback, I'll have a go at addressing it tomorrow.

I've now tested it (without data or notify), and it is possible to use with GTK if calling gtk_main_do_event within the callback. I can do this to evesdrop on GTK events.

If I understand right you can't pass a closure to set_handler because of the &'static restriction, so there is never any data to clean up anyway. Frankly I don't see much use for the notify callback because normally only one callback is registered (or two if overriding GTK), but GDK does provide it.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 4, 2018

I recommend you to see how we handle callbacks with signals. It should help you to fix the last parts.

Also, I agree with @EPashkin on the unsafe part. This seems like not the good way to do this. Having it is nice but I'd prefer to let people know that they're using it at they own risk. (if you have an opinion on this @sdroege?)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 4, 2018

Just some comments here

  • First of all, how would you use this and why? Can you give a use-case and example? :) This only seems useful if implementing a new toolkit on top of GDK that is not GTK?

  • What others said already: The closure should be taken be value and not reference, it should take only the event as argument and the lifetime of the closure should be managed via the destroy notify. Look at how we do callbacks (e.g. for signals or other things) elsewhere. I don't see why this is not possible here. Everything else is simply unsafe and you can as well use the bindings from gdk-sys then, this would not add any advantage over that here. In short: you pass the closure via the data pointer, you have a trampoline for the actual function pointer that extracts the closure from the data pointer given to the trampoline function via argument, you destroy the closure via the destroy notify. The function signature should be set_handler<F: Fn(&gdk::Event)>(handler: Option<F>)

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 5, 2018

There's a problem:

extern "C" fn event_handler_destroy(ptr: glib_ffi::gpointer) {
    if ptr != ptr::null_mut() {
        // convert back to Box and free
        let raw: *mut (dyn Fn(&Event) + Send + Sync + 'static) = ptr as _;
        unsafe{
            let boxed: Box<dyn Fn(&Event) + Send + Sync + 'static> = Box::from_raw(raw);
        }
    }
}

Error:

error[E0277]: expected a `std::ops::Fn<(&event::Event,)>` closure, found `libc::c_void`
   --> src/event.rs:460:66
    |
460 |         let raw: *mut (dyn Fn(&Event) + Send + Sync + 'static) = ptr as _;
    |                                                                  ^^^ expected an `Fn<(&event::Event,)>` closure, found `libc::c_void`
    |
    = help: the trait `for<'r> std::ops::Fn<(&'r event::Event,)>` is not implemented for `libc::c_void`
    = note: required for the cast to the object type `(dyn for<'r> std::ops::Fn(&'r event::Event) + std::marker::Send + std::marker::Sync + 'static)`

The error message doesn't make much sense, but I suspect the error is because a *mut dyn Fn(..) is a fat pointer while *mut c_void is not.

Potential solutions:

  1. accept a function pointer instead (probably not a fat pointer; shouldn't need allocation): fn(&Event)
  2. double-allocate (sounds stupid but should be viable and I see this used elsewhere)
  3. use static memory to store the closure (which isn't an issue since GDK stores the pointer it receives in static memory anyway)
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 5, 2018

Check the code @EPashkin linked you to, or any other of the signal code in any of the gtk-rs crates. You need to box the Fn twice to get a normal pointer.

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 5, 2018

Which is one of the solutions I listed, but decidedly indirect. Very well.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 5, 2018

Sorry, I forgot to write something about your 3 options above.

  1. accept a function pointer instead (probably not a fat pointer; shouldn't need allocation): fn(&Event)

This would work but would limit functionality quite a bit as you can't have any state in your function (other than using global state)

  1. double-allocate (sounds stupid but should be viable and I see this used elsewhere)

As this not something you're setting very often, the allocation doesn't really matter and in the greater scheme of things it's going to be a very small fraction of the overall allocations compared to what GDK/GTK are doing anyway.

It means that there is a double pointer dereference (outer box to the trait object, trait object to the actual data), but also that shouldn't matter much in the greater scheme of things.

  1. use static memory to store the closure (which isn't an issue since GDK stores the pointer it receives in static memory anyway)

I'm not sure what the threading guarantees of this function are but this might've needed a mutex for the global storage. You're right that it would work though as there can be at most one such callback at a time, and you get the destroy notify for clearing the static memory for it and replacing it with NULL.

It would be nicer to keep the general pattern of how we do this in other places though for consistency, i.e. 2. Otherwise 3. is also a valid option IMHO.

@dhardy dhardy force-pushed the dhardy:master branch from abd2ef0 to 400d783 Sep 5, 2018

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 5, 2018

I'm not sure what the threading guarantees of this function are

True. I guess it makes sense that the closure passed should be Send and that if we did use global memory, its usage should be protected.

There are quite a lot of events passed, but I guess the double-dereference thing is still not a big deal.

I pushed an updated version but belatedly realised it doesn't work (segfault).

@dhardy dhardy force-pushed the dhardy:master branch from 400d783 to b7223bb Sep 5, 2018

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 5, 2018

Fixed. I didn't realise the type specification when boxing was important to generate the correct code!

Example usage:

        gdk::Event::set_handler(Some(|event: &mut gdk::Event| {
            println!("Event: {:?}", event);
            gtk::main_do_event(event);
        }));
@@ -421,3 +445,20 @@ macro_rules! event_subtype {
}
}
}

extern "C" fn event_handler_trampoline(event: *mut ffi::GdkEvent, ptr: glib_ffi::gpointer) {
let mut event = unsafe { from_glib_none(event) };

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

These functions should be unsafe extern "C" IMHO

This comment has been minimized.

@dhardy

dhardy Sep 5, 2018

Author Contributor

I guess, but since they are only used as internal callbacks the consequence is nil.

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

Yes but that marks the whole function unsafe. Everything in there is unsafe, not just the parts you previously put into an unsafe block :)

if ptr != ptr::null_mut() {
let raw: *mut Box<dyn Fn(&mut Event) + Send +'static> = ptr as _;
let f: &(dyn Fn(&mut Event) + Send +'static) = unsafe { &**raw };
f(&mut event)

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

Should the event be mutable? I don't know much about GDK events, can they be changed in a meaningful way and does that even work with the bindings like this?

This comment has been minimized.

@dhardy

dhardy Sep 5, 2018

Author Contributor

I did this to match up with gtk::main_do_event. No idea really, though otherwise one must clone the event to call that. Maybe main_do_event shouldn't be mutable; would have to check GTK.

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

Should be investigated then :)

Show resolved Hide resolved src/event.rs Outdated
/// not be called concurrently with itself or with either `gtk::init` or
/// `gtk::Application::new`.
pub unsafe fn set_handler<F: Fn(&mut Event) + Send +'static>(handler: Option<F>) {
if let Some(handler) = handler {

This comment has been minimized.

@EPashkin

EPashkin Sep 5, 2018

Member

Still need assert_initialized_main_thread!(); in first line,
this also fix some thread safety problem.

This comment has been minimized.

@EPashkin

EPashkin Sep 5, 2018

Member

Missed that @sdroege already says about this :(

This comment has been minimized.

@dhardy

dhardy Sep 5, 2018

Author Contributor

Not necessary; the GDK C code is simple, does only one read (from a null-initialised pointer) and is safe to call from any thread (though not reentrant):

void 
gdk_event_handler_set (GdkEventFunc   func,
		       gpointer       data,
		       GDestroyNotify notify)
{
  if (_gdk_event_notify)
    (*_gdk_event_notify) (_gdk_event_data);

  _gdk_event_func = func;
  _gdk_event_data = data;
  _gdk_event_notify = notify;
}

Though of course if gtk::init gets called after this, it will override the event handler.

This comment has been minimized.

@EPashkin

EPashkin Sep 5, 2018

Member

Then your code don't pass CI 😉

This comment has been minimized.

@EPashkin

EPashkin Sep 5, 2018

Member

Or you can use other macro, skip_assert_initialized!(); if you sure about no problem in rust,
but IMHO better allow change only from main thread.

This comment has been minimized.

@dhardy

dhardy Sep 5, 2018

Author Contributor

Ah, didn't look at the CI errors yet! Well we could force this to a single thread, in which case we can drop the Send requirement on the closure.

@sdroege this is what you mean above? assert_initialized_main_thread is defined in GDK.

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

This C code is not thread-safe. So either this needs the main thread assertion, or must be marked unsafe

This comment has been minimized.

@sdroege

sdroege Sep 5, 2018

Member

Yes you can remove the Send requirement then.

I meant this one here specifically: https://github.com/gtk-rs/gtk/blob/b17cc04141daeb47971040a58a3d90af42d82c00/src/rt.rs#L59-L62
I don't think the GDK one is actually checking for the main thread

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 6, 2018

Okay, I added the assert_initialized_main_thread check and removed unsafe from set_handler.

The remaining question is about whether we should have a &mut Event or &Event in the callback.

One side of this is whether gtk_main_do_event actually mutates the event or not — this isn't simple to check, but since rewrite_event_for_surface actually clones the event before adjusting, it hints that the function doesn't mutate the event.

The other side is whether it even matters — is there any reason it shouldn't be &mut Event? By examining all calls to _gdk_event_emit it appears there is no technical issue since the event is always destroyed immediately after the handler is called.

Since it's more flexible I suggest we leave it as &mut Event?

Show resolved Hide resolved src/event.rs Outdated

@dhardy dhardy force-pushed the dhardy:master branch from e97ba63 to bb0ba7d Sep 6, 2018

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Sep 6, 2018

@dhardy About &mut Event: as have setters: gdk_event_set_device etc. seems we need mutable event.
We also have pub fn get_scancode(&mut self) -> i32 and pub fn get_pointer_emulated(&mut self) -> bool but IMHO this just typo and better fixed (maybe not in this PR).

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 6, 2018

The other side is whether it even matters — is there any reason it shouldn't be &mut Event? By examining all calls to _gdk_event_emit it appears there is no technical issue since the event is always destroyed immediately after the handler is called.

Since it's more flexible I suggest we leave it as &mut Event?

&mut Event means that it's also possible to replace the whole Event with a completely new event. Does that work?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Sep 6, 2018

@dhardy Thanks.
IMHO this ready for merge now.

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 6, 2018

&mut Event means that it's also possible to replace the whole Event with a completely new event. Does that work?

Probably yes, but since the C code just passes GdkEvent *event, you can't replace what it sees (i.e. it will still free the right thing).

But within Rust, replacing a value frees (Drop) the old one. Does your wrapper type manage memory correctly in this case?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 6, 2018

Yes it probably works

@dhardy

This comment has been minimized.

Copy link
Contributor Author

dhardy commented Sep 6, 2018

Great. Then I agree with @EPashkin that this is ready (unless you want me to squash).

Thanks for the help guys.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 6, 2018

No it's fine. Thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit 48fc1af into gtk-rs:master Sep 6, 2018

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
You can’t perform that action at this time.