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 759544 - GeoJSON/KML/Layers: Formalize and add GUI
GeoJSON/KML/Layers: Formalize and add GUI
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Hashem Nasarat
gnome-maps-maint
Depends on: 759935
Blocks: 757171 760875
 
 
Reported: 2015-12-16 11:59 UTC by Jonas Danielsson
Modified: 2016-01-22 09:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix css to show which base map is selected (1.09 KB, patch)
2016-01-01 00:11 UTC, Hashem Nasarat
none Details | Review
Fix typo in this._addContacts (913 bytes, patch)
2016-01-01 00:11 UTC, Hashem Nasarat
none Details | Review
Allow for more than one overlay map source (1.19 KB, patch)
2016-01-01 00:12 UTC, Hashem Nasarat
none Details | Review
In parsing GeoJSON only try/catch where it's needed (1.93 KB, patch)
2016-01-01 00:12 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (16.79 KB, patch)
2016-01-01 00:12 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (3.10 KB, patch)
2016-01-01 15:46 UTC, Hashem Nasarat
none Details | Review
In parsing GeoJSON only try/catch where it's needed (1.87 KB, patch)
2016-01-01 17:31 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (3.07 KB, patch)
2016-01-01 20:38 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (17.13 KB, patch)
2016-01-02 00:38 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (13.79 KB, patch)
2016-01-05 06:25 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (3.05 KB, patch)
2016-01-05 06:25 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (13.73 KB, patch)
2016-01-05 06:30 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (13.72 KB, patch)
2016-01-05 06:46 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (14.36 KB, patch)
2016-01-05 19:04 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (2.66 KB, patch)
2016-01-05 19:05 UTC, Hashem Nasarat
none Details | Review
Fix css to show which base map is selected (1.09 KB, patch)
2016-01-05 19:19 UTC, Hashem Nasarat
committed Details | Review
Fix typo in this._addContacts (913 bytes, patch)
2016-01-05 19:19 UTC, Hashem Nasarat
committed Details | Review
Allow for more than one overlay map source (1.19 KB, patch)
2016-01-05 19:19 UTC, Hashem Nasarat
none Details | Review
In parsing GeoJSON only try/catch where it's needed (1.87 KB, patch)
2016-01-05 19:19 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (14.36 KB, patch)
2016-01-05 19:20 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (2.66 KB, patch)
2016-01-05 19:20 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (14.45 KB, patch)
2016-01-06 02:37 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (15.72 KB, patch)
2016-01-07 05:20 UTC, Hashem Nasarat
none Details | Review
Display markers from multiple overlay sources (2.61 KB, patch)
2016-01-07 05:20 UTC, Hashem Nasarat
none Details | Review
Correct DND API usage for overlay layer loading (1.70 KB, patch)
2016-01-07 05:20 UTC, Hashem Nasarat
needs-work Details | Review
mainWindow: set correct DND drag status (1.46 KB, patch)
2016-01-07 06:26 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (15.45 KB, patch)
2016-01-07 17:06 UTC, Hashem Nasarat
none Details | Review
mainWindow: set correct DND drag status (1.59 KB, patch)
2016-01-10 06:15 UTC, Hashem Nasarat
committed Details | Review
GeoJSON/Layers: Formalize and add GUI (22.22 KB, patch)
2016-01-10 06:16 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (22.19 KB, patch)
2016-01-10 06:45 UTC, Hashem Nasarat
none Details | Review
Utils: If Error is passed to debug() print stack (1.19 KB, patch)
2016-01-10 06:45 UTC, Hashem Nasarat
committed Details | Review
GeoJSON/Layers: Formalize and add GUI (22.14 KB, patch)
2016-01-10 06:46 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (23.58 KB, patch)
2016-01-10 22:43 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (24.00 KB, patch)
2016-01-14 22:01 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (23.93 KB, patch)
2016-01-15 17:22 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (25.42 KB, patch)
2016-01-17 06:18 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (25.73 KB, patch)
2016-01-17 23:21 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (25.84 KB, patch)
2016-01-18 00:17 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (25.70 KB, patch)
2016-01-18 17:06 UTC, Hashem Nasarat
none Details | Review
GeoJSON/Layers: Formalize and add GUI (25.39 KB, patch)
2016-01-18 17:36 UTC, Hashem Nasarat
committed Details | Review

Description Jonas Danielsson 2015-12-16 11:59:49 UTC
We have some mockups from the ever excellent andreasn on how the handling of custom layers such as GeoJSON and KML could look like:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/custom-map.png
Comment 1 Jonas Danielsson 2015-12-16 12:03:36 UTC
hashem: I feel this is included in your Outreachy project, do you concur?
Comment 2 Andreas Nilsson 2015-12-16 12:12:29 UTC
Less than excellent I would say, since I took forever until I drew these up :)
Comment 3 Hashem Nasarat 2015-12-16 15:52:34 UTC
Andreas, your beautiful mockups are always appreciated! 

I anticipated that this would be part of my Outreachy project yes, and I actually had envisioned something similar, so it's nice to feel validated by one of our ui/ux people :)

Shall I assign this bug to myself?
Comment 4 Jonas Danielsson 2015-12-16 18:13:38 UTC
Go ahead and assign yourself!
Comment 5 Hashem Nasarat 2015-12-21 00:50:22 UTC
Andreas, does the new design make the designs on https://wiki.gnome.org/Design/Apps/Maps obsolete?

Maps has support for bikes and transport, but we currently don't have a way in the UI to enable it.
Comment 6 Hashem Nasarat 2015-12-21 00:53:39 UTC
And for clarity, public transportation and bikes aren't layers, they're entirely different maps, akin to satellite vs street view.

(https://developer.gnome.org/libchamplain/unstable/ChamplainMapSourceFactory.html#CHAMPLAIN-MAP-SOURCE-OSM-CYCLE-MAP:CAPS)
Comment 7 Zeeshan Ali 2015-12-22 14:38:48 UTC
(In reply to Hashem Nasarat from comment #6)
> And for clarity, public transportation and bikes aren't layers, they're
> entirely different maps, akin to satellite vs street view.
> 
> (https://developer.gnome.org/libchamplain/unstable/ChamplainMapSourceFactory.
> html#CHAMPLAIN-MAP-SOURCE-OSM-CYCLE-MAP:CAPS)

https://git.gnome.org/browse/gnome-maps/commit/?id=4f501e7361fc39ce9f7fc967b1ffb14edb9a7d64
Comment 8 Hashem Nasarat 2015-12-22 17:08:14 UTC
Thanks Zeeshan, yeah that provides some illuminating background. I feel like we already have support for non-overlay transport & bikes, and while overlay support would be better, the infrastructure and data sources that would be required aren't here yet.

So I feel like showing support for what we have now and then iterating on that would provide the most value for the least amount of work in the short term.
Comment 9 Hashem Nasarat 2015-12-23 05:40:54 UTC
I have a preliminary implementation of this[0], but I ran into some design roadblocks.

1. If we load lots of layers, the popover will grow. Should we have scrolling? Where?
(I think we should only have the overlay layers scroll).

2. Should we save the included overlay layers and restore them when the user starts the app?
(I think yes)

3. What is the mechanism for toggling the display of an overlay layer? Currently the design only shows a 'x' button. I presume this removes it from the list and hides whatever overlays it contained. This seems like a hassle if the user has to find the overlay file on their filesystem to reenable it each time.
(Easy solution would be to have a checkbox to toggle hidden or not, but this doesn't seem very elegant design-wise).
PS: The code currently can't display more than one simultaneous overlay, but it ought to... I need to look into that.


[0] https://github.com/Hnasar/gnome-maps/tree/overlay-ui
Comment 10 Jonas Danielsson 2015-12-23 18:34:23 UTC
(In reply to Hashem Nasarat from comment #9)
> I have a preliminary implementation of this[0], but I ran into some design
> roadblocks.
> 

Cool I will hold out reviewing until it appears as patches here. In order to have all history / comments on this page. For trace-ability.

> 1. If we load lots of layers, the popover will grow. Should we have
> scrolling? Where?
> (I think we should only have the overlay layers scroll).

I kind of lean to no. That we instead say 5 layers is enough for everybody.
I do not see the use-case for many many layers and adding complexity in code (getting that pixel perfect and stuff) and UI does not seem worth it. Instead we can show an in-app notification if one tries to add more, and/or gray out the load-layer button.

> 
> 2. Should we save the included overlay layers and restore them when the user
> starts the app?
> (I think yes)
> 

I am not sure. I am trying to think of other apps. Do we restart evince with the last pdf? or videos with the last movies. For these kinds of layers (KML and GeoJSON) I see Maps as a file-viewer. A way to view the meta-data contained in this shapefile.

For other layers like layers fetching trafic info or some other stuff that is more general I could see the benefit. But I would say no for the first version. Let's keep it simple to start and we can create another bug for that. I do not want it as part as a first patch series at least.

> 3. What is the mechanism for toggling the display of an overlay layer?
> Currently the design only shows a 'x' button. I presume this removes it from
> the list and hides whatever overlays it contained. This seems like a hassle
> if the user has to find the overlay file on their filesystem to reenable it
> each time.
> (Easy solution would be to have a checkbox to toggle hidden or not, but this
> doesn't seem very elegant design-wise).

I say remove the overlay when pressing x, we could have an "are you sure" thing and even an in-app notification with undo-button. But I think we remove it and user has to re-add it. It is like closing a file in Gedit for me.

Andreas, your take?

> PS: The code currently can't display more than one simultaneous overlay, but
> it ought to... I need to look into that.
> 

There is nothing inherently preventing it. It should just be creating a new instance of geoJSONSource. But the code is not written for it right now I believe. 

> 
> [0] https://github.com/Hnasar/gnome-maps/tree/overlay-ui


Awesome! Thanks for working on this and for the great questions!
Comment 11 Jonas Danielsson 2015-12-23 18:40:03 UTC
(In reply to Jonas Danielsson from comment #10)

> 
> > 3. What is the mechanism for toggling the display of an overlay layer?
> > Currently the design only shows a 'x' button. I presume this removes it from
> > the list and hides whatever overlays it contained. This seems like a hassle
> > if the user has to find the overlay file on their filesystem to reenable it
> > each time.
> > (Easy solution would be to have a checkbox to toggle hidden or not, but this
> > doesn't seem very elegant design-wise).
> 
> I say remove the overlay when pressing x, we could have an "are you sure"
> thing and even an in-app notification with undo-button. But I think we
> remove it and user has to re-add it. It is like closing a file in Gedit for
> me.
> 
> Andreas, your take?
> 

But yeah, a mechanism for hiding layers would be needed. Andreas do you have an idea? Do we have prio art in gnome? I feel gimp has the eye thing to show layers, but gimp is not that GNOMEy.


Also, I want keyboard-bindings! If possible, like Alt-1 toggle layer 1?
Comment 12 Jonas Danielsson 2015-12-23 18:44:00 UTC
(In reply to Jonas Danielsson from comment #10)

> > 
> > 2. Should we save the included overlay layers and restore them when the user
> > starts the app?
> > (I think yes)
> > 
> 
> I am not sure. I am trying to think of other apps. Do we restart evince with
> the last pdf? or videos with the last movies. For these kinds of layers (KML
> and GeoJSON) I see Maps as a file-viewer. A way to view the meta-data
> contained in this shapefile.
> 
> For other layers like layers fetching trafic info or some other stuff that
> is more general I could see the benefit. But I would say no for the first
> version. Let's keep it simple to start and we can create another bug for
> that. I do not want it as part as a first patch series at least.
> 

Or maybe we do want to remember the file names. But it adds complexity. What if the file went away?

Another patch at least. Not included in first.
Comment 13 Hashem Nasarat 2016-01-01 00:11:46 UTC
Created attachment 318106 [details] [review]
Fix css to show which base map is selected

A radio button is only :active when it's clicked and held. :checked is
for when it's selected.

Also, since there is a 2px border when :checked, we make the unchecked
border 1px.
Comment 14 Hashem Nasarat 2016-01-01 00:11:55 UTC
Created attachment 318107 [details] [review]
Fix typo in this._addContacts

Function calls still work with spaces after the '.', but it looks wrong.
Comment 15 Hashem Nasarat 2016-01-01 00:12:02 UTC
Created attachment 318108 [details] [review]
Allow for more than one overlay map source

The code is capable of displaying separate overlay_sources
simultaneously, and we soon will allow the user to select multiple
overlay files to display.
Comment 16 Hashem Nasarat 2016-01-01 00:12:10 UTC
Created attachment 318109 [details] [review]
In parsing GeoJSON only try/catch where it's needed

geoJSONSource.parse() is the only thing that raises exceptions. Having
the try/catch around all of the code makes it unclear at which stage of
the function does the process fail.
Comment 17 Hashem Nasarat 2016-01-01 00:12:16 UTC
Created attachment 318110 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 18 Hashem Nasarat 2016-01-01 00:21:41 UTC
This is ready for review, I'm not very satisfied with the styling of the layers in that my css sucks, I don't know where to read to learn about how gnome's css works, the css in adwaita is very complicated, and the mockups only show a white square.

Attached are screenshots of the current state. You can create your own geojson files with http://geojson.io/#id=gist:anonymous/d9a113c4fd20f2198035&map=16/42.3773/-71.1191
Comment 19 Hashem Nasarat 2016-01-01 00:27:26 UTC
http://imgur.com/a/98iVz screenshots because using bugzilla to add multiple files is horrendous. 

Also, would "Load Overlay File" or "Load Layer File" be more clear? I could see users being surprised that "Load Map Layer" opens the file chooser.
Comment 20 Hashem Nasarat 2016-01-01 15:46:53 UTC
Created attachment 318118 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 21 Jonas Danielsson 2016-01-01 17:14:50 UTC
Review of attachment 318107 [details] [review]:

Agreed :)

Thanks
Comment 22 Jonas Danielsson 2016-01-01 17:17:22 UTC
Review of attachment 318109 [details] [review]:

Sure!

A nit below though!
Thanks!

::: src/mapView.js
@@ +204,2 @@
         } catch(e) {
+            let msg = _("Failed to parse GeoJSON file.");

You added a dot here, I do not feel it should be there and also it forces all translations to be re-made I feel.
Comment 23 Jonas Danielsson 2016-01-01 17:30:45 UTC
Review of attachment 318110 [details] [review]:

Thanks Hashem!

::: data/gnome-maps.css
@@ +25,3 @@
+    background: white;
+}
+

We should strive to limit the amount of custom CSS we use. CSS should be for the theme writers.

::: data/ui/layers-popover-row.ui
@@ +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <template class="Gjs_LayersRowBox" parent="GtkBox">

Let's make sure the class name matches up with the file name. How abut layer-row or custom-layer-row => Gjs_CustomLayerRow?

::: data/ui/layers-popover.ui
@@ +57,3 @@
     </child>
+    <child>
+      <object class="GtkButton" id="load-layer-button">

Should this button use the flat class?

::: src/application.js
@@ +41,3 @@
 const Settings = imports.settings;
 const Utils = imports.utils;
+const LayersManager = imports.layersManager;

I do not want to have a LayersManager class. This should be simple enough to manage without, right? What is a manager anyway. we have to many already if you ask me.

@@ +156,3 @@
     },
 
+    _onLoadLayerActivate: function() {

Shouldn't this code be in the layers-popover.js file? There is were we have the user-interactive opening of layers, right? It is were I would look for this code.

@@ +245,3 @@
+            },
+            'load-layer': {
+                onActivate: this._onLoadLayerActivate.bind(this)

And if this were and action I would expect it to take some sort of filename,

::: src/layersManager.js
@@ +16,3 @@
+ *
+ * Author: Hashem Nasarat <hashem@riseup.net>
+ */

Nice work but I'd rather not have this file. If you feel an overpowering need of refactoring, maybe Utils.js could grow some helper function?

::: src/layersPopover.js
@@ +27,3 @@
+const LayersRowBox = new Lang.Class({
+    Name: 'LayersRowBox',
+    Extends: Gtk.Box,

Should be Gtk.ListBoxRow.

@@ +29,3 @@
+    Extends: Gtk.Box,
+    Template: 'resource:///org/gnome/Maps/ui/layers-popover-row.ui',
+    Children: ['layer-label', 'close-button'],

This class could also have a reference to the overlay_source, so that it would know how to remove it.

@@ +46,3 @@
+                                      'street-layer-button',
+                                      'aerial-layer-button',
+                                      'load-layer-button' ]);

I wish we could make this widget use template instead do not remember if we ever tried.
A project for some other time maybe.

@@ +79,3 @@
+                                    1,
+                                    1);
+    },

No, I do not think I want it this way at all. I want you to add a GtkListBox in the layers-popover ui-file and then just add LayerRow widgets to that GtkListBox using listbox.add(new CustomLayerRow({ name: name, source: source). This will also eliminate the need for custom CSS I feel.
Comment 24 Hashem Nasarat 2016-01-01 17:31:56 UTC
Created attachment 318133 [details] [review]
In parsing GeoJSON only try/catch where it's needed

geoJSONSource.parse() is the only thing that raises exceptions. Having
the try/catch around all of the code makes it unclear at which stage of
the function does the process fail.
Comment 25 Jonas Danielsson 2016-01-01 17:32:46 UTC
Review of attachment 318118 [details] [review]:

Thanks!

::: src/mapView.js
@@ +206,3 @@
+            for (let i in markers) {
+                this._annotationMarkerLayer.remove_marker(markers[i]);
+            }

overlay.placeMarkers.forEach(function(marker)) {
...
}

@@ +233,3 @@
         this.view.add_overlay_source(geoJSONSource, 255);
+        let markers = geoJSONSource.placeMarkers;
+        for (let i in markers) {

geoJSONSource.placeMarkers.forEach()function(marker) {
...
}).bind(this));
Comment 26 Jonas Danielsson 2016-01-01 17:36:41 UTC
Review of attachment 318108 [details] [review]:

Looks fine!

How about the mapView class emit a signal when a custom layer is added? And that can be used be the layers-popover?
Comment 27 Jonas Danielsson 2016-01-01 17:40:15 UTC
Review of attachment 318133 [details] [review]:

Great!
Comment 28 Jonas Danielsson 2016-01-01 17:48:46 UTC
Review of attachment 318110 [details] [review]:

::: src/layersPopover.js
@@ +79,3 @@
+                                    1,
+                                    1);
+    },

Or maybe the source does not need to be part of the LayerRow class, but instead just sent as user_data to to remove button click callback.
Comment 29 Hashem Nasarat 2016-01-01 20:38:51 UTC
Created attachment 318139 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 30 Hashem Nasarat 2016-01-02 00:33:51 UTC
Review of attachment 318110 [details] [review]:

Going to attach a new version with some, but not all, suggestions. Thanks for the review.

::: data/gnome-maps.css
@@ +25,3 @@
+    background: white;
+}
+

I agree, but what should be done then? The mockups provided had each row with a white background with spacing in between.

::: data/ui/layers-popover-row.ui
@@ +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <template class="Gjs_LayersRowBox" parent="GtkBox">

I like custom layer row.

::: data/ui/layers-popover.ui
@@ +57,3 @@
     </child>
+    <child>
+      <object class="GtkButton" id="load-layer-button">

Nope

::: src/application.js
@@ +41,3 @@
 const Settings = imports.settings;
 const Utils = imports.utils;
+const LayersManager = imports.layersManager;

It follows the mediator pattern and provides a MVC-esque separation between the different components. I wanted to name it mediator, but I already saw we had other Managers, so I followed suit. The way I see it things are complicated enough that decoupling is worth it:

the mapView needs to know when a file is selected, so it can parse it
the popover needs to know when the file is parsed

either the mainWindow funnels data between these two components, or we have a mediator which any number of components can interact with. The mainWindow does a lot already, so I figured it makes sense to have a non-view-related class to structure the business logic of this operation.

In addition, if we move the GeoJSON parsing code to the mediator it would mean that the mapView would only need to know when the file is parsed as well and that we could remove the distinction between the file being opened and it being parsed. Additionally this would allow other parts of the app to do things with the contents of the GeoJSON file (e.g. show a list of contained features like marble or google earth does).

I'm open to things being cleaner, but idk what that would be.

@@ +156,3 @@
     },
 
+    _onLoadLayerActivate: function() {

I'm not sure. I think this code should also be in mainWindow.js.

The win.map-type action lives within mainWindow.js and this action is triggered by the popover. Similarly, opening the file dialog isn't particular to the popover, it's just where the initiating action lives.  Additionally, this is also how gedit handles the GtkFileChooserDialog.

@@ +245,3 @@
+            },
+            'load-layer': {
+                onActivate: this._onLoadLayerActivate.bind(this)

yep. I renamed it to open-layer-file-dialog

::: src/layersPopover.js
@@ +79,3 @@
+                                    1,
+                                    1);
+    },

I originally implemented it with a GtkListBox, but it doesn't match the mockups because the mockups have spacing in between the custom layers, and ListBoxRows are all adjacent.

Good idea with the user_data though....errr actually gjs doesn't support user_data argument in callbacks... but a closure works.
Comment 31 Hashem Nasarat 2016-01-02 00:38:06 UTC
Created attachment 318145 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 32 Jonas Danielsson 2016-01-02 09:33:02 UTC
Review of attachment 318110 [details] [review]:

::: data/gnome-maps.css
@@ +25,3 @@
+    background: white;
+}
+

Maybe one could use the GtkListBox header-func to create the effect. Or we do not follow the mockups completly. And do not have the spacing between the custom layers.

I feel we want this list to behave like a listbox, with selection and highlight. When I played with this I felt I wanted to be able to press the layer to go-to / zoom to it.

Do you agree?

::: data/ui/layers-popover.ui
@@ +57,3 @@
     </child>
+    <child>
+      <object class="GtkButton" id="load-layer-button">

It looks flat in the mockups, but when I tried it in code the result was a bit off yes :)

::: src/application.js
@@ +41,3 @@
 const Settings = imports.settings;
 const Utils = imports.utils;
+const LayersManager = imports.layersManager;

I think we can just have the mapView be the final opener, that is have the openGeoJSON method.

And when it adds a layer it fires a signal "custom-layer-added" and the popover can take a mapView (just like the sidebar does today) in its constructor. And when the file-open-dialog opens a file it can send it to the mapView openGeoJSON and it can also listen to the signal.

I understand what you are saying, but I do not think we need the level of abstraction for this. If, and only if, we want to do more fancy stuff that requires more of a mediator we can move towards it. But I do not want to add complexity were we do not need to.

@@ +156,3 @@
     },
 
+    _onLoadLayerActivate: function() {

I see.

I still want it in the popover tho. It is the GUI component of the layers, it is where they are shown, selected and removed. As a developer I would expect the code to be there I think. Nothing else will trigger the file-chooser than the button in the popover. So for now I think I want the code there, and no action.
Comment 33 Jonas Danielsson 2016-01-02 09:39:06 UTC
Review of attachment 318145 [details] [review]:

Thanks Hashem!

::: src/layersPopover.js
@@ +35,3 @@
+    }
+});
+

Maybe this clas can do more, to make the code below prettier. See comment further down.

And also. Could we prettify the filename? Do we need to have the ".geojson" included in the name? Maybe these kinds of things can have a name parameter? (Maybe KML can?) but before we have something like that maybe just stripping a way the extenstion would improve stuff?

@@ +71,3 @@
+        }).bind(this));
+
+        row.show();

I am pretty sure that gjs converts the children to camelCase. I think this code would be prettier as (with better formatting than I can manage):


let row = new CustomLayerRow({ visible: true,
filename: filename });
row.closeButton.connect('clicked', this._onRemoveClicked.bind(this, file));

And have the CustomLayerRow class deal with the making stuff pretty.
Comment 34 Jonas Danielsson 2016-01-02 09:39:08 UTC
Review of attachment 318145 [details] [review]:

Thanks Hashem!

::: src/layersPopover.js
@@ +35,3 @@
+    }
+});
+

Maybe this clas can do more, to make the code below prettier. See comment further down.

And also. Could we prettify the filename? Do we need to have the ".geojson" included in the name? Maybe these kinds of things can have a name parameter? (Maybe KML can?) but before we have something like that maybe just stripping a way the extenstion would improve stuff?

@@ +71,3 @@
+        }).bind(this));
+
+        row.show();

I am pretty sure that gjs converts the children to camelCase. I think this code would be prettier as (with better formatting than I can manage):


let row = new CustomLayerRow({ visible: true,
filename: filename });
row.closeButton.connect('clicked', this._onRemoveClicked.bind(this, file));

And have the CustomLayerRow class deal with the making stuff pretty.
Comment 35 Hashem Nasarat 2016-01-05 06:07:25 UTC
Review of attachment 318110 [details] [review]:

::: data/gnome-maps.css
@@ +25,3 @@
+    background: white;
+}
+

Yeah that seems pleasant.

::: src/application.js
@@ +41,3 @@
 const Settings = imports.settings;
 const Utils = imports.utils;
+const LayersManager = imports.layersManager;

alright. app and mainwindow talk to the popover which talks to the mapsView. No signals needed.

@@ +156,3 @@
     },
 
+    _onLoadLayerActivate: function() {

alright
Comment 36 Hashem Nasarat 2016-01-05 06:23:28 UTC
Review of attachment 318145 [details] [review]:

Pushing a hopefully final version. You can now click on the listBoxRows to move the bbox.

::: src/layersPopover.js
@@ +35,3 @@
+    }
+});
+

Yes KML can include a display name for a document, but from what I can tell GeoJSON doesn't have such information. Stripping the file extension may be the best thing for now.

@@ +71,3 @@
+        }).bind(this));
+
+        row.show();

Good idea with moving the code to set the labels to the class.
Unfortunately GJS underscores template children rather than camelCasing them.
Comment 37 Hashem Nasarat 2016-01-05 06:25:14 UTC
Created attachment 318237 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 38 Hashem Nasarat 2016-01-05 06:25:21 UTC
Created attachment 318238 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 39 Hashem Nasarat 2016-01-05 06:30:37 UTC
Created attachment 318240 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 40 Hashem Nasarat 2016-01-05 06:46:56 UTC
Created attachment 318243 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 41 Jonas Danielsson 2016-01-05 14:53:48 UTC
Review of attachment 318243 [details] [review]:

Thanks!

I have some comments below. I also would like comments from Andreas so mayve a screen cast could be provided?

Regarding hiding/showing layers, do we have a plan? Do we want the eye that gimp has on its layers?
Should we have a separate bug for it?

Great Work!

::: data/gnome-maps.css
@@ +8,3 @@
 
+.layer-radio-button,
+#layers-row-box {

Is this still needed? Why? Can't we rely on the standard listbox behavior?

::: src/layersPopover.js
@@ +22,3 @@
 const Lang = imports.lang;
 
+const Application = imports.application;

Where is this used?

@@ +44,3 @@
+            Gio.FileQueryInfoFlags.NONE,
+            null
+        ).get_attribute_byte_string(Gio.FILE_ATTRIBUTE_STANDARD_NAME);

Shouldn't this be: G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME ?

I was reading here:
https://developer.gnome.org/gio/stable/GFile.html#g-file-get-basename
To find out why we couldn't just do get_basename.

@@ +48,3 @@
+        /* Remove file extension in lieu of an overlay display name */
+        this.layer_label.label = filename.replace(/\.[^\.]+$/, '');
+        this.layer_label.tooltip_text = filename;

This should be this.layerLabel since we use camelCase in rest of Maps.

@@ +85,3 @@
+    },
+
+    _onRemoveClicked: function(button, file) {

I think it would be better to send the row as user_data to this function. Since the row will be our custom-row-class and that can have the file as a property.

@@ +90,3 @@
+            this._files.splice(index, 1);
+        }
+        this.ui.layersListBox.remove(button.parent.parent);

Then we do not have to do this dangerous parent.parent dance. We can just do row.destroy(); which is the recommended way of removing a child from a container.

All this code bacomes row.destroy();

@@ +93,3 @@
+        if (this._files.length <= 0)
+            this.ui.layersListBox.hide();
+        this._mapView.removeOverlay(file);

And here just this._mapView.view.remove_overlay_source(row.source);

@@ +98,3 @@
+    _onLoadLayerClicked: function(button) {
+        let fileChooserDialog = new Gtk.FileChooserDialog({
+            title: _("Load Map Layer File"),

Maybe something with Custom Layer since we call it that in other places, the file name for custom-layer-row for instance.

What do other application have for title in Gtk.FileChooserDialogs?

@@ +120,3 @@
+    },
+
+    openIfOverlay: function(file) {

This name is a bit to explicit. It should just be openCustomLayer or addCustomLayer and it can return false if it fails. Or throw an exception

@@ +126,3 @@
+
+        for (let i in this._files) {
+            if (this._files[i].get_uri() == uri) {

This should be ===, see: http://www.sitepoint.com/javascript-truthy-falsy/

And is this the best way of comparing uri? Is there no GLib convenience for this?

@@ +130,3 @@
+                break;
+            }
+        }

Hmm, so this to avoid duplicates? Maybe split it out in a separate function with a clear name?

if (this._customLayerExists(file)) ...

@@ +133,3 @@
+        if (!already_seen &&
+            (GEOJSON_MIMES.indexOf(content_type) > -1) &&
+                this._mapView.openGeoJSON(file)) {

This if statement is busy.

Maybe

if (this._customLayerExists(file) || this._supportMimeType(file))
   return false;

above? to make this function neater?

@@ +135,3 @@
+                this._mapView.openGeoJSON(file)) {
+            this._files.push(file);
+            let row = new CustomLayerRow({file: file});

As said I think this should take a ref to the actual source as well, it can be returned from openGeoJSON above.

@@ +138,3 @@
+            row.close_button.connect('clicked', (function(button) {
+                this._onRemoveClicked(button, file);
+            }).bind(this));

And this can take a row instead:

row.closeButton.connect('clicked',
this._onRemoveClicked.bind(this, row));

maybe?

@@ +140,3 @@
+            }).bind(this));
+
+            this.ui.layersListBox.insert(row, -1);

This could be this.ui.layersListBox.add(row);
I think?

@@ +141,3 @@
+
+            this.ui.layersListBox.insert(row, -1);
+            this.ui.layersListBox.show();

And create the row with { visible: true ... } included instead to avoid explicit show.

@@ +149,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoOverlayFileBBox(row.file);

This should be this._mapView.gotoBBox(row.source.bbox);

::: src/mainWindow.js
@@ +90,3 @@
 
+        this.layersPopover = new LayersPopover.LayersPopover(
+            { mapView: this._mapView }

Please have the { on the same line as the (

@@ +163,1 @@
+            if (this.layersPopover.openIfOverlay(file)) {

addCustomLayer

::: src/mapView.js
@@ +95,3 @@
         this._storeId = 0;
         this._connectRouteSignals();
+        this._overlay_by_file = {};

I do not think this should be needed if the customLayerRow has a ref to the actual source.

@@ +194,3 @@
     },
 
+    removeOverlay: function(file) {

this function is actually not needed I think, since the mapView has a public property 'view'

@@ +208,1 @@
     openGeoJSON: function(file) {

This could return a ref to the source or null instead of true/false.

@@ +234,1 @@
     },

Not needed if the customLayerRow has a ref to the source.
Comment 42 Jonas Danielsson 2016-01-05 15:02:38 UTC
Review of attachment 318145 [details] [review]:

::: src/layersPopover.js
@@ +71,3 @@
+        }).bind(this));
+
+        row.show();

Ah, ok, hmm, then make the id camelCase, like we do in the others!
for instance zoomControl or sidebar!
Comment 43 Hashem Nasarat 2016-01-05 17:15:40 UTC
Review of attachment 318145 [details] [review]:

::: src/layersPopover.js
@@ +71,3 @@
+        }).bind(this));
+
+        row.show();

OK
Comment 44 Hashem Nasarat 2016-01-05 19:04:56 UTC
Created attachment 318277 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 45 Hashem Nasarat 2016-01-05 19:05:15 UTC
Created attachment 318278 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 46 Hashem Nasarat 2016-01-05 19:06:48 UTC
Review of attachment 318243 [details] [review]:

Thanks! New version attached.

::: data/gnome-maps.css
@@ +8,3 @@
 
+.layer-radio-button,
+#layers-row-box {

It makes it look more like the mockups and I think the standard look is a little weird in this context. http://imgur.com/a/khs9Y

::: src/layersPopover.js
@@ +22,3 @@
 const Lang = imports.lang;
 
+const Application = imports.application;

oops. Removed.

@@ +44,3 @@
+            Gio.FileQueryInfoFlags.NONE,
+            null
+        ).get_attribute_byte_string(Gio.FILE_ATTRIBUTE_STANDARD_NAME);

Argh yeah I meant to do that but I think my brain got confused. Nice.

@@ +48,3 @@
+        /* Remove file extension in lieu of an overlay display name */
+        this.layer_label.label = filename.replace(/\.[^\.]+$/, '');
+        this.layer_label.tooltip_text = filename;

ok

@@ +90,3 @@
+            this._files.splice(index, 1);
+        }
+        this.ui.layersListBox.remove(button.parent.parent);

great idea.

@@ +93,3 @@
+        if (this._files.length <= 0)
+            this.ui.layersListBox.hide();
+        this._mapView.removeOverlay(file);

no because removeOverlay does more than just remove_overlay_source in the following commit

@@ +98,3 @@
+    _onLoadLayerClicked: function(button) {
+        let fileChooserDialog = new Gtk.FileChooserDialog({
+            title: _("Load Map Layer File"),

evince has "Open Document", we could do "Open Custom Layer"

@@ +120,3 @@
+    },
+
+    openIfOverlay: function(file) {

OK

@@ +126,3 @@
+
+        for (let i in this._files) {
+            if (this._files[i].get_uri() == uri) {

good idea I'll use g_file_equal

@@ +130,3 @@
+                break;
+            }
+        }

OK

@@ +133,3 @@
+        if (!already_seen &&
+            (GEOJSON_MIMES.indexOf(content_type) > -1) &&
+                this._mapView.openGeoJSON(file)) {

I like creating a supportMimeType function but I don't want like separating the single 'if' statement into two because it doesn't save any lines nor is more readable.

@@ +138,3 @@
+            row.close_button.connect('clicked', (function(button) {
+                this._onRemoveClicked(button, file);
+            }).bind(this));

yeah

@@ +140,3 @@
+            }).bind(this));
+
+            this.ui.layersListBox.insert(row, -1);

yeah

@@ +141,3 @@
+
+            this.ui.layersListBox.insert(row, -1);
+            this.ui.layersListBox.show();

No because this is to ensure that the if there is a GtkListBoxRow, then the GtkListBox is visible.

@@ +149,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoOverlayFileBBox(row.file);

ok

::: src/mainWindow.js
@@ +90,3 @@
 
+        this.layersPopover = new LayersPopover.LayersPopover(
+            { mapView: this._mapView }

ok

@@ +163,1 @@
+            if (this.layersPopover.openIfOverlay(file)) {

ok

::: src/mapView.js
@@ +95,3 @@
         this._storeId = 0;
         this._connectRouteSignals();
+        this._overlay_by_file = {};

true

@@ +194,3 @@
     },
 
+    removeOverlay: function(file) {

it's needed in a later patch

@@ +208,1 @@
     openGeoJSON: function(file) {

Yeah

@@ +234,1 @@
     },

and I'll make _gotoBBox public
Comment 47 Hashem Nasarat 2016-01-05 19:19:27 UTC
Created attachment 318280 [details] [review]
Fix css to show which base map is selected

A radio button is only :active when it's clicked and held. :checked is
for when it's selected.

Also, since there is a 2px border when :checked, we make the unchecked
border 1px.
Comment 48 Hashem Nasarat 2016-01-05 19:19:37 UTC
Created attachment 318281 [details] [review]
Fix typo in this._addContacts

Function calls still work with spaces after the '.', but it looks wrong.
Comment 49 Hashem Nasarat 2016-01-05 19:19:47 UTC
Created attachment 318282 [details] [review]
Allow for more than one overlay map source

The code is capable of displaying separate overlay_sources
simultaneously, and we soon will allow the user to select multiple
overlay files to display.
Comment 50 Hashem Nasarat 2016-01-05 19:19:56 UTC
Created attachment 318283 [details] [review]
In parsing GeoJSON only try/catch where it's needed

geoJSONSource.parse() is the only thing that raises exceptions. Having
the try/catch around all of the code makes it unclear at which stage of
the function does the process fail.
Comment 51 Hashem Nasarat 2016-01-05 19:20:10 UTC
Created attachment 318284 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 52 Hashem Nasarat 2016-01-05 19:20:19 UTC
Created attachment 318285 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 53 Jonas Danielsson 2016-01-05 19:48:41 UTC
Review of attachment 318284 [details] [review]:

Thanks!

Some more comments below!
Getting there!

::: data/gnome-maps.css
@@ +8,3 @@
 
+.layer-radio-button,
+#layers-row-box {

Hmm, I am still a bit wary of this. How about we instead add a GtkFrame object around the box object in the custom-layer-row.ui ? Then we get the border, I just want the GtkListBox to be like rest of GNOME...

::: src/layersPopover.js
@@ +49,3 @@
+
+        /* Remove file extension in lieu of an overlay display name */
+        this.layerLabel.label = filename.replace(/\.[^\.]+$/, '');

And there is no GLib function for this?

And what exactly is the DISPLAY_NAME? :)

@@ +91,3 @@
+        if (index > -1) {
+            this._files.splice(index, 1);
+        }

If we make this a javascript object type associative array instead of an array  (this._layers = {} instead of this._file = [] maybe) we should be able to go:

delete this._layers[row.fileNmae];

or something like that, without all this cumbersome index things that are so bug prone.

@@ +92,3 @@
+            this._files.splice(index, 1);
+        }
+        this._mapView.removeOverlay(row.overlay);

this._mapView.view.remove_overlay_source(row.source);

@@ +95,3 @@
+        row.destroy();
+        if (this._files.length <= 0)
+            this.ui.layersListBox.hide();

If we want to get fancy we could a GObject-property to this class that is gboolean "isEmpty" and bind the property to the visible property of the layersListBox and this would happen automagicly. But not sure it is worth it.

@@ +122,3 @@
+    },
+
+    openCustomLayer: function(file) {

A thought: Since this is now the primary place of open a layer, should the inApp-notification be launched here on failure now? Instead from the mapView.js?

What do you think?

@@ +125,3 @@
+        if (this._customLayerExists(file) ||
+            !this.supportLayerFileMimeType(file))
+            return false;

I prefer to have braces { } if the if-statment is multi-line. There is noway to make it single line? Maybe a simpler name for : this._mimeTypeSupported(file) ? (Does it need to be public?)

@@ +128,3 @@
+
+        let overlay = this._mapView.openGeoJSON(file);
+        if (overlay) {

I would prefer source over overlay.

@@ +131,3 @@
+            this._files.push(file);
+            let row = new CustomLayerRow({file: file,
+                                          overlay: overlay});

same here.

@@ +134,3 @@
+            row.closeButton.connect('clicked', (function(button) {
+                this._onRemoveClicked(button, row);
+            }).bind(this));

It would be too long line to do this without closure?

row.closeButton.connect('vlivked',
this._onRemove.bind(this, row));

@@ +144,3 @@
+
+    _customLayerExists: function(file) {
+        for (let i in this._files)

If we want an array then: this._files.forEach... if we go with object then jhust this._layers[filename] or something?

@@ +156,3 @@
+
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();

Why unselect_all?

@@ +157,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoBBox(row.overlay.bbox);

We might need to check if it is valid? Here or in the function I guess might make more sense.

::: src/mapView.js
@@ +195,3 @@
+    removeOverlay: function(overlay) {
+        this.view.remove_overlay_source(overlay);
+    },

I am not sure I want to add this, when all who needs it has access to mapView.view public property.

But I understand the need  next patch. But this will only on geoJSONSource at the moment, right? So it is icky. It cannot be removeOverlay, maybe removeGeoJSON?

But hmm, maybe we could get a way with overloading the dispose function of geoJSONSource:

vfunc_dispose: function() {
   this.placeMarker ... 
}


in geoJSONSource, so that when the source get's removed (unreffed) all the marker will be removed. It will be more self-contained and this will not be needed.
Comment 54 Jonas Danielsson 2016-01-05 19:50:47 UTC
Review of attachment 318284 [details] [review]:

::: src/mapView.js
@@ +195,3 @@
+    removeOverlay: function(overlay) {
+        this.view.remove_overlay_source(overlay);
+    },

it would need to chain up as well with a this.parent(); call
Comment 55 Hashem Nasarat 2016-01-06 02:26:04 UTC
Review of attachment 318284 [details] [review]:

Another one! Thx

::: src/layersPopover.js
@@ +49,3 @@
+
+        /* Remove file extension in lieu of an overlay display name */
+        this.layerLabel.label = filename.replace(/\.[^\.]+$/, '');

not that I could find. I clarified the comment.

@@ +91,3 @@
+        if (index > -1) {
+            this._files.splice(index, 1);
+        }

ok. Not that we need whatever the key is in this associative array but it does make this section easier. I really miss python's builtin types right now.

@@ +92,3 @@
+            this._files.splice(index, 1);
+        }
+        this._mapView.removeOverlay(row.overlay);

we can't just inline the call to mapView.view because removeOverlay removes the markers too.

@@ +95,3 @@
+        row.destroy();
+        if (this._files.length <= 0)
+            this.ui.layersListBox.hide();

yeah that'd be slick, but it doesn't seem worth it in this case.

@@ +122,3 @@
+    },
+
+    openCustomLayer: function(file) {

I don't think it matters, but I'll move it here.

@@ +125,3 @@
+        if (this._customLayerExists(file) ||
+            !this.supportLayerFileMimeType(file))
+            return false;

ok

@@ +128,3 @@
+
+        let overlay = this._mapView.openGeoJSON(file);
+        if (overlay) {

ok

@@ +134,3 @@
+            row.closeButton.connect('clicked', (function(button) {
+                this._onRemoveClicked(button, row);
+            }).bind(this));

good idea.

@@ +144,3 @@
+
+    _customLayerExists: function(file) {
+        for (let i in this._files)

I still want to use g_file_equal (because GFiles with different filenames can refer to the same underlying file) so there's still going to be a bit of iteration.

@@ +156,3 @@
+
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();

With unselect_all the ListBoxRows act kind of like a button that centers the view when you click it. Without unselect_all when you click on a row it highlights in blue which is kind of useless as it conveys no information.

@@ +157,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoBBox(row.overlay.bbox);

when would we have an invalid bbox?

::: src/mapView.js
@@ +195,3 @@
+    removeOverlay: function(overlay) {
+        this.view.remove_overlay_source(overlay);
+    },

There's no reason removeOverlay can't work for KML too.

overloading dispose wouldn't be that clean because the mapView needs to do stuff with private instance variables when a geoJSONSource is removed.
Comment 56 Hashem Nasarat 2016-01-06 02:37:36 UTC
Created attachment 318304 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 57 Damián Nohales 2016-01-06 04:49:03 UTC
Review of attachment 318304 [details] [review]:

This is a really nice work!

Some comments below.

::: data/ui/custom-layer-row.ui
@@ +19,3 @@
+                <property name="ellipsize">PANGO_ELLIPSIZE_END</property>
+                <property name="max-width-chars">1</property>
+                <property name="xalign">0</property>

Damn, I was trying to not use xalign here since it's deprecated, but looks like halign=start doesn't play well with ellipsize, maybe something to report to Gtk+ devs.

::: data/ui/layers-popover.ui
@@ +77,3 @@
+        <property name="top-attach">3</property>
+        <property name="width">1</property>
+        <property name="height">1</property>

There is no need to specify width and height here (though the same goes for the street-layer-button and aerial-layer-button buttons).

::: src/application.js
@@ +279,3 @@
 
+        if (!this._mainWindow.layersPopover.openCustomLayer(file)) {
+            Utils.debug('Unsupported file');

Do we need to be more verbose here (path and mimetype at least)?
Also, don't use curly braces for this one-line if body.

::: src/layersPopover.js
@@ +50,3 @@
+
+        /* Remove file extension and use that in lieu of a fileformat-specific
+        * display name. */

Align these asterisks (I thought we were using // comments in Maps, but found mixed feelings around the code :) )

@@ +81,3 @@
         this.get_style_context().add_class('maps-popover');
+        this.add(this.ui.grid);
+        this._files_by_row = {};

I think there is no need to use this associative array at all. You can just get the children rows using this.ui.layersListBox.get_children(). So you can count the children in _onRemoveClicked to hide the listbox and use the row "file" property while iterating children to make the comparisons in _customLayersExists.

@@ +116,3 @@
+        if (fileChooserDialog.run() === Gtk.ResponseType.OK) {
+            this.openCustomLayer(fileChooserDialog.get_file());
+        }

Don't use curly braces here.

@@ +123,3 @@
+    openCustomLayer: function(file) {
+        if (this._customLayerExists(file) || !this._fileTypeSupported(file))
+            return false;

In case that the custom layer exists I would goto the layer BBox anyway, make sense, right?
Maybe you can rename _customLayerExists to _findCustomLayerRow and return the existing row to make this easy.

@@ +137,3 @@
+            return true;
+        }
+        else {

} else {

@@ +158,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoBBox(row.source.bbox);

About validating the bbox here, could we just check the bbox validity in gotoBBox method and throw a warning in the console in case of invalid. Then we can remove the "if (geoJSONSource.bbox.is_valid())" from MapView.openGeoJSON method.
Comment 58 Damián Nohales 2016-01-06 04:58:56 UTC
I also have some general comments after doing some usage tests.

* Do we want to add support for multiple file selection and multiple file DnD? at least for a future ticket?

* I really don't like how the close button looks over the row when you hover it. I'd love to make the hovered button style more flat (maybe by changing the image color on hover and removing all backgrounds, borders, etc). But I'm afraid that would require lots of CSS.

* Once we load the GeoJSON using the file chooser dialog we should hide the layers popover. It feel weird that once I select a file to visualize it I cannot make use of the map, having to dismiss the layer popover first.
Comment 59 Jonas Danielsson 2016-01-06 06:32:11 UTC
Review of attachment 318304 [details] [review]:

Thanks for reviewing Damian!

Responded to a few of your comments and added some!

::: data/ui/layers-popover.ui
@@ +71,3 @@
+        <property name="visible">True</property>
+        <property name="can-focus">True</property>
+        <property name="label" translatable="yes">Load map layer</property>

Is Load map layer reduntant? What other layer could it be?

Load layer?

::: src/application.js
@@ +279,3 @@
 
+        if (!this._mainWindow.layersPopover.openCustomLayer(file)) {
+            Utils.debug('Unsupported file');

Not sure, this is for debug so only when MAPS_DEBUG=1 and then you probably know what file you tried to add? Maybe, maybe: 'openCustomLayer failed for %s'.format(file.get_basename()); ?

::: src/layersPopover.js
@@ +81,3 @@
         this.get_style_context().add_class('maps-popover');
+        this.add(this.ui.grid);
+        this._files_by_row = {};

Yes! Nice!

@@ +123,3 @@
+    openCustomLayer: function(file) {
+        if (this._customLayerExists(file) || !this._fileTypeSupported(file))
+            return false;

Agreed.

@@ +157,3 @@
+
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();

Ok, after reading why you want this I think we can remove it :)

And replace it with adding:
        <property name="selection_mode">none</property>

in layers-popover.ui as property on the listBox.

@@ +158,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoBBox(row.source.bbox);

Agreed about validating in gotoBBox, but not sure about warning. In console? What should the user do with that information if s/he sees it? Maybe Utils.debug but possibly nothing.

::: src/mapView.js
@@ +194,3 @@
 
+    removeOverlay: function(overlay) {
+        this.view.remove_overlay_source(overlay);

So I still disagree :)

I would love to have it on the geoJSONSource, it adds markers to the markerLayer and it could remove them as well (if we keep the markerLayer as a property on the geoJSONSource)

But if you still disagree with that strongly. At least it cannot be named removeOverlay.
That we are using overlay_source on champlain view is an implementation detail of the custom layers. We could have used a ChamplainLayer as well.

And even so, all overlay sources does not have this. And an Overlay is also a GtkOverlay. So this needs to be removeCustomLayer. Which feels bad since the open method is in layersPopover. So maybe this should be in layersPopover? it has a mapView reference, so maybe the loop belongs there as well. _removeCustomLayer as a private method of layersPopover?
Comment 60 Jonas Danielsson 2016-01-06 10:22:47 UTC
(In reply to Damián Nohales from comment #58)
> I also have some general comments after doing some usage tests.
> 
> * Do we want to add support for multiple file selection and multiple file
> DnD? at least for a future ticket?

Maybe? I do not feel strongly that it is something we need, but it is a quiet feature and should be easy enough, might be a 1/2-liner. So yeah a new bug sounds fine, marked as gnome-love.

> 
> * I really don't like how the close button looks over the row when you hover
> it. I'd love to make the hovered button style more flat (maybe by changing
> the image color on hover and removing all backgrounds, borders, etc). But
> I'm afraid that would require lots of CSS.

I think it might be an issue with the button being the same height as the row, so it seems like part of the row is a button, maybe some margins on the button or something could help? Or some padding on the box?

> 
> * Once we load the GeoJSON using the file chooser dialog we should hide the
> layers popover. It feel weird that once I select a file to visualize it I
> cannot make use of the map, having to dismiss the layer popover first.
Comment 61 Jonas Danielsson 2016-01-06 10:27:56 UTC
Also make sure we do not break keyboard accessibility on the layers-popover!
Comment 62 Hashem Nasarat 2016-01-07 05:18:21 UTC
Review of attachment 318304 [details] [review]:

Thanks a lot for the reviews! I'm learning a lot about the Gtk api and this code is getting pretty slick. I'm attaching an updated version that overhauls the error handling and makes lots of little improvements for clarity here and there.

::: data/ui/custom-layer-row.ui
@@ +19,3 @@
+                <property name="ellipsize">PANGO_ELLIPSIZE_END</property>
+                <property name="max-width-chars">1</property>
+                <property name="xalign">0</property>

hm? https://developer.gnome.org/gtk3/unstable/GtkLabel.html#GtkLabel--xalign xalign seems to be new since 3.16 and it doesn't say it's deprecated.

::: data/ui/layers-popover.ui
@@ +71,3 @@
+        <property name="visible">True</property>
+        <property name="can-focus">True</property>
+        <property name="label" translatable="yes">Load map layer</property>

I like "Open overlay file". 
- Open is the typical verb used
- overlay better connotes what GeoJSON and KML file formats support, and how the app treats them.
- file indicates to the user that the file chooser dialog will open.

@@ +77,3 @@
+        <property name="top-attach">3</property>
+        <property name="width">1</property>
+        <property name="height">1</property>

nice catch

::: src/application.js
@@ +279,3 @@
 
+        if (!this._mainWindow.layersPopover.openCustomLayer(file)) {
+            Utils.debug('Unsupported file');

I overhauled the error handling regarding custom layers so we should print out more useful messages all the time not just here.

::: src/layersPopover.js
@@ +81,3 @@
         this.get_style_context().add_class('maps-popover');
+        this.add(this.ui.grid);
+        this._files_by_row = {};

great!

@@ +116,3 @@
+        if (fileChooserDialog.run() === Gtk.ResponseType.OK) {
+            this.openCustomLayer(fileChooserDialog.get_file());
+        }

OK

@@ +123,3 @@
+    openCustomLayer: function(file) {
+        if (this._customLayerExists(file) || !this._fileTypeSupported(file))
+            return false;

OK

@@ +137,3 @@
+            return true;
+        }
+        else {

OK

@@ +157,3 @@
+
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();

OK Great

@@ +158,3 @@
+    _onRowActivated: function(listBox, row) {
+        listBox.unselect_all();
+        this._mapView.gotoBBox(row.source.bbox);

OK. I feel like it should throw an exception, but going around and fixing up the code seems a bit invasive for unknown benefit. Idk when the BBox would be invalid in common use cases anyway.

::: src/mapView.js
@@ +194,3 @@
 
+    removeOverlay: function(overlay) {
+        this.view.remove_overlay_source(overlay);

I implemented this using GeoJSONSource's dispose function, but of course it doesn't work with gjs:

(org.gnome.Maps:29693): Gjs-CRITICAL **: Attempting to call back into JSAPI during the sweeping phase of GC. This is most likely caused by not destroying a Clutter actor or Gtk+ widget with ::destroy signals connected, but can also be caused by using the destroy() or dispose() vfuncs. Because it would crash the application, it has been blocked and the JS callback not invoked.

Additionally I would override the view's vfunc_remove_overlay_source but alas we don't have access to that either.

> That we are using overlay_source on champlain view is an implementation detail of the custom layers. We could have used a ChamplainLayer as well.

I agree and that's why I initially had all these functions passing files around, but now we have GtkListBoxRow's dealing with ChamplainMapSources so the only thing that is an implementation detail is that mapView has an annotationMarkerLayer. So either the popover makes a call to remove_overlay_source AND removeOverlaySourceMarkers, or we have in mapView a removeOverlaySource which removes the source and the markers. To me, the second one seems like a good enough solution to this small problem.
Comment 63 Hashem Nasarat 2016-01-07 05:20:19 UTC
Created attachment 318403 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 64 Hashem Nasarat 2016-01-07 05:20:24 UTC
Created attachment 318404 [details] [review]
Display markers from multiple overlay sources

Before the code only showed the markers from the most recently-loaded
GeoJSON file, but now we show all the markers from all loaded GeoJSON
files.
Comment 65 Hashem Nasarat 2016-01-07 05:20:29 UTC
Created attachment 318405 [details] [review]
Correct DND API usage for overlay layer loading

Sometimes maps wouldn't accept a drop because we weren't setting the
drag_status appropriately.

Also change drag_finish to denote that the originally-dropped file is
not a semantic move and that the source shouldn't delete the original
data because Maps doesn't really take responsibility for the file -- it
just displays it until the app quits.
Comment 66 Jonas Danielsson 2016-01-07 06:13:50 UTC
Review of attachment 318405 [details] [review]:

Thanks Hashem!

I've been meaning to look into this! Nice catch!

Some comments below.

For the commit message, how about:

"mainWindow: Set correct drag status"

I do not like the use of "overlay" in code when talking about this. It is confusing since we use Overlay for other stuff as well, like GtkOverlay.
And we only have GeoJSON atm, we will have KML/KMZ :) But new users do not know that. So talking about GeoJSON is better right now!

Awesome!

::: src/mainWindow.js
@@ +157,3 @@
+            /* TODO Call gtk_drag_get_data() and check the data's MIME type in
+             * drag-data-received's callback when GJS becomes able to create
+             * the GdkAtom struct needed for the function invocation. */

This will never happen. A GdkAtom can never be used from Maps. A GdkAtom is just a pointer, we cannot deal with that. We would need new API in GTK to be able to do this. Something we might want to do.

But I do not want this TODO here, I think we should remove it. I much prefer to create bugzilla bugs rather than sprinkle TODO's in the code. They tend to linger and are not that discoverable or traceable :)
Comment 67 Hashem Nasarat 2016-01-07 06:26:22 UTC
Created attachment 318406 [details] [review]
mainWindow: set correct DND drag status

Sometimes Maps wouldn't accept a drop because we weren't setting the
drag_status appropriately.

Also change drag_finish to denote that the originally-dropped file is
not a semantic move and that the source shouldn't delete the original
data because Maps doesn't really take responsibility for the file -- it
just displays it until the app quits.
Comment 68 Hashem Nasarat 2016-01-07 06:27:13 UTC
Review of attachment 318405 [details] [review]:

I know GtkOverlay is possible to get confused with, but overlay is also a source that Champlain uses so I don't think with the context people will get confused. But i'll try to avoid it.
Thanks for the tips w/ commit message and such. Attaching new version....

::: src/mainWindow.js
@@ +157,3 @@
+            /* TODO Call gtk_drag_get_data() and check the data's MIME type in
+             * drag-data-received's callback when GJS becomes able to create
+             * the GdkAtom struct needed for the function invocation. */

I didn't know where to create this bug. Either gjs or gtk. And I didn't want to look through existing bugs to see if there was something already reported :/


Buuuut I'll remove the TODO! :)
Comment 69 Hashem Nasarat 2016-01-07 06:29:41 UTC
Review of attachment 318405 [details] [review]:

How do I obsolete this patch manually if I forgot to do git bz attach -e?
Comment 70 Jonas Danielsson 2016-01-07 06:34:04 UTC
Review of attachment 318403 [details] [review]:

Thanks!

Some comments below.

::: data/ui/layers-popover.ui
@@ +68,3 @@
+        <property name="visible">True</property>
+        <property name="can-focus">True</property>
+      </object>

I really do not like "overlay file", I see this as a layer, and googling also talks about layers. I will defer to Andreas but I much prefer layer.

::: src/layersPopover.js
@@ +87,3 @@
+                this._mapView.gotoBBox(row.source.bbox);
+            }).bind(this)
+        );

I do not like this kind of function line-breaks and we do not use it elsewhere in Maps.

If the line is way to long, I would prefer to use shorter variable names, maybe:

this.ui.layersListBox.connect('row-activated', (function(l, row) {

...

}).bind(this));

@@ +97,3 @@
+        row.destroy();
+        if (this.ui.layersListBox.get_children().length <= 0)
+

Could we avoid showing/hiding the listBox if we made the open-button part of the listBox?

@@ +102,3 @@
+    _onLoadLayerClicked: function(button) {
+        let fileChooserDialog = new Gtk.FileChooserDialog({
+                                        this._onLoadLayerClicked.bind(this));

Again, do not like Overlay for this.

@@ +107,3 @@
+
+        let fileFilter = new Gtk.FileFilter();
+    _onRemoveClicked: function(row, button) {

Should this be CUSTOM_MIMES?

@@ +127,3 @@
+        try {
+            if (!this.fileTypeSupported(file)) {
+            transient_for: this.get_parent()

No need to translate debug information

@@ +128,3 @@
+            if (!this.fileTypeSupported(file)) {
+                let msg = _("File type not among supported types: %s");
+            transient_for: this.get_parent()

I do not want to throw mime-types at the user.

@@ +138,3 @@
+                } catch (e) {
+                    throw e;
+            fileFilter.add_mime_type(mime);

Why have this nested try/catch, it does not give us anything?

@@ +153,3 @@
+        } catch (e) {
+            Utils.debug(e);
+        fileChooserDialog.add_button('_Cancel', Gtk.ResponseType.CANCEL);

No, we want to keep MAPS_DEBUG=1 usable, this is to much to print here.
Just Utils.debug('failed to open file %s'.format(e.message));

Will do.

@@ +156,3 @@
+            Application.notificationManager.showMessage(
+                _("Failed to open file: %s").format(e.message)
+

Do not include e.message here. Maybe just _("Failed to add custom layer"); ?

We need to stay our urge to try to explain "Why" to the user all the time, most often there is nothing that can be done.

@@ +163,3 @@
+    _findCustomLayerRow: function(file) {
+        let rows = this.ui.layersListBox.get_children();
+        fileChooserDialog.destroy();

rows.forEach...

@@ +169,3 @@
+    },
+
+    openCustomLayer: function(file) {

Does this need to be public?

::: src/mapView.js
@@ +195,3 @@
+    removeOverlaySource: function(source) {
+        this.view.remove_overlay_source(source);
+    },

Again, do not like this: It is not for overlaySources. overlaySources does not have placeMarkers overlay_sources are just map sources (like GeoJSONSource that are placed on top of other map sources).

The view has a function for removing an ordinary overlay source. What we want is to remove a customLayer. To do that we remove it as an overlay source and then remove the markers. So I do not like the name of this.

And I do not like adding these small helper functions to mapView.js it just grows and grows. Can't this be a part of layersPopover? That it does this._mapView.view.remove_overlay_source(row.source) and then removes the markers?

row.source.placeMarkers.forEach(function(marker) { marker.destroy });

@@ +208,3 @@
         } catch(e) {
+            e.message = "GeoJSON error: " + e.message;
+            throw e;

Keep the Utils.debug, do not throw e, just return null here.

@@ +296,3 @@
+    gotoBBox: function(bbox, linear) {
+        if (!bbox.is_valid()) {
+            Utils.debug(_("Bounding box is invalid"));

We do not translate debug texts, we do not want to be bound by string freeze for these type of debug prints.
Comment 71 Jonas Danielsson 2016-01-07 06:35:05 UTC
Review of attachment 318405 [details] [review]:

It is alot of clicking details and edit details and stuff like that, try to find it otherwise I can do it later.
Comment 72 Jonas Danielsson 2016-01-07 06:42:16 UTC
(In reply to Hashem Nasarat from comment #67)
> Created attachment 318406 [details] [review] [review]
> mainWindow: set correct DND drag status
> 
> Sometimes Maps wouldn't accept a drop because we weren't setting the
> drag_status appropriately.
> 
> Also change drag_finish to denote that the originally-dropped file is
> not a semantic move and that the source shouldn't delete the original
> data because Maps doesn't really take responsibility for the file -- it
> just displays it until the app quits.

Could this be rebased on top of master instead? That way we could push it sooner?

Say if we do not include the rest in 3.19.4 (januari 14) but want to include this one.
Comment 73 Jonas Danielsson 2016-01-07 07:06:30 UTC
Review of attachment 318403 [details] [review]:

::: src/mapView.js
@@ +195,3 @@
+    removeOverlaySource: function(source) {
+        this.view.remove_overlay_source(source);
+    },

Or maybe even have a method removeMarkers on GeoJSONSource that does marker.destroy(); on all its markers?
then layersPopover.js can do:

row.source.removeMarkers();
this._mapView.view.remove_overlay_source(row.source);
Comment 74 Andreas Nilsson 2016-01-07 12:30:48 UTC
(In reply to Hashem Nasarat from comment #5)
> Andreas, does the new design make the designs on
> https://wiki.gnome.org/Design/Apps/Maps obsolete?
> 
> Maps has support for bikes and transport, but we currently don't have a way
> in the UI to enable it.

Hi, and sorry for the delayed reply. I've been on vacation over the holidays and only got back today.

I would say yes, bike overlays doesn't make sense for the moment. Lets focus on custom layers.
Comment 75 Andreas Nilsson 2016-01-07 12:38:24 UTC
(In reply to Jonas Danielsson from comment #10)
> (In reply to Hashem Nasarat from comment #9)
> > I have a preliminary implementation of this[0], but I ran into some design
> > roadblocks.
> > 
> 
> Cool I will hold out reviewing until it appears as patches here. In order to
> have all history / comments on this page. For trace-ability.
> 
> > 1. If we load lots of layers, the popover will grow. Should we have
> > scrolling? Where?
> > (I think we should only have the overlay layers scroll).
> 
> I kind of lean to no. That we instead say 5 layers is enough for everybody.
> I do not see the use-case for many many layers and adding complexity in code
> (getting that pixel perfect and stuff) and UI does not seem worth it.
> Instead we can show an in-app notification if one tries to add more, and/or
> gray out the load-layer button.
> 
> > 
> > 2. Should we save the included overlay layers and restore them when the user
> > starts the app?
> > (I think yes)
> > 
> 
> I am not sure. I am trying to think of other apps. Do we restart evince with
> the last pdf? or videos with the last movies. For these kinds of layers (KML
> and GeoJSON) I see Maps as a file-viewer. A way to view the meta-data
> contained in this shapefile.

I think Videos do resume where you left off since a version or two back.
Restoring the state when opening the app again doesn't sound like a bad idea to me. It depends a bit on the exact use cases we want to cover, but if you have a custom map for a conference that runs over 3 days, having to add it again every time you launch the app would be a bit annoying.
For the first version I'm OK with us keeping it basic though.

> > 3. What is the mechanism for toggling the display of an overlay layer?
> > Currently the design only shows a 'x' button. I presume this removes it from
> > the list and hides whatever overlays it contained. This seems like a hassle
> > if the user has to find the overlay file on their filesystem to reenable it
> > each time.
> > (Easy solution would be to have a checkbox to toggle hidden or not, but this
> > doesn't seem very elegant design-wise).
> 
> I say remove the overlay when pressing x, we could have an "are you sure"
> thing and even an in-app notification with undo-button. But I think we
> remove it and user has to re-add it. It is like closing a file in Gedit for
> me.

I would really not like to see a dialog here. It's not a super-destructive action, and Boxes makes less of a fuzz about deleting an entire Virtual Machine :)
A in-app notification with undo sounds better in that case.
Comment 76 Hashem Nasarat 2016-01-07 17:06:35 UTC
Created attachment 318430 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 77 Hashem Nasarat 2016-01-07 17:06:54 UTC
Review of attachment 318403 [details] [review]:

Attaching new version with small changes, and a screencast.

::: src/layersPopover.js
@@ +87,3 @@
+                this._mapView.gotoBBox(row.source.bbox);
+            }).bind(this)
+        );

ok

@@ +97,3 @@
+        row.destroy();
+        if (this.ui.layersListBox.get_children().length <= 0)
+            this.ui.layersListBox.hide();

I don't like that. It would mean either we construct the button only in code or programmatically move it from the Grid to the listBox. Additionally, the layersListBox semantically should only have layers. And the styling would change.

@@ +107,3 @@
+
+        let fileFilter = new Gtk.FileFilter();
+        GEOJSON_MIMES.forEach(function(mime) {

for now no. This can change when kml gets added

@@ +127,3 @@
+        try {
+            if (!this.fileTypeSupported(file)) {
+                let msg = _("File type not among supported types: %s");

But this is shown to the user in the UI

@@ +128,3 @@
+            if (!this.fileTypeSupported(file)) {
+                let msg = _("File type not among supported types: %s");
+                throw new Error(msg.format(GEOJSON_MIMES.join(', ')));

OK

@@ +138,3 @@
+                } catch (e) {
+                    throw e;
+                }

Just shows that exceptions are expected to be thrown from this call. I'll remove it

@@ +153,3 @@
+        } catch (e) {
+            Utils.debug(e);
+            Utils.debug(e.stack);

If there's any unintended programmatic error in the custom layer codepaths it gets caught here and it would be nice to show a stack trace. This isn't a commonly run code path anyway. It'll only occur if you're loading broken custom layer files or if you drag and drop incompatible files.

@@ +156,3 @@
+            Application.notificationManager.showMessage(
+                _("Failed to open file: %s").format(e.message)
+            );

Yeah I know there's nothing to be done, but at least this provides the user with a little bit of information with which to try and solve the problem, or report to the developers, or google. I want to see what Andreas thinks

@@ +163,3 @@
+    _findCustomLayerRow: function(file) {
+        let rows = this.ui.layersListBox.get_children();
+        for (let i in rows)

It'd be nice to short circuit once we find the matching row though.

@@ +169,3 @@
+    },
+
+    fileTypeSupported: function(file) {

It might be used by the DND code at some point.
Comment 78 Hashem Nasarat 2016-01-07 17:09:42 UTC
Andreas, here's a screencast of the lastest version.

We're having difficulty figuring out strings, and I think we'll defer to you if you don't mind:
* Terminology: "Custom Layer" (Jonas likes, I don't) vs "Overlay File" (I think is tolerable, jonas doesn't like)
* Button in the popover
* Title in FileChooser
* Error messages (more detailed as they are in the video, or less detailed like "Failed to load custom layer"
Comment 79 Hashem Nasarat 2016-01-07 17:12:45 UTC
screencast: http://webm.land/w/lDsN/
Comment 80 Damián Nohales 2016-01-07 20:38:42 UTC
Review of attachment 318304 [details] [review]:

::: data/ui/custom-layer-row.ui
@@ +19,3 @@
+                <property name="ellipsize">PANGO_ELLIPSIZE_END</property>
+                <property name="max-width-chars">1</property>
+                <property name="xalign">0</property>

Ah, you're right, I got confused since in 3.14 GtkMisc was deprecated, deprecating [x|y]pad and [x|y]align properties, and GtkLabel inherited those properties from GtkMisc. I remember that Gtk+ had some problems with multiline labels + halign, so looks like xalign got to be added again in 3.16 :)
Comment 81 Damián Nohales 2016-01-07 22:45:38 UTC
Review of attachment 318430 [details] [review]:

Thanks! besides we still have some philosophical things to tie up :) , I think we are almost there.

::: src/layersPopover.js
@@ +125,3 @@
+        try {
+            if (!this.fileTypeSupported(file))
+                throw new Error(_("File type not among supported types"));

So, we are not showing this file in the UI, does it need to be translated.

@@ +131,3 @@
+                let source = this._mapView.loadGeoJSON(file);
+                row = new CustomLayerRow({file: file,
+                                              source: source});

This is bad indented.

@@ +143,3 @@
+        } catch (e) {
+            Utils.debug(e);
+            Utils.debug(e.stack);

Hmmm, I'd rather improve Utils.debug to detect when caller pass an exception object so it could print the stack in that case. Too verbose?

@@ +145,3 @@
+            Utils.debug(e.stack);
+            Application.notificationManager.showMessage(
+                _("Failed to open file.")

Don't end this string with a dot.

::: src/mapView.js
@@ +296,3 @@
+    gotoBBox: function(bbox, linear) {
+        if (!bbox.is_valid()) {
+            Utils.debug(_("Bounding box is invalid"));

Don't translate this.
Comment 82 Jonas Danielsson 2016-01-08 06:13:52 UTC
Hi Hashem and Damián. Regarding your talk while I slept:

<dnohales> I don't know if I'm explaining this ok. It's just to have the UI and map data separated, and concise roles for the classes

<hashem> dnohales, I agree with you and you're explaining it well :) I think jonas wants to have the simplest code though even if it isn't the most decoupled, and I want to get it merged in a form that most people are reasonably happy with

<dnohales> hashem: I know :D , then I think we need to wait to jonasdn opinion


----

I agree with what I think Damián is saying here. And I feel it is sort of the same that I suggested in comment 23: https://bugzilla.gnome.org/show_bug.cgi?id=759544#c32

To have mapView as "the model" or "mediator" so that actual adding/opening is done by mapView and the UI, or "view" (the layersPopover) will listen to a signal from the mapView to see when the "model" has updated, that is, addad a layer. And the UI will then reflect that.

That way application.js would not need to know about a layersPopover for instance and we would have a better decoupling.
Comment 83 Jonas Danielsson 2016-01-08 06:25:03 UTC
Review of attachment 318430 [details] [review]:

Thanks!

::: data/ui/custom-layer-row.ui
@@ +10,3 @@
+            <property name="name">layers-row-box</property>
+            <property name="visible">True</property>
+            <property name="orientation">GTK_ORIENTATION_HORIZONTAL</property>

This, by way of magic can be just "horizontal" that is what we use in other ui-files.

::: src/layersPopover.js
@@ +50,3 @@
+
+        /* Remove file extension and use that in lieu of a fileformat-specific
+

It is to early for me to parse regex (which is also why I wonder if we should use javascript string operations or something else instead here), so I wonder, what exactly will this cut?

A file like layer.json will become layer?
A file like layer.geo.json will become?

@@ +91,3 @@
+
+    _onRemoveClicked: function(row, button) {
+

I would much prefer this being:

this._mapView.view.remove_overlay_source(row.source);
row.source.placeMarkers.forEach(marker) {
    marker.destroy();
});

And avoid the extra method in mapView. Or have a mathing removeGeoJSON method on mapView that does this thing if we go the signal route discussed in bug and IRC.

@@ +131,3 @@
+                let source = this._mapView.loadGeoJSON(file);
+                row = new CustomLayerRow({file: file,
+            fileFilter.add_mime_type(mime);

And I think we usually have spaces before/after { }

@@ +143,3 @@
+        } catch (e) {
+            Utils.debug(e);
+                                                      Gtk.ResponseType.OK);

I think so, just Utils.debug ('something: %s'.format(e.message)) is fine I feel.

@@ +162,3 @@
+    fileTypeSupported: function(file) {
+        let content_type = Gio.content_type_guess(file.get_uri(), null)[0];
+

Keep this private (_isSupported() might be better? the argument tells us it is a file) until some modul outside this needs to use it. Otherwise we might never have an outside caller and it will be public for ever without being public :)

::: src/mapView.js
@@ +195,3 @@
+    removeOverlaySource: function(source) {
+        this.view.remove_overlay_source(source);
+    },

Still not liking this, at least remove it in this patch were it makes no sense, and add it in the patch that you need to do extra. Or go the route described in review above.
Comment 84 Andreas Nilsson 2016-01-08 12:19:52 UTC
(In reply to Hashem Nasarat from comment #78)
> Andreas, here's a screencast of the lastest version.
> 
> We're having difficulty figuring out strings, and I think we'll defer to you
> if you don't mind:
> * Terminology: "Custom Layer" (Jonas likes, I don't) vs "Overlay File" (I
> think is tolerable, jonas doesn't like)

"Custom layer map" gives 38 900 000 hits on google.
"Overlay file map" gives 21 800 000 hits on google.

My mockup had the term "Load map layer". I guess that could be shortened to 
just "Load layer" if needed. Depends a bit on what it would end up as in locales such as german.
"Load map layer" gives 61 500 000 hits on google.

Between "custom layer" and "overlay file", the words are fairly similar.


> * Title in FileChooser

Jonas version: "Load custom layer map"
Hashems version: "Load overlay file"
Mine: "Load map layer"


> * Error messages (more detailed as they are in the video, or less detailed
> like "Failed to load custom layer"

Less detailed and more straight forward is better I think.
"Unable to open file: File type not among supported types"
to
"Unable to load layer: file type is not supported"


"Failed to open file: GeoJSON Error: invalid coordinate"
to
"Unable to load layer: invalid coordinate"

I'll ask for some advice in #gnome-design on the exact terminology.
Comment 85 Andreas Nilsson 2016-01-08 12:23:51 UTC
In the webcast, it seem the new error message overlays the old. I think it would look nicer if the old one went away as soon as the new one arrives, so they don't overlay.
Comment 86 Hashem Nasarat 2016-01-08 20:47:27 UTC
Thanks andreas for taking the time to suggest some wording.

I forgot to mention the wording on the button. Right now it's "Open overlay file". Do you think this should be "Open map layer"? From what I can tell "Open" is more used than "Load" for actions that open a file chooser.

As for the error messages stacking, it's a known bug: https://bugzilla.gnome.org/show_bug.cgi?id=760252
Comment 87 Hashem Nasarat 2016-01-10 06:15:34 UTC
Created attachment 318638 [details] [review]
mainWindow: set correct DND drag status

Sometimes Maps wouldn't accept a drop because we weren't setting the
drag_status appropriately.

Also change drag_finish to denote that the originally-dropped file is
not a semantic move and that the source shouldn't delete the original
data because Maps doesn't really take responsibility for the file -- it
just displays it until the app quits.

https://bugzilla.gnome.org/show_bug.cgi?id=759544

https://bugzilla.gnome.org/show_bug.cgi?id=760256
Comment 88 Hashem Nasarat 2016-01-10 06:16:09 UTC
Created attachment 318639 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.

https://bugzilla.gnome.org/show_bug.cgi?id=759544

https://bugzilla.gnome.org/show_bug.cgi?id=760256
Comment 89 Hashem Nasarat 2016-01-10 06:42:24 UTC
Review of attachment 318430 [details] [review]:

Attached a new version which refactors everything based on suggestions and makes it more modular.

For variable/naming: instead of overlayLayer or customLayer, I believe featureLayer is best. That's what is contained within filetypes like ShapeFile, GeoJSON, KML..... "overlay" is bad because it might get confused with GtkOverlay, and "custom" is bad because it doesn't give much information and the loaded featureLayers aren't customizable from within the app anyway. 

As I suspected, the mapView is most simply implemented without signals. As Damian and Jonas suspected, having mapView in the center of things works out best.

To match the HIG I also removed the frames around the GtkListBoxRows, and instead put one around the outer GtkListBox. I'd like to have separators in between the rows like https://developer.gnome.org/hig/stable/drop-down-lists.html.en but I guess maybe this is a recent adwaita change.  http://i.imgur.com/dOxcdFZ.png

https://en.wikipedia.org/wiki/Shapefile
https://en.wikipedia.org/wiki/Geography_Markup_Language
https://en.wikipedia.org/wiki/GeoJSON

::: data/ui/custom-layer-row.ui
@@ +10,3 @@
+            <property name="name">layers-row-box</property>
+            <property name="visible">True</property>
+            <property name="orientation">GTK_ORIENTATION_HORIZONTAL</property>

OK

::: src/layersPopover.js
@@ +50,3 @@
+
+        /* Remove file extension and use that in lieu of a fileformat-specific
+         * display name. */

This will just strip everything after the last '.'.

In the next patch it strips everything after the first '.'

This isn't really a clearly defined operation. If you like I could make it strip everything after the last '.' and then special case srip '.geo.json' .

@@ +125,3 @@
+        try {
+            if (!this.fileTypeSupported(file))
+                throw new Error(_("File type not among supported types"));

We are, so it should be.

@@ +131,3 @@
+                let source = this._mapView.loadGeoJSON(file);
+                row = new CustomLayerRow({file: file,
+                                              source: source});

OK

@@ +143,3 @@
+        } catch (e) {
+            Utils.debug(e);
+            Utils.debug(e.stack);

I like that idea Damian. Without printing the stack here, the way the error message handling is coded would have meant that every time I make a programming mistake I wouldn't be able to see what was causing it.

@@ +145,3 @@
+            Utils.debug(e.stack);
+            Application.notificationManager.showMessage(
+                _("Failed to open file.")

OK

::: src/mapView.js
@@ +296,3 @@
+    gotoBBox: function(bbox, linear) {
+        if (!bbox.is_valid()) {
+            Utils.debug(_("Bounding box is invalid"));

OK
Comment 90 Hashem Nasarat 2016-01-10 06:45:01 UTC
Created attachment 318641 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.

https://bugzilla.gnome.org/show_bug.cgi?id=759544

https://bugzilla.gnome.org/show_bug.cgi?id=760256
Comment 91 Hashem Nasarat 2016-01-10 06:45:07 UTC
Created attachment 318642 [details] [review]
Utils: If Error is passed to debug() print stack

This helps in development if unexpected errors occur in sections that
do error handling and logging. By having the stack you can observe
the unexpected errors and fix the issue.
Comment 92 Hashem Nasarat 2016-01-10 06:46:22 UTC
Created attachment 318643 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 93 Jonas Danielsson 2016-01-10 10:03:33 UTC
Review of attachment 318280 [details] [review]:

Thanks!
Comment 94 Jonas Danielsson 2016-01-10 10:04:02 UTC
Review of attachment 318281 [details] [review]:

Thanks!
Comment 95 Jonas Danielsson 2016-01-10 10:04:26 UTC
Review of attachment 318638 [details] [review]:

Awesome!
Comment 96 Jonas Danielsson 2016-01-10 10:04:55 UTC
Attachment 318280 [details] pushed as 2b653df - Fix css to show which base map is selected
Attachment 318281 [details] pushed as 01bf31b - Fix typo in this._addContacts
Attachment 318638 [details] pushed as b1c076f - mainWindow: set correct DND drag status
Comment 97 Jonas Danielsson 2016-01-10 10:08:44 UTC
Review of attachment 318642 [details] [review]:

Ok.

I also wonder, should log in both these two instances be print?
Comment 98 Jonas Danielsson 2016-01-10 10:27:19 UTC
Review of attachment 318643 [details] [review]:

Thanks Hashem!

Yes feature is a part of shape files... but so is geometry and meta-data.
What about shape-layer? Is that not more conveying?

Some more comments below!

::: data/ui/layers-popover.ui
@@ +61,3 @@
+            <property name="visible">False</property>
+            <property name="can_focus">False</property>
+            <property name="selection-mode">GTK_SELECTION_NONE</property>

Just have none here

@@ +66,3 @@
+            <property name="left-attach">0</property>
+            <property name="top-attach">2</property>
+          </packing>

This is should not be here, it gives:
(org.gnome.Maps:13764): Gtk-WARNING **: GtkFrame does not have a child property called left-attach

(org.gnome.Maps:13764): Gtk-WARNING **: GtkFrame does not have a child property called top-attach

::: src/featureLayer.js
@@ +24,3 @@
+
+const GeoJSONSource = imports.geoJSONSource;
+

Extra new line

@@ +65,3 @@
+
+
+const GeoJSONFeatureLayer = new Lang.Class({

This should be a separate file: geoJSONShapeLayer.js

@@ +81,3 @@
+            markerLayer: this._markerLayer
+        });
+        this.mapSource.parse();

We do not have constructors that can fail, so move this to a separate function,

@@ +86,3 @@
+GeoJSONFeatureLayer.MIMES = ['application/vnd.geo+json',
+                             'application/json'];
+GeoJSONFeatureLayer.NAME = 'GeoJSON';

Move these to above the class.
Or maybe better, have them as


get mimes() {
   return this._mimes;
}

On the base class, and then set this._mimes in the subclass

::: src/layersPopover.js
@@ +70,3 @@
+
+        this.ui.layersListBox.bind_model(this._mapView.featureLayerStore,
+                                         this._listBoxCreateWidget.bind(this));

So I do not like this, actually.
I do not see tha gain from doing bind_model for the listbox, it feels a bit over engineered. And it makes mapView grow more than I want.

So I want, same as Damián, to use signals. It is far more elegant I feel.
So instead.

this._mapView.connect('add-shape-layer', this._addShapeRow.bind(this));

_addShapeRow(view, layer) {
    let row = ...
},


Then mapView does not need to have a model and we do not need to store the source/layer in multiple places.

@@ +93,3 @@
+        let fileFilter = new Gtk.FileFilter();
+        for (let i in this._mapView.featureLayerTypes) {
+            let Cls = this._mapView.featureLayerTypes[i];

Why involve mapView here?

ShapeLayer.TYPES[i] ?

::: src/mapView.js
@@ +90,3 @@
         this.setMapType(mapType);
 
+        this.featureLayerTypes = [FeatureLayer.GeoJSONFeatureLayer];

No, keep them in ShapeLayer or somewhere or at least keep as a contant somewhere.

@@ +91,3 @@
 
+        this.featureLayerTypes = [FeatureLayer.GeoJSONFeatureLayer];
+        this.featureLayerStore = new Gio.ListStore(GObject.TYPE_OBJECT);

As said, I do not want this Store, it feels redundant, to store stuff on view and here.

@@ +250,3 @@
+                featureLayer = new Cls({ file: file, mapView: this });
+                break;
+            }

This is to much code in mapView, move some of this to shapeLayer.js

You can have static constructing functions like in place.js

let shapeLayer = _findShapeLayer(file);
if (!shapeLayer) {
    let shapeLayer = ShapeLayer.ShapeLayer.newFromFile(file);
    shapeLayer.load();
    this.emit('add-shape-layer', shapeLayer);
}
this.gotoBBox(shapeLayer.source.bbox);


_findShapeLayer: function() {
  this.view.get_overlay_sources().forEach(function(source) {
       if (source instanceof ShapeLayer.ShapeLayer) {
            if (file equals)
               return true;  
       }
   return false;
},



And for 
in openShapeLayer function, maybe?
Comment 99 Jonas Danielsson 2016-01-10 10:28:00 UTC
Comment on attachment 318642 [details] [review]
Utils: If Error is passed to debug() print stack

Attachment 318642 [details] pushed as 47e52ea - Utils: If Error is passed to debug() print stack
Comment 100 Jonas Danielsson 2016-01-10 10:31:23 UTC
(In reply to Hashem Nasarat from comment #89)
> Review of attachment 318430 [details] [review] [review]:
> 
> Attached a new version which refactors everything based on suggestions and
> makes it more modular.
> 
> For variable/naming: instead of overlayLayer or customLayer, I believe
> featureLayer is best. That's what is contained within filetypes like
> ShapeFile, GeoJSON, KML..... "overlay" is bad because it might get confused
> with GtkOverlay, and "custom" is bad because it doesn't give much
> information and the loaded featureLayers aren't customizable from within the
> app anyway. 

I like shapeLayer for prettty much the same reason.

> 
> As I suspected, the mapView is most simply implemented without signals. As
> Damian and Jonas suspected, having mapView in the center of things works out
> best.
> 

I think with signals is the most elegant. Using a store for GtkListBox feels a bit miuch and duplicates storage.

> To match the HIG I also removed the frames around the GtkListBoxRows, and
> instead put one around the outer GtkListBox. I'd like to have separators in
> between the rows like
> https://developer.gnome.org/hig/stable/drop-down-lists.html.en but I guess
> maybe this is a recent adwaita change.  http://i.imgur.com/dOxcdFZ.png

Nice!
No it is not a recent Adwaita change, we have them in favorite popover if you check that code. You need to add separators in code, like:

        this._list.set_header_func(function(row, before) {
            let header = before ? new Gtk.Separator() : null;
            row.set_header(header);
        });

In the constructor if layersPopover maybe,
Comment 101 Jonas Danielsson 2016-01-10 10:41:28 UTC
Review of attachment 318643 [details] [review]:

::: src/mapView.js
@@ +215,3 @@
+            Utils.debug(e);
+            Application.notificationManager.showMessage(
+                _("Failed to load layer: %s").format(e.message)

And do not send this e.message to the users. It can contain messages from JSON.parse functiions about UTf-8 bytesequence.

@@ +250,3 @@
+                featureLayer = new Cls({ file: file, mapView: this });
+                break;
+            }

This is to much code in mapView, move some of this to shapeLayer.js

You can have static constructing functions like in place.js

let shapeLayer = _findShapeLayer(file);
if (!shapeLayer) {
    let shapeLayer = ShapeLayer.ShapeLayer.newFromFile(file);
    shapeLayer.load();
    this.emit('add-shape-layer', shapeLayer);
}
this.gotoBBox(shapeLayer.source.bbox);


_findShapeLayer: function() {
  this.view.get_overlay_sources().forEach(function(source) {
       if (source instanceof ShapeLayer.ShapeLayer) {
            if (file equals)
               return true;  
       }
   return false;
},



And for 
in openShapeLayer function, maybe?
Comment 102 Hashem Nasarat 2016-01-10 22:10:12 UTC
Review of attachment 318643 [details] [review]:

Thanks for pointing out some ways to improve it.

::: data/ui/layers-popover.ui
@@ +61,3 @@
+            <property name="visible">False</property>
+            <property name="can_focus">False</property>
+            <property name="selection-mode">GTK_SELECTION_NONE</property>

OK

@@ +66,3 @@
+            <property name="left-attach">0</property>
+            <property name="top-attach">2</property>
+          </packing>

OK

::: src/featureLayer.js
@@ +65,3 @@
+
+
+const GeoJSONFeatureLayer = new Lang.Class({

haha I originally had it in a different file and then thought you wouldn't like that. Oh well!

@@ +81,3 @@
+            markerLayer: this._markerLayer
+        });
+        this.mapSource.parse();

OK

@@ +86,3 @@
+GeoJSONFeatureLayer.MIMES = ['application/vnd.geo+json',
+                             'application/json'];
+GeoJSONFeatureLayer.NAME = 'GeoJSON';

That won't work as we need these two constants as class variables.

::: src/layersPopover.js
@@ +70,3 @@
+
+        this.ui.layersListBox.bind_model(this._mapView.featureLayerStore,
+                                         this._listBoxCreateWidget.bind(this));

Your previous suggestion was
"To have mapView as "the model" or "mediator" so that actual adding/opening is done by mapView and the UI, or "view" (the layersPopover) will listen to a signal from the mapView to see when the "model" has updated, that is, addad a layer. And the UI will then reflect that." 

This is exactly what this code is doing. The mapView has the model. It does have a signal, it's the model's "items-changed" signal, and the popover listens for that. Implementing this with custom signals, would effectively just be reimplementing GListModel in a less elegant way, and it would take more lines. shapeLayerStore is only used in 6 lines in mapView.

If mapView didn't have the model, and layersPopover was the model and the view, then mapView would emit duplicate shape-layer-added signals, and it would be up to each callback to implement deduplication detection. Also, any interested widget would either need to implement a parallel model to the one contained within the layersPopover or we'd be right back to where we started where the layersPopover was the center of everything.

@@ +93,3 @@
+        let fileFilter = new Gtk.FileFilter();
+        for (let i in this._mapView.featureLayerTypes) {
+            let Cls = this._mapView.featureLayerTypes[i];

Great idea

::: src/mapView.js
@@ +91,3 @@
 
+        this.featureLayerTypes = [FeatureLayer.GeoJSONFeatureLayer];
+        this.featureLayerStore = new Gio.ListStore(GObject.TYPE_OBJECT);

We're only storing the shapeLayers in the ListBoxRows for convenience we could just as easily only keep a reference to the needed instance variables.

@@ +215,3 @@
+            Utils.debug(e);
+            Application.notificationManager.showMessage(
+                _("Failed to load layer: %s").format(e.message)

OK but this is also what prints out "File type is not supported.", or any other error related to the shapeLayers.

Instead I'll change GeoJSONShapeLayer so the message is something that okay to be shown to the user.

@@ +250,3 @@
+                featureLayer = new Cls({ file: file, mapView: this });
+                break;
+            }

I like the class factory method but I think _findShapeLayerIndex and _findExistingShapeLayer can only be where the model is, and I think the model is best suited to being in the mapView because there is not necessarily just a single view that cares about the shapeLayers.
Comment 103 Hashem Nasarat 2016-01-10 22:40:08 UTC
I tried implementing the GtkSeparator as you mentioned, but I get a warning, and the ListBox no longer hides() when empty.

(org.gnome.Maps:21380): Gtk-WARNING **: Negative content height -1 (allocation 1, extents 1x1) while allocating gadget (node border, owner GtkFrame)

Also, it doesn't look like how things look in the HIG https://developer.gnome.org/hig/stable/drop-down-lists.html.en or in other apps 

maps vs builder 3.18.1: http://imgur.com/a/bkRn9
Comment 104 Hashem Nasarat 2016-01-10 22:43:47 UTC
Created attachment 318682 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 105 Damián Nohales 2016-01-10 23:30:20 UTC
Review of attachment 318643 [details] [review]:

::: src/featureLayer.js
@@ +24,3 @@
+
+const GeoJSONSource = imports.geoJSONSource;
+

I know you miss Python, but PEP-8 doesn't apply here! :D :D
Comment 106 Hashem Nasarat 2016-01-11 00:12:22 UTC
Review of attachment 318643 [details] [review]:

::: src/featureLayer.js
@@ +24,3 @@
+
+const GeoJSONSource = imports.geoJSONSource;
+

Haha you found me out! It doesn't look like there's an explicit rule so I appreciate you pointing out conventions https://wiki.gnome.org/Projects/Gjs/StyleGuide

That said, I am using gjslint and jshint over the new code I write.
Comment 107 Damián Nohales 2016-01-11 00:29:41 UTC
Review of attachment 318643 [details] [review]:

::: src/layersPopover.js
@@ +70,3 @@
+
+        this.ui.layersListBox.bind_model(this._mapView.featureLayerStore,
+                                         this._listBoxCreateWidget.bind(this));

I actually like this approach. I suggested signals like add/remove (and presumed to have a way to get the list of shapelayer) just as a way to have a good separation of model and view. I didn't suggested to use GListStore/GListModel/bind_model because I didn't know about those, but it looks as a better mechanism for me.

How this is over engineered? It's a component used just as GLib developers suggests to be used and the code is more straightforward than using signals (we don't need to clear the list manually and we don't have to loop manually over the list).

But... It's impossible to not check how PlaceStore and FavoritesPopover are implemented, which is I think something similar to what Jonas was expecting. We are going to end with a mess of different implementation techniques for similar things, do we want to refactor PlaceStore/FavoritesPopover as well?

Anyway, I feel both options are valid, but I like the GListModel approach, I feel that is more simple and makes better use of GLib.
Comment 108 Jonas Danielsson 2016-01-11 00:33:16 UTC
Review of attachment 318682 [details] [review]:

Thanks!

::: src/geoJSONShapeLayer.js
@@ +31,3 @@
+
+        /* Remove file extension and use that in lieu of a fileformat-specific
+         * display name. */

Multiple line comments should be of the form:

/*
 * ...
 * ...
 */

@@ +53,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

We usually have const like these at top. And have them as const.

::: src/mapView.js
@@ +54,3 @@
 const MapMinZoom = 2;
 
+

Extra new line

@@ +91,3 @@
         this.setMapType(mapType);
 
+        this.shapeLayerTypes = [GeoJSONShapeLayer.GeoJSONShapeLayer];

This seems unused?

@@ +199,3 @@
     },
 
+    openShapeLayer: function(file) {

if (this.shapeLayerStore.iter_n_children(NULL) >= SOME_MAX_CONST) {
    ...
    return;
}

shou

@@ +200,3 @@
 
+    openShapeLayer: function(file) {
+        let ret = false;

No need for this temp?

@@ +207,3 @@
+                if (!shapeLayer)
+                    throw new Error(_("File type is not supported"));
+                shapeLayer.load();

if (this.shapeLayerStore.iter_n_children(NULL) >= SOME_MAX_CONST) {
    ...
    return;
}

should we have some check like this here or in the load function, to avoid long list in UI and to many files. Maybe 5?

@@ +213,1 @@
+            this.gotoBBox(shapeLayer.mapSource.bbox);

Should shapeLayer have a:

get bbox {
    return this._mapSource.bbox;
}

property maybe? Then we might avoid having mapSource public?

@@ +213,2 @@
+            this.gotoBBox(shapeLayer.mapSource.bbox);
+            ret = true;

just return true?

@@ +219,3 @@
+            );
+        }
+        return ret;

return false?

@@ +238,3 @@
+                found = true;
+                break;
+            }

This is just far too much code for doing this, what is wrong with something like:


_findShapeLayer: function(file) {
    this.view.get_overlay_sources().forEach(function(source) {
        if (source.file && source.file.equal(file))
            return source;
    });
    return null;
},

or maybe something like to still user shapeLayer:

_findShapeLayer: function(file) {
    this.shapeLayerStore.forEach(function(model, path, iter) {
     let shape = model.get_value(iter, 0);
     if (shape.file.equal(file)
         return shape;
    });
    return null;
},

but drop "existing" since that is implied.

::: src/shapeLayer.js
@@ +23,3 @@
+const Lang = imports.lang;
+
+const SUBCLASS_TYPES = [];

Maybe SUPPORTED_TYPES?
Comment 109 Jonas Danielsson 2016-01-11 00:37:13 UTC
Review of attachment 318643 [details] [review]:

::: src/layersPopover.js
@@ +70,3 @@
+
+        this.ui.layersListBox.bind_model(this._mapView.featureLayerStore,
+                                         this._listBoxCreateWidget.bind(this));

Thanks for reviewing!

placeStore is a subclass of a ListStore, and that should stay that way because it interacts nicely with gtkentrycompletion and things like that.

And yes I can come around to this. Main main objection is keeping the lines of code in mapView down for this. We do not want to refactor placeStore favoritesPopover because the placeStore _being_ a liststore makes sence, because that is all it is.

My objection with mapView was more that it didn't need the extra explicit model to _be a model_ since it already had the array of overlay_sources.

But I agree with you that it is ok!
Comment 110 Jonas Danielsson 2016-01-11 00:46:29 UTC
(In reply to Hashem Nasarat from comment #103)
> I tried implementing the GtkSeparator as you mentioned, but I get a warning,
> and the ListBox no longer hides() when empty.
> 
> (org.gnome.Maps:21380): Gtk-WARNING **: Negative content height -1
> (allocation 1, extents 1x1) while allocating gadget (node border, owner
> GtkFrame)
> 
> Also, it doesn't look like how things look in the HIG
> https://developer.gnome.org/hig/stable/drop-down-lists.html.en or in other
> apps 
> 
> maps vs builder 3.18.1: http://imgur.com/a/bkRn9

Weird, I thought that was the way one should do, could you check how builder does it then with its listbox? So we can use the same way?
Comment 111 Jonas Danielsson 2016-01-11 00:55:17 UTC
(In reply to Jonas Danielsson from comment #110)
> (In reply to Hashem Nasarat from comment #103)
> > I tried implementing the GtkSeparator as you mentioned, but I get a warning,
> > and the ListBox no longer hides() when empty.
> > 
> > (org.gnome.Maps:21380): Gtk-WARNING **: Negative content height -1
> > (allocation 1, extents 1x1) while allocating gadget (node border, owner
> > GtkFrame)
> > 
> > Also, it doesn't look like how things look in the HIG
> > https://developer.gnome.org/hig/stable/drop-down-lists.html.en or in other
> > apps 
> > 
> > maps vs builder 3.18.1: http://imgur.com/a/bkRn9
> 
> Weird, I thought that was the way one should do, could you check how builder
> does it then with its listbox? So we can use the same way?

Also check later builders, seem 3.18.1 does not use a listbox?
It uses similar technique elsewhere:
https://git.gnome.org/browse/gnome-builder/tree/libide/search/ide-omni-search-group.c#n273
Comment 112 Damián Nohales 2016-01-11 01:44:12 UTC
Review of attachment 318682 [details] [review]:

Wow! the refactor was really fast, thanks!

::: data/ui/shape-layer-row.ui
@@ +11,3 @@
+        <child>
+          <object class="GtkLabel" id="layerLabel">
+            <property name="margin-start">5</property>

I see each row with really short margin and tiny separation between each other (http://i.imgur.com/R06PDXN.png). If you remove margin-start from this label and add margin=5 to the GtkBox looks better.

::: src/geoJSONShapeLayer.js
@@ +32,3 @@
+        /* Remove file extension and use that in lieu of a fileformat-specific
+         * display name. */
+        this.name = this.filename.replace(/\..+$/, '');

What if we do this in the ShapeLayer class?

@@ +52,3 @@
+});
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];

If we add a file format in the future that is JSON based, we are going to have a bad time :D (leave it as is for now... but we need to remember this once we add a new JSON based format)

@@ +53,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

If we do that, how can we do...

for (let i in ShapeLayer.SUBCLASS_TYPES) {
    let Cls = ShapeLayer.SUBCLASS_TYPES[i];
    for (let j in Cls.MIMES)
...

...later?

::: src/layersPopover.js
@@ +73,3 @@
+                                         this._listBoxCreateWidget.bind(this));
+        this.ui.layersListBox.connect('row-activated', (function(lb, row) {
+            this._mapView.gotoBBox(row.shapeLayer.mapSource.bbox);

Should the mapSource be internal of the ShapeLayer class? Maybe we want a better abstraction like shapeLayer.bbox?

@@ +107,3 @@
+
+        if (fileChooser.run() === Gtk.ResponseType.OK)
+            this._mapView.openShapeLayer(fileChooser.get_file());

Coff coff...

fileChooser.get_uris().forEach((function (uri) {
    this._mapView.openShapeLayer(Gio.file_new_for_uri(uri));
}.bind(this))

::: src/mapView.js
@@ +32,3 @@
 const ContactPlace = imports.contactPlace;
 const Geoclue = imports.geoclue;
+const GeoJSONShapeLayer = imports.geoJSONShapeLayer;

Since this.shapeLayerTypes is no longer used, you're not going to need this import.

@@ +207,3 @@
+                if (!shapeLayer)
+                    throw new Error(_("File type is not supported"));
+                shapeLayer.load();

If we do this, we also want to notify the user for this situation, explaining that user needs to remove layers to open new ones. Note that disabling or hiding the add button is not enough (we have DnD and Gio.Application.open) + is confusing.

@@ +238,3 @@
+                found = true;
+                break;
+            }

Getting the index of the shapeLayer object is still useful for the removeShapeLayer method... Weird that GListStore doesn't have a remove_object or remove_item method.

::: src/shapeLayer.js
@@ +30,3 @@
+
+    _init: function(params) {
+        this.parent();

Add a new line here.

@@ +35,3 @@
+
+        this.file = params.file;
+        delete params.file;

Is necessary to clear the params dict if you're chaining the parent constructor?

@@ +59,3 @@
+        this._mapView.view.remove_layer(this._markerLayer);
+        this._mapView.view.remove_overlay_source(this.mapSource);
+

Extra new line.

@@ +64,3 @@
+
+ShapeLayer.newFromFile = function(file, mapView) {
+        let content_type = Gio.content_type_guess(file.get_uri(), null)[0];

contentType

@@ +67,3 @@
+        let shapeLayer = null;
+        for (let i in SUBCLASS_TYPES) {
+            let Cls = SUBCLASS_TYPES[i];

I understand why you capitalized this, but I prefer to use lowercase anyway.

@@ +73,3 @@
+            }
+        }
+        return shapeLayer;

Maybe "return null;" here and "return new cls(...)" up there.

@@ +74,3 @@
+        }
+        return shapeLayer;
+}

Missing semicolon (detected with jshint, are you sure you have it installed?).
Also, the whole function is indented with 8 spaces.
Comment 113 Jonas Danielsson 2016-01-11 06:26:58 UTC
Review of attachment 318682 [details] [review]:

::: src/layersPopover.js
@@ +73,3 @@
+                                         this._listBoxCreateWidget.bind(this));
+        this.ui.layersListBox.connect('row-activated', (function(lb, row) {
+            this._mapView.gotoBBox(row.shapeLayer.mapSource.bbox);

I think so, yes, shapeLayer should have its own bbox property.

::: src/mapView.js
@@ +207,3 @@
+                if (!shapeLayer)
+                    throw new Error(_("File type is not supported"));
+                shapeLayer.load();

Yes, a inApp notification seems fine. I do not see the use-case for having a bunch of these. And if there is one, it will be very very marginal. Most Maps users will never open a single shapeLayer I would think.

@@ +238,3 @@
+                found = true;
+                break;
+    _findShapeLayerIndex: function(file) {

Sorry this should be foreach not forEach (see placeStore.js) the remove does not need to use it either, see below.

@@ +249,1 @@
     },

this.shapeLayerStore.foreach(model, path, iter) {
    let shape = model.get_value(iter, 0);
    if (shape.file.equals(file)
         this.shapeLayerStore.remove(iter);
    shapeLayer.unload();
});
Comment 114 Jonas Danielsson 2016-01-11 06:32:48 UTC
(In reply to Jonas Danielsson from comment #111)
> (In reply to Jonas Danielsson from comment #110)
> > (In reply to Hashem Nasarat from comment #103)
> > > I tried implementing the GtkSeparator as you mentioned, but I get a warning,
> > > and the ListBox no longer hides() when empty.
> > > 
> > > (org.gnome.Maps:21380): Gtk-WARNING **: Negative content height -1
> > > (allocation 1, extents 1x1) while allocating gadget (node border, owner
> > > GtkFrame)
> > > 
> > > Also, it doesn't look like how things look in the HIG
> > > https://developer.gnome.org/hig/stable/drop-down-lists.html.en or in other
> > > apps 
> > > 
> > > maps vs builder 3.18.1: http://imgur.com/a/bkRn9
> > 
> > Weird, I thought that was the way one should do, could you check how builder
> > does it then with its listbox? So we can use the same way?
> 
> Also check later builders, seem 3.18.1 does not use a listbox?
> It uses similar technique elsewhere:
> https://git.gnome.org/browse/gnome-builder/tree/libide/search/ide-omni-
> search-group.c#n273

Btw, does:

        this.ui.layersListBox.set_header_func((function(row, before) {
            let header = null;

            if (before && this.ui.layersListBox.visible)
                header = new Gtk.Separator();

            row.set_header(header);
        }).bind(this));


Work better?
Comment 115 Hashem Nasarat 2016-01-13 15:23:11 UTC
Some things from the meeting:

Small design questions: 
* pixels: Until https://bugzilla.gnome.org/show_bug.cgi?id=760482 is fixed, should I add a 2px padding-bottom to the ListBox?
* multiple files: Should we allow loading multiple files at once from the FileChooserDialog?


Future work/nice to haves:
* Undo notification if the user removes a layer
* loading big files without blocking ui (maybe not possible with gjs?)
* warning dialog if the user tries to open a big file
Comment 116 Andreas Nilsson 2016-01-13 15:58:17 UTC
(In reply to Hashem Nasarat from comment #115)
> Some things from the meeting:
> 
> Small design questions: 
> * pixels: Until https://bugzilla.gnome.org/show_bug.cgi?id=760482 is fixed,
> should I add a 2px padding-bottom to the ListBox?

Only if you leave a big message in the file with the overrides, something like:
*/ HACK - override until bug #760482 is fixed /*

I added override hacks to projects in the past, and then much later forgot about them. :/


> * multiple files: Should we allow loading multiple files at once from the
> FileChooserDialog?

Sure. I don't see any harm in that.

 
> Future work/nice to haves:

> * warning dialog if the user tries to open a big file

Needs design with wording and all. Will take care of that.
Comment 117 Jonas Danielsson 2016-01-14 06:38:50 UTC
Thanks for these notes!

(In reply to Andreas Nilsson from comment #116)
> (In reply to Hashem Nasarat from comment #115)
> > Some things from the meeting:
> > 
> > Small design questions: 
> > * pixels: Until https://bugzilla.gnome.org/show_bug.cgi?id=760482 is fixed,
> > should I add a 2px padding-bottom to the ListBox?
> 
> Only if you leave a big message in the file with the overrides, something
> like:
> */ HACK - override until bug #760482 is fixed /*
>

No, do not add any workaround. We expect this to be fixed within the cycle, right?

> I added override hacks to projects in the past, and then much later forgot
> about them. :/
> 
> 
> > * multiple files: Should we allow loading multiple files at once from the
> > FileChooserDialog?
> 
> Sure. I don't see any harm in that.
> 
>  
> > Future work/nice to haves:
> 
> > * warning dialog if the user tries to open a big file
> 
> Needs design with wording and all. Will take care of that.

Thanks Andreas! Bug for the dialog here: https://bugzilla.gnome.org/show_bug.cgi?id=760611
Comment 118 Hashem Nasarat 2016-01-14 21:49:33 UTC
Review of attachment 318682 [details] [review]:

Thanks, you two! 

New version adds GtkSeparators, fixes hiding the GtkFrame (and listbox) when no shapeLayers are loaded, allows multiple files being opened at once, and reduces the code in mapView.

::: data/ui/shape-layer-row.ui
@@ +11,3 @@
+        <child>
+          <object class="GtkLabel" id="layerLabel">
+            <property name="margin-start">5</property>

That's really weird. Do you have GTK+ installed from master? For me margin = 5 makes it look too large  ( http://i.imgur.com/hmJZWby.png ). I'll change it so margin-start=5 is on the GtkBox (http://i.imgur.com/8G5kDNm.png).

::: src/geoJSONShapeLayer.js
@@ +31,3 @@
+
+        /* Remove file extension and use that in lieu of a fileformat-specific
+         * display name. */

OK

@@ +32,3 @@
+        /* Remove file extension and use that in lieu of a fileformat-specific
+         * display name. */
+        this.name = this.filename.replace(/\..+$/, '');

OK

@@ +53,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

I originally had it so SUBCLASS_TYPES was an array of arrays and I did something like

for (let i in ShapeLayer.SUBCLASS_TYPES) {
  let [name, mimes, Cls] in ShapeLayer.SUBCLASS_TYPES[i];
...
}

but I didn't like it that way (I feel it's less clean because it requires typing out name, mimes, cls too many times when you just one (among other reasons)), so I'd prefer to leave it.

::: src/layersPopover.js
@@ +73,3 @@
+                                         this._listBoxCreateWidget.bind(this));
+        this.ui.layersListBox.connect('row-activated', (function(lb, row) {
+            this._mapView.gotoBBox(row.shapeLayer.mapSource.bbox);

Good ideas

@@ +107,3 @@
+
+        if (fileChooser.run() === Gtk.ResponseType.OK)
+            this._mapView.openShapeLayer(fileChooser.get_file());

I did one better!

::: src/mapView.js
@@ +32,3 @@
 const ContactPlace = imports.contactPlace;
 const Geoclue = imports.geoclue;
+const GeoJSONShapeLayer = imports.geoJSONShapeLayer;

Nice catch

@@ +54,3 @@
 const MapMinZoom = 2;
 
+

OK

@@ +91,3 @@
         this.setMapType(mapType);
 
+        this.shapeLayerTypes = [GeoJSONShapeLayer.GeoJSONShapeLayer];

Yep whoops.

@@ +200,3 @@
 
+    openShapeLayer: function(file) {
+        let ret = false;

OK

@@ +207,3 @@
+                if (!shapeLayer)
+                    throw new Error(_("File type is not supported"));
+                shapeLayer.load();

I don't like having an artificial limit because the app can handle it, but I think moving the the listview into a GtkScrolledWindow would be a nice gnome-love enhancement once this patch lands. For now though I don't think it's a priority. I don't expect that users will have many shapeLayers

@@ +213,1 @@
+            this.gotoBBox(shapeLayer.mapSource.bbox);

Good idea

@@ +213,2 @@
+            this.gotoBBox(shapeLayer.mapSource.bbox);
+            ret = true;

OK

@@ +219,3 @@
+            );
+        }
+        return ret;

OK

@@ +238,3 @@
+                found = true;
+                break;
+            }

foreach or forEach are not what I want in this case because I'm using the 'for' statements to search through the array rather than for side effects. My brain clearly wasn't fully turned on when I wrote this function because I made it way too complicated. I'm not going to remove this function but I will make this code significantly shorter so as to improve code density in mapView

@@ +249,1 @@
     },

This would work but would go through the entire model even after it found something which is unnecessary because of the invariant that the model contains no duplicates. However, since this function is only called by layersPopover, I'll just inline it there because I know you don't want mapView to grow too cumbersome.

::: src/shapeLayer.js
@@ +23,3 @@
+const Lang = imports.lang;
+
+const SUBCLASS_TYPES = [];

OK

@@ +30,3 @@
+
+    _init: function(params) {
+        this.parent();

OK

@@ +35,3 @@
+
+        this.file = params.file;
+        delete params.file;

You're correct

@@ +59,3 @@
+        this._mapView.view.remove_layer(this._markerLayer);
+        this._mapView.view.remove_overlay_source(this.mapSource);
+

OK

@@ +64,3 @@
+
+ShapeLayer.newFromFile = function(file, mapView) {
+        let content_type = Gio.content_type_guess(file.get_uri(), null)[0];

OK

@@ +67,3 @@
+        let shapeLayer = null;
+        for (let i in SUBCLASS_TYPES) {
+            let Cls = SUBCLASS_TYPES[i];

JSHint complains that it's not Capitalized but OK

@@ +73,3 @@
+            }
+        }
+        return shapeLayer;

OK

@@ +74,3 @@
+        }
+        return shapeLayer;
+}

haha yeah I just missed the warning notification. I'm moving this to a module function because it's not really conceptually particular to this abstract base class.
Comment 119 Hashem Nasarat 2016-01-14 22:01:23 UTC
Created attachment 319057 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 120 Jonas Danielsson 2016-01-15 06:44:54 UTC
Review of attachment 319057 [details] [review]:

::: src/geoJSONShapeLayer.js
@@ +49,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

Why couldn't these be:

const Mimes = ...
const NAME = ...

And be on top iof this file, like we do with other constants?
And btw, where is NAME used?

::: src/mapView.js
@@ +147,3 @@
         this.view.add_layer(this._annotationMarkerLayer);
+
+        ShapeLayer.SUPPORTED_TYPES.push(GeoJSONShapeLayer.GeoJSONShapeLayer);

Feel a bit icky about this, maybe add in shapeLayer instead?

@@ +200,3 @@
     },
 
+    openShapeLayers: function(files) {

Not wild about this, I think I would prefer openShapeLayer and iterate over it, but ok.

@@ +202,3 @@
+    openShapeLayers: function(files) {
+        let bbox = new Champlain.BoundingBox();
+        let errors = [];

Remove this array

@@ +231,3 @@
+            msg = gettext.ngettext("Failed to load %s file",
+                                   "Failed to load %s files",
+                                   errors.length).format(errors.length);

Do not include an error message here, just say Failed to load layer.

::: src/shapeLayer.js
@@ +23,3 @@
+const Lang = imports.lang;
+
+const SUPPORTED_TYPES = [];

We can just add these here maybe? And kep this private?

const _IMPLEMENTATIONS = [GeoJSONLayer.GeoJSONLAYER]; ?

@@ +28,3 @@
+    let contentType = Gio.content_type_guess(file.get_uri(), null)[0];
+    for (let i in SUPPORTED_TYPES) {
+        let cls = SUPPORTED_TYPES[i];

What does cls stand for? could it b e layer instead?
Comment 121 Hashem Nasarat 2016-01-15 17:22:20 UTC
Review of attachment 319057 [details] [review]:

Thanks!

::: src/geoJSONShapeLayer.js
@@ +49,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

They could be, but they're more like class constants rather than module constants. E.g. if we were to subclass GeoJSONShapeLayer for some reason, we may want to override the MIMES and the NAME but we couldn't do that within the same file if these were module constants rather than class constants. Furthermore, it would mean where we currently do

SUPPORTED_TYPES.push(GeoJSONShapeLayer.GeoJSONShapeLayer);

and

    for (let i in SUPPORTED_TYPES) {
        let cls = SUPPORTED_TYPES[i];
        if (cls.MIMES.indexOf(contentType) > -1) {
            return new cls({ file: file, mapView: mapView });
        }
    }

we'd instead do 

SUPPORTED_TYPES.push([GeoJSONShapeLayer.NAME,
                      GeoJSONShapeLayer.MIMES,
                      GeoJSONShapeLayer.GeoJSONShapeLayer])

and 

    for (let i in SUPPORTED_TYPES) {
        let [name, mimes, cls] = SUPPORTED_TYPES[i];
        if (cls.MIMES.indexOf(contentType) > -1) {
            return new cls({ file: file, mapView: mapView });
        }
    }

but that doesn't get us anything and makes the code harder to read.

::: src/mapView.js
@@ +147,3 @@
         this.view.add_layer(this._annotationMarkerLayer);
+
+        ShapeLayer.SUPPORTED_TYPES.push(GeoJSONShapeLayer.GeoJSONShapeLayer);

It doesn't work in shapeLayer because of circular imports between shapeLayer and GeoJSONShapeLayer. When I do it this way, this.parent() fails in GeoJSONShapeLayer's _init()

@@ +200,3 @@
     },
 
+    openShapeLayers: function(files) {

All the places we call this method we would need to use the plural form of it so making a function that would only take one would just increase the number of methods in mapView. Additionally it would make composing the bboxes take more lines of code.

@@ +202,3 @@
+    openShapeLayers: function(files) {
+        let bbox = new Champlain.BoundingBox();
+        let errors = [];

But i need it to figure out if all the layers loaded properly, and to print error messages if some didn't

@@ +231,3 @@
+            msg = gettext.ngettext("Failed to load %s file",
+                                   "Failed to load %s files",
+                                   errors.length).format(errors.length);

OK. I really like telling the user how many layers failed to load if they tried to load multiple ones though.

::: src/shapeLayer.js
@@ +23,3 @@
+const Lang = imports.lang;
+
+const SUPPORTED_TYPES = [];

we'd have circular imports and it makes the code break in odd ways. If it was private this module would no longer follow the https://en.wikipedia.org/wiki/Open/closed_principle .

@@ +28,3 @@
+    let contentType = Gio.content_type_guess(file.get_uri(), null)[0];
+    for (let i in SUPPORTED_TYPES) {
+        let cls = SUPPORTED_TYPES[i];

cls stands for class, it could be layerCls, but it's not a ShapeLayer, it's a ShapeLayer class.
Comment 122 Hashem Nasarat 2016-01-15 17:22:33 UTC
Created attachment 319130 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 123 Hashem Nasarat 2016-01-15 17:23:29 UTC
Review of attachment 319057 [details] [review]:

::: src/geoJSONShapeLayer.js
@@ +49,3 @@
+GeoJSONShapeLayer.MIMES = ['application/vnd.geo+json',
+                           'application/json'];
+GeoJSONShapeLayer.NAME = 'GeoJSON';

Also, NAME is used in the GtkFileChooser to give a name to the set of mime types added to the GtkFileFilter
Comment 124 Jonas Danielsson 2016-01-15 20:02:54 UTC
Review of attachment 319130 [details] [review]:

Thanks!

::: src/geoJSONShapeLayer.js
@@ +26,3 @@
+    Name: 'GeoJSONShapeLayer',
+    Extends: ShapeLayer.ShapeLayer,
+

I am no javascript expert, but can't the mimes and stuff be here?

Name: 'GeoJSON,
Extends: ...,
Mimes: [ 'application/vnd.geo+json', 'application/json' ],

?

::: src/layersPopover.js
@@ +102,3 @@
+
+        let fileFilter = new Gtk.FileFilter();
+        for (let i in ShapeLayer.SUPPORTED_TYPES) {

forEach since it really is for each

@@ +104,3 @@
+        for (let i in ShapeLayer.SUPPORTED_TYPES) {
+            let layerCls = ShapeLayer.SUPPORTED_TYPES[i];
+            for (let j in layerCls.MIMES)

same here

@@ +112,3 @@
+        let openButton = fileChooser.add_button('_Open',
+                                                Gtk.ResponseType.OK);
+        openButton.get_style_context().add_class('suggested-action');

This can't be done in ui-file? for fileChooser?

@@ +126,3 @@
+        row.closeButton.connect('clicked',
+                                this._onRemoveClicked.bind(this, row));
+        row.show();

why row.show? set visible: true when creating shapelayerrow instead. Or have visible TRUE in the ui-file

::: src/mapView.js
@@ +227,3 @@
+        let msg;
+        if (files.length === 1)
+            msg = _("Failed to load layer").format(errors[0].message);

format not needed

@@ +231,3 @@
+            msg = gettext.ngettext("Failed to load %s layer",
+                                   "Failed to load %s layers",
+                                   errors.length).format(errors.length);

What exactly does this do?

@@ +239,3 @@
+        for (let i = 0; i < this.shapeLayerStore.get_n_items(); i++)
+            if (this.shapeLayerStore.get_item(i).file.equal(file))
+				return i;

Indent problems?

::: src/shapeLayer.js
@@ +28,3 @@
+    let contentType = Gio.content_type_guess(file.get_uri(), null)[0];
+    for (let i in SUPPORTED_TYPES) {
+        let layerCls = SUPPORTED_TYPES[i];

it is really not obvious to me what layerCls is support to mean
Comment 125 Jonas Danielsson 2016-01-15 20:31:05 UTC
Review of attachment 319130 [details] [review]:

::: src/geoJSONShapeLayer.js
@@ +26,3 @@
+    Name: 'GeoJSONShapeLayer',
+    Extends: ShapeLayer.ShapeLayer,
+

Or maybe

DisplayName: 'GeoJSON',
Comment 126 Hashem Nasarat 2016-01-17 06:12:52 UTC
Review of attachment 319130 [details] [review]:

Thanks! Getting close!

::: src/geoJSONShapeLayer.js
@@ +26,3 @@
+    Name: 'GeoJSONShapeLayer',
+    Extends: ShapeLayer.ShapeLayer,
+

Wow yes, I can't believe I didn't think to do this. Nice!

* tries it *

Well... of course it doesn't work in GJS. DisplayName becomes a property of a ShapeLayer instance rather than the class. I spent too long looking into why this is in GJS and the object oriented paradigm is based off of mootools.js which doesn't have a supported way to do static/class methods other than doing exactly what I have below. Unfortunately when you do things as they are below they're not inherited in subclasses. Still, for convenience — as we're using them — defining class properties like we have below isn't bad.

::: src/layersPopover.js
@@ +102,3 @@
+
+        let fileFilter = new Gtk.FileFilter();
+        for (let i in ShapeLayer.SUPPORTED_TYPES) {

OK

@@ +104,3 @@
+        for (let i in ShapeLayer.SUPPORTED_TYPES) {
+            let layerCls = ShapeLayer.SUPPORTED_TYPES[i];
+            for (let j in layerCls.MIMES)

OK

@@ +112,3 @@
+        let openButton = fileChooser.add_button('_Open',
+                                                Gtk.ResponseType.OK);
+        openButton.get_style_context().add_class('suggested-action');

Good idea

@@ +126,3 @@
+        row.closeButton.connect('clicked',
+                                this._onRemoveClicked.bind(this, row));
+        row.show();

OK

::: src/mapView.js
@@ +227,3 @@
+        let msg;
+        if (files.length === 1)
+            msg = _("Failed to load layer").format(errors[0].message);

Whoops.

@@ +231,3 @@
+            msg = gettext.ngettext("Failed to load %s layer",
+                                   "Failed to load %s layers",
+                                   errors.length).format(errors.length);

If you try to open up e.g. 4 layers at once, it will tell either if:

* a layer (one)
* layers (multiple)

failed to load based on the number of errors.

@@ +239,3 @@
+        for (let i = 0; i < this.shapeLayerStore.get_n_items(); i++)
+            if (this.shapeLayerStore.get_item(i).file.equal(file))
+				return i;

For some reason tabs got inserted. Whoops.

::: src/shapeLayer.js
@@ +28,3 @@
+    let contentType = Gio.content_type_guess(file.get_uri(), null)[0];
+    for (let i in SUPPORTED_TYPES) {
+        let layerCls = SUPPORTED_TYPES[i];

I'll change it to layerClass. It's intended to convey that the variable layerClass is a subclass of ShapeLayer.
Comment 127 Hashem Nasarat 2016-01-17 06:18:28 UTC
Created attachment 319203 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 128 Damián Nohales 2016-01-17 21:56:06 UTC
Review of attachment 319203 [details] [review]:

Thanks!

::: src/layersPopover.js
@@ +105,3 @@
+        row.shapeLayer.unload();
+        let index = this._mapView.findShapeLayerIndex(row.shapeLayer.file);
+        this._mapView.shapeLayerStore.remove(index);

I'm sorry, I really feel we are coming and going here. Why we removed the removeShapeLayer method? I agree we don't need a huge MapView class but I feel we are going through the internals of shapeFile unloading/removal, just for 5 lines of code in MapView --that I feel belongs there.

We could a more specialized version of GListStore within Maps that implements remove_object and exists to make the MapView code simpler (we could propose those methods to GLib afterwards). That's just an idea, but it doesn't feel right to have this code here.

::: src/mapView.js
@@ +227,3 @@
+        let msg;
+        if (files.length === 1)
+            msg = _("Failed to load layer");

You don't need this, you can do it just with ngettext.

::: src/shapeLayer.js
@@ +57,3 @@
+         * display name.
+         */
+        this.name = this.filename.replace(/\..+$/, '');

You should not match dots as part of the extension:

"a.file.name.with.dots.json".replace(/\..+$/, '')
a

"a.file.name.with.dots.json".replace(/\.[^.]+$/, '')
a.file.name.with.dots
Comment 129 Hashem Nasarat 2016-01-17 23:19:55 UTC
Review of attachment 319203 [details] [review]:

Thanks! I think we have a lot of back and forth because I feel the need to guess at what you will like because I'm not sure exactly what you're looking for (e.g.
* "A file like layer.json will become layer? A file like layer.geo.json will become?"
* "This is just far too much code for doing this", and comments in IRC about wanting to keep minimal new additions to mapView with maximum code density, and then you recommend I remove findShapeLayerIndex() which is used by removeShapeLayer. Based on that I assumed you didn't want small helper functions and I assumed you believed removeShapeLayer was an extraneous low-density helper function). 
Also, it's a bit difficult because I can't just guess based on what I like because it seems I like some things that you dislike (e.g. error messages). IDK how this workflow could be improved.

::: src/layersPopover.js
@@ +105,3 @@
+        row.shapeLayer.unload();
+        let index = this._mapView.findShapeLayerIndex(row.shapeLayer.file);
+        this._mapView.shapeLayerStore.remove(index);

OK I'll move the code back to a method in mapView.

::: src/mapView.js
@@ +227,3 @@
+        let msg;
+        if (files.length === 1)
+            msg = _("Failed to load layer");

I believe "failed to load layer" and "failed to load 1 layer" are slightly different in what they convey, but OK I'll just use ngettext.

::: src/shapeLayer.js
@@ +57,3 @@
+         * display name.
+         */
+        this.name = this.filename.replace(/\..+$/, '');

OK I'll change it back to only match the last dot and add a special case to remove '.geo.json'.
Comment 130 Hashem Nasarat 2016-01-17 23:21:18 UTC
Created attachment 319233 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 131 Damián Nohales 2016-01-18 00:08:31 UTC
Review of attachment 319203 [details] [review]:

I wasn't against the helper method nor error messages, that was Jonas, I'm Damián :)

::: src/mapView.js
@@ +227,3 @@
+        let msg;
+        if (files.length === 1)
+            msg = _("Failed to load layer");

Oh, you're right, sorry, I misread "if (errors.length === 1)"
Comment 132 Hashem Nasarat 2016-01-18 00:17:41 UTC
Created attachment 319236 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 133 Damián Nohales 2016-01-18 00:35:47 UTC
Review of attachment 319236 [details] [review]:

Thanks! besides these nits, I think this looks good to me.

::: src/mapView.js
@@ +147,3 @@
         this.view.add_layer(this._annotationMarkerLayer);
+
+        ShapeLayer.SUPPORTED_TYPES.push(GeoJSONShapeLayer.GeoJSONShapeLayer);

Moving this to the bottom of geoJSONShapeLayer.js doesn't cause any circular import problems.

@@ +241,3 @@
+                return i;
+        return -1;
+    },

Can we move this method below removeShapeLayer so we have the open/remove methods together =)
Comment 134 Damián Nohales 2016-01-18 00:40:51 UTC
Just some IRC thoughts that came from the .geo.json file extension handling:

---
8:55 PM <hashem> we remove .geo.json for the case you had two files named the same with different file extensions
8:56 PM <dnohales> I didn't get that, what happen in that case?
8:56 PM <hashem> for example, if you have foo.geojson and foo.kml at least you'd be able to see in the popover that they are different
8:57 PM <hashem> Jonas told me to remove the file extension, but I decided to put the full file name in the popup text
8:57 PM <hashem> in the tooltip, I mean
8:57 PM <dnohales> yeah, we need to still take an extra look, we could actually have two files named exactly the same but in different directories
8:58 PM <hashem> oh yeah
8:58 PM <hashem> the code works for it, but I guess you wouldn't be able to tell
8:58 PM <dnohales> yeah... but that's another story
8:59 PM <dnohales> we could have a number between parenthesis in the label and show the full path in the tooltip
9:00 PM <dnohales> or (in <directory>)
9:00 PM <hashem> or just like Gedit's Open popover
9:01 PM <dnohales> ah, right
9:01 PM <dnohales> anyway, I would wait for Jonas or Andreas confirmation before coding
9:02 PM <hashem> good idea
---

Andreas, Jonas, thoughts?
Comment 135 Jonas Danielsson 2016-01-18 06:44:55 UTC
(In reply to Damián Nohales from comment #134)
> Just some IRC thoughts that came from the .geo.json file extension handling:
> 
> ---
> 8:55 PM <hashem> we remove .geo.json for the case you had two files named
> the same with different file extensions
> 8:56 PM <dnohales> I didn't get that, what happen in that case?
> 8:56 PM <hashem> for example, if you have foo.geojson and foo.kml at least
> you'd be able to see in the popover that they are different
> 8:57 PM <hashem> Jonas told me to remove the file extension, but I decided
> to put the full file name in the popup text
> 8:57 PM <hashem> in the tooltip, I mean
> 8:57 PM <dnohales> yeah, we need to still take an extra look, we could
> actually have two files named exactly the same but in different directories
> 8:58 PM <hashem> oh yeah
> 8:58 PM <hashem> the code works for it, but I guess you wouldn't be able to
> tell
> 8:58 PM <dnohales> yeah... but that's another story
> 8:59 PM <dnohales> we could have a number between parenthesis in the label
> and show the full path in the tooltip
> 9:00 PM <dnohales> or (in <directory>)
> 9:00 PM <hashem> or just like Gedit's Open popover
> 9:01 PM <dnohales> ah, right
> 9:01 PM <dnohales> anyway, I would wait for Jonas or Andreas confirmation
> before coding
> 9:02 PM <hashem> good idea
> ---
> 
> Andreas, Jonas, thoughts?

Just having a (1) after the name seems fine by me.
Comment 136 Jonas Danielsson 2016-01-18 06:52:05 UTC
Review of attachment 319236 [details] [review]:

Thanks!

This looks great!

::: src/layersPopover.js
@@ +18,3 @@
  */
 
+const Application = imports.application;

What is this import used for?

@@ +19,3 @@
 
+const Application = imports.application;
+const Gio = imports.gi.Gio;

Or this?

::: src/mapView.js
@@ +217,3 @@
+            } catch (e) {
+                Utils.debug(e);
+                    layer = ShapeLayer.newFromFile(file, this);

My preference is still just show a notification here, 'Failed to open layer' and do not have the code below.

@@ +230,3 @@
+        else
+            msg = gettext.ngettext("Failed to load %s layer",
+                                   "Failed to load %s layers",

load => open
Comment 137 Andreas Nilsson 2016-01-18 12:07:53 UTC
(In reply to Jonas Danielsson from comment #135)
> (In reply to Damián Nohales from comment #134)

> > 8:59 PM <dnohales> we could have a number between parenthesis in the label
> > and show the full path in the tooltip
> > 9:00 PM <dnohales> or (in <directory>)
> > 9:00 PM <hashem> or just like Gedit's Open popover
> > 9:01 PM <dnohales> ah, right
> > 9:01 PM <dnohales> anyway, I would wait for Jonas or Andreas confirmation
> > before coding
> > 9:02 PM <hashem> good idea
> > ---
> > 
> > Andreas, Jonas, thoughts?
> 
> Just having a (1) after the name seems fine by me.

Yeah, this sounds like a sane solution.
Comment 138 Hashem Nasarat 2016-01-18 17:05:27 UTC
Review of attachment 319236 [details] [review]:

Thanks! I tried doing what gedit did but our popover isn't as wide as theirs so printing the full path isn't really feasible with the space we have (http://i.imgur.com/KQcS0qG.png)

Instead, I think a good solution is just showing the full path in the tooltip http://i.imgur.com/pIPy4iW.png Thoughts?

Adding the string "(1)" would be slightly cumbersome coding-wise, and might confuse the user if they opened files that also ended in "(1)" unless we printed our "(1)" with custom styling and it all seems like a big headache for an unlikely use-case. For that reason I like the tooltip as it is unambiguous and easy-to-code.

::: src/layersPopover.js
@@ +19,3 @@
 
+const Application = imports.application;
+const Gio = imports.gi.Gio;

I cleaned up these imports.

::: src/mapView.js
@@ +217,3 @@
+            } catch (e) {
+                Utils.debug(e);
+                errors.push(e);

I don't think this is best because then if multiple files fail to load we'll stack notifications.

@@ +230,3 @@
+        else
+            msg = gettext.ngettext("Failed to load %s layer",
+                                   "Failed to load %s layers",

OK

@@ +241,3 @@
+                return i;
+        return -1;
+    },

yes
Comment 139 Hashem Nasarat 2016-01-18 17:06:15 UTC
Created attachment 319280 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 140 Jonas Danielsson 2016-01-18 17:23:26 UTC
Review of attachment 319280 [details] [review]:

::: src/geoJSONShapeLayer.js
@@ +44,3 @@
+        else
+            return this.parent();
+    },

Either this becomes a property, get name() { } or we should call it getName()

::: src/mapView.js
@@ +217,3 @@
+            } catch (e) {
+                Utils.debug(e);
+                    layer = ShapeLayer.newFromFile(file, this);

I prefer stacking error messages I think, then you will also see how many failed depending on the how many you have to click [x] on. It is not like we expect people to at random open a bunch of layers and some might fail.

It is redundant information to also write how many failed, you will notice that.
Comment 141 Hashem Nasarat 2016-01-18 17:30:29 UTC
Review of attachment 319280 [details] [review]:

Thanks!

::: src/geoJSONShapeLayer.js
@@ +44,3 @@
+        else
+            return this.parent();
+    },

I tried to make it a property, but this.parent() doesn't work with gjs in overridden properties :(
I'll change it to getName.

::: src/mapView.js
@@ +217,3 @@
+            } catch (e) {
+                Utils.debug(e);
+                errors.push(e);

OK
Comment 142 Hashem Nasarat 2016-01-18 17:36:12 UTC
Created attachment 319283 [details] [review]
GeoJSON/Layers: Formalize and add GUI

Maps supports displaying GeoJSON files as overlays on top of the map via
drag and drop or by specifying the file on the command line.

This commit adds the capability to select and load these files via the
UI via a button in the Layers Popover; furthermore, the Layers Popover
displays which overlays are loaded and allows the user to remove them at
will.
Comment 143 Jonas Danielsson 2016-01-18 17:41:04 UTC
Review of attachment 319283 [details] [review]:

Thanks a lot Hashem!

Great work, and I really like how this feels in the UI! Let's get it in and see if people can break it for us!
Comment 144 Jonas Danielsson 2016-01-18 17:42:01 UTC
Attachment 319283 [details] pushed as 37e8ac4 - GeoJSON/Layers: Formalize and add GUI
Comment 145 Jonas Danielsson 2016-01-18 17:53:51 UTC
Could you file a bug about hiding/showing a layer? Should we use the eye?
Comment 146 Andreas Nilsson 2016-01-22 09:32:58 UTC
Hashem opened a bug about this, but I honestly lost the bug number :(