Build size and comparison to main:

Run in InfiniEmu

If you're feeling up to it, I think a review of every screen not using LV_DISP_DEF_REFR_PERIOD would be good step forward in terms of improving responsiveness. There's probably a reason for the slow refreshes in some places - maybe performance of other things on the screen, maybe designs that don't use DirtyValue and therefore redraw things every frame, maybe bugs which have long been fixed, who knows! In most cases I think updating every refresh should be fine as there should be no drawing to do if nothing has changed (but some refactoring may be needed to allow this e.g if DirtyValue needs to be added)

I've been getting my head around the drawing of each screen and the lv_task callbacks and will try to have a further look at a general review.
There appears to be an intention in doc/code/Apps.md but no code, that I can see, uses Refresh() as the callback.

Sounds good, feel free to ask any questions (here or the pinetime-dev chat channel). I'm happy to explain general things too if an overview on a part would be useful

How's it going? I'm happy to look at reviewing just this change for now and equally happy with waiting if you're planning on reviewing more places where the refresh time is long, just let me know :)

How's it going? I'm happy to look at reviewing just this change for now and equally happy with waiting if you're planning on reviewing more places where the refresh time is long, just let me know :)

Hi @mark9064 I've not been able to progress this to look at all of the code. It is on my mind that I really should but it won't be for a while.

No worries at all, whenever you're ready for review just say and I'll try take a look

To better understand the code, I reviewed every lv_task_create and found the following:

1. There is a mixture of taskUpdate = lv_task_create, taskRefresh = lv_task_create and refreshTask = lv_task_create. Suggestion is to refactor all to the most common which is taskRefresh = lv_task_create.
2. There are a number of instances where the Refresh() method is called immediately after the refresh task is created. Where the refresh period is 20msec, it's pretty pointless performing this Refresh() and it should be removed. eg.

   InfiniTime/src/displayapp/screens/WatchFaceInfineat.cpp

   Line 300 in 7128fc0

   Refresh();

3. Where possible, modify instances where a magic number is used instead of LV_DISP_DEF_REFR_PERIOD. As above, this may require the use of the dirty value utility.
4. Some apps do not follow the design pattern used for lv_task_create(RefreshTaskCallback, which uses the Refresh() method. Can these apps be updated to the pattern all other apps use?

Thanks a lot for going through and reviewing them all :)

1. sounds great!
2. id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)
3. sounds good to me
4. yeah agree, having just one design pattern makes sense

2. id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)

I'd not thought about that. I'll run a quick test on a few watchfaces to see the impact on the UX.

2. id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)

Tested on the simulator as it's pretty slow on my machine so would exaggerate any UX problems and you're absolutely right @mark9064 the Refresh() is required to fully update the WatchFaces, ready for when they scroll into view. I moved the Refresh() before the refresh task is created and it didn't appear to make any difference to the UX, so is ok to be left as it is.

Dirty Value alternative proposed in #2471

#2471 is a better solution