Skip to content

Implement DirtyValues for Quick Settings screen - #2471

Open
SteveAmor wants to merge 1 commit into
InfiniTimeOrg:mainfrom
SteveAmor:dirtyValueQuickSettingsScreen
Open

Implement DirtyValues for Quick Settings screen#2471
SteveAmor wants to merge 1 commit into
InfiniTimeOrg:mainfrom
SteveAmor:dirtyValueQuickSettingsScreen

Conversation

@SteveAmor

Copy link
Copy Markdown
Contributor

Implemented as an alternative to #2257

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 385296B 64B
data 944B 0B
bss 22640B 0B

Run in InfiniEmu

@SteveAmor

Copy link
Copy Markdown
Contributor Author

@mark9064 I would very much appreciate your review of this.

I see that statusIcons.Update() already uses dirty values to only update if there is a change

void StatusIcons::Update() {

So I only needed to update the time at the top of the screen to use dirty values.

I thought I might have to use override for the UpdateScreen but the compiler didn't like that


If I got it right, I'll move on to a few other screens - like the apps screen which has a similar layout with status icons and the time.
It works as expected on my watch.

@mark9064 mark9064 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a nicer solution!

The logic with dirtyvalue all looks good. When you said you thought you might have to use override, what did you mean by that?

There's a few other places that have Refresh() in the wrong place - maybe you will find this useful for your changes: mark9064@43bfddc

@mark9064 mark9064 added this to the 1.17.0 milestone Aug 8, 2026
@mark9064 mark9064 added the maintenance Background work label Aug 8, 2026
@SteveAmor

Copy link
Copy Markdown
Contributor Author

This is a nicer solution!
The logic with dirtyvalue all looks good.

Thank you

When you said you thought you might have to use override, what did you mean by that?

I saw in other screens that use DirtyValues that the header file has void Refresh() override; and I'm getting out of my depth with that one.

There's a few other places that have Refresh() in the wrong place - maybe you will find this useful for your changes: mark9064@43bfddc

Very useful, thank you.

I have updated Tile.cpp (not pushed yet) and then started on BatteryInfo and the lvgl was not lining up. Now I can see why!
BatterInfo needs a lot of work to use DirtyValues. I think I need to track the charging/discharging status to see if that changes which is more of a refactor than I thought it would be. Rather than track changes in charging.

@mark9064

mark9064 commented Aug 9, 2026

Copy link
Copy Markdown
Member

Ahhhhh right I see

The Screen class, which (almost?) all app screens inherit from provides a dummy Refresh method and a short helper which you can pass to LVGL (see Screen.cpp). Refresh is purposefully a virtual method: if a method is virtual, it means that classes inheriting from it are allowed to override it (i.e. replace the contents of the method with something else), and when you do override it you need to use the override keyword.

Since QuickSettings is using its own helper for LVGL and its own UpdateScreen, it's not overriding anything. But it would probably make sense to use Refresh and the normal helper here like other apps, so maybe worth changing?

@SteveAmor

Copy link
Copy Markdown
Contributor Author

Got it. Thank you. I had started to refactor using Refresh() and backed out for two reasons. First was the "override" or not question and not really understanding it (and not wanting to use the excuse "well it compiled so must be ok") and second because I wanted to go back and tidy up the few cases that use UpdateScreen() in a PR just for that change. And then I saw your lvgl PR and all I could think about was the merge conflicts 🤣

@mark9064

mark9064 commented Aug 9, 2026

Copy link
Copy Markdown
Member

LOL don't worry that isn't a PR yet. I can rebase around any conflicts, I don't mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Background work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants