parameter types of functions in lv_obj_style_gen.h are incorrect

[kdschlosser]

Introduce the problem

static inline int32_t lv_obj_get_style_width(const lv_obj_t * obj, uint32_t part)
{
    lv_style_value_t v = lv_obj_get_style_prop(obj, part, LV_STYLE_WIDTH);
    return (int32_t)v.num;
}

the function that gets the property is correctly declared

lv_style_value_t lv_obj_get_style_prop(const lv_obj_t * obj, lv_part_t part, lv_style_prop_t prop);

Proposal

Should be

static inline int32_t lv_obj_get_style_width(lv_obj_t * obj, lv_part_t part)
{
    lv_style_value_t v = lv_obj_get_style_prop(obj, part, LV_STYLE_WIDTH);
    return (int32_t)v.num;
}

[liamHowatt]

Thanks! PR opened.

Just to verify, did you mean to remove the const in your "should be" section?

[kdschlosser]

I am not 100% sure if the const is incorrect. I believe it is, I would have to check the underlying functions that get called to see. What is really wrong is the uint32_t part. While this will work because that is what the part is actually typed to it causes an incomplete connection between the function parameter and the actual enumeration when the documentation gets compiled. uint32_t part should be lv_part_t part.. You also have selector that might be typed incorrectly. The underlying functions also need to be checked to make sure they are typed correctly.

[liamHowatt]

Seems to be const all the way down.

You also have selector that might be typed incorrectly

I see some inconsistencies with trans_delete now that you point it out. It's hard to tell exactly what's to be corrected.

@kisvegabor should this function's part param be a selector? Or should its invocations be passing parts, here and here? Thanks.

[kdschlosser]

Most of those functions are wrapper functions. so if the parameter gets directly passed to the function it wraps then go and look at the types for the underlying function they should align. If they do not then they need to be corrected.

Lets look at this function.

void lv_obj_add_style(lv_obj_t * obj, const lv_style_t * style, lv_style_selector_t selector);

specifically lv_style_selector_t selector

that parameter actually can take lv_state_t, lv_part_t and lv_style_selector_t or a combination of those OR'ed together.

This is what those 3 types actually are

typedef uint32_t lv_part_t;
typedef uint16_t lv_state_t;
typedef uint32_t lv_style_selector_t;

for the purposes of the documentation this is what is done in order to get the connection between the type and the enumerations

#ifdef DOXYGEN
typedef _lv_state_t lv_state_t;
typedef _lv_part_t lv_part_t;
typedef _lv_obj_flag_t lv_obj_flag_t;
#else
typedef uint16_t lv_state_t;
typedef uint32_t lv_part_t;
typedef uint32_t lv_obj_flag_t;
#endif /*DOXYGEN*/

There are enums for the part and the state but oddly enough there is no enum for lv_style_selector_t. There is also no connection from the typedef for lv_style_selector_t to any enum like you see in the above code block.

enum _lv_part_t {
    LV_PART_MAIN         = 0x000000,   /**< A background like rectangle*/
    LV_PART_SCROLLBAR    = 0x010000,   /**< The scrollbar(s)*/
    LV_PART_INDICATOR    = 0x020000,   /**< Indicator, e.g. for slider, bar, switch, or the tick box of the checkbox*/
    LV_PART_KNOB         = 0x030000,   /**< Like handle to grab to adjust the value*/
    LV_PART_SELECTED     = 0x040000,   /**< Indicate the currently selected option or section*/
    LV_PART_ITEMS        = 0x050000,   /**< Used if the widget has multiple similar elements (e.g. table cells)*/
    LV_PART_CURSOR       = 0x060000,   /**< Mark a specific place e.g. for text area's cursor or on a chart*/

    LV_PART_CUSTOM_FIRST = 0x080000,    /**< Extension point for custom widgets*/

    LV_PART_ANY          = 0x0F0000,    /**< Special value can be used in some functions to target all parts*/
};

enum _lv_state_t {
    LV_STATE_DEFAULT     =  0x0000,
    LV_STATE_CHECKED     =  0x0001,
    LV_STATE_FOCUSED     =  0x0002,
    LV_STATE_FOCUS_KEY   =  0x0004,
    LV_STATE_EDITED      =  0x0008,
    LV_STATE_HOVERED     =  0x0010,
    LV_STATE_PRESSED     =  0x0020,
    LV_STATE_SCROLLED    =  0x0040,
    LV_STATE_DISABLED    =  0x0080,
    LV_STATE_USER_1      =  0x1000,
    LV_STATE_USER_2      =  0x2000,
    LV_STATE_USER_3      =  0x4000,
    LV_STATE_USER_4      =  0x8000,

    LV_STATE_ANY = 0xFFFF,    /**< Special value can be used in some functions to target all states*/
};

The question now becomes how does one go about creating a type that links to 3 different types so the documentation is able to make a proper connection to the 3 different enums.

That is a question that I am not able to answer and someone with more knowledge in C code should be able to. I don't know if we can do it using types or if it is only able to be done by changing the documentation and expressly stating that information. There would not be able to be a link that goes from the type to the enum in the docs which would not be ideal either.

[kdschlosser]

What I have though is that there is no need to have these enumerations spread across 3 different enums. they should all be in a single enum. they would retain their names as not to change the API at the user end of things. It would make no difference to the user what a parameter is typed to only the name of the enumeration is what would matter.

The micropython code can be altered to handle this so it can correctly create the same API..

[kdschlosser]

From a documentation standpoint the styles and how to work with them is quite ugly, no way to make the proper connections between things.

[kisvegabor]

for the purposes of the documentation this is what is done in order to get the connection between the type and the enumerations

Here I've commented about that we should probably drop the practice of typedef uint32_t some_enum_t. It should help with the docs too.

[kdschlosser]

The issue is using enumerations from 3 separate enums and OR'ing them together, There is no way to type the 3 enums into a single type and there is no way to link the parameter type to each of the 3 enums to have the documentation link together correctly.

lv_part_t, lv_state_t and lv_style_selector_t are the 3 enums that are the issue.

If the enumerations in those enums were in a single enum this would be a non issue.

[kisvegabor]

If added all parts and states to lv_style_selector_t, would it work well in MicroPython? I'm concerned because in lv_style_selector_t the elements should look like LV_STYLE_SELECTOR_..., right?

[kdschlosser]

we need to make a decision with regards to the MicroPython API. personally I find it rather cumbersome to use because it doesn't follow the C API. The lack of documentation make that all kinds of worse. The enumerations should not be done like they are, The pythonic way of handling constants are them being at the module level and not nested which is what is currently done.

There is a thing called pep8 for Python. This is a standard that aims at normalizing how people write Python code. The hope with it is to not have code that is written with a slew of different styles like what is seen in C code (which is getting better). One of the things that pep 8 addresses is integer constants...

https://peps.python.org/pep-0008/#constants

Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

They are summed up in a single line. It states usually done at the module level and they are supposed to be named like they are already done in LVGL.

The nested class structure that has been done is not how it should have been done. If we flatten that out and drop all the enumerations to the module level there will be no issues with mixing the names inside of an enumeration. It also becomes easier to use because the names don't get all kinds of mangles with the user typically having to guess where the name has to be split with a "."

as an example.

instead of

lv.obj.FLAG.HIDDEN
lv.obj.FLAG.CLICKABLE

you would end up with

lv.OBJ_FLAG_HIDDEN
lv.OBJ_FLAG_CLICKABLE

The confusing bits come from the anomalies in the naming as seen below.

This just seems wrong.

lv.label.LONG.WRAP
lv.label.LONG.DOT

If anything it should have been

lv.label.LONG_WRAP
lv.label.LONG_DOT

This would be better IMHO

lv.LABEL_LONG_WRAP
lv.LABEL_LONG_DOT

[liamHowatt]

What's the relationship between the C constant names being in an enum and the binding generator namespacing the constants like lv.label.LONG.WRAP? All of the enum members I've seen so far have full prefixes. Their names fully describe themselves independent of the enum. Can the binding generator pull out nested enums into the internal representation of the global namespace during generation? It'd be nice to keep enums for auto numbering. Pardon me if I misunderstood anything.

[kdschlosser]

yeah but you never know where to put the dots instead of the underscores and you really don't know if the enum has been added to an object class or if it it out in the wild on it's own.

Let me ask you. Where do you think that LV_SIZE_CONTENT is located?? You would make the assumption that it would be under lv.SIZE.CONTENT and you would be mistaken. it is placed in lv.SIZE_CONTENT

and you have ones like LV_SCROLLBAR_MODE_OFF which you would think would be under lv.SCROLLBAR.MODE_OFF and yet again this is wrong. it is placed at lv.SCROLLBAR_MODE.OFF

Figuring out where these things are is one of the most infuriating things about using the binding.

What makes it a pain is there are macros that are mixed into this which is what LV_SIZE_CONTENT is and that's the reason why it doesn't conform to the typical naming design. Because it ends up being placed into it's own enum and when the code is read what it ends up reading is ENUM_LV_SIZE_CONTENT

[kdschlosser]

I have not been able to fully grasp how the naming is done for the enums. The reason why is there are places all over the generator that change things about and following the data bath through the generator is not an easy task. It would be easier to write the thing over again then it would be to try and make heads or tails of what it is doing.

[kdschlosser]

I did manage to extend the generator so it spits out a JSON file that I am able to use to build a stub file..

I have attached both the stub file and the json file.

lv_mpy.json

lvgl.pyi.txt

[kdschlosser]

enums that begin with SCR_LOAD_ANIM_ need to be changed to SCREEN_LOAD_ANIM_ to keep with the recent API changes

[kdschlosser]

Here is a better version of the stub file.

lvgl.pyi.txt

[kisvegabor]

yeah but you never know where to put the dots instead of the underscores and you really don't know if the enum has been added to an object class or if it it out in the wild on it's own.

I absolutely agree. Following LVGL's API as it would be a huge simplification. Autocomplete still works the same way as it works in C (which is good enough).

We need to maintain backward compatibility though, so the binding generator should support both APIs (with flag?). In the examples we should also support both and slowly migrate all to the new API.

[kdschlosser]

I personally say to flatten the entire API so it is identical to what LVGL is.

Can work the backwards compatibility one of 3 ways.

• We can either add a compile time flag to compile it the old way
• Have the generator script write a module that can be imported to provide the old API.
• Have the generator script create a stub file that will let the user know to use the new style API if using an old API name for something.

I am not really keen on the creation of a module that is able to be imported. The reason why is the file would be HUGE and it would consume a lot of ram when loaded. It would also take a while to load the thing. However... I am able to leverage python so the imported module actually points to a class and in that class I can lazily load portions of the API that are being requested. This would consume a lot less memory and the only performance hit it would have would be when a portion of the API gets used the first time. After that it will be cached so it will load faster. This way only the bits that are used actually take up additional memory.

It would basically be pointers. I am not sure how complicated it would be to do that tho. I would need to mess about with it to find out.

[kdschlosser]

@kisvegabor

What do you use as an IDE?

Try taking that stub file I uploaded and remove the ".txt" extension. Place the file into the root of a project you created in your IDE. Then make a python source file (".py" file). at the top add the line import lvgl as lv and below that type in "lv." and a list should open up with everything that is available. You should have autocomplete and documentation. Granted the documentation is not added to this but I did place markers for it saying it wasn't available. Type hinting should also work as well so if a parameter type is supposed to be an int and you try to pass a string it is going to show you an error for that parameter.

[kisvegabor]

I don't know Python well enough to comment on the performance/memory issues. However having a compile time flag for the new API, in a year making the new the default and have flag for the old one for backward compatibility seems good enough for me. Could a script convert the user's code to the new API?

What do you use as an IDE?

Eclipse for both C and Python.

Try taking that stub file I uploaded and remove the ".txt" extension. Place the file into the root of a project you created in your IDE. [...]

I'm into some other things now, but it's really great! So autocomplete worked only in REPL, right?