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

Generate builder for widgets #757

Merged
merged 10 commits into from May 12, 2019

Conversation

Projects
None yet
4 participants
@antoyo
Copy link
Member

commented May 10, 2019

The usage of the generated code looks like:

let button = LockButtonBuilder::new()
    .text_lock("Verrouiller")
    .text_unlock("Déverrouiller")
    .build();

This creates a gtk::LockButton and set the properties text_lock and text_unlock at the widget construction.

The generated code looks like:

#[cfg(any(feature = "builders", feature = "dox"))]
pub struct LockButtonBuilder {
    #[cfg(any(feature = "v3_14", feature = "dox"))]
    text_lock: Option<String>,
    #[cfg(any(feature = "v3_14", feature = "dox"))]
    text_unlock: Option<String>,
    // ...
}

#[cfg(any(feature = "builders", feature = "dox"))]
impl LockButtonBuilder {
    pub fn new() -> Self {
        Self {
            #[cfg(any(feature = "v3_14", feature = "dox"))]
            text_lock: None,
            #[cfg(any(feature = "v3_14", feature = "dox"))]
            text_unlock: None,
            #[cfg(any(feature = "v3_14", feature = "dox"))]
            tooltip_lock: None,
            #[cfg(any(feature = "v3_14", feature = "dox"))]
            tooltip_not_authorized: None,
            #[cfg(any(feature = "v3_14", feature = "dox"))]
            tooltip_unlock: None,
        }
    }
    pub fn build(self) -> LockButton {
        let mut n_properties = 0;
        let mut property_names: Vec<CString> = vec![];
        let mut names = vec![];
        let mut values = vec![];
        #[cfg(any(feature = "v3_14", feature = "dox"))]
        {
        if let Some(text_lock) = self.text_lock {
            property_names.push(CString::new("text-lock").unwrap());
            names.push(property_names[property_names.len() - 1].as_ptr());
            let property = text_lock.to_value();
            values.push(property.into_raw());
            n_properties += 1;
        }
        }
        #[cfg(any(feature = "v3_14", feature = "dox"))]
        {
        if let Some(text_unlock) = self.text_unlock {
            property_names.push(CString::new("text-unlock").unwrap());
            names.push(property_names[property_names.len() - 1].as_ptr());
            let property = text_unlock.to_value();
            values.push(property.into_raw());
            n_properties += 1;
        }
        }
        // ...
        unsafe {
            crate::Object::from_glib_none(gobject_sys::g_object_new_with_properties(
                LockButton::static_type().to_glib(), n_properties, names.as_mut_ptr(), values.as_ptr())
            as *mut _).downcast().expect("downcast")
        }
    }
    #[cfg(any(feature = "v3_14", feature = "dox"))]
    pub fn text_lock(mut self, text_lock: &str) -> Self {
        self.text_lock = Some(text_lock.to_string());
        self
    }
    #[cfg(any(feature = "v3_14", feature = "dox"))]
    pub fn text_unlock(mut self, text_unlock: &str) -> Self {
        self.text_unlock = Some(text_unlock.to_string());
        self
    }
    // ...
}

antoyo added some commits May 10, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 10, 2019

If all fields require the v3_14, then it would be nicer to just set the feature to v3_14 on the struct directly instead.

if analysis.properties.iter().any(|property| property.construct || property.construct_only) {
let mut methods = vec![];
let mut properties = vec![];
writeln!(w, "#[cfg(any(feature = \"builders\", feature = \"dox\"))]")?;

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 10, 2019

Member

You have the right to add empty line so your code can "breath" a bit. ;)

writeln!(w, " let mut n_properties = 0;")?;
writeln!(w, " let mut property_names: Vec<CString> = vec![];")?;
writeln!(w, " let mut names = vec![];")?;
writeln!(w, " let mut values = vec![];")?;

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 10, 2019

Member

One writeln! is enough I think. :)

writeln!(w, " let property = {}.to_value();", name)?;
writeln!(w, " values.push(property.into_raw());")?;
writeln!(w, " n_properties += 1;")?;
writeln!(w, " }}")?;

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 10, 2019

Member

One writeln! to rule them all!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Looks nice, thanks! Few things to update though. ;)

}
}
writeln!(w, " unsafe {{")?;
writeln!(w, " crate::Object::from_glib_none(gobject_sys::g_object_new_with_properties(")?;

This comment has been minimized.

Copy link
@sdroege

sdroege May 10, 2019

Member

Why so complicated? :) glib::Object::new() can do exactly this here for you with only safe code

@@ -109,6 +111,96 @@ pub fn generate(
try!(writeln!(w));
}

if analysis.properties.iter().any(|property| property.construct || property.construct_only) {

This comment has been minimized.

Copy link
@sdroege

sdroege May 10, 2019

Member

So it currently only generates a Builder if there's any construct property. But wouldn't it also make sense for some types to generate it otherwise and provide access to the non-construct, writable properties via the builder?

Another problem is that for some types it is not enough to call g_object_new() with their properties to create a valid instance, but the actual foo_new() function of the specific type has to be called.

And yet another problem I see is that object construction could fail like this if some of the properties must be provided during construction and the default values are not enough. This would end up as a g_critical and then we're in UB country. g_object_new() can't ever fail unfortunately, it must always return an instance... but that instance might be unusable because of this.

There are the GInitable and GAsyncInitable interfaces in gio for that very reason, but they're not widely used.

I think what I'm saying here is that the generation of the builders for now should probably be opt-in per type.

This comment has been minimized.

Copy link
@antoyo

antoyo May 10, 2019

Author Member

Can you give examples of types where g_object_new() is not enough?

You're right, I forgot the properties without default values. For that, I thought I could just add the required properties in the new() method.

This comment has been minimized.

Copy link
@sdroege

sdroege May 10, 2019

Member

Can you give examples of types where g_object_new() is not enough?

GstGhostPad is one I remember currently. Basically everything where the foo_new() functions do more than just calling g_object_new().

You're right, I forgot the properties without default values. For that, I thought I could just add the required properties in the new() method.

All properties have default values (unless they're object/boxed types, in which case the default is always None). But those default values might not be enough for constructing a valid, usable object.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Looks good generally, thanks :)

};
let name = nameutil::mangle_keywords(nameutil::signal_to_snake(&property.name));
if let Some(version) = property.version {
writeln!(w, " #[cfg(any(feature = \"{}\", feature = \"dox\"))]", version.to_feature())?;

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 10, 2019

Member

IMHO version_condition can be used here

writeln!(w, " {}: Option<{}>,", name, attribute_type)?;
let prefix =
if let Some(version) = property.version {
format!(" #[cfg(any(feature = \"{}\", feature = \"dox\"))]\n", version.to_feature())

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 10, 2019

Member

version_condition_string ?

for property in &analysis.properties {
if (!property.is_get && property.construct) || property.construct_only {
generate_builder = true;
break;

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 10, 2019

Member

This not different condition from that used in generate?
Maybe better define separate function here?

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 10, 2019

Member

This function can return iterator filtered by (!property.is_get && property.construct) || property.construct_only,
and it can be used in all 3 places

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Can you move builder code generation to function too

let name = nameutil::mangle_keywords(nameutil::signal_to_snake(&property.name));
version_condition(w, env, property.version, false, 1)?;
writeln!(w, " {}: Option<{}>,", name, type_string)?;
let prefix = version_condition_string(env, property.version, false, 1).unwrap_or_default();

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 11, 2019

Member

As both version string is same you can get it only one time.

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Maybe better add generate_builder: bool to GObject instead?
Anyway Gir.toml format changes required change of README.md too or we will forget about it

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Also config checking IMHO better do on analysis phase

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 12, 2019

@antoyo Thanks.
I don't like way of configure, but lets be it.

@EPashkin EPashkin merged commit 6a2bf39 into gtk-rs:master May 12, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 12, 2019

@EPashkin Why merging if you didn't agree with it? We could have discussed about it you know. :) (we still can discuss about it)

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I don't want stall #759
Current version don't generate anything new without direct order,
so we can postpone discussion without any problem.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Sure but then let's not forget about it!

@EPashkin

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@antoyo There problem with parent properties, even if builder not generated many use's added from them.

@EPashkin EPashkin referenced this pull request May 14, 2019

Merged

Fix builders #764

@antoyo antoyo referenced this pull request May 14, 2019

Closed

Generate builders #286

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.