Respect time format system-wide #370
Conversation
|
Thanks for this PR! We indeed need to make the time formatting more consistent across the application and to remove code duplication across multiple Screens and Watchfaces. However, in previous PR, we tried to remove most of the call the While your code probably works fine, I would like to see if we can find a better way to do it. With these changes, every time we want to generate the time string, we need to instantiate a char array of 9 chars. If a developer instantiates a smaller array, memory will be corrupted. If they create a bigger array, memory will be wasted. Can we try to find a way to encapsulate the string formatting a bit more? |
|
Maybe just return a |
| sprintf(minutesChar, "%02d", static_cast<int>(minute)); | ||
| char hoursChar[3]; | ||
| char ampmChar[3] = " "; | ||
| if (is24h) { | ||
| sprintf(hoursChar, "%02d", hour); | ||
| } else { | ||
| if (hour == 0) { | ||
| hour = 12; | ||
| sprintf(ampmChar, "AM"); | ||
| } else if (hour < 12) { | ||
| sprintf(ampmChar, "AM"); | ||
| } else if (hour >= 12) { | ||
| hour = hour - 12; | ||
| sprintf(ampmChar, "PM"); | ||
| } | ||
| sprintf(hoursChar, "%02d", hour); | ||
| if (hoursChar[0] == '0') { | ||
| hoursChar[0] = ' '; | ||
| } | ||
| } | ||
|
|
||
| sprintf(string, "%c%c:%c%c %c%c", hoursChar[0], hoursChar[1], minutesChar[0], minutesChar[1], ampmChar[0], ampmChar[1]); |
There was a problem hiding this comment.
This can be reduced down to:
if (is24h) {
sprintf(string, "%02u:%02u ", hour, minute);
} else {
char const* const ampmStr = hour < 12 ? "AM" : "PM;
auto tempHour = hour;
if (tempHour == 0) {
tempHour = 12;
} else if (tempHour > 12) {
tempHour -= 12;
}
sprintf(string, "%2u:%02u %s", tempHour, minute, ampmStr);
}
| } else { | ||
| if (hour == 0) { | ||
| hour = 12; |
There was a problem hiding this comment.
This is a bug. This code was moved from elsewhere in the code where hour was a local variable. Here it's a member of DateTime so modifying it here actually changes the time other parts of the code see.
| "--", "January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"}; | ||
|
|
||
| void DateTime::GetTimeStr(char* string, bool is24h) { |
There was a problem hiding this comment.
It's a personal preference but I dislike using a bool as a stand-in for a two state enum. Making the second argument a ClockType instead would be better IMO. This would actually make the calling code simpler too since they all end up doing a comparison like someClockTypeVariable == ClockType::H24 now anyways.
I thought about this recently and would also like to have the Bluetooth/charging icon display consistently between all screens. Plus I would like a little preview of the timer and stopwatch when active. The idea I had come up with is to create a component to manage this "top bar". The component holds a 12 char cstring. Any screen who wants to use that gets that pointer and calls lv_label_set_text_static() with it. Then it has to call an update method on the component every refresh. (or not, currently the time isn't getting refreshed either) This has the benefit of further unifying things and being able to statically allocate that memory. Its not very flexible though. I imagine there would have to be a method to toggle the time display so its not included in a watch face, but there wouldn't be super fine grain control. I suggest we close this and I make a new PR for that bar controller. |
|
Having a top bar component is a good idea, and having more icons, but the top bar could just use this function for time, so this PR doesn't need to be closed.
If we did this, we wouldn't be able to have the time on the left and icons on the right right?
It is getting refreshed by an LVGL task every 5s currently, which we could just keep using.
I don't think the watch faces shouldn't use the bar anyway. Only the digital watch face might slightly benefit from this, but I don't think it's worth the extra complication. Also about this implementation. Maybe in the DateTime class there should be a char array, and when we run GetTimeStr(), it checks if the array needs to be updated, updates it if necessary, and finally returns a pointer to it. |
|
The top bar is a good idea! Maybe you can have a look at the top layer provided by LVGL, it might be useful to implement it.
Maybe we can split the DateTime class in 2 parts : the one that knows what time it is (based on a timepoint) and another one, closer to the UI, that converts the timepoint into a human readable string (and caches the last string to ensure it's refreshed only when needed). This way, we keep a clear separation between the DateTime component and the UI.
It's up to you! Right now, this PR makes sense, and you can finish it so that your work is not lost and you can still implement the top bar in a new PR. |
|
I've just added this PR to the next milestone so that I know I'm reviewing it. Tell me if you want to close it to open a new one ;) |
|
I think ill finish this one up. Should also get to work on it now since my exams are over. |
|
Where did this leave off? I know @JF002 is wanting to finish up 1.4.... |
I released without this feature, but I'll gladly review and merge it once it's finished :) |
|
Can you please rebase/fix conflicts with this and address jonvmey's comments? |
|
this one should be closed since it have been resolved, unless i'm wrong @Avamander or @JF002 @Riksu9000 ? |
|
This has been implemented in #821 |
added a function in DateTime to give a correctly formatted time. It is used by tiles and quick settings. fixes #368