| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Build size and comparison to main:
|
Sorry, something went wrong.
|
@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 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
Thank you
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.
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! |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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 🤣 |
Sorry, something went wrong.
|
LOL don't worry that isn't a PR yet. I can rebase around any conflicts, I don't mind |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
I think rolling them into this PR should be fine - sounds like the overall change isn't going to be huge |
Sorry, something went wrong.
|
After the discussion about dirty values in #2450 (comment) I have updated the steps goal to be a dirty value. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Implemented as an alternative to #2257