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
fix(display): load screen from matching display #6189
Conversation
If the animation starts and a different display has been set as the default in the meantime, the screen set as "previous" was loaded from the wrong display. Fix: lv_screen_active() replaced with lv_display_get_screen_active()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Here and here too, I believe.
diff --git a/src/display/lv_display.c b/src/display/lv_display.c
index 4c061d095..4eda20a3c 100644
--- a/src/display/lv_display.c
+++ b/src/display/lv_display.c
@@ -570,7 +570,7 @@ void lv_screen_load_anim(lv_obj_t * new_scr, lv_screen_load_anim_t anim_type, ui
bool auto_del)
{
lv_display_t * d = lv_obj_get_display(new_scr);
- lv_obj_t * act_scr = lv_screen_active();
+ lv_obj_t * act_scr = lv_display_get_screen_active(d);
if(act_scr == new_scr || d->scr_to_load == new_scr) {
return;
@@ -586,7 +586,7 @@ void lv_screen_load_anim(lv_obj_t * new_scr, lv_screen_load_anim_t anim_type, ui
if(d->del_prev) {
lv_obj_delete(act_scr);
}
- act_scr = lv_screen_active(); /*Active screen changed.*/
+ act_scr = lv_display_get_screen_active(d); /*Active screen changed.*/
scr_load_internal(d->scr_to_load);
}
Based on this small test
static void scr_load_anim_issue(lv_display_t * d1, lv_display_t * d2)
{
lv_display_set_default(d1);
lv_obj_t * d1_s1 = lv_display_get_screen_active(d1);
lv_label_set_text(lv_label_create(d1_s1), "d1_s1");
lv_obj_t * d2_s1 = lv_display_get_screen_active(d2);
lv_label_set_text(lv_label_create(d2_s1), "d2_s1");
lv_display_set_default(d2);
lv_obj_t * d2_s2 = lv_obj_create(NULL);
lv_display_set_default(d1);
lv_label_set_text(lv_label_create(d2_s2), "d2_s2");
lv_screen_load_anim(d2_s2, LV_SCR_LOAD_ANIM_OUT_RIGHT, 3000, 1000, false);
}
Also I think d->act_scr
would be fine instead of lv_display_get_screen_active(d)
.
Thank you for your answer!
At the calls, in the function "lv_screen_load_anim()" I was unsure, because when creating objects they are always created on the current default display. But I would also find it more logical and better to change it here as well, since no object is created but one (screen) is passed, and the screen belongs to a display.
For "lv_display_get_screen_active()" I wanted to keep it similar to the original call, but of course these are unnecessary function calls. I'll add/update this quickly. |
changed lv_screen_active() calls to d->act_scr
Thank you! I went through the same thought process. For consistency it would make sense for the functions to operate on the active display, but the screen being passed belongs to a display which may not be the default one, and if it's not, it's an error to try to load it to a different display. As I say that though, I have some doubts because I'm not sure if/why screens can't/shouldn't be moved between displays. The other reviewers may be able to check the claim. There's another way this could be fixed, which is to change |
Nice catch! Thank you for opening this PR! Loading the same screen on others displays is an interesting idea, but it would require more consideration. E.g. is the display has a list for its screens it might lead to all sort of issues, if we load a screen on a foreign display. Anyway, this PR looks good. 🙂 If there is an interest to discuss "cross-loading" screens, please open an issue for it. |
If the function "lv_screen_load_anim()" is used to change the screen with a defined delay and the default display is changed within the delay in a multi-display setup, the wrong screen is loaded as the "previous" screen.
Fix: lv_screen_active() replaced with lv_display_get_screen_active()
(lv_display.c)
lvgl_bug_load_screen.mp4
The bug can be seen in the video:
the top left display is set as the default display. When starting the animation from the lower display and the upper right display, the active screen is loaded from the upper left display.
Kind regards and have a nice day! :)