InfiniTimeOrg
Add weather to the terminal watch face
This was originally part of #2001 but is being split into its own PR without the non-weather changes to give it the ability to be merged without those changes.
This PR adds the current temperature and condition to the terminal watch face with dynamic coloring. In it's current state many of the conditions will overflow off the right side of the screen, but this will be fixed in #2134 which has already been added to the 1.16.0 milestone.
Build size and comparison to main:
| Section | Size | Difference |
|---|---|---|
| text | 373376B | 368B |
| data | 948B | 0B |
| bss | 22536B | 0B |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Looks good! Thanks for splitting this out, definitely easier to review now - I get that more branches is more work for you
So currently there are 4 "bands", as the weather app uses an LVGL table which requires a "style" for every colour used (from what I can see, maybe there are some workarounds). Would it makes sense to use a continuous colour scale (maybe somewhat like #1964) in this scenario? It might be better to stick to the colour bands for consistency, I'm not sure. Either way I think is might be a good opportunity to pull out temperature->colour conversion into the temperature class (support both bands and continuous or just bands), as it's probably not a good idea to duplicate colour conversion code across the codebase
Looks good! Thanks for splitting this out, definitely easier to review now - I get that more branches is more work for you
So currently there are 4 "bands", as the weather app uses an LVGL table which requires a "style" for every colour used (from what I can see, maybe there are some workarounds). Would it makes sense to use a continuous colour scale (maybe somewhat like #1964) in this scenario? It might be better to stick to the colour bands for consistency, I'm not sure. Either way I think is might be a good opportunity to pull out temperature->colour conversion into the temperature class (support both bands and continuous or just bands), as it's probably not a good idea to duplicate colour conversion code across the codebase
I created a new commit moving the TemperatureColor "band" logic into SimpleWeatherService's Temperature class as the function Color. I'm not sure if this PR is the right place make this change, perhaps instead myself or someone else should create a new PR pulling the TemperatureColor function out of displayapp/screens/Weather.cpp along with the TemperatureStyle function if desired. Let me know your thoughts on this.
Any thoughts on whether a continuous scale would be better or not in general?
@mark9064 I do think it could be helpful to have a "continuous" color range, probably with each band getting its own range. I could see this being particularly helpful in the weather app where you could just glance at the colors instead of reading all the actual temperatures. However I don't see this being too helpful in the terminal watch face with only a single temperature... so I feel like this should probably be put into its own PR. I'd be willing to try this out at some point, but I have a few other branches I'd like to focus on first, so if someone else wants to take the most recent commit and use it in there own PR attempting this before me, feel free to!
@JustScott @mark9064 I consider this also helpful in the terminal watchface. It already is pretty colorful. Why should the color of the temperature not vary according its value? It makes sense to me. In other watchfaces the feature might disturb the color theme, but in the terminal watchface this makes sense to me.
On the topic of temperature colouring: I find that the distinct colours for each line in the terminal watch face makes it easier to parse at a glance as you can quickly scan for whichever colour of text you are looking for. As such, I believe that the weather line would be easier to quickly read (and more consistent) if it used a constant colour instead of varying based off of temperature. If a varying colour is used, then I think that discrete colour steps (as opposed to the proposed continuous changes) make more sense with the theming of the watch (ttys having limited colours and all).
As someone who lives in Australia, where 8C is cold, 25C is pleasant, and 40C is hot but normal for summer, the colours people choose to represent temperature always feel a bit arbitrary, like it's designed for someone else. So I'd also rather not have the colours change.
On the topic of temperature colouring: I find that the distinct colours for each line in the terminal watch face makes it easier to parse at a glance as you can quickly scan for whichever colour of text you are looking for. As such, I believe that the weather line would be easier to quickly read (and more consistent) if it used a constant colour instead of varying based off of temperature. If a varying colour is used, then I think that discrete colour steps (as opposed to the proposed continuous changes) make more sense with the theming of the watch (ttys having limited colours and all).
I agree. In keeping with the rest of the terminal ui, having a static color for each line would be preferable
@pbone64 @AlbeyAmakiir @fladrif I agree that the color should be static, so how about using yellow (#ffdd00) as that seems like the universal color for weather (sunshine) and it's not used for any other fields. I left the empty weather value ("---°") as the default white, but can change it to the same yellow if that's what everyone wants (like how the heartbeat field stays red even without any data).
I like the yellow + I think that it should remain yellow when there's no weather data to match the heart rate sensor (whether the heart rate sensor is correct is it's own discussion).
It looks like the degrees symbol shows up even when there is no weather data. Is that intentional?
I like the yellow + I think that it should remain yellow when there's no weather data to match the heart rate sensor (whether it is correct is another story, but that is it's own discussion).
It looks like the degrees symbol shows up even when there is no weather data. Is that intentional?
Yes it's intentional, but whether it should stay or not without data doesn't matter to me... I'm open to removing it if that's what everyone wants. I feel like it may fit with the rest better without the degree symbol, as then it would match the HR field when empty.
@JustScott thank you so much for this. One minor thought, would it make sense to have weather and status lines swapped? Idea being the status should be distinct and being last in the list is a good way to do so
All of them need to be distinct for various reasons and use cases. Did you have a particular use case for needing status more distinct than the others? I don't personally look at it much.
@AlbeyAmakiir Status is in my opinion a meta value, it's different from the other values on the basis where it isn't a direct use case of the watch (time, steps, weather...) but tells you of the status of the watch. It's a second order utility. But because of that it's useful to have it distinct. More plainly i find it useful to have it distinct to know if my phone is close by and if I'm potentially missing notifications
@fladrif I swapped the bluetooth and weather positions as I agree it makes more sense to have weather with the rest of the data lines, and bluetooth to be on its own as its a status instead of data. I also removed the degree symbol from weather when there is no data as it seems a bit unecessary.
As a daily user of the terminal watchface, this layout looks perfect. I agree with the swapped weather and status position + removing the degree symbol when there is no whether data.
One last thing I want to bring up: most of the labels before data are truncated words, rather than shortenings (TIME, DATE, BATT, STEP, STAT; only with the exception of L_HR though I'm sure there's a reasoning behind that label). Would a label like [TEMP] (for temperature) make more sense, as opposed to [WTHR]?
I think [WTHR] is fine as it's more accurate to the purpose, even if it doesn't match truncation. I don't think any of the other words can be usefully shortened to 4 characters in a way that isn't truncation. It's probably this way for convenience and clarity over making a pattern. Like, maybe [BATT] could be [CHRG], but that would be less clear.
[L_HR] is probably "(something)_HeartRate".
Is this one ready for re-review now? It looks like an implementation everyone is happy with has been settled on
@mark9064 PR #2135 is also on the 1.16.0 milestone, and this code would need to be changed slightly to work with it... so maybe you want to hold off on reviewing this until after that PR gets merged and I update this commit. (Also has to change after #2134 gets merged, but that's only a single line)