fix(lv_msgbox): Automatically adjust msgbox's content height. #6176

TridentTD · 2024-05-04T11:04:52Z

Description of the feature or fix

When msgbox is resized, such as:

lv_obj_set_size(msgbox, lv_pct(80), lv_pct(80));

the height of the msgbox's content will automatically adjust,
and the scrollable feature will be removed from msgbox's buttons.

Notes

Update the Documentation if needed.
Add Examples if relevant.
Add Tests if applicable.
If you added new options to lv_conf_template.h run lv_conf_internal_gen.py and update Kconfig.
Run scripts/code-format.py (astyle version v3.4.12 needs to be installed) and follow the Code Conventions.
Mark the Pull request as Draft while you are working on the first version, and mark is as Ready when it's ready for review.
When changes were requested, re-request review to notify the maintainers.
Help us to review this Pull Request! Anyone can approve or request changes.

- auto msgbox's content-height - remove scrollable for buttons, header, footer

- add test_cases for msgbox change size, msgbox's content will auto change height.

PierreRambaud · 2024-05-06T08:17:42Z

src/widgets/msgbox/lv_msgbox.c

@@ -145,6 +146,7 @@ lv_obj_t * lv_msgbox_add_title(lv_obj_t * obj, const char * title)
        lv_obj_set_size(mbox->header, lv_pct(100), lv_display_get_dpi(lv_obj_get_display(obj)) / 3);
        lv_obj_set_flex_flow(mbox->header, LV_FLEX_FLOW_ROW);
        lv_obj_set_flex_align(mbox->header, LV_FLEX_ALIGN_START, LV_FLEX_ALIGN_CENTER, LV_FLEX_ALIGN_CENTER);
+        lv_obj_remove_flag(mbox->header, LV_OBJ_FLAG_SCROLLABLE);


I don't think scroll for header and footer should be disable by default 🤔

Thank you for the review.

If the header of the msgbox is scrollable,
I noticed that when accidentally touched directly on the header,
it will have a similar appearance to this clip.

https://www.veed.io/view/047f1414-a8e6-4d1b-952d-c507a0349ff0?panel=quality-survey

I still can't recall of an example of a case
where the header (or titlebar) of msgbox has a scrollbar.
Could you provide an example of a titlebar/header with a scrollbar?

I was just thinking about a scrollbar on a little screen with a lot of buttons (bad interface, but still capable of).
Since elements can be retrieved by using lv_msgbox_get_header, maybe the scrollbar flag can be removed by the user himself.

Maybe Core reviewers have something different in mind but even the lv_obj_set_flex_grow(mbox->content, 1); can be resolved by doing:

lv_obj_t* mbox = lv_msgbox_create(NULL); lv_obj_set_flex_grow(lv_msgbox_get_content(mbox), 1);

kisvegabor

Hi,

Thanks for the suggestion.

I've just tested it, but with this update lv_example_msgbox_1 is not working as the default LV_SIZE_CONTENT height of the message box and flex_grow on an element is conflicting. (It's a circular dependency)

I would like to keep the current set-height-to-the-content behaviour, but I also see the value in your suggestion. What about creating a new example, where some controls are added to the message box (slider, text input, etc) and the height is set manually?

I agree with removing the SCROLLABLE flag.

kisvegabor · 2024-05-06T11:21:21Z

tests/src/test_cases/widgets/test_msgbox.c

@@ -44,7 +44,7 @@ void test_msgbox_creation_successful_with_close_button(void)

    TEST_ASSERT_NOT_NULL(msgbox);

-    TEST_ASSERT_EQUAL_SCREENSHOT("widgets/msgbox_ok_with_close_btn.png");
+    TEST_ASSERT_EQUAL_SCREENSHOT("ref_imgs/widgets/msgbox_ok_with_close_btn.png");


"ref_imgs/" is appended automatically here so the original path should be correct.

kisvegabor · 2024-05-06T11:21:45Z

tests/src/test_cases/widgets/test_msgbox.c

@@ -144,4 +144,38 @@ void test_msgbox_close_async_modal(void)
    TEST_ASSERT_NOT_NULL(msgbox);
 }

+void test_msgbox_content_auto_height(void)
+{
+    // If parent is NULL the message box will be modal


We use /**/ comments:

Suggested change

// If parent is NULL the message box will be modal

/* If parent is NULL the message box will be modal */

kisvegabor · 2024-05-06T11:22:27Z

tests/src/test_cases/widgets/test_msgbox.c

+
+    TEST_ASSERT_EQUAL(h_obj_content, h_msgbox_element_sum);
+
+    // Since msgbox has no parent, it won´t be clean up at tearDown()


Suggested change

// Since msgbox has no parent, it won´t be clean up at tearDown()

/* Since msgbox has no parent, it won´t be clean up at tearDown() */

Now , support both msgbox's default size (by LV_SIZE_CONTENT) and msgbox' manual size.

However, for test_msgbox.c in the test_cases folder,
when I tested whether the msgbox's height is set to LV_SIZE_CONTENT or not,
it's OK.

But when the command

lv_obj_update_content(msgbox);

is added, an error occurs.

I don't know what caused the error in test_msgbox.c.

msgbox with fixed size

support - default height : LV_SIZE_CONTENT - manual height : fixed height or height by lv_pct(...)

- add lv_obj_update_layout(msgbox); but TEST error ?

kisvegabor · 2024-05-07T21:42:58Z

Thank you for adding the test. I fixed the crash and few other minor things in 17c9fdc

- remove pad_top =0 of the container - adjust position of content's scrollbar

- Check the size comparison of the msgbox, both default size and manual size

TridentTD · 2024-05-08T01:03:13Z

Thank you for fixing the crash.
I have already updated the test_msgbox (at commit 9d4269d).

(I apologize for accidentally uploading the wrong commit
since after commit 687691a.
Looks like deleting unwanted commits needs to be done through git command line.
However, due to my network's inability to fully git clone lvgl,
a mid-process failure occurred.
So I can't delete unwanted commits using git command line.)

kisvegabor · 2024-05-08T21:51:42Z

No problem, I've reset it to the 687691a.

TridentTD · 2024-05-09T03:10:55Z

Thank you.

TridentTD added 2 commits May 4, 2024 17:39

Update lv_msgbox.c

24ef46e

- auto msgbox's content-height - remove scrollable for buttons, header, footer

Update test_msgbox.c

8b6426c

- add test_cases for msgbox change size, msgbox's content will auto change height.

TridentTD changed the title ~~[feat] Automatically adjust msgbox's content height.~~ fix: Automatically adjust msgbox's content height. May 4, 2024

TridentTD changed the title ~~fix: Automatically adjust msgbox's content height.~~ fix(lv_msgbox): Automatically adjust msgbox's content height. May 4, 2024

PierreRambaud reviewed May 6, 2024

View reviewed changes

kisvegabor reviewed May 6, 2024

View reviewed changes

TridentTD and others added 11 commits May 6, 2024 23:34

Create lv_example_msgbox_2.c

9354d52

msgbox with fixed size

Update lv_msgbox.c

13370e4

support - default height : LV_SIZE_CONTENT - manual height : fixed height or height by lv_pct(...)

Update test_msgbox.c

57d3f8e

Update test_msgbox.c

4ac40c2

Update lv_example_widgets.h

1c0bde0

Update test_msgbox.c

061cb13

Update lv_example_msgbox_2.c

079bba9

Update test_msgbox.c

91861f4

- add lv_obj_update_layout(msgbox); but TEST error ?

Update test_msgbox.c

421e258

Update test_msgbox.c

0e0b039

minor fixes

17c9fdc

TridentTD added 3 commits May 8, 2024 07:37

Update lv_example_msgbox_2.c

02d19ba

- remove pad_top =0 of the container - adjust position of content's scrollbar

Update test_msgbox.c

bdfff5a

- Check the size comparison of the msgbox, both default size and manual size

Update test_msgbox.c

9d4269d

Merge branch 'lvgl:master' into auto-msgbox's-content-height

687691a

kisvegabor force-pushed the auto-msgbox's-content-height branch from 7b93777 to 687691a Compare May 8, 2024 21:50

liamHowatt approved these changes May 12, 2024

View reviewed changes

PierreRambaud approved these changes May 13, 2024

View reviewed changes

kisvegabor approved these changes May 14, 2024

View reviewed changes

kisvegabor requested a review from PGNetHun May 14, 2024 09:49

kisvegabor requested review from XuNeo and FASTSHIFT May 14, 2024 09:49

XuNeo approved these changes May 15, 2024

View reviewed changes

XuNeo merged commit ec3d979 into lvgl:master May 15, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lv_msgbox): Automatically adjust msgbox's content height. #6176

fix(lv_msgbox): Automatically adjust msgbox's content height. #6176

TridentTD commented May 4, 2024

PierreRambaud May 6, 2024

TridentTD May 6, 2024 •

edited

PierreRambaud May 6, 2024

kisvegabor left a comment •

edited

kisvegabor May 6, 2024

kisvegabor May 6, 2024

kisvegabor May 6, 2024

TridentTD May 7, 2024

kisvegabor commented May 7, 2024

TridentTD commented May 8, 2024 •

edited

kisvegabor commented May 8, 2024

TridentTD commented May 9, 2024

	// If parent is NULL the message box will be modal
	/* If parent is NULL the message box will be modal */


		TEST_ASSERT_EQUAL(h_obj_content, h_msgbox_element_sum);

		// Since msgbox has no parent, it won´t be clean up at tearDown()

	// Since msgbox has no parent, it won´t be clean up at tearDown()
	/* Since msgbox has no parent, it won´t be clean up at tearDown() */

fix(lv_msgbox): Automatically adjust msgbox's content height. #6176

fix(lv_msgbox): Automatically adjust msgbox's content height. #6176

Conversation

TridentTD commented May 4, 2024

Description of the feature or fix

Notes

PierreRambaud May 6, 2024

Choose a reason for hiding this comment

TridentTD May 6, 2024 • edited

Choose a reason for hiding this comment

PierreRambaud May 6, 2024

Choose a reason for hiding this comment

kisvegabor left a comment • edited

Choose a reason for hiding this comment

kisvegabor May 6, 2024

Choose a reason for hiding this comment

kisvegabor May 6, 2024

Choose a reason for hiding this comment

kisvegabor May 6, 2024

Choose a reason for hiding this comment

TridentTD May 7, 2024

Choose a reason for hiding this comment

kisvegabor commented May 7, 2024

TridentTD commented May 8, 2024 • edited

kisvegabor commented May 8, 2024

TridentTD commented May 9, 2024

TridentTD May 6, 2024 •

edited

kisvegabor left a comment •

edited

TridentTD commented May 8, 2024 •

edited