Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(obj): add API to set/get object ID. #6278

Merged
merged 4 commits into from
Jun 12, 2024
Merged

feat(obj): add API to set/get object ID. #6278

merged 4 commits into from
Jun 12, 2024

Conversation

XuNeo
Copy link
Collaborator

@XuNeo XuNeo commented May 25, 2024

Description of the feature or fix

Add macro LV_USE_OBJ_ID_ASSIGN(default ON) to automatically assign ID to object after it's created.
Add API lv_obj_[get|set]_id.
Add API lv_obj_get_child_by_id . It could be useful for binding language.

Notes

src/core/lv_obj.c Outdated Show resolved Hide resolved
Add macro LV_USE_OBJ_ID_ASSIGN to automatically assign ID to object after it's created.

Signed-off-by: Neo Xu <neo.xu1990@gmail.com>
Signed-off-by: Neo Xu <neo.xu1990@gmail.com>
Comment on lines 336 to 337
/* Automatically assign an ID when obj is created */
#define LV_USE_OBJ_ID_ASSIGN 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Automatically assign an ID when obj is created */
#define LV_USE_OBJ_ID_ASSIGN 1
# if LV_USE_OBJ_ID != 0
/* Automatically assign an ID when obj is created. */
#define LV_OBJ_ID_AUTO_ASSIGN 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out we can use #define LV_OBJ_ID_AUTO_ASSIGN LV_USE_OBJ_ID So it's automatically enabled and always defined.

Do you think it's acceptable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think kisvegabor's suggestion matches the existing pattern in the config file. Configs that depend on another config are inside an #if ... #endif and indented.

Copy link
Collaborator Author

@XuNeo XuNeo Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated conf.h looks like this.

/* Automatically assign an ID when obj is created */
#ifndef LV_OBJ_ID_AUTO_ASSIGN
    #ifdef CONFIG_LV_OBJ_ID_AUTO_ASSIGN
        #define LV_OBJ_ID_AUTO_ASSIGN CONFIG_LV_OBJ_ID_AUTO_ASSIGN
    #else
        #define LV_OBJ_ID_AUTO_ASSIGN   LV_USE_OBJ_ID
    #endif
#endif

So LV_OBJ_ID_AUTO_ASSIGN is always defined, and default to 1 if LV_USE_OBJ_ID is enabled.

This is what we want(always define the macro with correct value).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you want to use LV_OBJ_ID_AUTO_ASSIGN as a sort of compound macro that, when 1, means LV_USE_OBJ_ID && (!defined(LV_OBJ_ID_AUTO_ASSIGN) || (defined(LV_OBJ_ID_AUTO_ASSIGN) && LV_OBJ_ID_AUTO_ASSIGN != 0)).

Until now, using defines like LV_DEMO_MUSIC_LARGE when LV_USE_DEMO_MUSIC=0 is "undefined behavior". Code that uses LV_DEMO_MUSIC_LARGE first checks that LV_USE_DEMO_MUSIC == 1 before considering the value of LV_DEMO_MUSIC_LARGE. In your changes, you use LV_OBJ_ID_AUTO_ASSIGN as a shortcut.

It makes sense, but I cannot say whether it's okay to break the old pattern for the first time. @kisvegabor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. This breaks the old pattern.
For system using Kconfig, it normally uses #ifdef instead of #if .

LVGL follow the latter style, thus it makes sense to discuss whether we should always define those macros, either enabled as 1 or disabled as 0.

I can follow the old pattern in this PR and raise another discussion if that's more appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liamHowatt just pointed out this unresolved thread.

If we use #if it would really make more sense to not add guards around the child configs. Let's keep it as it is in this feature, and discuss it a new issue.

lv_conf_template.h Show resolved Hide resolved
docs/others/obj_id.rst Outdated Show resolved Hide resolved
Signed-off-by: Neo Xu <neo.xu1990@gmail.com>
src/core/lv_obj.c Show resolved Hide resolved
src/core/lv_obj.c Show resolved Hide resolved
src/core/lv_obj.h Outdated Show resolved Hide resolved
src/core/lv_obj.h Outdated Show resolved Hide resolved
src/core/lv_obj.h Outdated Show resolved Hide resolved
Signed-off-by: Neo Xu <neo.xu1990@gmail.com>
@kisvegabor
Copy link
Member

@liamHowatt please review and merge it if all look good.

@liamHowatt liamHowatt merged commit cab55d8 into lvgl:master Jun 12, 2024
17 checks passed
@XuNeo XuNeo deleted the t30 branch June 13, 2024 06:40
@kisvegabor kisvegabor mentioned this pull request Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants