InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 907 forks source link

Add D/M/Y, M/D/Y format support #1698

Open Brod8362 opened 1 year ago

Brod8362 commented 1 year ago

This PR adds the baseline support for date formats. As part of it, I merged the time format screen into the Date & Time screen (see below).

This addresses #1166, #1590

image

This is more of a draft, there will definitely need to be some more changes system wide.

I did not implement it across the watchfaces. I'm more than willing to, but I wanted to hold off on that in case it should be a separate PR.

I increased the settings version number (from 4 to 5) as a precaution, since there is now a new setting. I do not know the implications of this, so it should be considered before (if, rather) this gets merged.

I don't feel that it is necessary to add Y/D/M or Y/M/D formats, as not all watchfaces show the year.

Possible improvements

I'm still new to the codebase, so please let me know if I've committed some heinous crime in the commits.

Brod8362 commented 1 year ago

A couple people mentioned Y/M/D support as an option - this is very doable, I will add it if this PR has some support behind it. At that point, the question of Y/D/M (even if its a silly format) comes up, for the sake of completion.

minacode commented 1 year ago

No offense and a little off-topic, but I wonder if we could do this as something like a compile-time setting, because it is a good example of a "choose once" setting. Same would apply for the 12/24-hour format.

Brod8362 commented 1 year ago

No offense taken (why would I?). I like the idea of it being a compile time setting, but I think it's a bit unreasonable to expect a user who just opened their watch to immediately then go and start recompiling and flashing new firmware onto their watch just to change their date/time format.

Some of my non-techy friends have PineTimes (not by my recommendation, either). I don't think it's very fair to them to not only remove their ability to change their time format on-watch, but then force them to learn the entire build process just to change their date/time format.

Of course, we're aiming for "sensible defaults", but I (personally) feel that small customizations like this are essential for the user experience.

Brod8362 commented 1 year ago

Any movement on this? I think it would be great to have it for InfiniTime 1.12.

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 407436B 948B
data 940B 0B
bss 53568B 8B
Brod8362 commented 1 year ago

You should implement this in at least one watch face, so we can test it.

Sure. May as well implement it in all of them while I'm at it, it's not a ton of work. This also would make moving it to a bitfield more viable later.

I would rather use the old TimeFormat screen for now.

Sure. I still think it's worth placing the date & time format screens as sub-screens of set date & time screens, what do you think? It feels a bit silly to have one settings for "Set date/time" and another for "Date/Time format"

Riksu9000 commented 1 year ago

I think the time format is not likely to be changed more than once, unlike the current time, so the use cases can be a bit different, but If you think that having separate setting entries for the time format and current time can be considered an issue, you can open an issue about it and we'll continue there.

RageGamerBoi commented 1 year ago

No offense and a little off-topic, but I wonder if we could do this as something like a compile-time setting, because it is a good example of a "choose once" setting. Same would apply for the 12/24-hour format.

Remember, not everyone is going to compile their own build. Most people aren't developers, and/or don't care enough to.

RageGamerBoi commented 1 year ago

This is a good idea, not sure about the "highlighted buttons" though. A little unreadable. Radio buttons should work fine.

Y/M/D is now the new international standard btw, as xkcd helpfully points out:

image

So that should probably be an option as well.