Skip to content

Implement DirtyValues on Tile, Quick Settings, Steps and Batt Info screens - #2471

Open
SteveAmor wants to merge 6 commits into
InfiniTimeOrg:mainfrom
SteveAmor:dirtyValueQuickSettingsScreen
Open

Implement DirtyValues on Tile, Quick Settings, Steps and Batt Info screens#2471
SteveAmor wants to merge 6 commits 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 385280B 48B
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

@SteveAmor
SteveAmor force-pushed the dirtyValueQuickSettingsScreen branch from b3268ae to 95b48a2 Compare August 11, 2026 17:11
@SteveAmor

Copy link
Copy Markdown
Contributor Author

@mark9064

Forced push above was to refactor for Refresh().

I now have tested changes to Tile.cpp, Steps.cpp and BatteryInfo.cpp that include dirty values for screen changes, lvgl refactoring and screenUpdate replaced with Refresh() where required. I can add them to this PR, raise individual PRs for each screen or a new PR with those three screens. What would you prefer me to do?

@mark9064

Copy link
Copy Markdown
Member

I think rolling them into this PR should be fine - sounds like the overall change isn't going to be huge

@SteveAmor SteveAmor changed the title Implement DirtyValues for Quick Settings screen Implement DirtyValues on Tile, Quick Settings, Steps and Batt Info screens Aug 12, 2026
@SteveAmor

SteveAmor commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

After the discussion about dirty values in #2450 (comment) I have updated the steps goal to be a dirty value.

@SteveAmor
SteveAmor force-pushed the dirtyValueQuickSettingsScreen branch from e8da016 to db6ea32 Compare August 13, 2026 15:08
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