GNOME Bugzilla – Bug 706291
Add icon uri from nominatim as place property
Last modified: 2013-08-26 16:29:47 UTC
Nominatim exposes URI for icons that represent the type of the place in the search results from a query. The patch below adds this as a property, "icon" to GeocodePlace. Thanks.
Created attachment 252189 [details] [review] Add icon uri from nominatim as place property
Created attachment 252190 [details] [review] Add icon uri from nominatim as place property v2 typo fixups
Created attachment 252194 [details] [review] Add icon uri from nominatim as place property v3 I guess we still want to build docs, even tho we offer icon uris. (removed docs because my setup gives me errors while building with docs)
Review of attachment 252194 [details] [review]: A couple of comments and questions: - first, I would expect the "icon" property to be an actual icon (a GIcon, a GdkPixbuf, etc.) not a URI. Can you rename this to "icon-uri"? - How many types of places are there? Does it make sense to do this on the client side instead? What do the icons look like? Is it better to have icons be handled client-side, where we can theme them? Marking as needs-work for the first item, but having a discussion about the other items is probably a good idea before you make any more changes.
(In reply to comment #4) > Review of attachment 252194 [details] [review]: > > A couple of comments and questions: > - first, I would expect the "icon" property to be an actual icon (a GIcon, a > GdkPixbuf, etc.) not a URI. Can you rename this to "icon-uri"? Sure, makes sense. > - How many types of places are there? Does it make sense to do this on the > client side instead? What do the icons look like? Is it better to have icons be > handled client-side, where we can theme them? > There are around 30 places, you can see the list of them in geocode-place.h. Having them client so they can be themed are of course nice. It is possible for applications today, to just look at the place-type property and select their own custom icon for that. The icons looks like this: http://nominatim.openstreetmap.org/images/mapicons/poi_place_city.p.20.png For a city. You can also search in nominatim, for instance for New York: http://nominatim.openstreetmap.org/search.php?q=new+york&viewbox=13%2C55.59%2C13.02%2C55.58 Too see. The icons are supposed to be a visual aid to see what kind of place you have found in the search results. The plan with adding this to geocode-glib was to use it in a search results popup in gnome-maps, possibly. Maybe it is possible to have the possibility of both client-side and server-side icons. The mapping service MapQuest seem to do that, in their API (http://developer.mapquest.com/content/as/v/osm/documentation/devref/com/mapquest/services/nominatim/Nominatim.html) they have a flag useOsmIcons: "Get the flag for wheter or not we're using the default OSM icons". But I'm not sure about that. Maybe just need to name the property so that it's clear that it's the Nominatim default icon-uri you are getting? > Marking as needs-work for the first item, but having a discussion about the > other items is probably a good idea before you make any more changes. Agreed.
BTW, I have talked to jimmac about this and IIRC he promised to provide our own icons for all the (important one at least) place types. But for now, we can just expose what nominatim provides.
What would the property be called then? osm-icon-uri?
(In reply to comment #7) > What would the property be called then? > > osm-icon-uri? Whats wrong with 'icon-uri' that Bastien suggested?
Well, if geocode gets its own icons then we might want to differentiate them from the ones nominatim provides. That was my thinking. But I'm fine with icon-uri, if Bastien is.
(In reply to comment #9) > Well, if geocode gets its own icons then we might want to differentiate them > from the ones nominatim provides. That was my thinking. But I'm fine with > icon-uri, if Bastien is. I think we shouldn't expose where these icons are coming from if possible, at least not through the prop name. Rather when we have our own icons, we can provide an API to switch icon sets: enum GeocodeIconSet { GEOCODE_ICON_SET_DARK_BG, GEOCODE_ICON_SET_LIGHT_BG }; geocode_glib_set_icon_set (GeocodeIconSet iconset); or something similar. The nominatim icons are obviously meant for light bg.
Created attachment 252757 [details] [review] Add icon uri from nominatim as place property v4
Review of attachment 252757 [details] [review]: Looks good otherwise. ::: geocode-glib/geocode-place.c @@ +500,3 @@ + pspec = g_param_spec_string ("icon-uri", + "Icon URI", + "URI of an icon representing the type of the place", I think 'the type of' is redundant here. Who knows they might support specific icons for specific places in future for popular places, e.g statue of liberty, Eiffel tower etc. ::: geocode-glib/geocode-place.h @@ +198,3 @@ const char *geocode_place_get_continent (GeocodePlace *place); +void geocode_place_set_icon_uri (GeocodePlace *place, I don't think we want to expose the setter to apps.
Created attachment 252764 [details] [review] Add icon uri from nominatim as place property v5
I don't see the use for the OSM icons if we're going to do our own themed ones, based on type. Mixing icons from different sources is going to look bad.
(In reply to comment #14) > I don't see the use for the OSM icons if we're going to do our own themed ones, > based on type. Its temoprary. I'm hoping we can still get the related Maps change (with freeze exception) in time for 3.10. > Mixing icons from different sources is going to look bad. Oh, I don't we should mix icons. When (and if) we have our own icons, we either drop the OSM icons or provide option to choose between either.
Created attachment 252772 [details] Screenshot of maps with icons Icons aren't quite placed well in the screenshot but it gives you an idea of how they look like in Maps.
(In reply to comment #15) > (In reply to comment #14) > > I don't see the use for the OSM icons if we're going to do our own themed ones, > > based on type. > > Its temoprary. But the API won't be, you need to come up with an API that will survive this temporary usage. In the worst case, use a GIcon (eg. GFileIcon for the OSM icons, GThemedIcon once we get our own icons) property.
(In reply to comment #17) > (In reply to comment #15) > > (In reply to comment #14) > > > I don't see the use for the OSM icons if we're going to do our own themed ones, > > > based on type. > > > > Its temoprary. > > But the API won't be, you need to come up with an API that will survive this > temporary usage. Sure, 'icon-uri' can also provide a local URI. > In the worst case, use a GIcon (eg. GFileIcon for the OSM > icons, GThemedIcon once we get our own icons) property. Yeah, that would be better API.
Created attachment 252828 [details] [review] Add icon from nominatim as place GIcon property So this is how that would look. Thanks.
Review of attachment 252828 [details] [review]: Looks good otherwise. ::: geocode-glib/geocode-glib.symbols @@ +68,3 @@ geocode_place_get_continent +geocode_place_get_icon +_geocode_place_set_icon You want to place this above with other internal API. ::: geocode-glib/geocode-place.c @@ +501,3 @@ + "Icon", + "An icon representing the the place", + G_TYPE_ICON, Thinking more about this, I'm a bit afraid that exposing a GIcon would mean that applications will have to use different ways to load the icon depending on whether its a GFileIcon or GThemedIcon. The former implements GLoadableIcon, while the latter does not. Unless there is a common interface that apps can use regardless of implementation? Bastien?
Review of attachment 252828 [details] [review]: ::: geocode-glib/geocode-place.c @@ +501,3 @@ + "Icon", + "An icon representing the the place", + G_TYPE_ICON, Oh this very short IRC conversation makes me think we are better off with the 'icon-uri': <zeenix> jimmac: hey. When you provide the place type icons, would the be part of the theme or part of geocode-glib? <jimmac> zeenix, sorry I haven't gotten to it yet :/ <jimmac> zeenix, I would prefer having it in geocode-glib
(In reply to comment #20) > Review of attachment 252828 [details] [review]: > ::: geocode-glib/geocode-place.c > @@ +501,3 @@ > + "Icon", > + "An icon representing the the place", > + G_TYPE_ICON, > > Thinking more about this, I'm a bit afraid that exposing a GIcon would mean > that applications will have to use different ways to load the icon depending on > whether its a GFileIcon or GThemedIcon. The former implements GLoadableIcon, > while the latter does not. Unless there is a common interface that apps can use > regardless of implementation? > > Bastien? From the GIcon docs: "If you want to consume GIcon (for example, in a toolkit) you must be prepared to handle at least the three following cases: GLoadableIcon, GThemedIcon and GEmblemedIcon." I think that the map widget should handle those cases (though the emblemed icon might not be necessary). (In reply to comment #21) > Review of attachment 252828 [details] [review]: > > ::: geocode-glib/geocode-place.c > @@ +501,3 @@ > + "Icon", > + "An icon representing the the place", > + G_TYPE_ICON, > > Oh this very short IRC conversation makes me think we are better off with the > 'icon-uri': > > <zeenix> jimmac: hey. When you provide the place type icons, would the be part > of the theme or part of geocode-glib? > <jimmac> zeenix, sorry I haven't gotten to it yet :/ > <jimmac> zeenix, I would prefer having it in geocode-glib Which means we'd have the icons in geocode-glib, and we could pass them on to the applications. But we still need them to be themable (at the very least for a11y purposes), so it's best that we hide the original location from the application as much as possible (even if that means having apps handles a couple of common cases).
(In reply to comment #22) > (In reply to comment #20) > > Review of attachment 252828 [details] [review] [details]: > > ::: geocode-glib/geocode-place.c > > @@ +501,3 @@ > > + "Icon", > > + "An icon representing the the place", > > + G_TYPE_ICON, > > > > Thinking more about this, I'm a bit afraid that exposing a GIcon would mean > > that applications will have to use different ways to load the icon depending on > > whether its a GFileIcon or GThemedIcon. The former implements GLoadableIcon, > > while the latter does not. Unless there is a common interface that apps can use > > regardless of implementation? > > > > Bastien? > > From the GIcon docs: > "If you want to consume GIcon (for example, in a toolkit) you must be prepared > to handle at least the three following cases: GLoadableIcon, GThemedIcon and > GEmblemedIcon." > > I think that the map widget should handle those cases (though the emblemed icon > might not be necessary). Ah ok, thanks! > (In reply to comment #21) > > Review of attachment 252828 [details] [review] [details]: > > > > ::: geocode-glib/geocode-place.c > > @@ +501,3 @@ > > + "Icon", > > + "An icon representing the the place", > > + G_TYPE_ICON, > > > > Oh this very short IRC conversation makes me think we are better off with the > > 'icon-uri': > > > > <zeenix> jimmac: hey. When you provide the place type icons, would the be part > > of the theme or part of geocode-glib? > > <jimmac> zeenix, sorry I haven't gotten to it yet :/ > > <jimmac> zeenix, I would prefer having it in geocode-glib > > Which means we'd have the icons in geocode-glib, and we could pass them on to > the applications. But we still need them to be themable (at the very least for > a11y purposes), so it's best that we hide the original location from the > application as much as possible (even if that means having apps handles a > couple of common cases). Good. So in that case the latest patch seems good to go.
Review of attachment 252828 [details] [review]: ACK from my side then. I'll wait if Bastien has any input before pushing it..
Do we also need to expose the size of the icons? In order to use the load-functions for them?
(In reply to comment #25) > Do we also need to expose the size of the icons? > > In order to use the load-functions for them? Looking at the GLoadableIcon docs and GFileIcon source, it seems that size is totally ignored for GFileIcon at least so atm it doesn't really matter. As for the GThemedIcon, you're likely going to use gtk_icon_theme_load_icon() and docs for its size argument suggests that its just a suggestion: size : the desired icon size. The resulting icon may not be exactly this size; see gtk_icon_info_load_icon(). So I'd say we dont need that right now. We can introduce this property later if needed.
(In reply to comment #26) > (In reply to comment #25) > > Do we also need to expose the size of the icons? > > > > In order to use the load-functions for them? > > Looking at the GLoadableIcon docs and GFileIcon source, it seems that size is > totally ignored for GFileIcon at least so atm it doesn't really matter. As for > the GThemedIcon, you're likely going to use gtk_icon_theme_load_icon() and docs > for its size argument suggests that its just a suggestion: > > size : the desired icon size. The resulting icon may not be exactly this size; > see gtk_icon_info_load_icon(). On more closer look at the docs, its the app that decides what size it needs the icon to be and the gtk_icon_theme_load_icon/gtk_icon_info_load_icon are supposed to do their best to provide it in that size, scaling it if needed. Looking at the following in the docs, I still dont think this property is needed: "Note that the resulting pixbuf may not be exactly this size; an icon theme may have icons that differ slightly from their nominal sizes, and in addition GTK+ will avoid scaling icons that it considers sufficiently close to the requested size".
Attachment 252828 [details] pushed as dfd51c5 - Add icon from nominatim as place GIcon property