Fixed clock module sunrise/sunset timezone bug #3070

JakeBinney · 2023-03-20T14:11:32Z

Updated sunrise/sunset times to display user-requested timezone rather than system timezone.

Note: rebase of #3069 against develop rather than master

Hello and thank you for wanting to contribute to the MagicMirror² project

Please make sure that you have followed these 4 rules before submitting your Pull Request:

Base your pull requests against the develop branch.

Include these infos in the description:

Does the pull request solve a related issue?

If so, can you reference the issue like this Fixes #<issue_number>?

What does the pull request accomplish? Use a list if needed.

If it includes major visual changes please add screenshots.

Please run npm run lint:prettier before submitting so that
style issues are fixed.

Don't forget to add an entry about your changes to
the CHANGELOG.md file.

Note: Sometimes the development moves very fast. It is highly
recommended that you update your branch of develop before creating a
pull request to send us your changes. This makes everyone's lives
easier (including yours) and helps us out on the development team.

Thanks again and have a nice day!

@angeldeejay

## [2.22.0] - 2023-01-01 Thanks to: @angeldeejay, @buxxi, @dariom, @dWoolridge, @KristjanESPERANTO, @MagMar94, @naveensrinivasan, @retroflex, @SkySails and @tom. Special thanks to @khassel, @rejas and @sdetweil for taking over most (if not all) of the work on this release as project collaborators. This version would not be there without their effort. Thank you! ### Added - Added test for remoteFile option in compliments module - Added hourlyWeather functionality to Weather.gov weather provider - Removed weatherEndpoint definition from weathergov.js (not used) - Added css class names "today" and "tomorrow" for default calendar - Added Collaboration.md - Added new github action for dependency review (MagicMirrorOrg#2862) - Added a WeatherProvider for Open-Meteo - Added Yr as a weather provider - Added config options "ignoreXOriginHeader" and "ignoreContentSecurityPolicy" ### Removed - Removed usage of internal fetch function of node until it is more stable ### Updated - Cleaned up test directory (MagicMirrorOrg#2937) and jest config (MagicMirrorOrg#2959) - Wait for all modules to start before declaring the system ready (MagicMirrorOrg#2487) - Updated e2e tests (moved `done()` in helper functions) and use es6 syntax in all tests - Updated da translation - Rework weather module - Make sure smhi provider api only gets a maximum of 6 digits coordinates (MagicMirrorOrg#2955) - Use fetch instead of XMLHttpRequest in weatherprovider (MagicMirrorOrg#2935) - Reworked how weatherproviders handle units (MagicMirrorOrg#2849) - Use unix() method for parsing times, fix suntimes on the way (MagicMirrorOrg#2950) - Refactor conversion functions into utils class (MagicMirrorOrg#2958) - The `cors`-method in `server.js` now supports sending and recieving HTTP headers - Replace `…` by `…` - Cleanup compliments module - Updated dependencies including electron to v22 (MagicMirrorOrg#2903) ### Fixed - Correctly show apparent temperature in SMHI weather provider - Ensure updatenotification module isn't shown when local is _ahead_ of remote - Handle node_helper errors during startup (MagicMirrorOrg#2944) - Possibility to change FontAwesome class in calendar, so icons like `fab fa-facebook-square` works. - Fix cors problems with newsfeed articles (as far as possible), allow disabling cors per feed with option `useCorsProxy: false` (MagicMirrorOrg#2840) - Tests not waiting for the application to start and stop before starting the next test - Fix electron tests failing sometimes in github workflow - Fixed gap in clock module when displayed on the left side with displayType=digital - Fixed playwright issue by upgrading to v1.29.1 (MagicMirrorOrg#2969) Signed-off-by: naveen <172697+naveensrinivasan@users.noreply.github.com> Co-authored-by: Karsten Hassel <hassel@gmx.de> Co-authored-by: Malte Hallström <46646495+SkySails@users.noreply.github.com> Co-authored-by: Veeck <github@veeck.de> Co-authored-by: veeck <michael@veeck.de> Co-authored-by: dWoolridge <dwoolridge@charter.net> Co-authored-by: Johan <jojjepersson@yahoo.se> Co-authored-by: Dario Mratovich <dario_mratovich@hotmail.com> Co-authored-by: Dario Mratovich <dario.mratovich@outlook.com> Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com> Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com> Co-authored-by: buxxi <buxxi@omfilm.net> Co-authored-by: Thomas Hirschberger <47733292+Tom-Hirschberger@users.noreply.github.com> Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>

Create Thai Language file. Translation from English to Thai. Co-authored-by: veeck <michael@veeck.de>

Updated sunrise/sunset times to display user-requested timezone rather than system timezone

codecov-commenter · 2023-03-20T14:21:49Z

Codecov Report

Merging #3070 (d68d5bb) into develop (4ef030a) will increase coverage by 0.42%.
The diff coverage is 60.78%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #3070      +/-   ##
===========================================
+ Coverage    27.52%   27.95%   +0.42%     
===========================================
  Files           52       52              
  Lines        11568    11574       +6     
===========================================
+ Hits          3184     3235      +51     
+ Misses        8384     8339      -45

Impacted Files	Coverage Δ
modules/default/calendar/calendar.js	`34.18% <0.00%> (ø)`
modules/default/clock/clock.js	`0.00% <0.00%> (ø)`
modules/default/weather/weather.js	`0.00% <0.00%> (ø)`
modules/default/utils.js	`49.71% <100.00%> (+10.26%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rejas

small change necessary. but thanks for the PR, do you feel experienced enough for writing a test about this too?

rejas · 2023-03-22T22:42:41Z

modules/default/clock/clock.js

+
+			// Convert sunrise/sunset time to local representation of requested timezone
+			if (this.config.timezone) {
+				sunTimes.sunrise = sunTimes.sunrise.toLocaleString("en-us", { timeZone: this.config.timezone });


probably needs to respect the "locale" config value instead of the hardcoded "en-us"

Will update to respect "locale" and investigate writing an integ test

rejas

so, any luck with a test?

rejas

sorry, tested it and doesnt work.
Locale has to be done somthign like

const locale = config.locale??'en-US';

but there seem to be other issues with toLocaleString:

Adding locale: "de-DE" to my config leads then to an "invalid date" on the ui...

JakeBinney · 2023-03-25T13:01:41Z

I'll make that change to fall back to en-US but also start to troubleshoot other locales to figure out what's going on there.

As for a test, is there an existing test module i should add to or is the intention for me to write a new test for the clock module?

JakeBinney · 2023-03-25T16:35:46Z

Confirming that locale de-DE produces same "invalid date" error on my end.

The error is not necessarily within toLocaleString, but rather traces back to the existing formatTime function as the moment call in that function is expecting a datetime in the en-US locale. The de-DE locale gives a date format delimiter of . whereas moment is expecting / delimiter.

Will need to research better internationalization support for this function

JakeBinney · 2023-03-30T03:02:24Z

Found a fix for localization of time. Don't have time to implement it tonight but it is possible to call toLocaleString with options such as:

date.toLocaleString(this.config.locale, {hour: '2-digit', minute: '2-digit', hour12: false} to honor locale.

@cybex-dev

Fixes MagicMirrorOrg#3056 One question here would be if the default for this new option should be true or false. True: keeps the current behaviour, nobody needs to change his config if they rely on this option False: keeps the clock notifications quiet, doesnt waste time/resources, keeps the noise low Maybe the original author @cybex-dev can weigh in on this, and why he added this notification. --------- Co-authored-by: veeck <michael@veeck.de>

@cybex-dev

Fixes MagicMirrorOrg#3056 One question here would be if the default for this new option should be true or false. True: keeps the current behaviour, nobody needs to change his config if they rely on this option False: keeps the clock notifications quiet, doesnt waste time/resources, keeps the noise low Maybe the original author @cybex-dev can weigh in on this, and why he added this notification. --------- Co-authored-by: veeck <michael@veeck.de>

rejas · 2023-04-01T21:33:57Z

Found a fix for localization of time. Don't have time to implement it tonight but it is possible to call toLocaleString with options such as:

date.toLocaleString(this.config.locale, {hour: '2-digit', minute: '2-digit', hour12: false} to honor locale.

Do we need the locale at all? For formatting the time we already have showPeriod / showPeriodUpper in the module, so what else would the locale help here in terms of foramtting time?

Take alook at my PR here: #3073

Maybe you could just use that new util method for formatting the sunrise/sunset time (once it is merged of course)?

JakeBinney · 2023-04-02T15:07:10Z

Using locale was initially intended to be a lightweight fix just for converting timezones, but using moment.tz in the common util works just as well and introduces less disruption to the existing code base. If you're already refactoring formatTime then I like your solution for converting to timezone in that function. I'll update this PR considering that solution. Thanks!

…3079) - current electron v22 has end of life 07 Jul 2023 so it has to be upgraded before next release - removed th.json from root dir (came back with latest release) - updated other dependencies

after the year in calendar repeatingCountTitle (MagicMirrorOrg#2896, second attempt ...)

node v14 will have reached end-of-life by the next release. From April 18th we can use node v20 instead of v19 for testing.

- add test for serveronly, fixes MagicMirrorOrg#3076 - use template literals in port_variable.js.template

While looking at MagicMirrorOrg#3070 I noticed that the weather and clock module do some formatTime stuff, so why not use a common function for that? --------- Co-authored-by: veeck <michael@veeck.de>

needed for new formatTime unit tests. Avoiding ugly TZ setting in github workflows, see MagicMirrorOrg#3073

rejas · 2023-04-08T06:56:49Z

Hi @JakeBinney with #3073 merged, can you check if the latest develop branch fixes your bug too?

…nto develop

JakeBinney · 2023-04-09T00:19:38Z

A bug is still present in latest develop branch. Sunrise/sunset times are now showing in local timezone but are only showing current time. E.g. current time = sunrise time = sunset time. Trying to figure out if I'm crazy...

JakeBinney · 2023-04-09T01:44:37Z

Updated utils.js. Was converting current time instead of converting time passed by SunCalc. Sunrise/Sunset times look good now!

rejas · 2023-04-09T07:10:14Z

Good catch :-) Adjusted my faulty tests so that they would fail if your PR wasnt there.

khassel · 2023-04-09T11:34:34Z

@rejas looking in the changed files of this PR is a mess, there are 18 files changed but in the resulting merge commit only 2 changed.

So in the future I would appreciate to rebase (or merge latest develop) before merging such PR's ...

rejas · 2023-04-09T12:02:23Z

I agree @khassel. Weird why github does this changed-files mess though...

JakeBinney · 2023-04-09T14:00:44Z

I was wondering about that yesterday. Was that something i did? I'm still trying to get better at git hygiene.

khassel · 2023-04-09T15:15:30Z

Was that something i did? I'm still trying to get better at git hygiene.

not actively ...

your PR was very old and I think you never get the changes from develop since you created your branch. With this when your branch is compared to develop it will show your changes and all changes done on develop.

To avoid this you should rebase your branch on current develop (or merge current develop in your branch).

My point was not to merge such a PR without rebase/merge to current develop. We should ask the author to do this or do it ourself if the author is not experienced enough with git.

This all has no impact to the new develop branch after this PR was merged (would be identical if rebase/merge was done before). But if someone looks into this PR and wants to know what has changed, it would be better to see only the 2 files affected instead of 18. So this all is only to get better diffs for reviewing such a PR and for Github PR history.

JakeBinney · 2023-04-09T16:28:08Z

That all makes plenty of sense. I appreciate you taking the time to write all that out!

MichMich and others added 4 commits January 1, 2023 18:09

Fix badge status (MagicMirrorOrg#2991)

2eaf9df

Create Thai Language file (MagicMirrorOrg#3028)

f14e956

Create Thai Language file. Translation from English to Thai. Co-authored-by: veeck <michael@veeck.de>

Fixed clock module sunrise/sunset timezone bug

a9ee654

Updated sunrise/sunset times to display user-requested timezone rather than system timezone

JakeBinney mentioned this pull request Mar 20, 2023

Fixed clock module sunrise/sunset timezone bug #3069

Closed

JakeBinney added 2 commits March 20, 2023 09:48

Fixed style with linter

473219b

Fix tense in CHANGELOG

0822c6c

rejas requested changes Mar 22, 2023

View reviewed changes

JakeBinney and others added 2 commits March 23, 2023 20:40

Minor update to respect user config locale

69d2b69

Merge branch 'develop' into develop

cffa83a

rejas approved these changes Mar 25, 2023

View reviewed changes

rejas requested changes Mar 25, 2023

View reviewed changes

rejas mentioned this pull request Mar 25, 2023

Refactor formatTime into util class #3073

Merged

rejas and others added 2 commits April 1, 2023 21:40

MichMich and others added 9 commits April 4, 2023 20:13

Prepare release 2.23.0

fa40a3e

Merge branch 'master' into develop

c5a8b85

Prepare v2.24.0-develop

cc11b77

Update electron to v24, remove th.json from root dir (MagicMirrorOrg#…

9703226

…3079) - current electron v22 has end of life 07 Jul 2023 so it has to be upgraded before next release - removed th.json from root dir (came back with latest release) - updated other dependencies

removed unneeded (and unwanted) '.' (MagicMirrorOrg#3084)

b7eb21e

after the year in calendar repeatingCountTitle (MagicMirrorOrg#2896, second attempt ...)

Drop node v14 support (MagicMirrorOrg#3085)

0d3ad98

node v14 will have reached end-of-life by the next release. From April 18th we can use node v20 instead of v19 for testing.

added test for serveronly (MagicMirrorOrg#3086)

2c7beea

- add test for serveronly, fixes MagicMirrorOrg#3076 - use template literals in port_variable.js.template

Refactor formatTime into util class (MagicMirrorOrg#3073)

32192d1

While looking at MagicMirrorOrg#3070 I noticed that the weather and clock module do some formatTime stuff, so why not use a common function for that? --------- Co-authored-by: veeck <michael@veeck.de>

set Timezone Europe/Berlin in unit tests (MagicMirrorOrg#3087)

09f117c

needed for new formatTime unit tests. Avoiding ugly TZ setting in github workflows, see MagicMirrorOrg#3073

JakeBinney added 2 commits April 8, 2023 16:48

Merge branch 'develop' of https://github.com/JakeBinney/MagicMirror i…

6d54d11

…nto develop

Removing accidental merge

190b914

Fix timezone convert bug, was using current time

05afa14

Remove code from tests that would make them never fail

d68d5bb

rejas merged commit dee3cd3 into MagicMirrorOrg:develop Apr 9, 2023

Uh oh!

Fixed clock module sunrise/sunset timezone bug #3070

Fixed clock module sunrise/sunset timezone bug #3070

Uh oh!

Conversation

JakeBinney commented Mar 20, 2023

Uh oh!

codecov-commenter commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rejas left a comment

Choose a reason for hiding this comment

Uh oh!

rejas Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

JakeBinney Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

rejas left a comment

Choose a reason for hiding this comment

Uh oh!

rejas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeBinney commented Mar 25, 2023

Uh oh!

JakeBinney commented Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeBinney commented Mar 30, 2023

Uh oh!

rejas commented Apr 1, 2023

Uh oh!

JakeBinney commented Apr 2, 2023

Uh oh!

rejas commented Apr 8, 2023

Uh oh!

JakeBinney commented Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeBinney commented Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rejas commented Apr 9, 2023

Uh oh!

khassel commented Apr 9, 2023

Uh oh!

rejas commented Apr 9, 2023

Uh oh!

JakeBinney commented Apr 9, 2023

Uh oh!

khassel commented Apr 9, 2023

Uh oh!

JakeBinney commented Apr 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-commenter commented Mar 20, 2023 •

edited

Loading

rejas left a comment •

edited

Loading

JakeBinney commented Mar 25, 2023 •

edited

Loading

JakeBinney commented Apr 9, 2023 •

edited

Loading

JakeBinney commented Apr 9, 2023 •

edited

Loading