Skip to content

Commit

Permalink
Changed weatherforecast to use dt_txt field
Browse files Browse the repository at this point in the history
  • Loading branch information
@jannekalliola
jannekalliola committed May 8, 2018
1 parent cfb39c6 commit 0e2e8d2
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -24,6 +24,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- In default module currentWeather/currentWeather.js line 296, 300, self.config.animationSpeed can not be found because the notificationReceived function does not have "self" variable.
- Fixed browser-side code to work on the Midori browser.
- Fixed issue where heat index was reporting incorrect values in Celsius and Fahrenheit. [#1263](https://github.com/MichMich/MagicMirror/issues/1263)
- Fixed weatherforecast to use dt_txt field instead of dt to handle timezones better

### Updated
- Updated Italian translation
Expand Down
4 changes: 2 additions & 2 deletions modules/default/weatherforecast/weatherforecast.js
View file
Expand Up @@ -333,8 +333,8 @@ Module.register("weatherforecast",{
var forecast = data.list[i];
this.parserDataWeather(forecast); // hack issue #1017

var day = moment(forecast.dt, "X").format("ddd");
var hour = moment(forecast.dt, "X").format("H");
var day = moment(forecast.dt_txt, "YYYY-MM-DD hh:mm:ss").format("ddd");
var hour = moment(forecast.dt_txt, "YYYY-MM-DD hh:mm:ss").format("H");

if (day !== lastDay) {
var forecastData = {
Expand Down

12 comments on commit 0e2e8d2

@berlincount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually broke the weatherforecast for me last night :(

monitor@hoschi:~/github-MichMich-MagicMirror$ date
Mon  2 Jul 12:25:52 CEST 2018
monitor@hoschi:~/github-MichMich-MagicMirror$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.3 LTS
Release:        16.04
Codename:       xenial

@jannekalliola
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that the patch broke the forecast or the original issue broke the forecast?

The date field that is parsed is coming from the weather provider - so the date from your local computer unfortunately does not help in debugging this further.

@berlincount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Reverting it gets me a forecast shown again :-/

@jannekalliola
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then it might cause it. Can you get a dump of JSON data retrieved from the weather service? Unfortunately, I'm not currently near my mirror (and won't be for a week), so I cannot see the problem or debug this by myself.

@berlincount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tired using console.log(), but for some reason nothing from the weatherforceast module shows up in the log - not even the Startup message. I'll have to look into this later, but someone else might be faster.

I linked to this commit on #1332

@Knurlnheim
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a dump of the retrieved JSON :
{"city":{"id":2998324,"name":"Lille","coord":{"lon":3.0586,"lat":50.633},"country":"FR","population":0},"cod":"200","message":7.9564336,"cnt":7,"list":[{"dt":1530529200,"temp":{"day":26.41,"min":19.95,"max":27.93,"night":19.95,"eve":27.04,"morn":26.41},"pressure":1020.55,"humidity":31,"weather":[{"id":800,"main":"Clear","description":"ciel dégagé","icon":"01d"}],"speed":5.61,"deg":69,"clouds":0},{"dt":1530615600,"temp":{"day":28.38,"min":19.27,"max":29.52,"night":19.37,"eve":28.41,"morn":19.27},"pressure":1022.44,"humidity":32,"weather":[{"id":800,"main":"Clear","description":"ciel dégagé","icon":"02d"}],"speed":4.47,"deg":64,"clouds":8},{"dt":1530702000,"temp":{"day":29.2,"min":17.81,"max":30.37,"night":17.81,"eve":28.98,"morn":18.31},"pressure":1019.16,"humidity":35,"weather":[{"id":800,"main":"Clear","description":"ciel dégagé","icon":"02d"}],"speed":2.01,"deg":16,"clouds":8},{"dt":1530788400,"temp":{"day":26.83,"min":16,"max":27.69,"night":17.6,"eve":26.89,"morn":16},"pressure":1021.67,"humidity":43,"weather":[{"id":802,"main":"Clouds","description":"partiellement nuageux","icon":"03d"}],"speed":2.57,"deg":283,"clouds":36},{"dt":1530874800,"temp":{"day":22.19,"min":16.08,"max":22.19,"night":17.44,"eve":21.67,"morn":16.08},"pressure":1026.86,"humidity":0,"weather":[{"id":500,"main":"Rain","description":"légère pluie","icon":"10d"}],"speed":4.51,"deg":34,"clouds":48,"rain":0.85},{"dt":1530961200,"temp":{"day":23.12,"min":17.4,"max":24.1,"night":18.26,"eve":24.1,"morn":17.4},"pressure":1032.35,"humidity":0,"weather":[{"id":800,"main":"Clear","description":"ciel dégagé","icon":"01d"}],"speed":2.7,"deg":315,"clouds":0},{"dt":1531047600,"temp":{"day":25.85,"min":18.9,"max":25.87,"night":18.9,"eve":25.87,"morn":20.04},"pressure":1033.12,"humidity":0,"weather":[{"id":800,"main":"Clear","description":"ciel dégagé","icon":"01d"}],"speed":3.92,"deg":295,"clouds":0}]}

@jannekalliola
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. It does not have dt_txt field at all. No wonder that it does not work properly. As I'm away from my system and development environment, could you try the following code?

var day;
var hour;
if(!!forecast.dt_txt) {
	day = moment(forecast.dt_txt, "YYYY-MM-DD hh:mm:ss").format("ddd");
	hour = moment(forecast.dt_txt, "YYYY-MM-DD hh:mm:ss").format("H");
} else {
	day = moment(forecast.dt, "X").format("ddd");
	hour = moment(forecast.dt, "X").format("H");
}

Replace lines 336 and 337 with this snippet.

@Knurlnheim
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thanks.
I think it's related to the whole "old" vs "new" free API keys debate...
see:
https://openweathermap.org/forecast16#JSON => no dt_txt
but
https://openweathermap.org/forecast5#JSON => has a dt_txt since it's a 3h forecast...

@MichMich
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone sends a PR I’ll try to release an intermediate release to solve this issue.

@jannekalliola
Copy link
Contributor Author

@jannekalliola jannekalliola commented on 0e2e8d2 Jul 2, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1341

I made the pull request to the production branch to make sure that no other commits are on the way. Hopefully this is ok.

And sorry for the hassle, should have though of fallback when made the change. Or tested more thoroughly with various weather forecast configs.

@solelo
Copy link

@solelo solelo commented on 0e2e8d2 Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added replaced the snipped of code and date is still stuck.

@MichMich
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solelo Please try the develop branch.
@jannekalliola No worries. I merged it into develop. Will merge it into master as soon as it's tested.

Please sign in to comment.