lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
237 stars 156 forks source link

Spurious enum being generated? #199

Closed embeddedt closed 1 year ago

embeddedt commented 2 years ago

This isn't anything major, but I stumbled across this while using autocompletion on the online simulator. Apparently its type is:

>>> lv.OBJ_FLAG_FLEX_IN_NEW
<class 'ENUM_LV_OBJ_FLAG_FLEX_IN_NEW'>

There is no enum (or macro, for that matter) with this name. The closest I can find is this but that would mean the name is cut off.

I'm assuming the name of the macro is confusing the binding generator because it shares a prefix with the lv_obj_flag_t enum.

amirgon commented 2 years ago

The closest I can find is this but that would mean the name is cut off.

It's not really cut off: lv.OBJ_FLAG_FLEX_IN_NEW.TRACK
But I agree it's weird.

The reason is the way LV_EXPORT_CONST_INT is defined: https://github.com/lvgl/lv_binding_micropython/blob/e7ae68a0714fecee03f6c7e6842a5d1981cdba78/lv_conf.h#L298-L300

It's actually creating an enum whose only member is ENUM_LV_OBJ_FLAG_FLEX_IN_NEW_TRACK.

I think I can improve that by checking whether the enum consists of only a single element, and in such case bypass the logic that finds the common prefix of all enum elements.

amirgon commented 2 years ago

Actually, after looking at the code and refreshing my memory - it's a bit more complicated than that.

In case of "regular" enums - we find the common prefix and use it as the enum name. This is the simple case. For example, LV_PART_MAIN --> lv.PART.MAIN.

But there are more subtle cases:

What I can do, is to identify enums with a single element that start with ENUM_ (hints the usage of LV_EXPORT_CONST_INT) and convert them into regular integer constants instead of an enum.

That would affect the following cases:

ENUM_LV_DPI.DEF
ENUM_LV_ANIM_REPEAT.INFINITE
ENUM_LV_ANIM_PLAYTIME.INFINITE
ENUM_LV_COLOR.DEPTH
ENUM_LV_COLOR_16.SWAP
ENUM_LV_SIZE.CONTENT
ENUM_LV_IMG_ZOOM.NONE
ENUM_LV_RADIUS.CIRCLE
ENUM_LV_LABEL_DOT.NUM
ENUM_LV_LABEL_POS.LAST
ENUM_LV_LABEL_TEXT_SELECTION.OFF
ENUM_LV_TABLE_CELL.NONE
ENUM_LV_BTNMATRIX_BTN.NONE
ENUM_LV_DROPDOWN_POS.LAST
ENUM_LV_TEXTAREA_CURSOR.LAST
ENUM_LV_CHART_POINT.NONE
ENUM_LV_OBJ_FLAG_FLEX_IN_NEW.TRACK
ENUM_LV_GRID.CONTENT
ENUM_LV_GRID_TEMPLATE.LAST

I think it might make sense to convert all of these into integer constants (such that they can be referred to by lv.OBJ_FLAG_FLEX_IN_NEW_TRACK for example) but on the other hand that would break backwards compatibility if anyone is using any of them today.

What do you you think?

embeddedt commented 2 years ago

Backwards-compatibility is an issue, yes. Can we manually provide the old name as well for old scripts? I don't know if that's feasible for you to do or not, and it would add some extra noise to the autocompletion.

amirgon commented 2 years ago

Can we manually provide the old name as well for old scripts?

We can, it's not hard, but I'm not sure that's a good idea.
It's a bit confusing to have both lv.OBJ_FLAG_FLEX_IN_NEW with a TRACK member and lv.OBJ_FLAG_FLEX_IN_NEW_TRACK with the same value as TRACK, don't you think? It's not just a matter of code completion - it makes the API even less clear when we have such duplications. I'd rather wait for the next major release, break backwards compatibility and just convert them.