Add Legend watch face - #2450
Conversation
|
Build size and comparison to main:
|
mark9064
left a comment
There was a problem hiding this comment.
Looks great overall :)
A few comments, but nothing big
I am curioous, how much did you use LLMs (if at all) during development of this watchface?
88014ff to
db9d562
Compare
|
@mark9064 Thanks for the fast review! I have fixed the issues you highlighted and rebased the PR. Can you please review again? |
There was a problem hiding this comment.
I think the code itself is all good. Could you take a look at all the comments though, some of them seem a bit overblown - I would say that most of the description isn't needed because if I want detail I will read the code. Visual markers for sections are probably still useful though. I've highlighted a few here
I find LLMs quite like to do this with comments :)
Also, have you tested this with AOD? Does it still look OK? (It's fine if some UI elements disappear that aren't important, but it needs to stay usable)
|
@mark9064 Thanks! |
|
@mark9064 If any other adjustments are needed, please let me know! |
|
I think there are still a couple verbose comments, I don't think any of them need to be longer than a line. The rewords you have done so far are nice improvements :) Looks great! |
|
@mark9064 Thanks for the feedback! Can you please review? |
|
@mark9064 Could it be possible to put this already in 1.17.0? P.S. The force-push a couple minutes ago was just rebase using GitHub web (no code changes there). |
|
Nothing to do on your end, just waiting for a second review |
|
Hi @DavisNT thanks for this watch face, I really like it. I've tested on the simulator and am running this watch face as my daily driver now and that's working well too. Would you be able to move the It would be good if @mark9064 could chime in on this suggestion. The Services (Alarm, Timer, StopWatch) here https://github.com/DavisNT/InfiniTime/blob/3caae05b9b5f6130c36bbdc2d79c04abc75fc9d8/src/displayapp/screens/WatchFaceLegend.cpp#L168 will never update after the watch face is created so they don't really need to be dirty values and can be "drawn" on entry here https://github.com/DavisNT/InfiniTime/blob/3caae05b9b5f6130c36bbdc2d79c04abc75fc9d8/src/displayapp/screens/WatchFaceLegend.cpp#L57. When they do change, you have moved away from the watch face. |
|
Good spot - yes, the refresh call should move before Your dirtyvalue analysis is correct. I think it's nice to be defensive though when it's almost free. If we wanted to allow changing them say with the companion app in the future, there is now less to change PS you can use the review tool on the files page when you have comments like this and leave them in specific locations so people can see what you're talking about easily |
An elegant and very practical digital watch face. Uses only fonts already in InfiniTime and no external resources. Has the standard functionality: * shows time, date (without year) and day of week * supports 24 and 12 hour time formats * BLE, new notification and alarm clock icons * battery percentage (green when charging) * steps and heart rate * temperature and weather icon Additionally: * visual indication (amber dot) for any notification(s) * visual indication (red or blue line) when notifications are muted * battery percentage turns red when 20% or less * stopwatch and timer icons (when active) * colorful sun, snow and thunderstorm weather icons * generally uses brighter colors for more urgent things
22037cb to
5c0fdd2
Compare
|
@SteveAmor Thank you very much for the review and analysis! I have moved the P.S. Sorry for the repeated pushes! I accidentally pushed old version first. |



An elegant and very practical digital watch face.
Uses only fonts already in InfiniTime and no external resources. Has the standard functionality:
Additionally: