Add g_idle_add()

[vojtechkral]

No description provided.

[GuillaumeGomez]

I don't get what you want to do. If the function g_idle_add isn't binded, why uncommenting it ?

[vojtechkral]

I was expecting it to be used soon. If you want the whole thing at once, that's allright too...

[GuillaumeGomez]

I'd prefer, otherwise it doesn't make sense. If didn't comment all unused functions, the only we'd have to know they're not binded would be with rust compiler warnings. Not very wonderful...

I'll let this PR open. When you have binded the function, please notify me.

Thanks in advance ! =D

[gkoz]

I don't see anything wrong with having unused definitions in the sys crates (as long as they're correct).

Here, Rust is capable of expressing the SourceFunc type so substituting a gpointer for it doesn't look good.

[GuillaumeGomez]

@gkoz: It's mostly to avoid warnings and make easier the future bindings. Otherwise, we could just uncomment all the non-binded functions and say it's fine. Or maybe I didn't understand the point here ?

[gkoz]

There hardly can be such thing as an unused published member of a library, so there won't be any warnings.

Uncommenting everything wholesale would be problematic because someone has to review the definitions so it makes sense to avoid that effort while there's no need for it.

[GuillaumeGomez]

Hum... Okay. But then why not providing directly the binding for this function ? That's where I'm lost.

[vojtechkral]

I'll try to add the binding.

@gkoz Regarding GSourceFunc, I was basically copying what can be seen in g_timeout_add() - GSourceFunc is commented and a version with gpointer is used instead. Is there a reason for this?

[GuillaumeGomez]

@vojtechkral: It was to avoid boring stuff haha (super explanation !). It was a (successfull) test I made, I wanted to do the same with a closure but for now there is an ICE in rust for it and I didn't try to avoid it (I'm working on cairo's documentation and rustdoc). For me, it's totally fine, it allows to avoid annoying stuff but I'm not sure that's the best solution... I'll dig to find more information.

[gkoz]

This is why we shouldn't be committing hacks. People try to stay consistent with the hacks, leading to more hacks.

[GuillaumeGomez]

Well, that's not really a hack.

[vojtechkral]

Ok, I added a binding.

Using only a fn() for now, because I have no idea how to manage the lifetime of a closure. Glib doesn't seem to provide any means for this like it does with signals.

Here's code I used to test the thing: https://gist.github.com/vojtechkral/ea7112bff8eaff0d0ff2

[GuillaumeGomez]

"to test the thing". It sounds very scary that way haha. Thanks a lot for your work ! I wait for travis and appveyor's confirmation and I merge.

[vojtechkral]

Wait wait I don't think this is ready to merge yet, there's a commented-out function header with closure which I'm not sure what to do about yet.

A version with only a fn() would probably be ok though, at least for now...

[GuillaumeGomez]

If you manage to make work a closure by passing it as a pointer, it could be nice. You can try if you feel like it. ;-)

[gkoz]

You want g_idle_add_full actually, not g_idle_add. It's similar to g_signal_connect_data. Only, there we box a closure twice and give Box<Box<Fn()>> as user_data, but here I'm leaning toward Box<RefCell<Box<FnMut>>>: the added RefCell will make sure we don't violate the mutability guarantees.

[vojtechkral]

Right, thanks. I'll have a look at how the signal closures are done...

[vojtechkral]

@gkoz In the gtk signals.rs, I don't understand how Box<Box<Fn>> can be cast as a pointer to function for C... Isn't it a double pointer?

[gkoz]

Note also how the event handlers return the Inhibit newtype on the Rust side to make the meaning of the return boolean explicit. This case calls for a similar type, the glib docs suggest Continue, it could also be Repeat.

[gkoz]

@vojtechkral

In the gtk signals.rs, I don't understand how Box<Box> can be cast as a pointer to function for C... Isn't it a double pointer?

The closures are passed as user_data, any cdecl functions needed -- the callbacks (that in turn call the closures) and the destructor are plain functions.

[vojtechkral]

Okay, I think I get it, will try to implement...

[vojtechkral]

This is the code so far. Problem is if I try to use the FnMut() version instead, I get

error: cannot borrow immutable `Box` content as mutable

in the trampoline function.

PS. I'll add a proper return type later.

[gkoz]

That's weird but can be sidestepped by using the DerefMut trait explicitly: http://is.gd/lOw3HJ, v2: http://is.gd/fqH1kr
Don't forget the 'static bound BTW. I've been guilty of this too. The inner box needs to be Box<FnMut() + 'static>.

[vojtechkral]

It works now, yay :-)

In the future, timeout_func.rs and maybe others would probably benefit from some of the machinery in Idle. Maybe merge? Not sure how to structure this. Also documentation...

[gkoz]

This nested module seems redundant.

[gkoz]

You're importing transmute already so the full path isn't needed.

[gkoz]

We might take a moment to think how this and the timeout functions will work together, the implications for the module name so we don't have to rename it ina week.

[vojtechkral]

Sure, by all means.

[gkoz]

One way to approach this could be to name the module source and the function idle_add, move the timeout::add function into it as timeout_add, and any other g_source_* bindings could be added in the future.

[GuillaumeGomez]

@gkoz: Isn't it possible to just move every non method function in a global namespace (like source if you want) ? It could be nice to give an easy access to "global" gtk functions.

[gkoz]

@GuillaumeGomez This is mostly about the internal organization.

I don't mind reexports in the topmost namespace. I've come to believe that the flat namespace is a necessary evil after all, because that's what other language bindings do and it's very hard to design some structure that will make sense from all points of view. The module might not even be public.

[GuillaumeGomez]

@gkoz: My bad, I thought you wanted to create a file and a namespace for each type of global gtk function. It seemed a bit too much for me. Splitting these functions inside modules is good, we'll just reexport them as one.

I don't think the flat namespace is "evil". However, if you have a better solution in mind, I'd be glad to know it ! ;-)

[gkoz]

My current preference is that the types and functions a user might need would be reexported at the top. (This will likely mean that the constants will have long names with just G_ or GTK_ stripped.) The "internals" stuff used by the bindings (utilities like glib::translate) would not be reexported.

The traits and some essential types would put into prelude and we would recommend glob-importing the prelude only.

[GuillaumeGomez]

Yes, that's what I meant. I think we don't understand each other on this. Nevermind haha.

[vojtechkral]

Naming the module source makes sense, but only if you know GSourceFunc, which I don't think users do, mostly. However, all these functions are part of The Main Event Loop , so maybe naming the module mainloop would make sense? As in mainloop::add_idle(|| { ... }) etc.

Just my two cents

[gkoz]

They call it MainContext mostly though. And it's a bit of a rabbit hole from there. But since we're going to reexport the functions at the crate root, the users won't have to hunt in the source module for them.

Oh, sorry, there is a MainLoop, yes. The point stands, you attach sources to a MainContext, so it's all intertangled.

[vojtechkral]

I can convert the idle module + I suppose the timeout module to the flat model, but I reckon the scope of this PR shouldn't be broader than that right?

[gkoz]

Yeah, the idle_add and a similar reimplementation of timeout_add seem sufficient for now.

[vojtechkral]

Ok, done. Test code also updated. Hope everything's ok.

edit: Sorry for typo in commit msg...

[gkoz]

We drop the G and similar prefixes everywhere.

[gkoz]

Could you also please mark the examples with ignore so cargo test won't complain that they don't compile (or alternatively make them compile)?
http://doc.rust-lang.org/book/documentation.html#running-documentation-tests

[vojtechkral]

Ok, I've added ignore. Making the test compile would make them much longer and I don't even know if it can be done without gtk...

[gkoz]

Good job, thanks!
ping @GuillaumeGomez

[GuillaumeGomez]

@gkoz: Pong ! Thanks for taking care of this PR too !
@vojtechkral: Thanks for your work, I merge !

[vojtechkral]

@gkoz Out of curiosity, what was the reason for the 'static bound again?

[gkoz]

@vojtechkral We pass the ownership of the closure to the library, its lifetime is outside our control, it could outlive everything. There are ways like this to make some lifetime guarantees, but they're not terribly ergonomic either.

[vojtechkral]

@gkoz Okay, thanks!