Watchface: Numerals #2047
Watchface: Numerals #2047
Conversation
|
Build size and comparison to main:
|
mark9064
left a comment
mark9064
left a comment
There was a problem hiding this comment.
Looks great from the screenshot! I'll give this a test run after any changes from feedback are in
|
I'll test this on my PT shortly and review after |
mark9064
left a comment
mark9064
left a comment
There was a problem hiding this comment.
The font ranges at the moment are very large, and the watchface cannot load (too much memory)
With these changes it works and everything seems as expected 😄
|
Gotcha, I'll make a commit when I have the chance! |
mark9064
left a comment
mark9064
left a comment
There was a problem hiding this comment.
Sorry for the slow review! I'm very busy, and there are many PRs. I think this is almost there!
| Controllers::DateTime::Days currentDayOfWeek = Pinetime::Controllers::DateTime::Days::Unknown; | ||
| uint8_t currentDay = 0; | ||
| Utility::DirtyValue<bool> notificationState {}; | ||
| Utility::DirtyValue<std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>> currentDateTime {}; |
There was a problem hiding this comment.
Instead of storing all of the current day/hour/etc as variables, how about having two time points:
Utility::DirtyValue<std::chrono::time_point<std::chrono::system_clock, std::chrono::minutes>> currentDateTime;
Utility::DirtyValue<std::chrono::time_point<std::chrono::system_clock, std::chrono::days>> currentDate;Then if the currentDateTime is updated, redraw hours and minutes / AM/PM. Then set currentDate to std::chrono::time_point_cast<std::chrono::days>(currentDateTime.Get());, and see if currentDate is updated. If so, then redraw the day, day of week etc
Make sure to remove the variables that are unused after this
|
|
||
| dateDay = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(dateDay, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_WHITE); | ||
| lv_label_set_text(dateDay, "--"); |
There was a problem hiding this comment.
If this is about to be set in Refresh(), is it possible to skip giving it a value here? Same goes for the day of week
| lfs_file f = {}; | ||
| if (filesystem.FileOpen(&f, "/fonts/rounded_large.bin", LFS_O_RDONLY) >= 0) { | ||
| filesystem.FileClose(&f); | ||
| font_large = lv_font_load("F:/fonts/rounded_large.bin"); |
| labelTimeAMPM1 = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_label_set_text_static(labelTimeAMPM1, ""); | ||
| lv_obj_set_style_local_text_font(labelTimeAMPM1, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, font_small); | ||
| lv_obj_set_style_local_text_color(labelTimeAMPM1, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, lv_color_hex(0x999999)); |
There was a problem hiding this comment.
Since this colour is being used a couple of times I think it would be best if were a static constexpr member of the class (i.e static constexpr lv_color_t AMPMColor = lv_color_hex(0x999999) or similar)
|
Are there any plans to add this to the main firmware? I would love this as my watchface. |
|
Waiting for @liamcharger to respond on review still. I think it's a good fit for InfiniTime :) |
|
Yep, I’ve just been busy :) I fixed the conflicts and updated the branch with the GitHub UI, I’ll break into the code soon to finish the review :) |
|
When can we expect to see a 1.16.1 build with this watchface? (or maybe 1.17.0, I don't know the plan) |
|
It'll definitely won't be before 1.17. We would only push 1.16.1 update if there was a critical bug. Anyways 1.17 would be a good bet. It just depends on how fast I finish out the review and what the maintainers decide to merge |
4 participants
This PR adds a Numeral watch face, with inspiration from Apple Watch's numeral watch face. An image is attached below.
