Inconsistent definitions of constants?

[WebDust21]

Introduce the problem

It appears to me that some LVGL constants are bound to the Lua interface as integers, and others are bound as strings.

From the example provided in the main page of the repository:

-- flex layout and align
root:set {
    flex = {
        flex_direction = "row",
        flex_wrap = "wrap",
        justify_content = "center",
        align_items = "center",
        align_content = "center",
    },
    w = 300,
    h = 75,
    align = lvgl.ALIGN.CENTER
}

Notice that all the "flex" properties are set as strings.
Meanwhile, the "align" (next to last line) is set as a named constant.

I can perhaps see where use of strings could have an advantage in cases where multiple LVGL constants can be "stacked together" as flags for a specific outcome. However, when the constants are part of a sequential enum (i.e. not "binary stackable flags"), use of strings seems rather odd.

Another potential usecase could stem from trying to differentiate between multiple different constants with the same name, e.g. "center"--although there should be ways around that, e.g. "lvgl.FLEX_ALIGN.CENTER". Possibly harder would be getting the linter to suggest from "FLEX_ALIGN" on those properties over just "ALIGN"...?

In source, here is where the "align" constants are defined:

luavgl/src/constants.c

Lines 125 to 150 in d25acfb

	static const rotable_Reg align_const_table[] = {
	{.name = "DEFAULT", .integer = LV_ALIGN_DEFAULT },
	{.name = "TOP_LEFT", .integer = LV_ALIGN_TOP_LEFT },
	{.name = "TOP_MID", .integer = LV_ALIGN_TOP_MID },
	{.name = "TOP_RIGHT", .integer = LV_ALIGN_TOP_RIGHT },
	{.name = "BOTTOM_LEFT", .integer = LV_ALIGN_BOTTOM_LEFT },
	{.name = "BOTTOM_MID", .integer = LV_ALIGN_BOTTOM_MID },
	{.name = "BOTTOM_RIGHT", .integer = LV_ALIGN_BOTTOM_RIGHT },
	{.name = "LEFT_MID", .integer = LV_ALIGN_LEFT_MID },
	{.name = "RIGHT_MID", .integer = LV_ALIGN_RIGHT_MID },
	{.name = "CENTER", .integer = LV_ALIGN_CENTER },
	{.name = "OUT_TOP_LEFT", .integer = LV_ALIGN_OUT_TOP_LEFT },
	{.name = "OUT_TOP_MID", .integer = LV_ALIGN_OUT_TOP_MID },
	{.name = "OUT_TOP_RIGHT", .integer = LV_ALIGN_OUT_TOP_RIGHT },
	{.name = "OUT_BOTTOM_LEFT", .integer = LV_ALIGN_OUT_BOTTOM_LEFT },
	{.name = "OUT_BOTTOM_MID", .integer = LV_ALIGN_OUT_BOTTOM_MID },
	{.name = "OUT_BOTTOM_RIGHT", .integer = LV_ALIGN_OUT_BOTTOM_RIGHT},
	{.name = "OUT_LEFT_TOP", .integer = LV_ALIGN_OUT_LEFT_TOP },
	{.name = "OUT_LEFT_MID", .integer = LV_ALIGN_OUT_LEFT_MID },
	{.name = "OUT_LEFT_BOTTOM", .integer = LV_ALIGN_OUT_LEFT_BOTTOM },
	{.name = "OUT_RIGHT_TOP", .integer = LV_ALIGN_OUT_RIGHT_TOP },
	{.name = "OUT_RIGHT_MID", .integer = LV_ALIGN_OUT_RIGHT_MID },
	{.name = "OUT_RIGHT_BOTTOM", .integer = LV_ALIGN_OUT_RIGHT_BOTTOM},
	{0, 0 },
	};

It's a nice and straightforward dictionary of integers, linked to Lua here:

luavgl/src/constants.c

Lines 612 to 619 in d25acfb

	static void luavgl_constants_init(lua_State *L)
	{
	rotable_setfiled(L, -2, "EVENT", event_const_table);
	rotable_setfiled(L, -2, "FLAG", obj_flag_const_table);
	rotable_setfiled(L, -2, "STATE", state_const_table);
	rotable_setfiled(L, -2, "PART", part_const_table);
	rotable_setfiled(L, -2, "ALIGN", align_const_table);

callback assigned here:

luavgl/src/obj.c

Lines 872 to 878 in d25acfb

	static const luavgl_property_ops_t obj_property_ops[] = {
	{.name = "align", .ops = obj_property_align },
	{.name = "id", .ops = obj_property_id },
	{.name = "h", .ops = obj_property_h },
	{.name = "w", .ops = obj_property_w },
	{.name = "user_data", .ops = obj_property_user_data},
	};

and easily handled on the receiving end here:

luavgl/src/obj.c

Lines 803 to 811 in d25acfb

	static int obj_property_align(lua_State *L, lv_obj_t *obj, bool set)
	{
	if (set) {
	if (lua_isinteger(L, -1)) {
	lv_obj_align(obj, lua_tointeger(L, -1), 0, 0);
	return 0;
	}
	if (!lua_istable(L, -1)) {

(code goes on further to allow other entries as well for more specific aligning if a Lua table is provided)

Conversely, in "luavgl_to_flex_align" (among several others), it's just a list of sequential strcmps with the goal of getting the corresponding integer constant:

luavgl/src/style.c

Lines 196 to 221 in d25acfb

	static lv_flex_align_t luavgl_to_flex_align(lua_State *L, int idx)
	{
	if (lua_type(L, idx) != LUA_TSTRING)
	return LV_FLEX_ALIGN_START;
	const char *str = lua_tostring(L, idx);
	if (lv_strcmp("flex-start", str) == 0)
	return LV_FLEX_ALIGN_START;
	if (lv_strcmp("flex-end", str) == 0)
	return LV_FLEX_ALIGN_END;
	if (lv_strcmp("center", str) == 0)
	return LV_FLEX_ALIGN_CENTER;
	if (lv_strcmp("space-evenly", str) == 0)
	return LV_FLEX_ALIGN_SPACE_EVENLY;
	if (lv_strcmp("space-around", str) == 0)
	return LV_FLEX_ALIGN_SPACE_AROUND;
	if (lv_strcmp("space-between", str) == 0)
	return LV_FLEX_ALIGN_SPACE_BETWEEN;
	return LV_FLEX_ALIGN_START;
	}

Note that with "flex_direction", strstr is used to allow "-reverse" as a flag keyword; however, as there are only 4 options, I still think this would better be handled with an integer constant. Besides, Lua allows adding together defined values:

luavgl/src/style.c

Lines 256 to 274 in d25acfb

	/**
	* flex-direction:
	* row | row-reverse | column | column-reverse
	*/
	lua_getfield(L, -1, "flex_direction");
	if (lua_type(L, -1) == LUA_TSTRING) {
	str = lua_tostring(L, -1);
	/* starts with */
	if (lv_strcmp("row", str) == 0) {
	flow = LV_FLEX_FLOW_ROW;
	} else if (lv_strcmp("column", str) == 0) {
	flow = LV_FLEX_FLOW_COLUMN;
	}
	/* if reverse presents */
	if (luavgl_strstr(str, "-reverse")) {
	flow |= LV_FLEX_REVERSE;
	}
	}

All that to say...I am not well versed in Lua, so I could be missing something obvious.
But: Is there a specific reason for the use of strings when binding LVGL constants to Lua?

Proposal

Port all "strcmp" sequential argument parsing to named constants?

[WebDust21]

It appears that this might be a WIP based on this? 369240c

[XuNeo]

Sorry for the late reply. I missed this message.

But: Is there a specific reason for the use of strings when binding LVGL constants to Lua?

There are some exceptions for the non-direct translation for constants like flex parameters. I was considering to follow css style.

The color is another example, so we can set color directly using "#RGB" or "#RRGGBB". This is handy in my opinion.

It's a nice and straightforward dictionary of integers

But I agree with you too. Use the straightforward translation of LVGL constants minimize the learning cost. So I agree to keep both methods.

It appears that this might be a WIP based on this? 369240c

This PR is to reduce RAM usage by moving the contants in lua to flash.

[WebDust21]

The color is another example, so we can set color directly using "#RGB" or "#RRGGBB". This is handy in my opinion.

Yes, I've noticed that, pretty elegant.

There are some exceptions for the non-direct translation for constants like flex parameters. I was considering to follow css style.

Understood, this can make sense.

I was referring to code where it is just doing set of sequential "strcmp" commands, to return a single LVGL constant--this does not make any sense.

Anyway, I have been making huge modifications to the project, adding support for the rest of the LVGL widgets. (I have done some fundamental revamps, particularly with the build system. Only ".h" files are #included now; ".c" files should be handled by the makefile--same as LVGL and Lua for that matter). Once I get all the widget support completed, is this something you would like to continue on this repo, or should I host it separately?

[XuNeo]

doing set of sequential "strcmp" commands, to return a single LVGL constant

That can be optimized using similar methods to other LVGL constants. I haven't got time to refine it yet.

Only ".h" files are #included now; ".c" files should be handled by the makefile

I got the idea from luv(libuv lua binding). But using LVGL's style is OK too.

would like to continue on this repo, or should I host it separately

It would be great if you could contribute to this repo. I had been discussing to host this repo under LVGL, so more developers can benefit from it. #49

There's issue #69 needs to be addressed firstly.

I have been making huge modifications to the project.

If possible, could you send the code in batches for review?

[WebDust21]

If possible, could you send the code in batches for review?

It's such a fundamental revamp that I'm not sure "batching" is going to be feasible. With the changes, plus adding all of the other LVGL widgets...the "src" folder has gone from the original 236KB to a current 381KB.

The main problem with #including C source files, is the inherent sequential inheritance. Dependency circles can very quickly turn into an impossible quagmire--which is why guarded C header files are generally used for interconnection of multiple source files.