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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 24, 2024
Merged

gtk/templates: Add missing traits for TemplateChild #1870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0bb45a
gtk/templates: Add missing traits for TemplateChild
bilelmoussaoui Oct 18, 2024
9d76133
macros: Fix blueprint warnings
bilelmoussaoui Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ yuraiz:
```rust
#[derive(Debug, Default, gtk::CompositeTemplate)]
#[template(string = "
template MyWidget : Widget {
template $MyWidget : Widget {
Label label {
label: 'foobar';
}
Expand Down
2 changes: 1 addition & 1 deletion gtk4-macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ pub fn include_blueprint(input: TokenStream) -> TokenStream {
///
/// #[derive(Debug, Default, gtk::CompositeTemplate)]
/// #[template(string = "
/// template MyWidget : Widget {
/// template $MyWidget : Widget {
/// Label label {
/// label: 'foobar';
/// }
Expand Down
2 changes: 1 addition & 1 deletion gtk4-macros/tests/my_widget.blp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
template MyWidget5 {
template $MyWidget5 {
Label label {
label: 'foobar';
}
Expand Down
49 changes: 48 additions & 1 deletion gtk4-macros/tests/templates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ mod imp4 {

#[derive(Debug, Default, gtk::CompositeTemplate)]
#[template(string = "
template MyWidget4 : Widget {
template $MyWidget4 : Widget {
Label label {
label: 'foobar';
}
Expand Down Expand Up @@ -303,3 +303,50 @@ glib::wrapper! {
fn blueprint_file() {
let _: MyWidget5 = glib::Object::new();
}

mod imp6 {
use super::*;

#[derive(Default, glib::Properties, gtk::CompositeTemplate)]
#[template(string = r#"
<?xml version="1.0" encoding="UTF-8"?>
<interface>
<template class="TestWidget" parent="GtkWidget">
<child>
<object class="GtkWidget" id="widget" />
</child>
</template>
</interface>
"#)]
#[properties(wrapper_type = super::TestWidget)]
pub struct TestWidget {
#[property(get)]
#[template_child]
widget: gtk::TemplateChild<gtk::Widget>,
}

#[glib::object_subclass]
impl ObjectSubclass for TestWidget {
const NAME: &'static str = "TestWidget";
type Type = super::TestWidget;
type ParentType = gtk::Widget;

fn class_init(klass: &mut Self::Class) {
klass.bind_template();
}

fn instance_init(obj: &glib::subclass::InitializingObject<Self>) {
obj.init_template();
}
}

#[glib::derived_properties]
impl ObjectImpl for TestWidget {}
impl WidgetImpl for TestWidget {}
impl TestWidget {}
}

glib::wrapper! {
pub struct TestWidget(ObjectSubclass<imp6::TestWidget>)
@extends gtk::Widget;
}
55 changes: 54 additions & 1 deletion gtk4/src/subclass/widget.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,12 +1211,13 @@ pub unsafe trait WidgetClassExt: ClassStruct {
unsafe impl<T: ClassStruct> WidgetClassExt for T where T::Type: WidgetImpl {}

#[derive(Debug, PartialEq, Eq)]
#[repr(transparent)]
Copy link
Member Author

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

Copy link
Member

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 :)

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

@bilelmoussaoui bilelmoussaoui Oct 24, 2024 •
edited
Loading

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?

#[repr(C)]
pub struct TemplateChild<T>
where
T: ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
{
ptr: *mut <T as ObjectType>::GlibType,
should_drop: bool,
}

impl<T> Default for TemplateChild<T>
Expand All @@ -1228,6 +1229,7 @@ where

Self {
ptr: std::ptr::null_mut(),
should_drop: false,
}
}
}
Expand All @@ -1245,6 +1247,44 @@ where
}
}

impl<T> ToValue for TemplateChild<T>
where
T: ToValue + ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
{
#[inline]
fn to_value(&self) -> glib::Value {
T::to_value(&self.get())
}

#[inline]
fn value_type(&self) -> glib::Type {
T::static_type()
}
}

impl<T> glib::value::ValueType for TemplateChild<T>
where
T: glib::value::ValueType + ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
{
type Type = <T as glib::value::ValueType>::Type;
}

unsafe impl<'a, T> glib::value::FromValue<'a> for TemplateChild<T>
where
T: ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
{
type Checker = glib::value::GenericValueTypeChecker<T>;

#[inline]
unsafe fn from_value(value: &'a glib::Value) -> Self {
skip_assert_initialized!();
TemplateChild {
ptr: T::from_value(value).into_glib_ptr(),
should_drop: true,
}
}
}

impl<T> std::ops::Deref for TemplateChild<T>
where
T: ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
Expand Down Expand Up @@ -1274,6 +1314,19 @@ where
}
}

impl<T> Drop for TemplateChild<T>
where
T: ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
{
fn drop(&mut self) {
if self.should_drop {
unsafe {
crate::glib::gobject_ffi::g_object_unref(self.ptr as *mut _);
}
}
}
}

impl<T> TemplateChild<T>
where
T: ObjectType + FromGlibPtrNone<*mut <T as ObjectType>::GlibType>,
Expand Down
Loading