cbrunet / python-poppler

Python binding to Poppler-cpp pdf library
GNU General Public License v2.0
95 stars 15 forks source link

Rotation Enum is very inconsistent and does not match the actual poppler-cpp #42

Closed mara004 closed 3 years ago

mara004 commented 3 years ago

As of the time of writing, the Rotation Enum of python-poppler looks like this

poppler.Rotation.rotate_0
poppler.Rotation.rotate_90
poppler.Rotation.rotate18_0
poppler.Rotation.rotate27_0

which can be confirmed with

>>> import poppler
>>> vars(poppler.Rotation)
mappingproxy({'__init__': <instancemethod __init__ at 0x7f2f00e265b0>, '__doc__': <pybind11_builtins.pybind11_static_property object at 0x7f2f00dfa4f0>, '__module__': 'poppler.cpp.global_', '__entries': {'rotate_0': (rotation_enum.rotate_0, None), 'rotate_90': (rotation_enum.rotate_90, None), 'rotate18_0': (rotation_enum.rotate18_0, None), 'rotate27_0': (rotation_enum.rotate27_0, None)}, '__repr__': <instancemethod  at 0x7f2f00e262b0>, 'name': <property object at 0x7f2f00dfa630>, '__members__': <pybind11_builtins.pybind11_static_property object at 0x7f2f00dfa450>, '__eq__': <instancemethod  at 0x7f2f00e26490>, '__ne__': <instancemethod  at 0x7f2f00e264f0>, '__getstate__': <instancemethod  at 0x7f2f00e26550>, '__hash__': <instancemethod  at 0x7f2f00e26550>, '__int__': <instancemethod __int__ at 0x7f2f00e26610>, '__index__': <instancemethod __index__ at 0x7f2f00e26670>, '__setstate__': <instancemethod  at 0x7f2f00e266d0>, 'rotate_0': rotation_enum.rotate_0, 'rotate_90': rotation_enum.rotate_90, 'rotate18_0': rotation_enum.rotate18_0, 'rotate27_0': rotation_enum.rotate27_0})

The position of the underscore is inconsistent and a potential cause of errors for users who don't look closely (also since there is no note in the documentation concerning the Enum's actual attributes). I think most people would expect all attributes to start with rotate_, rather than awkwardly having the underscore inside the rotation number for 180 and 270 degrees.

I think the Enum should be restructured to this

poppler.Rotation.rotate_0
poppler.Rotation.rotate_90
poppler.Rotation.rotate_180
poppler.Rotation.rotate_270
mara004 commented 3 years ago

For reference, in the actual poppler-cpp the position of the underscore is consistent: https://poppler.freedesktop.org/api/cpp/namespacepoppler.html#adfe4efbd3ec91ddf17ead7cf52cbbdea

mara004 commented 3 years ago

I think this is the code block that is responsible for how the Rotation Enum looks like: https://github.com/cbrunet/python-poppler/blob/84aae664ddf7b1e86ac5ce3bc1c5e0240a01308b/src/cpp/global.cpp#L68

    py::enum_<rotation_enum>(m, "rotation_enum")
        .value("rotate_0", rotation_enum::rotate_0)
        .value("rotate_90", rotation_enum::rotate_90)
        .value("rotate18_0", rotation_enum::rotate_180)
        .value("rotate27_0", rotation_enum::rotate_270)
        .export_values();

I plan to submit a PR soon.