-
-
Notifications
You must be signed in to change notification settings - Fork 82
Generate GtkStyleContext get property functions #876
base: master
Are you sure you want to change the base?
Conversation
These were ignored previously. Fixes https://github.com/gtk-rs/gtk/issues/874
👍 |
@@ -180,6 +183,8 @@ pub trait StyleContextExt: 'static { | |||
|
|||
//fn get_style(&self, : /*Unknown conversion*//*Unimplemented*/Fundamental: VarArgs); | |||
|
|||
fn get_style_property(&self, property_name: &str, value: &mut glib::Value); |
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.
Ah that's why it was ignored probably... this should return an Option<glib::Value>
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.
Not sure why gir gets the other function right and this one not though...
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.
@EPashkin any guess? :)
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.
IMHO because parameter not marked as out
<parameter name="value" transfer-ownership="none">
<doc xml:space="preserve" filename="gtk/gtkstylecontext.c" line="1727">Return location for the property value</doc>
<type name="GObject.Value" c:type="GValue*"/>
</parameter>
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.
That could be fixed with some addition to gir-files/fix.sh
:)
@@ -170,6 +171,8 @@ pub trait StyleContextExt: 'static { | |||
|
|||
fn get_path(&self) -> Option<WidgetPath>; | |||
|
|||
fn get_property(&self, property: &str, state: StateFlags) -> glib::Value; |
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.
And also this should be Option
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 can probably be configured in the toml by marking the return type as nullable
gtk_sys::gtk_style_context_get_style_property( | ||
self.as_ref().to_glib_none().0, | ||
property_name.to_glib_none().0, | ||
value.to_glib_none_mut().0, |
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.
May be this function better be manual as
fn get_style_property(&self, property_name: &str) -> glib::Value
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.
not sure that it can return Option
, same as gtk_style_context_get_property
at minimum by docs
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.
What would it return if the property name is invalid though?
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.
Docs don't say that in both cases :(
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.
Code says g_critical()
, so we would cause memory unsafety here. We need to check the existence of the property beforehand somehow... like in Object::get_property()
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.
We could have manual code that initializes the GValue
with mem::zeroed()
and afterwards checks if the type of it is still 0.
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.
Looking into it, this function is deprecated (though not documented as such). Maybe we could ignore this one, and fix get_property
?
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.
Sure, works for me. Since when is the other function deprecated?
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.
get_property
isn't deprecated, and I'm not sure when this one was deprecated. Someone mentioned it in IRC as deprecated, and sure enough it isn't in the GTK4 docs
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 they meant that it's deprecated as in "does not exist in GTK4 anymore".
I think for the "deprecated" one the way forward is to panic if it fails (the g_critical()
from C is basically a panic), or otherwise make it return a Result
.
The other one, if we can check beforehand then let's do that or otherwise like above.
How should we move forward here? @BrainBlasted ? |
These were ignored previously.
Fixes https://github.com/gtk-rs/gtk/issues/874