Use property() & implement Abstract Base Class PangoObject

[cAttte]

hey! this PR closes #44. i ended up going with property() since i completely agree that it ties the methods better, buuut i decided to keep @property for read-only properties, since otherwise it's unnecessarily verbose.

while writing this simple-ish PR, though, i realized that there was a lot of repetition between every single class, as they all need to interact with the underlying C APIs and pointers... so i created an abstract base class: PangoObject. this class implements the following methods/members, which removes a lot of repetition:

• __init__()
• pointer
• _init_pointer()
• from_pointer()
• copy() / _copy_() / __deepcopy_()
• __eq__()
• __repr__()

it works by utilizing these abstract properties, which are supposed to be provided by its subclasses:

• _INIT_METHOD -- a CFFI function that initializes the object (i.e. pango.pango_..._new()), alternatively, ffi.new
• _INIT_CLASS -- only if the method is set to ffi.new, a string representing the classname to initialize (e.g. "PangoLayout")
• _GC_METHOD -- a CFFI function that frees the object (i.e. pango.pango_..._free())
• _COPY_METHOD -- a CFFI function that copies the object (i.e. pango.pango_..._copy())
• _EQ_METHOD -- a CFFI function that compares the object with another (i.e. pango.pango_..._equals())

all of these are optional, and every base method is enabled or modified depending on these (i.e., __init__() will error if there's no _INIT_... properties; gc: bool will be disabled if there's no _GC_METHOD; comparisons (=) will directly use pointers if an _EQ_METHOD isn't provided, etc.) of course, these base methods can still be overwritten like in the case of Color#__eq__().

i'm sorry if this is kind of a sudden change, but it would've been pointless to make a PR that basically rewrites every class, to then make another one that rewrites them over again, and the current structure really needed a base class like this one. i believe the overall concept is good, but please let me know if there are any implementation details you'd like to change.

---

i updated all of the tests to conform to the changes and they should all be passing, but for some reason i'm geting seemingly random memory management errors (Access violation / Segmentation fault) only when running the test suite (if i run the same functions from a simple test file nothing happens), and the errors happen on code that's basically untouched (get_iter() -> pango_layout_get_iter; apply_markup() -> pango_layout_set_markup). i'll try to see what's causing it because i'm not even sure if it has to do with my machine or what.

[leifgehrmann]

First of all, great work! I do like the PangoObject refactoring.

I've ran the GitHub Actions just to see if the tests would run. Looks like there's a few issues that need to be fixed.

• Annoyingly the default installation of Ubuntu in GitHub actions uses a version of Pango older than 1.46.0. That version of Pango doesn't appear to have pango_attr_list_equal, which means the CFFI will raise an AttributeError when accessed at runtime. By moving that reference to the top of the file in _EQ_METHOD = pango.pango_attr_list_equal, it now raises an AttributeError whenever the pangocffi package is imported (Because Attribute is imported by pangocffi/__init__.py). Two workarounds I can think of at the moment are:
  • Upgrade the version of Pango used in the GitHub action, and drop support for version of Pango <1.46.0 (Though 1.46.0 was released only 2 years ago, so forcing an upgrade to use this package maybe isn't a good idea)
  • Remove _EQ_METHOD for Attribute and instead override the __eq__ method directly. That way, the AttributeError will only be raised when == is used. (and the tests will pass because they take that into account)
• A few linting issues in pango_object.py and test_color.py.

(On another note, I really should split linting and tests into two different GitHub Action workflows)

I've also checked out the code on my machine as well and I can replicate the segfault errors you mentioned. I've not looked too closely at the new pointer logic, but I'm only guessing it's because a PangoObject is being freed when it's not supposed to be.

Unrelated to the SegFault stuff, I also noticed you fixed a typo made by a contributor in the color.py class. Specifically, in the __eq__ method, where and self.red == self.red should have been and self.blue == other.blue. Thanks for spotting that! But strangely, when I run pytest tests/functional/test_color.py::TestColor::test_copy, the assertion appears to fail because apparently the property blue is 0, instead of 5... Curious if you get the same issue? I wonder if pango_color_copy is broken. For reference, I'm using Pango v1.50.10.

[cAttte]

yeah, i saw the attr_list_equal error in the Actions and that's why i removed _EQ_METHOD from AttrList, but i guess overriding the method directly like you say is a better option, so i'll do that in a second. being that Pango v1 is over 20 years old, yeah, we should probably update it, even if not to 1.46.

i'll try to see if i can figure out why the segfault is happening, even though i don't have a lot of experience with C haha; the pointer logic really shouldn't be any different from the pre-existing classes, but it's probably a small detail that i missed. i'll also fix the linting issues; i ran make lint and fixed everything but i guess i must've changed something without realizing!

i noticed the weird error when copying a Color, but i forgot to mention it. it does seem like pango_color_copy might be broken, because i remember testing the FFI function directly and the object i got back was wrong. i'll try doing some more experimenting though. if it somehow does end up being broken we could just manually copy the RGB attributes to a new pointer? (and file an issue with pango!).

i also see another test case that asserts that a NotImplementedError is raised in Color(...) == 1, i assume i.e. whenever a Color is compared to a non-object... i had changed PangoObject#__eq__() so that it wouldn't error and simply return False; should i revert the change or remove the failing test case?

[cAttte]

some updates:

• i overwrote __eq__(), and i changed it so it wouldn't error on an old version; just check if the comparison method exists, and if it doesn't just compare the pointers (we should probably somehow make it so they compare the attributes instead, like Color.__eq__() does)
• i still don't know what causes the Color copy issue, but the following code reproduces it:

from pangocffi import ffi, pango

color = ffi.new("PangoColor *")
color.red = 55
color.green = 155
color.blue = 255

copy = pango.pango_color_copy(color)
print(color.blue, copy.blue) # 255, 0

in addition to that, other methods like pango_color_parse also return a blue component of 0, but if we generate a string representation of the color, it looks like it's still present?:

spec = ffi.new("char[]", "#102030".encode("utf8"))
new_color = ffi.new("PangoColor *")
pango.pango_color_parse(new_color, spec)
print(new_color.blue, ffi.string(pango.pango_color_to_string(new_color))) # 0, b"#101020203030"

• the segmentation fault seems to only ever present in Layout; specifically:
  • apply_markup() (2nd line);
  • get_extents() (3rd line);
  • get_iter() (1st line)

if i comment these function calls out (e.g. pango.pango_layout_set_markup()), no segfault occurs (but of course the tests still fail because now these three methods return incorrect values). i tried commenting out _GC_METHOD to see if disabling GC would do something, but it didn't.

[leifgehrmann]

i noticed the weird error when copying a Color, but i forgot to mention it. it does seem like pango_color_copy might be broken, because i remember testing the FFI function directly and the object i got back was wrong. i'll try doing some more experimenting though. if it somehow does end up being broken we could just manually copy the RGB attributes to a new pointer? (and file an issue with pango!).

Yeah, I think manually creating a new Color object and copying the values is a reasonable solution for the short term. I looked at Pango's source code and I do see that they have a test for this, so it's a little strange for this to happen. I'll investigate it later and raise an issue with Pango if there is an issue.

i also see another test case that asserts that a NotImplementedError is raised in Color(...) == 1, i assume i.e. whenever a Color is compared to a non-object... i had changed PangoObject#__eq__() so that it wouldn't error and simply return False; should i revert the change or remove the failing test case?

Yeah, I think you are right, returning false should be fine. Just update the test to check Color(...) == 1 is False, that way code coverage is maintained.

[leifgehrmann]

Hi there! I spent some time trying to find the SegFault error and I believe I found the issue.

Basically it appears the _GC_METHOD for color.py and glyph_item_iter.py are not required. Don't ask me why it's not required, but that's the way I set it up originally in master apparently.

I've also left some comments suggesting some changes which should fix some of the other tests and also increase code coverage a tiny bit.

There are also a few lines of code that need changing, but annoyingly GitHub doesn't allow me to comment on them directly. So here's a list:

• In test_attributes.py modify line 75 to read markup_texts = ('pango.apply_markup(\'...\')', 'Attribute.from_shape()') (i.e. s/set_markup/apply_markup)
• In test_attr_list.py modify line 72 to read assert a != 2
• In test_attribute.py modify line 263 to read assert a != 6

All these changes pass on my machine, so hopefully it'll pass in the GitHub Actions as well.

[cAttte]

thank you so much for finding the issue! i spent some time messing with the classes and GC methods but it was pretty confusing so i couldn't get it to work, and i haven't had a lot of time during the week.

i'd actually implemented the three small fixes you said you couldn't comment on, but completely forgot to commit/push them! also, i fixed the copy method, but directly modified Color.copy() instead, since it's already called by __copy__() and __deepcopy__() up in PangoObject. thanks for mentioning the rest!

i hope everything's alright now, and i'm very sorry for the messy PR and amount of commits. the segfault when testing really hindered my ability to... you know... test the code, so i kind of just trusted myself. i'll make sure to be more careful when contributing further :)

[leifgehrmann]

Ran the checks again... Looks like one more change is required. (Hopefully 😅)

I think testing AttrList for equality by checking the pointers when using older versions of Pango is a bit confusing. __eq__ raising an AttributeError is fine, and is ultimately more honest to the user. So I'd change it to:

    def __eq__(self, other) -> bool:
        if isinstance(other, PangoObject):
            return bool(
                pango.pango_attr_list_equal(self.pointer, other.pointer)
            )
        return False

The alternative is to change the test to assert False if Pango <1.46.0 is running, and True if Pango >=1.46, but that seems a little hacky in opinion.

[cAttte]

you're right! while thinking of the concept of __eq__() i realized it's kind of "unfair" how some objects get their attributes compared individually, while others simply compare the pointers, which isn't really how it should work at all. i'll change it right now, but it does come to mind that PangoObject does the same thing:

if self._EQ_METHOD:
    return bool(self._EQ_METHOD(self.pointer, other.pointer))
else:
    return self.pointer == other.pointer

and considering that most classes don't have an _EQ_METHOD, this is true basically everywhere. should we override this in every class, find a way to make it declarable, or leave it as is? (or perhaps you only meant that, in the case of AttrList, the behavior is inconsistent, not that it's necessarily wrong everywhere else)

[codecov]

Codecov Report

Base: 99.27% // Head: 99.25% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (d80c0de) compared to base (2a56048).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   99.27%   99.25%   -0.02%     
==========================================
  Files          18       19       +1     
  Lines         965      940      -25     
==========================================
- Hits          958      933      -25     
  Misses          7        7              

Impacted Files	Coverage Δ
pangocffi/__init__.py	85.71% <100.00%> (+0.34%)	⬆️
pangocffi/attr_list.py	100.00% <100.00%> (ø)
pangocffi/attribute.py	100.00% <100.00%> (ø)
pangocffi/color.py	100.00% <100.00%> (ø)
pangocffi/context.py	100.00% <100.00%> (ø)
pangocffi/font_description.py	100.00% <100.00%> (ø)
pangocffi/glyph_item.py	100.00% <100.00%> (ø)
pangocffi/glyph_item_iter.py	100.00% <100.00%> (ø)
pangocffi/item.py	100.00% <100.00%> (ø)
pangocffi/layout.py	100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[leifgehrmann]

but it does come to mind that PangoObject does the same thing:

Hmm, I think this can all be improved by flipping the order of operations. In other words, check for pointer equality first, then use the _EQ_METHOD. So replace update PangoObject to have:

    def __eq__(self, other) -> bool:
        if isinstance(other, PangoObject):
            if self.pointer == other.pointer:
                return True
            if self._EQ_METHOD:
                return bool(self._EQ_METHOD(self.pointer, other.pointer))
        return False

and remove the override if __eq__ in attr_list.py and add _EQ_METHOD = pango.pango_attr_list_equal at the top.

That way, all PangoObjects correctly do pointer comparisons regardless of whether the object has _EQ_METHOD, and if they are different, fallback to the _EQ_METHOD. And if the _EQ_METHOD fails because of the raised AttributeError in old versions of Pango, it's up to the user to either upgrade their version of Pango to get real object comparisons or avoid doing object comparisons in the first place.

---

Other than that, I think this is good for merging. Coverage is going down by a tiny percentage, but that's okay. At the end of the day it's still 99% 😂

[cAttte]

sure, that makes sense! but won't adding _EQ_METHOD back to the top of the file crash outdated versions on start-up again?

[leifgehrmann]

Yep, you're right, I forgot about that! 🤦

So yeah, keep the override in attr_list.py, but reorder the conditionals to check pointer equality first in both implementations of __eq__().

[cAttte]

coverage is now -0.01% :')

[leifgehrmann]

I think a lower code coverage was always inevitable because you refactored redundant code neatly into pango_object.py. 😄

But this all looks great. I'll get this merged in a few minutes (unless you have any last minute additions) and I'll prepare a new release today if possible.

I'll also be updating pangocairocffi soon as well (Got a PR prepared here: leifgehrmann/pangocairocffi#29)

Thanks again for this contribution, I really appreciate the amount of work that went the refactoring all the objects into PangoObject!

[cAttte]

thank you too! this was a nice experience and i'll probably make a PR to implement more classes from the Pango API soon. :)