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 720250 - Use Maki icons
Use Maki icons
Status: RESOLVED FIXED
Product: geocode-glib
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: geocode-glib maintainer(s)
geocode-glib maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-12-11 12:18 UTC by f.alexander.wilms
Modified: 2017-02-15 10:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
So with this patch you can see what the icons would look like (245.40 KB, patch)
2013-12-18 13:38 UTC, Jonas Danielsson
none Details | Review
tooltip on searchresults (903.35 KB, image/png)
2013-12-19 13:41 UTC, Jonas Danielsson
  Details
Add Maki icons (46.88 KB, patch)
2014-01-10 12:24 UTC, Jonas Danielsson
needs-work Details | Review
Add Maki icons (4.51 KB, patch)
2014-01-11 06:04 UTC, Jonas Danielsson
needs-work Details | Review
place: make icon property read-only (1.79 KB, patch)
2014-01-11 06:04 UTC, Jonas Danielsson
committed Details | Review
Add Maki icons (45.93 KB, patch)
2014-01-12 11:33 UTC, Jonas Danielsson
committed Details | Review

Description f.alexander.wilms 2013-12-11 12:18:48 UTC
Mapbox' icon set looks more consistent and slick than the current icons. It'd be nice if Maps used them: https://www.mapbox.com/maki/
Comment 1 Zeeshan Ali 2013-12-11 12:28:51 UTC
They certainly look better but I was hoping that jimmac will provide me better icons of our own that fits the gnome theme.
Comment 2 f.alexander.wilms 2013-12-11 12:35:52 UTC
IMHO they similar enough to not stand out against the gnome ones.
Comment 3 Jakub Steiner 2013-12-11 12:37:12 UTC
The WIP is at https://github.com/gnome-design-team/gnome-icons/tree/master/maps-symbolic

The maki icons certainly look fantastic. If you can make it work license-wise, they would work rather well for POI overlays.
Comment 4 Jonas Danielsson 2013-12-16 09:08:00 UTC
The license of the Maki set seem to be BSD:
https://github.com/mapbox/maki/blob/gh-pages/LICENSE.txt

So it should be workable.

If we would want to use them: should we add the github repo as a dependency or do we download the icons we want from the set to use and add them to geocode?
Comment 5 Zeeshan Ali 2013-12-16 14:42:03 UTC
(In reply to comment #4)
> The license of the Maki set seem to be BSD:
> https://github.com/mapbox/maki/blob/gh-pages/LICENSE.txt
> 
> So it should be workable.
> 
> If we would want to use them: should we add the github repo as a dependency or
> do we download the icons we want from the set to use and add them to geocode?

They don't seem to have build/install system that would be very much compatible with our (or general free sw) tools. Also there is other stuff there that we dont really need. I'd say we should just copy them over.
Comment 6 Jonas Danielsson 2013-12-17 09:24:32 UTC
Ok,

When we added the use of Nominatim icons to geocode-glib I saw that the icons didn't seem to corelate to the place-type.
In other words I got an icon of something in the Maps application but geocode reported the type as UNKNOWN.

When we add our own icons we only have the place-type property to rely on.
Currently geocode-glib do not expose the type from nominatim directly but instead creates around 30 categories and tries to filter the type from nominatim into those. The maki icons covers more than those categories. Maki also does not cover a lot of those selected categories.

The filtration in geocode is done in geocode-forward.c in get_place_type_from_attributes and is a nest of if statements that tries to match some type strings to our categories. And we set unknown if we can not do it.

I do not think the filtration is exhaustive but I cannot find a list of all types that Nominatim/OSM contains, do anyone have one?

My question is do we limit ourselves to the categories we currently have as defined in geocode-place.h for icons, or do we try to set the icon based on the raw data from nominatim? Or maybe we can try to add to the current enum list of place types in geocode-glib?

Jonas
Comment 7 Jonas Danielsson 2013-12-17 09:43:37 UTC
And also, when we set place-type we only look at the OSM class "type", which limits us to only a certain type of locations.

I added a g_print in geocode-glib and then did a search in gnome-maps and got:

"class: place
type: suburb

class: place
type: neighbourhood

class: amenity
type: restaurant

class: shop
type: mall

class: amenity
type: pub
class: amenity

type: cafe
class: amenity

class: place
type: town

class: place
type: town

class: place
type: village

class: place
type: village"

Everything here that does not have class==place will not get a place-type in geocode-glib.

Jonas
Comment 8 Zeeshan Ali 2013-12-17 16:50:04 UTC
(In reply to comment #6)
> Ok,
> 
> When we added the use of Nominatim icons to geocode-glib I saw that the icons
> didn't seem to corelate to the place-type.
> In other words I got an icon of something in the Maps application but geocode
> reported the type as UNKNOWN.
> 
> When we add our own icons we only have the place-type property to rely on.
> Currently geocode-glib do not expose the type from nominatim directly but
> instead creates around 30 categories and tries to filter the type from
> nominatim into those. The maki icons covers more than those categories. Maki
> also does not cover a lot of those selected categories.
> 
> The filtration in geocode is done in geocode-forward.c in
> get_place_type_from_attributes and is a nest of if statements that tries to
> match some type strings to our categories. And we set unknown if we can not do
> it.
> 
> I do not think the filtration is exhaustive but I cannot find a list of all
> types that Nominatim/OSM contains, do anyone have one?


You can get types for each class using the URL:

http://taginfo.openstreetmap.org/keys/${CLASS_NAM}#values

e.g

http://taginfo.openstreetmap.org/keys/railway#values
http://taginfo.openstreetmap.org/keys/boundary#values

> My question is do we limit ourselves to the categories we currently have as
> defined in geocode-place.h for icons, or do we try to set the icon based on the
> raw data from nominatim? Or maybe we can try to add to the current enum list of
> place types in geocode-glib?

Sure, we can add more types but we already have a lot covered so I wouldn't want to extend it too much. I'd just go for the former approach.
Comment 9 Zeeshan Ali 2013-12-17 17:00:14 UTC
(In reply to comment #7)
> And also, when we set place-type we only look at the OSM class "type", which
> limits us to only a certain type of locations.
> 
> I added a g_print in geocode-glib and then did a search in gnome-maps and got:
> 
> "class: place
> type: suburb
> 
> class: place
> type: neighbourhood
> 
> class: amenity
> type: restaurant
> 
> class: shop
> type: mall
> 
> class: amenity
> type: pub
> class: amenity
> 
> type: cafe
> class: amenity
> 
> class: place
> type: town
> 
> class: place
> type: town
> 
> class: place
> type: village
> 
> class: place
> type: village"
> 
> Everything here that does not have class==place will not get a place-type in
> geocode-glib.

We should be looking at both and looking at code in geocode-glib/geocode-forward.c:get_place_type_from_attributes, I see that we do look at both. We just need to add handling for more types.
Comment 10 Jonas Danielsson 2013-12-17 18:10:37 UTC
> You can get types for each class using the URL:
> 
> http://taginfo.openstreetmap.org/keys/${CLASS_NAM}#values
> 
> e.g
> 
> http://taginfo.openstreetmap.org/keys/railway#values
> http://taginfo.openstreetmap.org/keys/boundary#values
> 

Thanks!

> > My question is do we limit ourselves to the categories we currently have as
> > defined in geocode-place.h for icons, or do we try to set the icon based on the
> > raw data from nominatim? Or maybe we can try to add to the current enum list of
> > place types in geocode-glib?
> 
> Sure, we can add more types but we already have a lot covered so I wouldn't
> want to extend it too much. I'd just go for the former approach.

I'll look into it, but the ones we have do not cover all that much interesting stuff :) And not a lot of the Maki set at all.
Comment 11 Jonas Danielsson 2013-12-17 18:11:53 UTC
> 
> We should be looking at both and looking at code in
> geocode-glib/geocode-forward.c:get_place_type_from_attributes, I see that we do
> look at both. We just need to add handling for more types.

Yeah I saw that and have started adding more types and classes, so that we can use more of the Maki, or our own, set.

Thanks.
Comment 12 Jonas Danielsson 2013-12-18 13:38:29 UTC
Created attachment 264477 [details] [review]
So with this patch you can see what the icons would look like

using gnome-maps.

Please comment the approach I have taken (adding more types)
They are not all commented yet.

Add Maki icons

This patch makes use of the Maki point of interest icon set
from MapBox.
Comment 13 Zeeshan Ali 2013-12-18 17:37:35 UTC
Review of attachment 264477 [details] [review]:

Right, while it looks much better with this patch, some of the important icons don't exactly help much in identifying what the result is about. Examples are cities, neighbourhoods, countries etc. I wonder if we should rather show nice labels to describe the type of place instead in Maps?
Comment 14 Jonas Danielsson 2013-12-19 13:41:11 UTC
I understand what you mean. But I would like to see some design input/mockups of it before I agree/disagree :)

Some results would have labels and some wouldn't? Or labels for all?

We could also create our own icons for countries etc, for the ones that does not have one in Maki. And maybe we could set tooltips for extra help?
Comment 15 Jonas Danielsson 2013-12-19 13:41:46 UTC
Created attachment 264544 [details]
tooltip on searchresults

How would tooltip look on search results?
Comment 16 Zeeshan Ali 2013-12-19 18:11:25 UTC
(In reply to comment #14)
> I understand what you mean. But I would like to see some design input/mockups
> of it before I agree/disagree :)

Sure, some designers are on vacation already though. :)

> Some results would have labels and some wouldn't? Or labels for all?

I think we can arrange for labels for all.

> We could also create our own icons for countries etc, for the ones that does
> not have one in Maki.

Sure but the issue is not that Maki does not have icon for that but icons that tell something to user.

> And maybe we could set tooltips for extra help?

Don't think tooltips are very much liked by designers (right?). If user needs some info, it should be provided without having to do anything extra.

How about we should icons or label, depending on whether there is a useful icon possible? E.g we should icon for cafe and pub etc but not for city and county etc.
Comment 17 Jonas Danielsson 2013-12-20 11:11:42 UTC
> 
> > Some results would have labels and some wouldn't? Or labels for all?
> 
> I think we can arrange for labels for all.
> 
> > We could also create our own icons for countries etc, for the ones that does
> > not have one in Maki.
> 
> Sure but the issue is not that Maki does not have icon for that but icons that
> tell something to user.
> 
> > And maybe we could set tooltips for extra help?
> 
> Don't think tooltips are very much liked by designers (right?). If user needs
> some info, it should be provided without having to do anything extra.
> 
> How about we should icons or label, depending on whether there is a useful icon
> possible? E.g we should icon for cafe and pub etc but not for city and county
> etc.

Sounds ok, if we can get labels that look good.

Maps aside. Geocode still provide icons that we might want to use, for results lists or maybe for POI overlays. So this bug still has relevance.

Do we want the Maki icons for geocode-glib? If so, should I split up the patch in two, one that adds new types and one that adds the Maki icons?

Any other comments on the Maki patch?
Comment 18 f.alexander.wilms 2013-12-21 10:30:32 UTC
The icons look a bit blurry compared to this rendering: https://github.com/mapbox/maki/blob/e4dc4e05e97aa97f74dafc218ae75248bb61432f/www/images/maki-sprite.png
Comment 19 Bastien Nocera 2014-01-07 13:08:37 UTC
Could we not start by using the icons that we can (restaurant, petrol stations, museums, etc.) and go for completion afterwards?

Let's look at the problem incrementally instead of hoping for completion right from the off.
Comment 20 Jonas Danielsson 2014-01-09 12:00:48 UTC
(In reply to comment #19)
> Could we not start by using the icons that we can (restaurant, petrol stations,
> museums, etc.) and go for completion afterwards?
> 
> Let's look at the problem incrementally instead of hoping for completion right
> from the off.

How do you mean exactly? Not add any new types to GeocodePlace?
If we do not add any new types and use the icons we can, then that would mean: building, airport, town, railway station, bus stop and street.

Or do you suggest adding some more types, for instance the one you exemplified?
Comment 21 Bastien Nocera 2014-01-09 13:23:10 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Could we not start by using the icons that we can (restaurant, petrol stations,
> > museums, etc.) and go for completion afterwards?
> > 
> > Let's look at the problem incrementally instead of hoping for completion right
> > from the off.
> 
> How do you mean exactly? Not add any new types to GeocodePlace?

No, not for now.

> If we do not add any new types and use the icons we can, then that would mean:
> building, airport, town, railway station, bus stop and street.
> 
> Or do you suggest adding some more types, for instance the one you exemplified?

Start by implementing support for the place types that are actually defined in geocode-glib. If done well, adding support for other place types should be trivial. And it gives us something to review and discuss.
Comment 22 Jonas Danielsson 2014-01-10 12:24:13 UTC
Created attachment 265912 [details] [review]
Add Maki icons

This patch makes use of the Maki point of interest icon set
from MapBox. The icons are BSD licensed.
Comment 23 Jonas Danielsson 2014-01-10 12:27:16 UTC
Review of attachment 265912 [details] [review]:

So this patch just adds icons for the types that geocode supports at the moment. You can check out the icons by using gnome-maps.

::: geocode-glib/geocode-place.c
@@ +497,3 @@
                                      "An icon representing the the place",
                                      G_TYPE_ICON,
+                                     G_PARAM_READABLE |

So this changes the icon proprty from READWRITE to READABLE, how bad is that?

@@ +1077,3 @@
+        default:
+                icon_name = "poi-marker"; /* generic marker */
+                break;

I add a generic marker to all types not supported, thoughts?
Comment 24 Zeeshan Ali 2014-01-10 14:43:13 UTC
Review of attachment 265912 [details] [review]:

Looks fine otherwise.

::: geocode-glib/geocode-place.c
@@ +497,3 @@
                                      "An icon representing the the place",
                                      G_TYPE_ICON,
+                                     G_PARAM_READABLE |

Only that you need to put it in separate patch with its justification in log?

@@ +1077,3 @@
+        default:
+                icon_name = "poi-marker"; /* generic marker */
+                break;

Sounds good, at least for now.
Comment 25 Jonas Danielsson 2014-01-10 21:18:52 UTC
Review of attachment 265912 [details] [review]:

::: geocode-glib/geocode-place.c
@@ +497,3 @@
                                      "An icon representing the the place",
                                      G_TYPE_ICON,
+                                     G_PARAM_READABLE |

Is that really better than keeping it in this patch and having the justification in the commit log for this patch?

Changing it with a patch that comes before this patch is hard, because the icon property was set from geocode-forward, so it needed to be writable to be workable. Doing it after would be possible, but since then I do not really know what the setter would do, since there is no priv->icon anymore.
Comment 26 Zeeshan Ali 2014-01-10 22:45:41 UTC
Review of attachment 265912 [details] [review]:

::: geocode-glib/geocode-place.c
@@ +497,3 @@
                                      "An icon representing the the place",
                                      G_TYPE_ICON,
+                                     G_PARAM_READABLE |

In that case putting it before this patch doesn't make sense. Put it after but this is a separate change.
Comment 27 Jonas Danielsson 2014-01-11 06:04:42 UTC
Created attachment 265990 [details] [review]
Add Maki icons

This patch makes use of the Maki point of interest icon set
from MapBox. The icons are BSD licensed.
Comment 28 Jonas Danielsson 2014-01-11 06:04:54 UTC
Created attachment 265991 [details] [review]
place: make icon property read-only

The icon for a place is derived from its type. Having the icon
property READWRITE does not make sense.
Comment 29 Zeeshan Ali 2014-01-11 19:16:07 UTC
Review of attachment 265990 [details] [review]:

The icon files were forgotten. :)
Comment 30 Zeeshan Ali 2014-01-11 19:16:59 UTC
Review of attachment 265991 [details] [review]:

ACK but i'll put 'now' after 'is' in the commit log.
Comment 31 Jonas Danielsson 2014-01-12 11:33:55 UTC
Created attachment 266055 [details] [review]
Add Maki icons

This patch makes use of the Maki point of interest icon set
from MapBox. The icons are BSD licensed.
Comment 32 Zeeshan Ali 2014-01-12 14:24:42 UTC
Review of attachment 266055 [details] [review]:

Seems good otherwise.

::: geocode-glib/geocode-place.c
@@ +1109,2 @@
 void
 _geocode_place_set_icon (GeocodePlace *place,

you can just remove this internal setter in this patch, while changing the prop to READABLE can go in separate patch.
Comment 33 Jonas Danielsson 2014-01-13 11:08:35 UTC
Ok, so I've pushed the two patches, but git bz push said to me:

<td id="error_msg" class="throw_error">
    You tried to change the
    <strong>Status</strong> field 
      from <em>UNCONFIRMED</em>
      to <em>RESOLVED</em>
    , but only
      the assignee or reporter 
      of the bug, or
    a user with the required permissions may change that field.
</td>
Comment 34 Zeeshan Ali 2014-01-13 12:24:30 UTC
(In reply to comment #33)
> Ok, so I've pushed the two patches, but git bz push said to me:
> 
> <td id="error_msg" class="throw_error">
>     You tried to change the
>     <strong>Status</strong> field 
>       from <em>UNCONFIRMED</em>
>       to <em>RESOLVED</em>
>     , but only
>       the assignee or reporter 
>       of the bug, or
>     a user with the required permissions may change that field.
> </td>

Looks like you need to talk to admin team. :) You can file a bug about it. I'll update this bug for now.
Comment 35 Laurent Bigonville 2017-02-15 10:01:56 UTC
What is the status of this bug? I think it can be closed, right?
Comment 36 Jonas Danielsson 2017-02-15 10:49:39 UTC
Seems so, thanks!