Allow to get Color from Analysis #168

romanzes · 2019-11-08T03:15:18Z

The idea is to use it like this:

let attrs = analysis.extra_attrs();
for attr in attrs {
    let class = attr.get_attr_class();
    let attr_type = class.get_type();
    match attr_type {
        AttrType::Foreground => {
            let color_attr = pango::AttrColor::from(attr);
            let color = color_attr.get_color();
            println!("red color {:?}", color.red());
        }
        _ => {}
    }
}

sdroege · 2019-11-08T07:39:33Z

src/attr_color.rs

+
+impl PartialEq for AttrColor {
+    fn eq(&self, other: &AttrColor) -> bool {
+        self.0 == other.0


This should probably call pango_attr_equal()

And a Drop impl that calls pango_attr_destroy() should probably exist too, and a Clone impl that calls pango_attr_copy()

I think the attrs should probably be implemented like the patterns in cairo. The generic pango_attr API works on all of them and then there are "subclasses" of the generic PangoAttr type.

@sdroege would you be able to provide me with some links to the code that I could use as references? Especially to the patterns in cairo. Thanks.

Take a look here: https://github.com/gtk-rs/cairo/blob/master/src/patterns.rs

There's a base Pattern type and specific struct FooPattern(Pattern) that provide specific API. Pattern impls Drop for memory management, and there are try_from() and deref() impls to get from one to another.

This still applies to the PR and requires some more work

@GuillaumeGomez See above, that's the main missing part here. Let's just defer that to a future release, it involves quite some work and figuring out a nice API. I wouldn't want to merge that just before a release but rather give people some time to try it first.

More like a Widgetand not Button. ;) From my point of view, we should merge this and implement what's left in the next release.

It's useless as it is and will only cause bug reports from people trying to figure out how to use it and not understanding :) And worst case we introduce a new actual bug here that slipped through the review.

I would rather wait until after the release, there's no advantage in having this in this release already and only the risk of introducing a new bug.

You mean except being able to create the attributes which we cannot currently? :p

It seems it's more of a "create and never modify" API from what I saw. So I guess it's mostly fine. Also, where could they get bugs? We don't access to anything...

What do you think about this @EPashkin ?

Waiting for his opinion then :) Maybe I'm just a bit too strict here.

GuillaumeGomez · 2019-11-29T23:09:42Z

Need a hand in here?

GuillaumeGomez · 2019-11-30T15:48:21Z

Ok, I updated the code. The Attribute API is a nightmare...

sdroege · 2019-12-01T08:13:28Z

src/attribute.rs

+use glib::translate::from_glib_full;
+use glib::translate::ToGlib;
+use glib::translate::ToGlibPtr;
+
 impl Attribute {
    #[cfg(any(feature = "v1_38", feature = "dox"))]
    pub fn new_background_alpha(alpha: u16) -> Option<Attribute> {


This (and all the others too) should probably return an AttributeBackgrondAlpha?

No, it's all casted as Attribute in the source code. Also, it's not really supposed to be "used" so better not give access to the fields.

sdroege · 2019-12-01T15:16:26Z

src/attribute.rs


    pub fn new_weight(weight: Weight) -> Option<Attribute> {
        unsafe { from_glib_full(pango_sys::pango_attr_weight_new(weight.to_glib())) }
    }
-
-    pub fn get_attr_class(&self) -> AttrClass {


Why remove this?

Went too quickly I think... Putting it back!

sdroege · 2019-12-01T15:16:39Z

src/attribute.rs

-        }
-    }
-
-    pub fn get_end_index(&self) -> u32 {


These two getters at least seem useful, the setters maybe not

Indeed. Don't remember why I removed them...

sdroege · 2019-12-01T15:17:23Z

src/attribute.rs

-            let stash = self.to_glib_none_mut();
-            (*stash.0).end_index = index;
-        }
-    }


Should implement Clone via pango_attribute_copy(), Drop via pango_attribute_destroy(), PartialEq/Eq via pango_attribute_equal()

Look at the auto impl maybe?

Ah I thought there's only the manual impl of it. Ok then

GuillaumeGomez · 2019-12-01T15:24:25Z

Updated.

EPashkin · 2019-12-02T14:05:39Z

I tried to look at code again after long time,
and now I can't find way to "get Color from Analysis" 😢
so I prefer postpone PR

GuillaumeGomez · 2019-12-02T14:09:26Z

You can't, that's the whole point. But at least you can now create all available attributes.

EPashkin · 2019-12-02T14:26:16Z

Then maybe remove changes in analysis.rs and Gir.toml.
Or user can do something significant with Vec<Attribute>?

This we have constructors and minimal get/set for Attribute (API really strange)
and getters for PangoColor.

sdroege · 2019-12-02T17:33:10Z

That's a good point. The PR as it is now does not even implement what the title says :)

romanzes mentioned this pull request Nov 8, 2019

Analysis::color() velyan/pango#1

Closed

sdroege reviewed Nov 8, 2019

View reviewed changes

sdroege mentioned this pull request Nov 29, 2019

New release gtk-rs/release#94

Closed

43 tasks

Velislava Yanchina and others added 2 commits November 30, 2019 15:31

bind

bccc15b

impl Color

44eca9d

GuillaumeGomez force-pushed the rp-vel-color branch from 3b6bc0f to 385dcec Compare November 30, 2019 15:47

GuillaumeGomez force-pushed the rp-vel-color branch from 385dcec to 52886eb Compare November 30, 2019 19:13

sdroege reviewed Dec 1, 2019

View reviewed changes

GuillaumeGomez force-pushed the rp-vel-color branch from 52886eb to 6796ab7 Compare December 1, 2019 15:23

Improve Attribute API

82d96e9

GuillaumeGomez force-pushed the rp-vel-color branch from 6796ab7 to 82d96e9 Compare December 1, 2019 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to get Color from Analysis #168

Allow to get Color from Analysis #168

romanzes commented Nov 8, 2019

sdroege Nov 8, 2019

romanzes Nov 10, 2019

sdroege Nov 11, 2019

sdroege Dec 1, 2019

sdroege Dec 1, 2019

GuillaumeGomez Dec 1, 2019

sdroege Dec 2, 2019

GuillaumeGomez Dec 2, 2019

GuillaumeGomez Dec 2, 2019

sdroege Dec 2, 2019

GuillaumeGomez commented Nov 29, 2019

GuillaumeGomez commented Nov 30, 2019

sdroege Dec 1, 2019

GuillaumeGomez Dec 1, 2019

sdroege Dec 1, 2019

sdroege Dec 1, 2019

GuillaumeGomez Dec 1, 2019

sdroege Dec 1, 2019

GuillaumeGomez Dec 1, 2019

sdroege Dec 1, 2019

GuillaumeGomez Dec 1, 2019

sdroege Dec 1, 2019

GuillaumeGomez commented Dec 1, 2019

EPashkin commented Dec 2, 2019

GuillaumeGomez commented Dec 2, 2019

EPashkin commented Dec 2, 2019

sdroege commented Dec 2, 2019

Allow to get Color from Analysis #168

Are you sure you want to change the base?

Allow to get Color from Analysis #168

Conversation

romanzes commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 29, 2019

GuillaumeGomez commented Nov 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 1, 2019

EPashkin commented Dec 2, 2019

GuillaumeGomez commented Dec 2, 2019

EPashkin commented Dec 2, 2019

sdroege commented Dec 2, 2019