-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
gtk/templates: Add missing traits for TemplateChild #1870
Conversation
66717ef
to
21cf62c
Compare
21cf62c
to
d1393c8
Compare
@@ -1211,12 +1211,12 @@ pub unsafe trait WidgetClassExt: ClassStruct { | |||
unsafe impl<T: ClassStruct> WidgetClassExt for T where T::Type: WidgetImpl {} | |||
|
|||
#[derive(Debug, PartialEq, Eq)] | |||
#[repr(transparent)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove the repr(transparent) as the struct contains an extra field now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope nothing was relying on it being the same representation as T
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this needs to be repr-C now so that we at least guarantee that the pointer is stored at the beginning? How is GtkBuilder filling this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this needs to be repr-C now so that we at least guarantee that the pointer is stored at the beginning? How is GtkBuilder filling this?
By calling gtk_widget_class_bind_template_child_full, which is used from rust with bind_template_child_with_offset. But yes, maybe it should be repr-C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it currently works with the offset to TemplateChild
so you need to make it repr-C at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, let us wait and see if @felinira got whatever use case they have working with this patch first. I tested it locally by changing one of the examples but still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does fix #1842 for me, if that's what you wanted to know :)
I'm a little surprised why this needs a FromValue implementation though. Isn't this readonly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised why this needs a FromValue implementation though. Isn't this readonly?
The requirement seems to come from ObjectExt::property
fn property<V: for<'b> FromValue<'b> + 'static>(&self, property_name: &str) -> V;
So it is expected? not sure why you are surprised about it, unless you meant the ToValue?
d1393c8
to
9d76133
Compare
As this fixes the issue, let us land it already, any further fixes could be done separately |
Fixes #1842
Only compile tested
cc @felinira @sdroege