jeremyletang / rgtk

GTK+ bindings and wrappers for Rust (DEPRECATED SEE https://github.com/rust-gnome )
GNU Lesser General Public License v3.0
120 stars 22 forks source link

RFC: Casts and traits reform #197

Closed gkoz closed 9 years ago

gkoz commented 9 years ago

This is not intended for merging, the only thing that works is the basic example.

Summary

Generalize the traits and casts system. Replace different structs corresponding to Gtk objects with a single struct Envelope that takes a native Gtk struct as a parameter. Add Cast and Downcast traits for converting between pointers to different native Gtk structs. Use these traits to express their hierarchical relationships.

Details

The macro-generated Cast implementations in gtk::ffi take care of correct up- and downcasting avoiding the need for gtk_glue. We declare static guarantees that upcasting will work.

cast_impl!(C_GtkButton, C_GtkBin, C_GtkContainer, C_GtkWidget, C_GObject);

Downcasting can be checked dynamically, but overriding the checks is possible too. This allowed to change e.g.

ffi::gtk_container_add(GTK_CONTAINER(self.get_widget()), widget.get_widget());

to

ffi::gtk_container_add(self.as_ptr().cast(), widget.as_ptr().cast());

The casting framework allows implementing the traits generically for all descending classes without redundantly specifying the relationships by hand. Implement Envelope generically instead of using impl macros.

impl <T> Clone for Envelope<T>
where T: GetGType, *mut T: Cast<C_GObject> {
    fn clone(&self) -> Envelope<T> {
        FromPtr::from_borrowed_ptr(self.as_ptr())
    }
}

Make all methods take &self because the borrow checker can't and shouldn't be expected to protect the underlying GTK objects from aliasing. Note that GTK isn't thread safe.

The constructors aren't supposed to return NULL so Option isn't used there. TODO: null checks for getting the pointers from other functions and NotNull...

Fix refcounting properly. All of Gtk objects are descendants of GInitiallyUnowned and when taking ownership we should call g_object_ref_sink. The more usual GObjects could be either owned by us (when returned from a constructor) or borrowed ("transfer_none"). FromPtr accommodates each case.

The traits and structs in this example live directly under the gtk module because I've found no benefit to splitting out constructors into the widgets module and separate structs for each class aren't really needed.

gkoz commented 9 years ago

Maybe I'm wrong about caring for mutability: we operate refcounted aliased pointers so might as well make all methods take &self.

gkoz commented 9 years ago

Added Envelope<T> and updated the RFC text for the latest improved design.

jeremyletang commented 9 years ago

Hey @gkoz !

Thanks for your stuff ! And sorry to answer so late, i'm a bit out of the game these times.

These are great changes, and for sure will be merge.

The constructors aren't supposed to return NULL so Option isn't used there

I just read the Gtk doc, and they say that object constructors cannot fail, so you're right we can remove the use of Option.

Maybe I'm wrong about caring for mutability: we operate refcounted aliased pointers so might as well make all methods take &self.

About this, we don't need mutability for everything, but it should reflect the behaviour of the function.

For example:

bx.add(&mut cb);

cb should probably not be mutable, but bx should, as we can think that adding cb to bx is probably a mutable operation.

For the other stuff, the design seems great, this is something i want to do, but I can't find the time to do it.

I just will take some time to test it, and read the code before merging ( when it will be finished ).

gkoz commented 9 years ago

Thanks for the comments! I'll probably be able to do the conversion, after getting the -sys crates out of the way.

Regarding mutability - at least nothing in the API or its reference instructs if a particular method should be mutable or not. So it seems the decisions would just be arbitrary. Consider that &mut isn't exactly about mutability, but about the reference not being aliased. I believe what we're getting from the GTK functions are aliased pointers (and GTK expects them to be used as such) so prohibiting aliasing further down the line isn't meaningful. There hardly is a way to guarantee that out &mut borrow really is the only reference to a widget. And as long as one can clone an object the restriction doesn't work anyway.

We need to make sure not to make any of the widgets Send though :)

That said, the design doesn't hinge on this point but having fn as_ptr(&self) -> *mut GtkSomething reduces the boilerplate factor (you can see the monstrous constructs in the earlier commits before I dropped the mut).

gkoz commented 9 years ago

One thing that bothers me is that assuming glib-sys, gtk-sys, glib and gtk are in separate crates, the coherence rules might require to define Cast and its impls in the -sys crates which clashes with the idea of them basically being the translations of C headers. Maybe casts could be expressed in terms of Envelope<GtkStruct> instead of plain GtkStruct though? ... No, that's not enough.