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

Clean up glib_wrapper! macro and IsA<_>, Wrapper traits and their impls #421

Merged
merged 18 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@sdroege
Copy link
Member

sdroege commented Jan 7, 2019

This PR is still WIP as it also requires a corresponding PR for gir. The changes are

  • Rename Wrapper trait to ObjectType to be more descriptive and add more useful API around it
  • glib_wrapper! does not need FFI types for the parent classes in other crates anymore
  • Downcast trait became CanDowncast, and the Cast trait impl was cleaned up
  • Generate Rust class structs for class types in glib_wrapper!, and distinguish between classes and interfaces
  • IsA<_> is cleaned up and does not require ToGlibPtr impls for everything anymore (that's why FFI types were needed). Instead IsA<T> requires AsRef<T>, i.e. we can always get back to a &T which then has all the traits we care about implemented.
  • IsA<Object> and ObjectType (former Wrapper) trait bounds are equivalent, i.e. IsA<Object> is implemented for every ObjectType. They can be used interchangeably.

sdroege added some commits Dec 16, 2018

Clean up Cast trait and rename Downcast<T> to CanDowncast<T>
All functionality of the Downcast trait is now directly part of the Cast
trait, and downcast_unchecked() was replaced with a generic unsafe_cast().
Rename UnsafeFrom::from() to unsafe_from() for clarity
Otherwise its callers can easily be confused with From::from()
Move UnsafeFrom and Wrapper to the object module, and rename Wrapper …
…to ObjectType

They are both specific to objects so let's have them stay in the correct
module and rename Wrapper to something more descriptive. It's a trait
implemented by Object types.
Remove IsA<Object> trait bound from various places
This allows using casts and all ObjectExt functions in any function that is generic over IsA<T>.

And also add an AsRef<T> bound to IsA<T> for getting a reference to the
underlying object.
Add all kinds of trait bounds for standard traits we implement to Obj…
…ectType

This allows using them all in generic functions without specifying them
one by one, e.g. to clone objects.
Remove all the duplicated ToGlibPtr impls for every single super-class
We can get those directly via the AsRef<T> impl on IsA<T> in all places
where it's needed, and this is the only reason why we currently have to
pass in the FFI types of all superclasses too.
Distinguish Objects and Interfaces in glib_wrapper!
Also require to always provide the Rust class name as we now implement
that automatically for classes, but not for interfaces.

Interfaces on the other hand always require an interface FFI name to be
given, but we don't do anything with that yet.
Implement IsA<Object> for every ObjectType
A trait bound of ObjectType should be equivalent with IsA<Object>.

As a side-effect we now have to manually implement IsA<Self> in
glib_wrapper!() but that also ensures that IsA<String> for example is
not automatically implemented, and things are overall cleaner.
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 7, 2019

Seems some impl AsRef<$name> is missing:

error[E0277]: the trait bound `auto::fontset_simple::FontsetSimple: std::convert::AsRef<glib::Object>` is not satisfied
  --> D:/eap/rust/0/pango\src\auto\fontset_simple.rs:13:1
   |
13 | / glib_wrapper! {
14 | |     pub struct FontsetSimple(Object<ffi::PangoFontsetSimple, ffi::PangoFontsetSimpleClass, FontsetSimpleClass>): Fontset;
15 | |
16 | |     match fn {
17 | |         get_type => || ffi::pango_fontset_simple_get_type(),
18 | |     }
19 | | }
   | |_^ the trait `std::convert::AsRef<glib::Object>` is not implemented for `auto::fontset_simple::FontsetSimple`
   |
   = help: the following implementations were found:
             <auto::fontset_simple::FontsetSimple as std::convert::AsRef<auto::fontset::Fontset>>
             <auto::fontset_simple::FontsetSimple as std::convert::AsRef<auto::fontset_simple::FontsetSimple>>
             <auto::fontset_simple::FontsetSimple as std::convert::AsRef<glib::object::ObjectRef>>
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 7, 2019

Good thing that Interface has its tag, but seems it lost it FFI link. Or it don't need anymore?

+++ b/src/auto/editable.rs
@@ -10,7 +10,7 @@ use std::fmt;
 use std::mem;
 
 glib_wrapper! {
-    pub struct Editable(Object<ffi::GtkEditable, ffi::GtkEditableInterface>);
+    pub struct Editable(Interface<ffi::GtkEditable>);
@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 7, 2019

Good thing that Interface has its tag, but seems it lost it FFI link. Or it don't need anymore?

For the class/iface struct that's not needed anymore. For interfaces there is currently nothing the macro could do with that.

From doing the bindings parts for implementing interfaces, I don't think anything will be needed here either. See e.g. https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/blob/master/gstreamer/src/subclass/uri_handler.rs

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 7, 2019

Thanks for explanation.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 8, 2019

Seems some impl AsRef<$name> is missing:

In some cases the macro does not expand to the part that implements AsRef<Object>, yes.

So generally we have another problem now though, that needs some further changes. We assume that the first parent type given is the direct parent class. Which is fine except for types that inherit directly from glib::Object (which is always omitted!) and implement an interface.

I think we'll need to always list glib::Object explicitly now. But that should be fine and nicer anyway as it makes this more explicit.

Will change it accordingly.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 8, 2019

Will change it accordingly.

Which is not trivial because we have to special-case glib::Object in the unwrapping of the parent types. Maybe the macro should distinguish extends (classes) and implements (interfaces) like in Java.

glib_wrapper! {
     pub struct Foo(Object<ffi::Foo, ffi::FooClass>) extends Bar, Baz, implements Meh;
     [...]

or similar.

@GuillaumeGomez @EPashkin do you have any opinions?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 8, 2019

IMHO extends/implements is better that direct adding glib::Object

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 8, 2019

Not sure that will solve problem with object pango::FontsetSimple, it has only one parent object Fontset that don't have parents at all.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 8, 2019

Not sure that will solve problem with object pango::FontsetSimple

It doesn't, that's a different problem but I'll fix that at the same time as the other one I mentioned above :) Same code paths in the macro need refactoring.

it has only one parent object Fontset that don't have parents at all.

Well, it has glib::Object as parent. Everything does :)

Distinguish between interfaces, parent classes and prerequisites in g…
…lib_wrapper!

This allows us to reliably generate a class type that can be cast to the
direct parent class, and it is simply more explicit.

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from 9a0103b to 6a6d3cb Jan 9, 2019

Don't let every ObjectType implement IsA<Object> automatically
This would then require type annotations in functions with IsA<_> bounds
when calling as_ref(), which is rather suboptimal.

@sdroege sdroege changed the title [WIP] Clean up glib_wrapper! macro and IsA<_>, Wrapper traits and their impls Clean up glib_wrapper! macro and IsA<_>, Wrapper traits and their impls Jan 14, 2019

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 14, 2019

This is now ready too

/// Objects -- classes and interfaces. Note that the class name, if
/// available, must be specified after the $foreign type; see below
/// for [non-derivable classes][#non-derivable-classes].
/// Objects -- classes. Note that the class name, if available, must be specified after the

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 15, 2019

Member

Two whitespaces in classes. Note. Is it expected? Otherwise please remove one or start a new paragraph.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 15, 2019

Looks good to me. Just a little nit but it's just a detail for let's just merge for now. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit caef186 into gtk-rs:master Jan 15, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
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.