FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement DirtyValues on Tile, Quick Settings, Steps and Batt Info screens by SteveAmor · Pull Request #2471 · InfiniTimeOrg/InfiniTime · GitHub

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

Copy link
Copy Markdown
Contributor

Implemented as an alternative to #2257

github-actions Bot commented Aug 8, 2026
edited
Loading

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

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 left a comment
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added this to the 1.17.0 milestone Aug 8, 2026
mark9064 added the maintenance Background work label Aug 8, 2026

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 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?

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 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 force-pushed the dirtyValueQuickSettingsScreen branch from b3268ae to 95b48a2 Compare August 11, 2026 17:11

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?

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 changed the title Implement DirtyValues for Quick Settings screen Implement DirtyValues on Tile, Quick Settings, Steps and Batt Info screens Aug 12, 2026

SteveAmor commented Aug 13, 2026
edited
Loading

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 force-pushed the dirtyValueQuickSettingsScreen branch from e8da016 to db6ea32 Compare August 13, 2026 15:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL