Lacerte / clima

MOVED TO https://codeberg.org/Lacerte/clima
Mozilla Public License 2.0
105 stars 14 forks source link

Some times don't respect local format #292

Open WhyNotHugo opened 2 years ago

WhyNotHugo commented 2 years ago

Describe the bug

I use 24hs time format, e.g.: without subtracting 12 after midday.

In a few spots, the application doesn't respect this and uses the am/pm time format.

Times that respect system locale

Times that DO NOT respect system locale

This last one is the really annoying one, since it makes reading the forecast a bit trickier.

Screenshots

image

image

image

Smartphone (please complete the following information):

Additional context

Thanks for the very beautiful and simple weather app! :beers:

triallax commented 2 years ago

This should be pretty easy to fix; I might ping you on the PR that fixes this so that you can take a look.

WhyNotHugo commented 2 years ago

The ones that are correct use DateFormat.Hm().format:

https://github.com/Lacerte/clima/blob/a652d03e4f223c3f98dd61e1829dde806e29b04f/packages/clima_ui/lib/widgets/weather/additional_info_widget.dart#L110-L112

These don't:

https://github.com/Lacerte/clima/blob/c6468367aac4606cdb0353771091be890bf1273a/packages/clima_ui/lib/screens/weather_screen.dart#L132

https://github.com/Lacerte/clima/blob/c6468367aac4606cdb0353771091be890bf1273a/packages/clima_ui/lib/widgets/weather/hourly_forecasts_widget.dart#L46-L48

WhyNotHugo commented 2 years ago

Huh, dart is pretty easy to read... At least for these simple UI bits.

triallax commented 2 years ago

Something is weird; j is supposed to follow the system locale, while H forces the 24-hour format (see https://api.flutter.dev/flutter/intl/DateFormat-class.html). Why isn't j following the system locale as it's supposed to?

WhyNotHugo commented 2 years ago

Yup, reading those docs it would seem that using j for all these instances should be the correct approach.

But the places that do use j are rendering in US format for me... Is the OS locale automatically picked up? Maybe initializeDateFormatting needs to be called with the system setting...?

WhyNotHugo commented 2 years ago

Never mind:

If the optional locale is omitted, the format will be created using the default locale in Intl.systemLocale.

triallax commented 2 years ago

From https://api.flutter.dev/flutter/intl/Intl/systemLocale.html:

Note that due to system limitations this is not automatically set, and must be set by importing one of intl_browser.dart or intl_standalone.dart and calling findSystemLocale().

Maybe this is it?

WhyNotHugo commented 2 years ago

Oh, nice find!

WhyNotHugo commented 2 years ago

So I guess it's a matter of calling that at startup, and the using j in https://github.com/Lacerte/clima/blob/a652d03e4f223c3f98dd61e1829dde806e29b04f/packages/clima_ui/lib/widgets/weather/additional_info_widget.dart#L110-L112

triallax commented 2 years ago

Yeah, that sems like what we should do. I'm wondering now though about how to handle the system locale changing while the app is running.

WhyNotHugo commented 2 years ago

It doesn't seem the abstractions provided account for that ever happening.

Edit: Maybe call findSystemLocale when the app re-gains focus?

triallax commented 2 years ago

Maybe, but I guess we can leave that to another PR?

WhyNotHugo commented 2 years ago

Yeah, that seems like another issue IMHO.

WhyNotHugo commented 2 years ago

FWIW, the top date could actually read "Just updated" and then "Updated X Y ago", where Y in ["minute", "hours", "days"] and X is an int.

triallax commented 2 years ago

Hmm, that's a nice suggestion; it makes it easier to know at a glance if the weather is outdated. On the other hand, we will have to handle updating the time, though it shouldn't be too much trouble.

Feel free to open an issue for this; I'll hide our comments because they're off-topic to the issue.

triallax commented 2 years ago

I tried to fix this yesterday, and it turned out to be more of a pain than I had expected, though it's mostly done. Due to locale stuff, fixing https://github.com/Lacerte/clima/issues/91 first is probably a good idea. In any case, I'm going to submit a draft PR with my work on this issue.