Adds page indicator to music app

[SteveAmor]

Fixes #1391

[github-actions]

Build size and comparison to main:

Section	Size	Difference
text	380180B	48B
data	944B	0B
bss	22544B	0B

Run in InfiniEmu

[SteveAmor]

[minacode]

I imagine it could look a little better with a little margin around the buttons. What do you think?

[SteveAmor]

The page indicator is 2px wide.
However, if I move the buttons 3px, it conveniently has a 3px space between the play/pause button too, making it nice and symmetrical.
What do people think?

For comparison, here are the buttons moved 2px

[minacode]

I like the first image where the bar is not touching the button.

[SteveAmor]

PR updated to move the buttons 3px.

[SteveAmor]

I experimented with hiding or deleting the page indicator object when you switch pages. According to the simulator, hiding the object used more lvgl memory than deleting and creating. So I went with deleting. I guess it used more memory as you actually had more objects at any one time, with one of them hidden.

[mark9064]

I experimented with hiding or deleting the page indicator object when you switch pages. According to the simulator, hiding the object used more lvgl memory than deleting and creating. So I went with deleting. I guess it used more memory as you actually had more objects at any one time, with one of them hidden.

Just thinking, would it make more sense to have a SetCurrentScreen() method for pageindicator so that you only need to store one instance of the pageindicator object too?

[SteveAmor]

I remember now. Music.cpp doesn't have more than one page. It has a single page and hides/unhides buttons to make it look like two pages. This is the reason I went down this route. Perhaps the best thing to do (which is what I did originally) is to hide and unhide two page indicators?

[SteveAmor]

I experimented with hiding or deleting the page indicator object when you switch pages. According to the simulator, hiding the object used more lvgl memory than deleting and creating. So I went with deleting. I guess it used more memory as you actually had more objects at any one time, with one of them hidden.

Just thinking, would it make more sense to have a SetCurrentScreen() method for pageindicator so that you only need to store one instance of the pageindicator object too?

I think that already exists:

PageIndicator(uint8_t nCurrentScreen, uint8_t nScreens)

But won't work for Music.cpp single page with hidden buttons for the "second page".

[mark9064]

What I'm suggesting is a new method for PageIndicator that tears down the existing one and redraws at a different position. This way you'd only have one page indicator as a member of the app and you could do pageIndicator.SetCurrentScreen(x) whenever you wanted to move it, rather than having two and having to create/delete or hide/unhide

[SteveAmor]

@mark9064 hopefully this implementation of SetCurrentScreen is what you had in mind? Please let me know :-)

[minacode]

Inside it, LVGL objects are created and dropped. Is that a good idea? Would it be better to create a function Refresh which can reset some of the properties of those objects based on the widgets attributes and call that instead and never use Delete? I imagine that this may take some work off the LVGL heap, but am not perfectly sure how it works internally.

[SteveAmor]

Inside it, LVGL objects are created and dropped. Is that a good idea? Would it be better to create a function Refresh which can reset some of the properties of those objects based on the widgets attributes and call that instead and never use Delete? I imagine that this may take some work off the LVGL heap, but am not perfectly sure how it works internally.

I'm certainly no expert but having looked at the pageindicator code, I can't see an easy way to update some of the properties. If not all of them. Any guidance would be much appreciated.
I've tried to ballance the additional code space required with the functionality of a page indicator.
To fit in with the pattern the Music page(s) uses, I'm very much tending towards my original thought (and test) of having two page indicators and hiding/unhiding them.
From a UI point of view, I hadn't realised that you can also skip tracks back and forwards by swiping left and right!

[mark9064]

Well I think the current implementation is pretty fine

But if you wanted to be more efficient, pull out

  const int16_t indicatorSize = LV_VER_RES / nScreens;
  const int16_t indicatorPos = indicatorSize * nCurrentScreen;

  pageIndicatorPoints[0].x = LV_HOR_RES - 1;
  pageIndicatorPoints[0].y = indicatorPos;
  pageIndicatorPoints[1].x = LV_HOR_RES - 1;
  pageIndicatorPoints[1].y = indicatorPos + indicatorSize;
  
  lv_line_set_points(pageIndicator, pageIndicatorPoints, 2);

to a different method (eg SetPageIndicatorPosition(u8 position)) and then call that at the end of Create, and directly from the code here.

This works as you can leave the background stripe, and only want to move the highlight (current position indicator) stripe.

[SteveAmor]

Well I think the current implementation is pretty fine

But if you wanted to be more efficient, pull out

  const int16_t indicatorSize = LV_VER_RES / nScreens;
  const int16_t indicatorPos = indicatorSize * nCurrentScreen;

  pageIndicatorPoints[0].x = LV_HOR_RES - 1;
  pageIndicatorPoints[0].y = indicatorPos;
  pageIndicatorPoints[1].x = LV_HOR_RES - 1;
  pageIndicatorPoints[1].y = indicatorPos + indicatorSize;
  
  lv_line_set_points(pageIndicator, pageIndicatorPoints, 2);

to a different method (eg SetPageIndicatorPosition(u8 position)) and then call that at the end of Create, and directly from the code here.

This works as you can leave the background stripe, and only want to move the highlight (current position indicator) stripe.

All done :-)

[mark9064]

If you move

  pageIndicator = lv_line_create(lv_scr_act(), nullptr);
  lv_obj_set_style_local_line_width(pageIndicator, LV_LINE_PART_MAIN, LV_STATE_DEFAULT, 3);
  lv_obj_set_style_local_line_color(pageIndicator, LV_LINE_PART_MAIN, LV_STATE_DEFAULT, Colors::lightGray);

to Create(), then you don't need an extra method at all as you won't be deleting the existing page indicator, instead it'll only be moved

[SteveAmor]

If you move

  pageIndicator = lv_line_create(lv_scr_act(), nullptr);
  lv_obj_set_style_local_line_width(pageIndicator, LV_LINE_PART_MAIN, LV_STATE_DEFAULT, 3);
  lv_obj_set_style_local_line_color(pageIndicator, LV_LINE_PART_MAIN, LV_STATE_DEFAULT, Colors::lightGray);

to Create(), then you don't need an extra method at all as you won't be deleting the existing page indicator, instead it'll only be moved

My bad, I should have noticed that in your earlier message. This (hopefully final) commit is the most efficient in terms of additional program memory required and it certainly (looks to me) cleaner than previous commits.

[SteveAmor]

@mark9064 could you please double check that I have declared everything that I need to, correctly, in PageIndicator.h

[mark9064]

All LGTM :)

[tituscmd]

Would be superseded by #2337 if merged

[minacode]

I would argue that we still merge this, because it's ready and easy to do. And then we discuss your version 2 on top of that. Do you agree, @mark9064?

[mark9064]

Yep, sounds good to me. Once this has a second review I'm happy to merge :)

[JF002]

LGTM, thanks!

[NeroBurner]

LGTM