After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 780404 - gdm's gnome-shell tries to fetch the weather
gdm's gnome-shell tries to fetch the weather
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.23.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2017-03-22 14:59 UTC by Bastien Nocera
Modified: 2017-11-21 18:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WeatherClient: set enabled providers after setting a valid location (2.09 KB, patch)
2017-04-02 23:12 UTC, Rares Visalom
none Details | Review
WeatherClient: set enabled providers after setting a valid location (2.36 KB, patch)
2017-04-13 09:11 UTC, Rares Visalom
committed Details | Review

Description Bastien Nocera 2017-03-22 14:59:22 UTC
gnome-shell-3.23.92-2.fc26.x86_64
gdm-3.23.92-1.fc26.x86_64

Looks like gdm's gnome-shell is trying to poke the network, probably before the Wi-Fi is up:
gdm       1021  0.1  0.7 2315444 121756 tty1   Sl+  15:17   0:02 /usr/bin/gnome-shell

Mar 22 15:17:13 classic gnome-shell[1021]: Failed to get Yr.no forecast data: 2 Error resolving “api.met.no”: Name or service not known

So:
1) it shouldn't try to get the weather if there's no connection to the outside internet
2) it shouldn't try to get the weather for gdm
Comment 1 Rares Visalom 2017-03-22 22:24:46 UTC
I will look into this :).
Comment 2 Rares Visalom 2017-04-02 23:12:31 UTC
Created attachment 349153 [details] [review]
WeatherClient: set enabled providers after setting a valid location

So far, the GWeatherInfo was given the enabled weather providers
as a parameter, at construction time. Because of the way in
which libgweather was designed, setting the providers right from
the beginning enabled libgweather to use them internally in order
to update its state. Updating the internal state is only relevant
when there is a valid location set, which is not guaranteed at the
time when the GWeatherInfo object is constructed.

In order to fix this, enable no providers at construction time and
only set valid providers after setting a valid location.
Comment 3 Florian Müllner 2017-04-05 23:43:40 UTC
Review of attachment 349153 [details] [review]:

::: js/misc/weather.js
@@ +57,3 @@
+        this._providers = GWeather.Provider.METAR |
+                          GWeather.Provider.YR_NO |
+                          GWeather.Provider.OWM;

Making this a property seems odd - resolving the enum values isn't so expensive that caching would be required, and the value is still only used in a single place, so you could just move the local variable to _setLocation() where it's used.

Alternatively, a const would be more appropriate than a property IMHO:

// Keep consistent with gnome-weather
const ENABLED_PROVIDERS = GWeather.Provider.METAR |
                          ...
Comment 4 Rares Visalom 2017-04-13 09:11:52 UTC
Created attachment 349770 [details] [review]
WeatherClient: set enabled providers after setting a valid location

So far, the GWeatherInfo was given the enabled weather providers
as a parameter, at construction time. Because of the way in
which libgweather was designed, setting the providers right from
the beginning enabled libgweather to use them internally in order
to update its state. Updating the internal state is only relevant
when there is a valid location set, which is not guaranteed at the
time when the GWeatherInfo object is constructed.

In order to fix this, enable no providers at construction time and
only set valid providers after setting a valid location.
Comment 5 Florian Müllner 2017-04-13 16:36:14 UTC
Review of attachment 349770 [details] [review]:

LGTM - shame it missed 3.24.1, but let's get this into the next release at least, so please push to both master and gnome-3.24.
Comment 6 Marco Trevisan (Treviño) 2017-11-21 18:09:46 UTC
Look like a similar version of this patch has been pushed already as commit 8e443a2aff44, so we can mark this as fixed.
Comment 7 Marco Trevisan (Treviño) 2017-11-21 18:11:11 UTC
Review of attachment 349770 [details] [review]:

.