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

Initial GMainLoop/GMainContext bindings #194

Merged
merged 10 commits into from Jul 11, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jul 11, 2017

///
/// `func` will be called repeatedly until it returns `Continue(false)`.
pub fn idle_source_new<'a, N: Into<Option<&'a str>>, F>(name: N, priority: Priority, func: F) -> Source
where F: FnMut() -> Continue + Send + 'static {

This comment has been minimized.

@sdroege

sdroege Jul 11, 2017

Member

The question here is mostly whether we want single functions for creating Sources, or create something with a builder pattern here. Single functions seems more simple to me

@sdroege

sdroege Jul 11, 2017

Member

The question here is mostly whether we want single functions for creating Sources, or create something with a builder pattern here. Single functions seems more simple to me

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Either way seems fine to me so do whatever seems better to you.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Either way seems fine to me so do whatever seems better to you.

@sdroege sdroege referenced this pull request Jul 11, 2017

Open

implement IO Channels #186

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

For MainContext::invoke() this could in theory be this, but I'm not sure how to handle FnOnce inside a Box<_>. What am I missing?
This fails with error[E0161]: cannot move a value of type std::ops::FnOnce() + std::marker::Send: the size of std::ops::FnOnce() + std::marker::Send cannot be statically determined

impl MainContext {
    pub fn invoke<F>(&self, func: F)
    where F: FnOnce() + Send + 'static {
        unsafe {
            glib_ffi::g_main_context_invoke_full(self.to_glib_none().0, glib_ffi::G_PRIORITY_DEFAULT_IDLE, Some(trampoline),
                into_raw(func), Some(destroy_closure))
        }
    }

    pub fn invoke_with_priority<F>(&self, priority: Priority, func: F)
    where F: FnOnce() + Send + 'static {
        unsafe {
            glib_ffi::g_main_context_invoke_full(self.to_glib_none().0, priority.to_glib(), Some(trampoline),
                into_raw(func), Some(destroy_closure))
        }
    }
}

unsafe extern "C" fn trampoline(func: gpointer) -> gboolean {
    let _guard = CallbackGuard::new();
    let func: &RefCell<Option<Box<FnOnce() + Send + 'static>>> = transmute(func);

    if let Some(func) = func.borrow_mut().take() {
        func();
    }

    glib_ffi::G_SOURCE_REMOVE
}

unsafe extern "C" fn destroy_closure(ptr: gpointer) {
    let _guard = CallbackGuard::new();
    Box::<RefCell<Option<Box<FnOnce() + Send + 'static>>>>::from_raw(ptr as *mut _);
}

fn into_raw<F: FnOnce() + Send + 'static>(func: F) -> gpointer {
    let func: Box<RefCell<Option<Box<FnOnce() + Send + 'static>>>> =
        Box::new(RefCell::new(Some(Box::new(func))));
    Box::into_raw(func) as gpointer
}
Member

sdroege commented Jul 11, 2017

For MainContext::invoke() this could in theory be this, but I'm not sure how to handle FnOnce inside a Box<_>. What am I missing?
This fails with error[E0161]: cannot move a value of type std::ops::FnOnce() + std::marker::Send: the size of std::ops::FnOnce() + std::marker::Send cannot be statically determined

impl MainContext {
    pub fn invoke<F>(&self, func: F)
    where F: FnOnce() + Send + 'static {
        unsafe {
            glib_ffi::g_main_context_invoke_full(self.to_glib_none().0, glib_ffi::G_PRIORITY_DEFAULT_IDLE, Some(trampoline),
                into_raw(func), Some(destroy_closure))
        }
    }

    pub fn invoke_with_priority<F>(&self, priority: Priority, func: F)
    where F: FnOnce() + Send + 'static {
        unsafe {
            glib_ffi::g_main_context_invoke_full(self.to_glib_none().0, priority.to_glib(), Some(trampoline),
                into_raw(func), Some(destroy_closure))
        }
    }
}

unsafe extern "C" fn trampoline(func: gpointer) -> gboolean {
    let _guard = CallbackGuard::new();
    let func: &RefCell<Option<Box<FnOnce() + Send + 'static>>> = transmute(func);

    if let Some(func) = func.borrow_mut().take() {
        func();
    }

    glib_ffi::G_SOURCE_REMOVE
}

unsafe extern "C" fn destroy_closure(ptr: gpointer) {
    let _guard = CallbackGuard::new();
    Box::<RefCell<Option<Box<FnOnce() + Send + 'static>>>>::from_raw(ptr as *mut _);
}

fn into_raw<F: FnOnce() + Send + 'static>(func: F) -> gpointer {
    let func: Box<RefCell<Option<Box<FnOnce() + Send + 'static>>>> =
        Box::new(RefCell::new(Some(Box::new(func))));
    Box::into_raw(func) as gpointer
}
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

Problem is that we need something like FnBox https://doc.rust-lang.org/alloc/boxed/trait.FnBox.html . So let's add a FIXME comment there for the time when this stabilizes...

Member

sdroege commented Jul 11, 2017

Problem is that we need something like FnBox https://doc.rust-lang.org/alloc/boxed/trait.FnBox.html . So let's add a FIXME comment there for the time when this stabilizes...

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

Good for review then IMHO, @GuillaumeGomez @EPashkin :) No further additions planned to this pull request

Member

sdroege commented Jul 11, 2017

Good for review then IMHO, @GuillaumeGomez @EPashkin :) No further additions planned to this pull request

glib_ffi::g_main_context_invoke_full(self.to_glib_none().0, glib_ffi::G_PRIORITY_DEFAULT_IDLE, Some(trampoline),
into_raw(func), Some(destroy_closure))
}
}

This comment has been minimized.

@EPashkin

EPashkin Jul 11, 2017

Member

We always pass to FFI boxed closure. And destroy_closure working with boxed.

@EPashkin

EPashkin Jul 11, 2017

Member

We always pass to FFI boxed closure. And destroy_closure working with boxed.

This comment has been minimized.

@EPashkin

EPashkin Jul 11, 2017

Member

Hm. Missed that you added your version destory_closure

@EPashkin

EPashkin Jul 11, 2017

Member

Hm. Missed that you added your version destory_closure

@@ -183,7 +190,7 @@ mod file_error;
mod key_file;
pub mod prelude;
pub mod signal;
mod source;
pub mod source;

This comment has been minimized.

@EPashkin

EPashkin Jul 11, 2017

Member

Part of source reexported, while part is not.
Not better do pub use source::*; ?

@EPashkin

EPashkin Jul 11, 2017

Member

Part of source reexported, while part is not.
Not better do pub use source::*; ?

This comment has been minimized.

@sdroege

sdroege Jul 11, 2017

Member

Your call :)

@sdroege

sdroege Jul 11, 2017

Member

Your call :)

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

No preference for me so do whatever you want.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

No preference for me so do whatever you want.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 11, 2017

Member

Code looks good to me, but PR missing update Gir submodule and regen to correct version.

Member

EPashkin commented Jul 11, 2017

Code looks good to me, but PR missing update Gir submodule and regen to correct version.

@@ -572,10 +569,6 @@ pub fn hostname_to_unicode(hostname: &str) -> Option<String> {
// unsafe { TODO: call ffi::g_idle_remove_by_data() }
//}
//pub fn idle_source_new() -> /*Ignored*/Option<Source> {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Since we bound Source, why is this ignored?

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Since we bound Source, why is this ignored?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Ah nevermind...

@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Ah nevermind...

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Looks good to me as well. Fix @EPashkin's issues and let's merge!

Member

GuillaumeGomez commented Jul 11, 2017

Looks good to me as well. Fix @EPashkin's issues and let's merge!

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

@EPashkin What do you want to have fixed? Should source::* be re-exported at the crate level?

Member

sdroege commented Jul 11, 2017

@EPashkin What do you want to have fixed? Should source::* be re-exported at the crate level?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 11, 2017

Member

@sdroege Yes + gir submodule update + regen

Member

EPashkin commented Jul 11, 2017

@sdroege Yes + gir submodule update + regen

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

OK, will do in an hour or two

Member

sdroege commented Jul 11, 2017

OK, will do in an hour or two

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

Done :)

Member

sdroege commented Jul 11, 2017

Done :)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

Ah I guess that's the reason for not re-exporting source::*. source::Id should be re-exported as SourceId. What do you suggest? Rename Id to SourceId?

Member

sdroege commented Jul 11, 2017

Ah I guess that's the reason for not re-exporting source::*. source::Id should be re-exported as SourceId. What do you suggest? Rename Id to SourceId?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

Let's try that...

Member

sdroege commented Jul 11, 2017

Let's try that...

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 11, 2017

Member

👍
Thanks @sdroege

Member

EPashkin commented Jul 11, 2017

👍
Thanks @sdroege

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

The "changes PR" is getting really huge. Next release will be certainly a worldwide ovation. People will throw money at us. Gods will sing our names and we'll be taken as examples by future generations. Can't wait to be at the release. 😭

Member

GuillaumeGomez commented Jul 11, 2017

The "changes PR" is getting really huge. Next release will be certainly a worldwide ovation. People will throw money at us. Gods will sing our names and we'll be taken as examples by future generations. Can't wait to be at the release. 😭

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

I'd like to get some more things into the next release though, most importantly removing generic impls for the various conversions traits (and move them into the macros) and implement them for enums/flags/boxed/etc types (which is not possible right now. so we can have those in GValues, for example)

Member

sdroege commented Jul 11, 2017

I'd like to get some more things into the next release though, most importantly removing generic impls for the various conversions traits (and move them into the macros) and implement them for enums/flags/boxed/etc types (which is not possible right now. so we can have those in GValues, for example)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

I don't intend to make the release before at least another month. I still need to work on gtk-rs/release anyway. (my comment was super ironic: it'll be hellish for me to make this release unless I end my automatic releaser)

Member

GuillaumeGomez commented Jul 11, 2017

I don't intend to make the release before at least another month. I still need to work on gtk-rs/release anyway. (my comment was super ironic: it'll be hellish for me to make this release unless I end my automatic releaser)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 11, 2017

Member

CI passed

Member

EPashkin commented Jul 11, 2017

CI passed

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 11, 2017

Member

🎉

Member

sdroege commented Jul 11, 2017

🎉

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 11, 2017

Member

Thanks a lot!

Member

GuillaumeGomez commented Jul 11, 2017

Thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit aaad196 into gtk-rs:master Jul 11, 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