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

Add WidgetExt::add_tick_callback binding #611

Merged
merged 1 commit into from Dec 30, 2017

Conversation

Projects
None yet
4 participants
@fengalin
Copy link
Contributor

fengalin commented Dec 28, 2017

No description provided.

@fengalin fengalin force-pushed the fengalin:master branch 2 times, most recently from 78d9383 to 628c12c Dec 28, 2017

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 28, 2017

Please add gtk_widget_remove_tick_callback too
FnMut really needed?

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Dec 28, 2017

Please add gtk_widget_remove_tick_callback too

gtk_widget_remove_tick_callback is auto-generated in:

fn remove_tick_callback(&self, id: u32);

FnMut really needed?

Not sure about this. I followed the example of:

pub fn idle_add<F>(func: F) -> SourceId

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 28, 2017

Ok, I missed both.
Then I only see problem that callback added for concrete type O but called only for Widget, so better make both trampoline and destroy internal generic function (something like https://github.com/gtk-rs/gio/blob/master/src/buffered_input_stream.rs#L24)
cc @sdroege too.

where
F: FnMut(&Self, &FrameClock) -> Continue + 'static,
{
unsafe extern "C" fn add_tick_callback_trampoline(

This comment has been minimized.

@EPashkin

EPashkin Dec 28, 2017

Member

Both internal function need be generalized by O: IsA<Widget> + IsA<Object> (maybe without Object)

This comment has been minimized.

@fengalin

fengalin Dec 28, 2017

Author Contributor

Ah Ok! Sorry about that.

This comment has been minimized.

@sdroege

sdroege Dec 29, 2017

Member

Why? All the other signal connectors use &Self too

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Dec 29, 2017

Please see the commit's message. I had to use Widget instead of Self for the callback. Hope this is acceptable. This is the way it works for e.g.:
https://github.com/sdroege/gstreamer-rs/blob/d44869711a958b2e9fe479e4dc0e1c56c2753cc0/gstreamer/src/pad.rs#L105

If you're OK with this version, I'll squash the commits.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 29, 2017

@fengalin Solution was it end of widget.rs.
This change at minimum compiles.

@@ -114,37 +114,35 @@ impl<O: IsA<Widget> + IsA<Object>> WidgetExtManual for O {
     #[cfg(any(feature = "v3_8", feature = "dox"))]
     fn add_tick_callback<F>(&self, func: F) -> u32
     where
-        F: FnMut(&Widget, &FrameClock) -> Continue + 'static,
+        F: FnMut(&Self, &FrameClock) -> Continue + 'static,
     {
-        type Callback = FnMut(&Widget, &FrameClock) -> Continue + 'static;
-        
-        unsafe extern "C" fn add_tick_callback_trampoline(
+        unsafe extern "C" fn add_tick_callback_trampoline<O: IsA<Widget>> (
             widget: *mut ffi::GtkWidget,
             frame_clock: *mut gdk_ffi::GdkFrameClock,
             func: gpointer,
         ) -> gboolean {
             callback_guard!();
-            let func: &RefCell<Box<Callback>> = transmute(func);
+            let func: &RefCell<Box<FnMut(&O, &FrameClock) -> Continue + 'static>> = transmute(func);
 
             (&mut *func.borrow_mut())(
-                &from_glib_borrow(widget),
+                &Widget::from_glib_borrow(widget).downcast_unchecked(),
                 &from_glib_borrow(frame_clock)
             ).to_glib()
         }
 
-        unsafe extern "C" fn destroy_closure(ptr: gpointer) {
+        unsafe extern "C" fn destroy_closure<O: IsA<Widget>>(ptr: gpointer) {
             callback_guard!();
-            Box::<RefCell<Box<Callback>>>::from_raw(ptr as *mut _);
+            Box::<RefCell<Box<FnMut(&O, &FrameClock) -> Continue + 'static>>>::from_raw(ptr as *mut _);
         }
 
-        let func: Box<RefCell<Box<Callback>>> = Box::new(RefCell::new(Box::new(func)));
+        let func: Box<RefCell<Box<FnMut(&O, &FrameClock) -> Continue + 'static>>> = Box::new(RefCell::new(Box::new(func)));
                 
         unsafe {
             ffi::gtk_widget_add_tick_callback(
                 self.to_glib_none().0,
-                Some(add_tick_callback_trampoline),
+                Some(add_tick_callback_trampoline::<O>),
                 Box::into_raw(func) as gpointer,
-                Some(destroy_closure),
+                Some(destroy_closure::<O>),
             )
         }
     }

@fengalin fengalin force-pushed the fengalin:master branch from ce72cd4 to 581de09 Dec 29, 2017

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Dec 29, 2017

@EPashkin , sorry didn't see your message as I was working on it. What I was missing was downcast_unchecked().

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 29, 2017

@fengalin Now it all good for me.

@sdroege How about you? I think that your comment about &Self was from &Widget version.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 29, 2017

Looks good to me now, yes. But I just noticed that basically all signal handlers in gtk can be FnMut :)

@fengalin

This comment has been minimized.

Copy link
Contributor Author

fengalin commented Dec 29, 2017

Yes indeed :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 30, 2017

@GuillaumeGomez Please take a look at this PR.

@@ -2,14 +2,20 @@
// 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>

#[cfg(any(feature = "v3_8", feature = "dox"))]

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 30, 2017

Member

We should maybe write a macro for things like this. It's getting a bit ugly when reading the code...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 30, 2017

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 1efdfd8 into gtk-rs:master Dec 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

vhdirk pushed a commit to vhdirk/gtk-rs that referenced this pull request Jan 16, 2019

Merge pull request gtk-rs#611 from fengalin/master
Add WidgetExt::add_tick_callback binding
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.