GNOME Bugzilla – Bug 754031
Add weather information to the calendar drop down
Last modified: 2017-03-10 18:05:48 UTC
Notifications and the calendar drop down were redesigned for 3.16. One thing that was in the mockups, but we didn't get around to implementing, was the inclusion of weather information in the calendar drop down. See: https://wiki.gnome.org/Design/OS/Notifications#Time_and_Date_Drop-Down The design is to include a simple, human readable string.
*** Bug 768155 has been marked as a duplicate of this bug. ***
Created attachment 346839 [details] [review] dateMenu: Enforce calendar column width We want the width of the calendar column to be determined by the calendar, other elements should adjust their allocation accordingly. However neither ellipsization nor wrapping will kick in unless the parent's width is restricted, so use a small custom layout manager that enforces the desired behavior.
Created attachment 346840 [details] [review] dateMenu: Do a better job at size freezing while browsing dates In order to avoid distracting popup size changes while browsing other dates, we freeze the size to the last size request. However in case of more complex size negotiations - wrapping or ellipsizing labels, scrollable elements etc. - there's a chance of stray calls to get_preferred_width/height() that are not used for the actual allocation. If such a call happens to be the last size request before the layout is frozen, the saved size will be wrong. To fix this, save the allocated size rather than the requested one.
Created attachment 346841 [details] [review] weather: Add WeatherClient In preparation of integrating GNOME Weather, add a helper class that retrieves weather information according to Weather's configuration if the application is installed.
Created attachment 346842 [details] [review] dateMenu: Add Weather section Similar to the Clocks integration we've had in the date+time drop-down for a while, the designs have called for a similar section that integrates GNOME weather as well. Use the WeatherClient added in the previous commit to implement that section and add it to the popover.
Created attachment 346843 [details] [review] weather: Skip loading indication when updating frequently Weather conditions - at least as far as online services are concerned - don't usually change in a couple of minutes. So when updating shortly after a previous update, assume the current conditions are still valid and trigger an update without showing a loading indication. This should help a bit with not getting stuck permanently in loading state when on a shitty network.
Created attachment 346844 [details] [review] weather: Provide non-capitalized condition strings The condition/sky strings provided by GWeather all use sentence capitalization, which is appropriate when displayed on its own, but results in odd mid-sentence capitals when concatenating several conditions in a sentence. Address this by providing our own non-capitalized versions of those strings and use them instead of the ones from GWeather where appropriate. This adds a fairly large pile of new strings obviously, but hopefully it doesn't impose too much work on translators when GWeather has translations for the capitalized version.
(In reply to Florian Müllner from comment #7) > Created attachment 346844 [details] [review] [review] > weather: Provide non-capitalized condition strings I'd like to ask for a freeze break for this bug, but I think it makes sense to leave out this patch for now. We'll end up with strings that look a bit awkward ("followed by Broken clouds"), but adding this many new strings that short before the string freeze seems unnecessarily rude (even with libgweather already shipping translations that should only differ in case).
Created attachment 346870 [details] [review] dateMenu: Enforce calendar column width Fixed some minor style regressions introduced by not hooking up the layout container to CSS.
Review of attachment 346839 [details] [review]: looks fine
Review of attachment 346840 [details] [review]: never noticed this was happening. the code makes sense
Review of attachment 346841 [details] [review]: ::: js/js-resources.gresource.xml @@ +97,3 @@ <file>ui/userWidget.js</file> <file>ui/viewSelector.js</file> + <file>ui/weather.js</file> I think this file would fit better under js/misc since there's really no UI in it ::: js/ui/weather.js @@ +29,3 @@ + GWeather.Provider.OWM; + this._weatherInfo = new GWeather.Info({ enabled_providers: providers }); + this._weatherInfo.connect('updated', () => { this.emit('changed'); }); you connect and emit here... @@ +65,3 @@ + + this._loading = false; + this.emit('changed'); ... and again here. Won't we get an extra needless 'changed' emission at the very least?
Review of attachment 346842 [details] [review]: looks fine
Review of attachment 346843 [details] [review]: ok
Review of attachment 346844 [details] [review]: not super happy with all this string duplication, meh ::: js/ui/weather.js @@ +399,3 @@ + if (phenom != GWeather.ConditionPhenomenon.NONE && + qualifier != GWeather.ConditionQualifier.NONE) + return conditionsToString(phenom, GWeather.ConditionQualifier.NONE); This logic is only applied when we're not capitalizing. Shouldn't we do this when using the GWeather capitalized versions too?
Review of attachment 346870 [details] [review]: ::: js/ui/dateMenu.js @@ +37,3 @@ // until the selected date changes. this.actor = new St.Button({ style_class: 'datemenu-today-button', + x_expand: true, x_align: St.Align.START, this seems unrelated
(In reply to Rui Matos from comment #16) > this.actor = new St.Button({ style_class: 'datemenu-today-button', > + x_expand: true, x_align: > St.Align.START, > > this seems unrelated It does, but it's not - StBoxLayout has an :x-fill child property that defaults to true. That is, the button takes up the full width of the container, and the align parameter left-aligns the button's child. By switching to the ClutterLayoutManager however, the button no longer receives the additional space by default, so it just takes up the width of the label and is horizontally centered. Adding the :x-expand property has the same effect as the :x-fill default, so this change really just keeps the existing behavior.
(In reply to Rui Matos from comment #15) > not super happy with all this string duplication, meh Yeah, neither am I ... as mentioned in comment #8, I tend towards leaving this one out for now. > ::: js/ui/weather.js > This logic is only applied when we're not capitalizing. Shouldn't we do this > when using the GWeather capitalized versions too? To be honest, I don't know how much of a problem that is in practice, it was just easy to do in this case. We can't really do the same for GWeather provided strings though: We can only get the conditions from a GWeatherInfo, either as string or as phenomenon/qualifier pair (that is, enum values). The functions to resolve the numerical values to a string are private, so the only way we could fix this is duplicating all the capitalized strings again :-(
Review of attachment 346870 [details] [review]: Ok, so this makes sense
(In reply to Rui Matos from comment #12) > I think this file would fit better under js/misc since there's really no UI > in it Yeah, makes sense. > ::: js/ui/weather.js > + this._weatherInfo.connect('updated', () => { this.emit('changed'); > }); > > you connect and emit here... > > @@ +65,3 @@ > + this._loading = false; > + this.emit('changed'); > > ... and again here. Won't we get an extra needless 'changed' emission at the > very least? Mmh, yes and no. Handlers run in the order they were connected, so the ::changed signal above is emitted before the _loading property is set to false - that is, the "Loading" indicator would get stuck. What should work though is to use connect_after() for the first handler and remove the second emission ...
Could we not add the new strings to libgweather instead? That would avoid having translations in 2 different places, which I'm sure the translators would appreciate.
Created attachment 346907 [details] [review] weather: Add WeatherClient Move to misc, use connect_after()
(In reply to Bastien Nocera from comment #21) > Could we not add the new strings to libgweather instead? That would certainly work for me.
Review of attachment 346841 [details] [review]: OK, I don't think it's worth changing it to use connect_after and only emitting 'changed' in one place, it would only make it harder to understand. Just wanted to make sure this wasn't something you left in inadvertently from a previous version.
(In reply to Florian Müllner from comment #23) > (In reply to Bastien Nocera from comment #21) > > Could we not add the new strings to libgweather instead? > > That would certainly work for me. Yes please.
Review of attachment 346907 [details] [review]: mid-air commenting *sigh* looks fine! ::: js/misc/weather.js @@ +65,3 @@ + let id = this._weatherInfo.connect('updated', () => { + this._weatherInfo.disconnect(id); + this._loading = false; maybe add a comment here that 'changed' will be emitted after this
Attachment 346840 [details] pushed as 4b166dc - dateMenu: Do a better job at size freezing while browsing dates Attachment 346842 [details] pushed as 62606c6 - dateMenu: Add Weather section Attachment 346843 [details] pushed as 0e0caee - weather: Skip loading indication when updating frequently Attachment 346870 [details] pushed as 796fdca - dateMenu: Enforce calendar column width Attachment 346907 [details] pushed as da831e8 - weather: Add WeatherClient
(In reply to Florian Müllner from comment #8) > (In reply to Florian Müllner from comment #7) > > Created attachment 346844 [details] [review] [review] [review] > > weather: Provide non-capitalized condition strings > > I'd like to ask for a freeze break for this bug, but I think it makes sense > to leave out this patch for now. We'll end up with strings that look a bit > awkward ("followed by Broken clouds"), but adding this many new strings that > short before the string freeze seems unnecessarily rude (even with > libgweather already shipping translations that should only differ in case). I would prefer to avoid randomly (from the user’s point of view) capitalized words in such a visible place and happily give you a +1 from i18n if you ask for a string freeze break in libgweather.
(In reply to Bastien Nocera from comment #21) > Could we not add the new strings to libgweather instead? Filed as bug 779872. (In reply to Piotr Drąg from comment #28) > I would [...] happily give you a +1 from i18n if you ask > for a string freeze break in libgweather. Heh, I'll be happy to come back to that offer once I got some feedback on the bug.