Created attachment 285488 [details] [review]
Add icons to default icon theme

Review of attachment 285488 [details] [review]:

Looks good to me, just a couple of questions from an ignorant:

 - Any particular reason why this icons are not in the public icons folder (/usr/share/icons)? (maybe because they are not specified in the icon theme specification).
 - Can these icons be overwritten by 3rd party icon themes located at /usr/share/icons?
 - Why icons that are right now in GResource are not part of this private icon theme (zoom-in.png, zoom-out.png, route-button.svg, etc)?
 - Is there any advantage on having the whole theme in a GResource path (/org/gnome/maps/icons)? What is the common way to do it?

Sorry, I think need to read a little bit more about icon themes :)

Anyways, I think this is a must have patch mainly for the annoying symbolic-ltr warnings, we can revisit the items I mention later if any makes sense or do changes now if they are easy.

::: src/searchResultMarker.js
@@ +35,3 @@
         this.parent(params);
 
+        this.add_actor(Utils.CreateActorFromIconName('pin'));

Maybe we need to rename this icon to something more specific (maps-pin)?

Review of attachment 285488 [details] [review]:

I am far far from an authority on icon handling.

- I didn't put them in public because I remember that mclasen said that geocode should install in private theme and not in the gnome theme.
- I do not know if they can be overwritten, do we want them to be able to?
- I only did the ones that we load from code right now.

I talked to mclasen and I feel the way forward is to have all icons in GResource, he called it the more modern way.
And then go Gtk.IconTheme.add_resource_path (https://developer.gnome.org/gtk3/unstable/GtkIconTheme.html#gtk-icon-theme-add-resource-path)

So I only did this to get rid of the warning really :) And to learn a bit about the icons. And I didn't feel comfortable going all the way to all resource based, because I don't know enough about it.

Are you ok with committing it?

::: src/searchResultMarker.js
@@ +35,3 @@
         this.parent(params);
 
+        this.add_actor(Utils.CreateActorFromIconName('pin'));

Agreed, I can rename it to maps-pin when I push.

Attachment 285488 [details] pushed as c3970a6 - Add icons to default icon theme

I started working on putting it in GResources + added new nice icons made by andreas in wip/hidpi-icons2.

There's some work still to be done:
 - Fix the icons so they are compatible with GDK for changing their colors on the fly (in the face of selections / dark theme etc). No idea what makes our icons fail there and Allan didn't either.
 - Re-work the patch a little bit so that I don't rename a bunch of icons unnecessarily.
 - Rebase upon current master.

I'll see if I can get this done on Sunday, so we can get it in before the hard code freeze on tuesday (this doesn't add or change any features or strings so should be fine).

(In reply to comment #5)
> I started working on putting it in GResources + added new nice icons made by
> andreas in wip/hidpi-icons2.
> 
> There's some work still to be done:
>  - Fix the icons so they are compatible with GDK for changing their colors on
> the fly (in the face of selections / dark theme etc). No idea what makes our
> icons fail there and Allan didn't either.
>  - Re-work the patch a little bit so that I don't rename a bunch of icons
> unnecessarily.
>  - Rebase upon current master.
> 
> I'll see if I can get this done on Sunday, so we can get it in before the hard
> code freeze on tuesday (this doesn't add or change any features or strings so
> should be fine).

Isn't hard code freeze September 15th?

https://wiki.gnome.org/Schedule#Release_Schedule

(In reply to comment #6)
> 
> Isn't hard code freeze September 15th?
> 
> https://wiki.gnome.org/Schedule#Release_Schedule

Indeed! :)