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

Alternate rgba implementation #140

Merged
merged 3 commits into from Mar 11, 2017

Conversation

Projects
None yet
3 participants
@Susurrus
Contributor

Susurrus commented Mar 7, 2017

That takes into account all the conversation over in #139 and implements RGBA as a custom struct. The end-use usability between this and #139 is vastly improved. Basically every function supported by RGBA corresponds to an existing trait in Rust.

I've done some testing of some of the various code here as I have GTK working with these changes and my own application using both. I don't fully understand the memory management done by GTK for the GdkRGBA struct behind the scenes but I haven't run into crashes here.

Do note that there is one outstanding issue here and that's the error type to use for the FromStr trait. Since the GTK interface just uses an Option type but this trait uses Result I had to make up an error type. It looks like Glib has some error types but none of them looked appropriate. I think it's good to implement this trait but I don't think this is the best way to do the error type. Anyone have any ideas?

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 7, 2017

Contributor

I just realized that I didn't implement copy or free like the GTK interface has. I don't know if they're necessary, as like I said above I don't really get the memory management strategy used by GTK and how gtk-rs interfaces with it. But I'll leave it for now until I get some feedback from you guys.

Contributor

Susurrus commented Mar 7, 2017

I just realized that I didn't implement copy or free like the GTK interface has. I don't know if they're necessary, as like I said above I don't really get the memory management strategy used by GTK and how gtk-rs interfaces with it. But I'll leave it for now until I get some feedback from you guys.

Show outdated Hide outdated src/rgba.rs
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

with this Gtks ColorChooserExt.addPalette again looks like

ffi::gtk_color_chooser_add_palette(self.to_glib_none().0, orientation.to_glib(),
    colors_per_line, colors.len() as c_int, colors.as_ptr() as *mut gdk_ffi::GdkRGBA)

and both function of StyleContext need minor change to use
let mut rgba = gdk::RGBA::uninitialized(); that don't needed from_glib_xx conversion

Member

EPashkin commented Mar 8, 2017

with this Gtks ColorChooserExt.addPalette again looks like

ffi::gtk_color_chooser_add_palette(self.to_glib_none().0, orientation.to_glib(),
    colors_per_line, colors.len() as c_int, colors.as_ptr() as *mut gdk_ffi::GdkRGBA)

and both function of StyleContext need minor change to use
let mut rgba = gdk::RGBA::uninitialized(); that don't needed from_glib_xx conversion

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

About usage copy, free: IMHO copy better just ignored
and maybe need add FromGlibPtrFull just in case.
It need call gdk_rgba_free after get local copy of RGBA

Member

EPashkin commented Mar 8, 2017

About usage copy, free: IMHO copy better just ignored
and maybe need add FromGlibPtrFull just in case.
It need call gdk_rgba_free after get local copy of RGBA

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

also cc @GuillaumeGomez
Seems I found way to build all gtk generated code when enabling RGBA.
Currently don't see way to move it to glib due conflicting implementation errors,
so gtk need additional dependence gobject-sys

impl glib::StaticType for RGBA {
    fn static_type() -> glib::types::Type {
        unsafe { from_glib(ffi::gdk_rgba_get_type()) }
    }
}

impl glib::value::FromValueOptional for RGBA {
    unsafe fn from_value_optional(value: &glib::Value) -> Option<Self> {
        from_glib_full(gobject_ffi::g_value_dup_boxed(value.to_glib_none().0) as *mut ffi::GdkRGBA)
    }
}

impl glib::value::SetValue for RGBA {
    unsafe fn set_value(value: &mut glib::Value, this: &Self)  {
        gobject_ffi::g_value_set_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as gconstpointer)
    }
}

impl glib::value::SetValueOptional for RGBA {
    unsafe fn set_value_optional(value: &mut glib::Value, this: Option<&Self>) {
        gobject_ffi::g_value_set_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as gconstpointer)
    }
}
Member

EPashkin commented Mar 8, 2017

also cc @GuillaumeGomez
Seems I found way to build all gtk generated code when enabling RGBA.
Currently don't see way to move it to glib due conflicting implementation errors,
so gtk need additional dependence gobject-sys

impl glib::StaticType for RGBA {
    fn static_type() -> glib::types::Type {
        unsafe { from_glib(ffi::gdk_rgba_get_type()) }
    }
}

impl glib::value::FromValueOptional for RGBA {
    unsafe fn from_value_optional(value: &glib::Value) -> Option<Self> {
        from_glib_full(gobject_ffi::g_value_dup_boxed(value.to_glib_none().0) as *mut ffi::GdkRGBA)
    }
}

impl glib::value::SetValue for RGBA {
    unsafe fn set_value(value: &mut glib::Value, this: &Self)  {
        gobject_ffi::g_value_set_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as gconstpointer)
    }
}

impl glib::value::SetValueOptional for RGBA {
    unsafe fn set_value_optional(value: &mut glib::Value, this: Option<&Self>) {
        gobject_ffi::g_value_set_boxed(value.to_glib_none_mut().0, this.to_glib_none().0 as gconstpointer)
    }
}
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 8, 2017

Member
unsafe fn set_value(value: &mut glib::Value, this: &Self)  {

Shouldn't Self be first?

Member

GuillaumeGomez commented Mar 8, 2017

unsafe fn set_value(value: &mut glib::Value, this: &Self)  {

Shouldn't Self be first?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

This as defined in glib:

/// Sets a value.
pub trait SetValue: StaticType {
    unsafe fn set_value(&mut Value, &Self);
}

may to be same as

pub trait SetValueOptional: SetValue {
    unsafe fn set_value_optional(&mut Value, Option<&Self>);
}
Member

EPashkin commented Mar 8, 2017

This as defined in glib:

/// Sets a value.
pub trait SetValue: StaticType {
    unsafe fn set_value(&mut Value, &Self);
}

may to be same as

pub trait SetValueOptional: SetValue {
    unsafe fn set_value_optional(&mut Value, Option<&Self>);
}
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 8, 2017

Member

Hum ok, too bad...

Member

GuillaumeGomez commented Mar 8, 2017

Hum ok, too bad...

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 8, 2017

Contributor

So what needs to be done to wrap up this PR? From your comments it looks like:

  1. Reorder trait implementations
  2. Implement FromGlibPtrFull
  3. Add glib trait impl shown in #140 (comment)

Is that it?

Contributor

Susurrus commented Mar 8, 2017

So what needs to be done to wrap up this PR? From your comments it looks like:

  1. Reorder trait implementations
  2. Implement FromGlibPtrFull
  3. Add glib trait impl shown in #140 (comment)

Is that it?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

For me it seems complete list.

Member

EPashkin commented Mar 8, 2017

For me it seems complete list.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 8, 2017

Contributor

Alright, that should all be implemented. Local code that I have is still working correctly. I also updated gtk-rs/gtk#462 so that it's now dependent on this PR.

Contributor

Susurrus commented Mar 8, 2017

Alright, that should all be implemented. Local code that I have is still working correctly. I also updated gtk-rs/gtk#462 so that it's now dependent on this PR.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

@Susurrus, Thanks
I have minor worry about FromStr trait error, but prefer skip it now.

@GuillaumeGomez, what you think: this PR or #139 ?

Member

EPashkin commented Mar 8, 2017

@Susurrus, Thanks
I have minor worry about FromStr trait error, but prefer skip it now.

@GuillaumeGomez, what you think: this PR or #139 ?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 8, 2017

Member

Code seems ok to me but CIs errors seem legit.

Member

GuillaumeGomez commented Mar 8, 2017

Code seems ok to me but CIs errors seem legit.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

Whose?

Member

EPashkin commented Mar 8, 2017

Whose?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 8, 2017

Member

check_init

Member

GuillaumeGomez commented Mar 8, 2017

check_init

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 8, 2017

Member

@GuillaumeGomez you right.
@Susurrus, all these functions need skip_assert_initialized!();

$ ./check_init_asserts
src/rgba.rs:black() -> RGBA 
src/rgba.rs:blue() -> RGBA 
src/rgba.rs:green() -> RGBA 
src/rgba.rs:red() -> RGBA 
src/rgba.rs:white() -> RGBA 
src/rgba.rs:static_type() -> glib::types::Type
Member

EPashkin commented Mar 8, 2017

@GuillaumeGomez you right.
@Susurrus, all these functions need skip_assert_initialized!();

$ ./check_init_asserts
src/rgba.rs:black() -> RGBA 
src/rgba.rs:blue() -> RGBA 
src/rgba.rs:green() -> RGBA 
src/rgba.rs:red() -> RGBA 
src/rgba.rs:white() -> RGBA 
src/rgba.rs:static_type() -> glib::types::Type
@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 8, 2017

Contributor

Whoops, forgot I left those out and meant to ask about what functions actually needed them. Good catch!

So I've pushed a new version with fixes.

Contributor

Susurrus commented Mar 8, 2017

Whoops, forgot I left those out and meant to ask about what functions actually needed them. Good catch!

So I've pushed a new version with fixes.

@Susurrus

This comment has been minimized.

Show comment
Hide comment
@Susurrus

Susurrus Mar 8, 2017

Contributor

I pushed another version that fixes a minor style issue and also exports the RgbaParseError type.

Contributor

Susurrus commented Mar 8, 2017

I pushed another version that fixes a minor style issue and also exports the RgbaParseError type.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Mar 11, 2017

Member

Ping

Member

EPashkin commented Mar 11, 2017

Ping

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 11, 2017

Member

Oups, forgot about it... Thanks!

Member

GuillaumeGomez commented Mar 11, 2017

Oups, forgot about it... Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 14b2ca0 into gtk-rs:master Mar 11, 2017

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