GNOME Bugzilla – Bug 728117
Add "links" to clocks/weather
Last modified: 2015-08-17 12:17:45 UTC
When selecting a location in gnome-maps, it would be nice to be able to add it easily to clocks or weather from within maps.
I think its severity should be enhancement as its a feature request.
Are there currently any ways/APIs to add locations to clocks or weather? They both seem to store their locations in gsettings. Key 'locations' for Weather and 'world-clocks' for Clocks. Maybe we could get some Clocks / Weather devs to comment on the feasibility of this bug.
Gnome-weather is dbus activatable, I'll be happy to add a method that opens the app to a specific location and offers to store it. The preferred format would obviously be a GWeatherLocation in GVariant, but if you don't want a dependency on libgweather a pair of coordinates would be fine as well. I'd like that gnome-clocks did not use our settings directly though.
(In reply to comment #3) > I'd like that gnome-clocks did not use our settings directly though. I guess you mean gnome-maps here. Same for clocks: I am ok with adding a dbus method to add a clock location.
Thanks for the input guys! I'm not sure yet that this is a feature that we want but now we know that we can have it :) Bastian: what are the use cases that you envision? That someone is using Maps to look up something and thinks: "I wonder what time it is here?" or "I wonder what the weather is like?" When adding locations to track weather or time on the natural thing might be to open one of the apps that deals with that. We shouldn't add things to Maps just because we can. But I can be persuaded. This bug depends on bug #722871, a bug that aims to implement the mockups shown here: https://raw.github.com/gnome-design-team/gnome-mockups/master/maps/v2/markers-and-bubbles.png If the designers can find a way to get UI for doing this nicely it might be a feature to have. Thanks!
(In reply to comment #5) > Thanks for the input guys! > I'm not sure yet that this is a feature that we want but now we know that we > can have it :) > > Bastian: what are the use cases that you envision? That someone is using Maps > to look up something and thinks: > "I wonder what time it is here?" or "I wonder what the weather is like?" I'll be travelling there, and I know the weather/time are different, so I want to add check the weather to pack ahead of me going there.
Yeah this seems like a nice integration, the important thing is to not make the bubbles too cluttered (see current designs here: https://raw.github.com/gnome-design-team/gnome-mockups/020b66b292801ae281c54cb71b11d2eea8536edf/maps/v2/markers-and-bubbles.png). Android pushes these types of actions into the "share"-menu. Would that make sense? Andreas what do you think?
That seems nice, we could add it to a "share"-button or in a "gear"-button. Paolo, Giovanni: I would like to request those dbus methods. What kind of data would you need? lat/lon/description?
(In reply to comment #8) > That seems nice, we could add it to a "share"-button or in a "gear"-button. > > Paolo, Giovanni: I would like to request those dbus methods. What kind of data > would you need? lat/lon/description? Ideally, you would just use a serialized GWeatherLocation - and if you do so, gnome-weather will need no changes, it already accepts show-location as a GAction with a single variant parameter (the serialized location). But I can understand that GWeatherLocation is inconvenient for you, so I'm ok with just latitude/longitude/description. It's a lot better if the description is cleaned up to be the city name only, without the country, because libgweather can figure that out on its own (but if you know how to get that from geocode-glib, please tell me!). And if you could get the ICAO code, that would give the most accurate results. In any case, please use a GAction (org.freedesktop.Application.ActivateAction) rather than a raw method call, because that deals with startup notification in a transparent way on the receiver side - the sender side is a little more convoluted but it's the only way to do proper complete startup notification (which is needed in case the app is not running yet). Alternatively, if you absolutely want a raw method call, please give us a timestamp to get working focus stealing prevention.
(In reply to comment #9) > (In reply to comment #8) > > That seems nice, we could add it to a "share"-button or in a "gear"-button. > > > > Paolo, Giovanni: I would like to request those dbus methods. What kind of data > > would you need? lat/lon/description? > > Ideally, you would just use a serialized GWeatherLocation - and if you do so, > gnome-weather will need no changes, it already accepts show-location as a > GAction with a single variant parameter (the serialized location). > But I can understand that GWeatherLocation is inconvenient for you, so I'm ok > with just latitude/longitude/description. It's a lot better if the description > is cleaned up to be the city name only, without the country, because > libgweather can figure that out on its own (but if you know how to get that > from geocode-glib, please tell me!). And if you could get the ICAO code, that > would give the most accurate results. Ok, thanks! I can mock it up using GWeatherLocation and see how much hassle it would be for us. We try to clean the city name up our self. Geocode-glib indeed messes up a bit by setting the name property of a place to what is actually a display name returned from the Nominatim service. We do some logic and also uses the Overpass API in some areas to get better information / name. Will do our best to send something pretty to you. Atm we do not use the ICAO code anywhere. > > In any case, please use a GAction (org.freedesktop.Application.ActivateAction) > rather than a raw method call, because that deals with startup notification in > a transparent way on the receiver side - the sender side is a little more > convoluted but it's the only way to do proper complete startup notification > (which is needed in case the app is not running yet). Alternatively, if you > absolutely want a raw method call, please give us a timestamp to get working > focus stealing prevention. Sure, thanks for the information!
Created attachment 290961 [details] [review] utils: Add activateAction function
Created attachment 290962 [details] [review] Add SharePopover Add the SharePopover module, which includes sharing a place to GNOME Weather.
Created attachment 290963 [details] [review] MapBubble: Add share button
Created attachment 290964 [details] [review] Add share button to userLocation and searchResult
Hi, So this adds weather to a popover when you press a share button on a map bubble. This is also a request for mockups because I am not sure how this should look or behave. Will attach a screen cast below. Giovanni: Review of the activateAction code would be appreciated since I haven't messed about with that before. I seems to made a mistake(?) since weather opens a new window for each request. I would expect it just to come into focus? Andreas: Atm I use the 'folder-publicshare-symbolic' icon for the button. Do you have an opinion on how this should behave? Should it be 'share'? And in that case should we have our own icon? Jonas
Created attachment 290965 [details] Cast of share popover opening weather
Also as I just noticed when watching the cast again. It seems to open to Malmö first time and second time correct to Stockholm. Giovanni: Is this a bug on my or your side? :)
Paolo: An action like "add-location" would be nice to have on Clocks, it could take a GWeather.Location if you like. If you are busy it would be nice if you could just give some details on how it should behave and maybe some one on the Maps side could look into it.
A patch for it would be more than welcome and I agree that exposing an action on the bus is the best way. I am not totally thrilled by exposing GWeather.Location on the public API since for clocks gweather is more of an internal implementation detail, on the other hand I do not have a much better idea out of the top of my head. I guess in the worse case we can just bump the dbus interface to not expose that action if we remove the dependency. For the implementation it should be relatively easy once the GAction/Dbus yak shaving is done: check how on_new_activate is implemented in world.vala. It pops up a GWeatherLocation dialog, which lets you pick a GWeather.Location and once you have that the codepath should be the same of a the new add-location action
Thanks! Will try to fiddle up a patch for Clocks when I get some time!
Another question for Paolo and Giovanni: Should the names of your applications be translatable? Or is it 'Weather' and 'Clocks' in all languages?
Created attachment 290977 [details] Cast of share popover opening Clocks
(In reply to comment #21) > Another question for Paolo and Giovanni: Should the names of your applications > be translatable? Or is it 'Weather' and 'Clocks' in all languages? They are translated in other languages.
Created attachment 290984 [details] [review] Add SharePopover Add the SharePopover module, which includes sharing a place to GNOME Weather.
(In reply to comment #17) > Also as I just noticed when watching the cast again. It seems to open to Malmö > first time and second time correct to Stockholm. > > Giovanni: Is this a bug on my or your side? :) Could perfectly be a bug in gnome-weather... You can verify using the gnome-weather search provider (which uses the same API once you click on a city) (In reply to comment #21) > Another question for Paolo and Giovanni: Should the names of your applications > be translatable? Or is it 'Weather' and 'Clocks' in all languages? How about you just grab the name from the desktop file? Then we don't duplicate translations, and you can check if the app is installed or not.
Review of attachment 290961 [details] [review]: ::: src/utils.js @@ +137,3 @@ +function _getPlatformData() { + let timestamp = Math.round(+new Date() / 1000); Absolutely no. You need a X11 server time here (which is clock_gettime MONOTONIC, but don't rely on that, and in any case it has nothing to do with wall clock time). Use Gtk.get_current_event_time() if you're inside an event handler, or keep the timestamp from the event that triggered the action otherwise. @@ +139,3 @@ + let timestamp = Math.round(+new Date() / 1000); + let id = GLib.Variant.new('s', '_TIME' + timestamp); + return { "desktop-startup-id": id }; Almost. A desktop-startup-id of _TIMExxxxxx is enough to get focus-stealing-prevention working (so the app will not start in the background and you won't get the annoying "is ready" notification), but it won't do startup notification (which is especially important for gnome-weather because it takes a while to load) What you want is: let context = Gdk.Display.get_default().get_app_launch_context() context.set_timestamp(timestamp); let id = new GLib.Variant('s', context.get_startup_notify_id());
Review of attachment 290984 [details] [review]: ::: src/sharePopover.js @@ +64,3 @@ + action, + new GLib.Variant('v', city.serialize())); + Application.application.unmark_busy(); This mark_busy()/unmark_busy() is pointless - the associated dbus messages will be emitted in close succession, and most likely within 16ms, so they will be ignored by the shell. You can mark_busy() here and unmark_busy() when receiving the dbus reply (which will be after the app is shown and ready), but again startup notification is better - after all it's not clocks who's busy. @@ +76,3 @@ + shares = true; + else + this._weatherRow.destroy(); While it probably works, it doesn't seem a good idea to destroy template children. How about .hide()?
Review of attachment 290961 [details] [review]: Thanks for review! The experience is _so_ much better with this, correct, approach.
(In reply to comment #25) > (In reply to comment #17) > > Also as I just noticed when watching the cast again. It seems to open to Malmö > > first time and second time correct to Stockholm. > > > > Giovanni: Is this a bug on my or your side? :) > > Could perfectly be a bug in gnome-weather... You can verify using the > gnome-weather search provider (which uses the same API once you click on a > city) > Will try to isolate it, I am still seeing it, it gives me Malmö first time and correct location second itme. > (In reply to comment #21) > > Another question for Paolo and Giovanni: Should the names of your applications > > be translatable? Or is it 'Weather' and 'Clocks' in all languages? > > How about you just grab the name from the desktop file? Then we don't duplicate > translations, and you can check if the app is installed or not. Sounds sane, will try that. Any hints on how I get the translated name from the desktop file? Do I need to know the locale and pick the correct name-key, or is there a helper for that somewhere? Will look around.
Review of attachment 290984 [details] [review]: ::: src/sharePopover.js @@ +64,3 @@ + action, + new GLib.Variant('v', city.serialize())); + Application.application.unmark_busy(); Yeah, you are right, thanks. The startup notification makes this seem silly, lucky you are around! @@ +76,3 @@ + shares = true; + else + this._weatherRow.destroy(); Hide works for me.
Created attachment 291058 [details] [review] utils: Add activateAction function
Created attachment 291059 [details] [review] Add SharePopover Add the SharePopover module, which includes sharing a place to GNOME Weather.
Created attachment 291060 [details] [review] MapBubble: Add share button
Created attachment 291061 [details] [review] Add share button to userLocation and searchResult
Created attachment 291062 [details] New cast!
Created attachment 291089 [details] [review] utils: Add activateAction function
Created attachment 291090 [details] [review] Add ShareDialog Add the ShareDialog module, which includes sharing a place to GNOME Weather and GNOME Clocks.
Created attachment 291092 [details] [review] MapBubble: Add share button
Created attachment 291093 [details] [review] Add share button to userLocation and searchResult
Created attachment 291094 [details] New cast!
Created attachment 291095 [details] [review] Add ShareDialog Add the ShareDialog module, which includes sharing a place to GNOME Weather and GNOME Clocks.
Review of attachment 291095 [details] [review]: ::: src/share-dialog.ui @@ +35,3 @@ + <style> + <class name="text-button"/> + <class name="suggested-action"/> I set this class, but the button is not blue :( What am I doing wrong?
(In reply to comment #41) > Created an attachment (id=291095) [details] [review] > Add ShareDialog + <property name="label" translatable="yes">_Cancel</property> + <property name="label" translatable="yes">_Choose</property> Are these two buttons (and mnemonics) in the very same dialog?
(In reply to comment #43) > (In reply to comment #41) > > Created an attachment (id=291095) [details] [review] [details] [review] > > Add ShareDialog > > + <property name="label" translatable="yes">_Cancel</property> > + <property name="label" translatable="yes">_Choose</property> > Are these two buttons (and mnemonics) in the very same dialog? They are in the same dialog, yes. Have I messed up in some way? Thanks Jonas
Using the very same mnemonic is just a bit unfortunate.
Created attachment 291139 [details] [review] Add ShareDialog Add the ShareDialog module, which includes sharing a place to GNOME Weather and GNOME Clocks.
Created attachment 291143 [details] [review] Add ShareDialog Add the ShareDialog module, which includes sharing a place to GNOME Weather and GNOME Clocks.
Good to see the buttons being separated now! They are still very close to each other though, so it would be good with a gap of, say, 6px or so between them. Just to make it look better. The dialog looks good, but I would rename the button "Choose" to "Share". Choosing is what you do by selecting an item in the list. Sharing is the actual action.
Created attachment 291168 [details] New cast! Thanks Andreas! How about something like this?
Looks great!
Created attachment 291171 [details] [review] utils: Add activateAction function
Created attachment 291172 [details] [review] Add ShareDialog Add the ShareDialog module, which includes sharing a place to GNOME Weather and GNOME Clocks.
Created attachment 291173 [details] [review] MapBubble: Add share button
Created attachment 291174 [details] [review] Add share button to userLocation and searchResult
Review of attachment 291171 [details] [review]: Looks good, except for fringe bug and nit (feel free to ignore that one, I know you're not a fan of ternary expressions :)) ::: src/utils.js @@ +146,3 @@ + +function activateAction(appId, action, parameter, timestamp) { + let wrappedParam = []; let wrappedParam = parameter ? [parameter] : []; @@ +151,3 @@ + + if (parameter) + wrappedParam = [parameter]; ...and then remove this. @@ +165,3 @@ + c.call_finish(res); + } catch(e) { + Utils.debug('ActivateApplication: ' + e); Just debug (Utils isn't defined here).
Review of attachment 291172 [details] [review]: Some code style thoughts and a little bug. Also do I need weather and clocks from JHBuild for this to work? Clocks doesn't start and Weather only shows malmö even if I choose new york. ::: src/shareDialog.js @@ +61,3 @@ + this._cancelButton.connect('clicked', (function() { + this.response(Response.CANCEL); + }).bind(this)); this._cancelButton.connect('clicked', this.response.bind(this, Response.CANCEL)); @@ +66,3 @@ + let rows = this._list.get_selected_rows(); + if (rows.length === 0) + this.response(RESPONSE.CANCEL); RESPONSE → Response @@ +69,3 @@ + + if (rows[0] === this._weatherRow || rows[0] === this._clocksRow) { + let world = imports.gi.GWeather.Location.get_world(); Import GWeather above instead and then: > let world = GWeather.Location.get_world(); @@ +89,3 @@ + this.response(Response.SUCCESS); + } + }).bind(this)); Break this out into _onChooseButtonClicked instead. @@ +115,3 @@ + + return shares; + }, A version with less state to think about: > let shareWeather = this._checkWeather(); > let shareClocks = this._checkClocks(); > > if (!shareWeather) > this._weatherRow.hide(); > if (!shareClocks) > this._clocksRow.hide(); > > return shareWeather || shareClocks; @@ +122,3 @@ + return false; + + return true; Assuming info can't be undefined: > let info = Gio.DesktopAppInfo.new(appId + '.desktop'); > return info !== null; If it can, I'd add this to utils.js: function hasValue(v) { return v !== undefined && v !== null; } and then > let info = Gio.DesktopAppInfo.new(appId + '.desktop'); > return Utils.hasValue(info); @@ +129,3 @@ + return false; + + if (!imports.gi.GWeather) Just GWeather @@ +132,3 @@ + return false; + + return true; This function can be condensed to a (to me) more readable one-liner: > return (GWeather !== null && this._checkApp(_WEATHER_APPID)); or if GWeather can be undefined as well us the hasValue helper from above: > return (Utils.hasValue(GWeather) && this._checkApp(_WEATHER_APPID)); @@ +143,3 @@ + + return true; + } Same remarks as checkWeather
Review of attachment 291173 [details] [review]: ACK
Review of attachment 291174 [details] [review]: ACK
Attachment 291171 [details] pushed as 0bdf305 - utils: Add activateAction function Attachment 291172 [details] pushed as b621c3a - Add ShareDialog Attachment 291173 [details] pushed as 210c9c8 - MapBubble: Add share button Attachment 291174 [details] pushed as 990681e - Add share button to userLocation and searchResult
Created attachment 291230 [details] [review] PlaceStore: Add getModelForPlaceType method https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291231 [details] [review] PlaceStore: Add removePlace method https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291232 [details] [review] Split out PlaceListRow from SearchPopup https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291233 [details] [review] Add FavoritesPopover https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291234 [details] [review] MapBubble: Add favorite button https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291235 [details] [review] MainWindow: Add favorites toggle https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291236 [details] [review] PlaceStore: Add addPlace method https://bugzilla.gnome.org/show_bug.cgi?id=722102
Created attachment 291237 [details] [review] SearchResultBubble: Add favorite button https://bugzilla.gnome.org/show_bug.cgi?id=722102