Created attachment 273249 [details] [review]
placeStore: add osm-id as unique identifier

The name of a place might change in OpenStreetMap or through code
changes in geocode-glib. It is safer to use the osm-id to assert the
uniqueness of a place.

Created attachment 273250 [details] [review]
placeStore: Change updateAddTime to updatePlace

Since the name and location of place could change, make sure
we update the place on re-visit and just not the add time.

The first patch adds the osm-id as a unique identifier to the place store. On load it will check if osm_id is present and if not it will not add that place.

This means that if this change lands all the users recent places will disappear,
I think that is ok. And that this change should go in before we add UI to add favorites. Since losing favorites is harsher.

As I've mentioned in the list [1], I think that all fields in a GeocodePlace are useful, mainly for create rich UIs to represent a place [2].

I think that Geocode is sadly playing against us :( , as I can see, a GeocodePlace object is starting to be not useful for us, it doesn't expose a lot of information from the OSM APIs (Nominatim in this case) that can be useful for us.

If you ask me, for me the best solution is to store just the raw data that comes from the OSM API with an "updated" field, so we can refresh the stored place information when it comes old. The thing is, to do that we need to change geocode-glib to add the possibility to get the raw data of the place and to instantiate GeocodePlace's from that raw data, or at least implement another temporal solution. I think that solution allows to improve geocode-glib without breaking the place store.

To make the transition we can advantage that the current filename for the place store is a bit ugly :). Maybe rename it from "$XDG_DATA_HOME/maps-places.json" to "$XDG_DATA_HOME/gnome-maps/places.json", so we can detect if the user still has the old place store format.

Sadly, if we want to migrate the PlaceStore format, we'll need to clear the current data and possibly add a store version number in the file that helps us to deal with future migrations without renaming. Since we don't have favorites feature right now, we can destroy the place store without cause much hassle.

 [1] https://mail.gnome.org/archives/maps-list/2014-June/msg00002.html
 [2] https://bugzilla.gnome.org/show_bug.cgi?id=722871

(In reply to comment #4)
> As I've mentioned in the list [1], I think that all fields in a GeocodePlace
> are useful, mainly for create rich UIs to represent a place [2].
> 
> I think that Geocode is sadly playing against us :( , as I can see, a
> GeocodePlace object is starting to be not useful for us, it doesn't expose a
> lot of information from the OSM APIs (Nominatim in this case) that can be
> useful for us.

And why do you think that can't be fixed? :) The only reason I see to make geocode-glib to give you raw data would be for us to be very lazy and not want to write a lot of C code. :) We really want as much code in the libraries as possible rather than Apps (especially if they are written in dynamically-typed languages).

(In reply to comment #5)
> (In reply to comment #4)
> > As I've mentioned in the list [1], I think that all fields in a GeocodePlace
> > are useful, mainly for create rich UIs to represent a place [2].
> > 
> > I think that Geocode is sadly playing against us :( , as I can see, a
> > GeocodePlace object is starting to be not useful for us, it doesn't expose a
> > lot of information from the OSM APIs (Nominatim in this case) that can be
> > useful for us.
> 
> And why do you think that can't be fixed? :) The only reason I see to make
> geocode-glib to give you raw data would be for us to be very lazy and not want
> to write a lot of C code. :) We really want as much code in the libraries as
> possible rather than Apps (especially if they are written in dynamically-typed
> languages).

I'm saying (in the next paragraph of the replied one) that if geocode-glib provides raw data, you can use it to serialize a place in the store, and, if geocode-glib add a new feature to GeocodePlace, then we can take advantage of it while we avoid recreating the store.

In the best case, geocode-glib should be feature-rich enough so the app doesn't need to use the raw data for anything else than serializing a GeocodePlace in a forward compatible way.

BTW, If we write geocode-glib in Vala we can avoid the laziness ;)

So, how do we do this?

We need a format for the store that allows us to add the information we need.
But it's hard to know what that is.

Everything that is in geocode-glib (which comes from Nominatim), I guess:

name,
osm-id,
location,
county,
street,
building,
area,
postal_code,
town,
country

+ any fields that come from Contacts (libfolks) ?

Maybe we should say that that is enough. We match what we get from search. Since the placeStore is for completion in search. And for routing. So we serialize a GeocodePlace object in some way. There is a lib for that, right?
https://developer.gnome.org/json-glib/stable/json-glib-GObject-Serialization.html

So maybe a Place.js that inherits from GeocodePlace and implements: https://developer.gnome.org/json-glib/stable/json-glib-Serializable-Interface.html#JsonSerializable

I think we are going to need our own Place.js pretty soon. It can probably inherit from GeocodePlace, but should add some stuff and we probably want to subclass it for a Contact. So that we can get an avatar for icon etc.

(In reply to comment #7)
> So, how do we do this?
> 
> We need a format for the store that allows us to add the information we need.
> But it's hard to know what that is.
> 
> Everything that is in geocode-glib (which comes from Nominatim), I guess:
> 
> name,
> osm-id,
> location,
> county,
> street,
> building,
> area,
> postal_code,
> town,
> country
> 
> + any fields that come from Contacts (libfolks) ?
> 
> Maybe we should say that that is enough. We match what we get from search.
> Since the placeStore is for completion in search. And for routing.

Well, except it isn't, I guess. If we add favourites. Unless we only allow favourites on contacts and searchresult? Not poi, since poi contains a bunch more information. Or we make sure that information goes in our Place.js as well and serialize it along with the rest.

Created attachment 287653 [details] [review]
placeStore: Store all relevant place data

Created attachment 287654 [details] [review]
searchResultBubble: Display more data for places

So all these four patches, and then maybe the placeStore is a bit of a better place?

If we add contacts later we would probably have to create a subclass that inherits GeocodePlace for them.

Created attachment 287657 [details] [review]
placeStore: Store all relevant place data

Created attachment 287658 [details] [review]
searchResultBubble: Display more data for places

Created attachment 288340 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

---

So, this is the class I was talking about, sorry for the delay.

Created attachment 288341 [details] [review]
searchResultBubble: Use PlaceFormatter class

This patch should obsoletes "searchResultBubble: Display more
data for places" if we go for the PlaceFormatter approach.

Note that the PlaceFormatter class could help to improve the UI for the search popup and favourites.

Review of attachment 288340 [details] [review]:

Thanks! I have some comments below.

Also I think we should think a bit on what we really want to add. County/area/state/town are tricky. Sometimes it will be the same for multiple of them. Maybe that is relevant information, maybe we want to filter it out. Maybe there is a smart way to do it. Maybe we never want to show county? Maybe just area/state, I am not sure at all.

And I think maybe this should be placeAddressFormatter? Or something towards that effect? I'm thinking later we might have other stuff in a place object as well. Such as wiki link and stuff.

::: src/placeFormatter.js
@@ +34,3 @@
+    set place(place) {
+        this._place = place;
+        this._update();

Setters should not do complex stuff. The update function is complex. Is this setter even needed? Can't we just call the update in the _init function? And just have a this._place around?
Then you don't need to send a local place around to functions as well.

@@ +43,3 @@
+    get titleProperty() {
+        return this._titleProperty;
+    },

Does this need to be a getter? Can we just have a private _titleProperty?

@@ +46,3 @@
+
+    get title() {
+        return this.getProperty(this.titleProperty);

So this just becomes return this._place[this._titleProperty] ?

@@ +51,3 @@
+    get infoPropertyGroups() {
+        return this._infoPropertyGroups;
+    },

get properties?

@@ +55,3 @@
+    getProperty: function(prop) {
+        return this.place[prop];
+    },

This function is not needed, just check against place directly where you want to do this.

@@ +79,3 @@
+
+        case Geocode.PlaceType.TOWN:
+        case Geocode.PlaceType.LOCAL_ADMINISTRATIVE_AREA:

This (LOCAL_ADMINISTRATIVE_AREA) is never set by geocode-glib.

@@ +97,3 @@
+            this._addInfoGroup(info, place, ['state', 'country']);
+            break;
+

GeocodeGlib does not have all that many placetypes, and some of which they have do not get enough of the different tags out there in OSM. So _a lot_ is going to be GeocodePlaceType.UNKNOWN. We need to try our best to gather information about that case.

So I do not like this way of structuring it. Sure we can add more Geocode place types here, but it is not going to matter much. We should try to get collect properties for the unknown case as well.
We should have default case that sets the title to name, (or maybe something hacky like the first item in a split(',') on name. And tries to add some more properties as well.

@@ +112,3 @@
+    },
+
+    _addInfoGroup: function(info, place, properties) {

What does the Group part of the name siginify? Can't it just be _addInfo? or _addProperty? And keep a this._properties around?

Any opinions on whether the placeStore patches could go in soon. Or if we need to rethink them more? At least with this patch we have a reader that tries to read all the properties it finds in the json.

Review of attachment 273249 [details] [review]:

I think this can go in.

Review of attachment 273250 [details] [review]:

I think this can go in too.

Review of attachment 273249 [details] [review]:

Ah, remember to capitalize the word "add" in the first line of the commit message.

Review of attachment 287657 [details] [review]:

Let's wait for this patch to commit all the committable part of the patch-set.

::: src/placeStore.js
@@ +98,3 @@
+             country_code: place.contry_code,
+             continent: place.continent,
+           }

Alignment and trailing comma here.

@@ +188,3 @@
             let jsonArray = JSON.parse(buffer);
             jsonArray.forEach((function(obj) {
+                if (!obj.place.id)

This can break the app when loading old databases format since we don't have the "place" key, so maybe this needs to be written as "if (!obj.place || !obj.place.id)".

BTW, in one of my comments on this bug, I talked about a version number for the place store, is that feasible or helpful?

Something like:
{ version: 2, places: [] }

Then we can detect what to do when we break the database again.

Also, since the old database is not useful anymore with this change (no places have "place.id" key) we could take advantage of it and rename the place store to a nicer name like "$XDG_DATA_HOME/gnome-maps/places.json" (if we do that, there is no need to change the "if" anymore)

Review of attachment 287657 [details] [review]:

::: src/placeStore.js
@@ +98,3 @@
+             country_code: place.contry_code,
+             continent: place.continent,
+           }

Yes, will fix.

@@ +188,3 @@
             let jsonArray = JSON.parse(buffer);
             jsonArray.forEach((function(obj) {
+                if (!obj.place.id)

Well, the breaking is intentional. It will trigger the warning in application.js:
    placeStore = new PlaceStore.PlaceStore();
        try {
            placeStore.load();
        } catch (e) {
            log('Failed to parse Maps places file, ' +
                'subsequent writes will overwrite the file!');
        }

I am not super convinced that we want versioning. I am just not sure how we would handle it, how many old versioning formats should we then carry. Might be better to try to not break the format and if we have to then we lose the data. But I might be wrong. We could at least add a version to the json file as a header-stuff. Want to do a patch?

We could change the file I guess. Hmm. We will not get that warning tho with failing to parse. Just silent and the data is not there.

Created attachment 290455 [details] [review]
placeStore: Add osm-id as unique identifier

The name of a place might change in OpenStreetMap or through code
changes in geocode-glib. It is safer to use the osm-id to assert the
uniqueness of a place.

Created attachment 290456 [details] [review]
placeStore: Change updateAddTime to updatePlace

Since the name and location of place could change, make sure
we update the place on re-visit and just not the add time.

Created attachment 290457 [details] [review]
placeStore: Store all relevant place data

Created attachment 290458 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 290459 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 290460 [details] [review]
Add placeInfo module

Created attachment 290461 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

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

Created attachment 290462 [details] [review]
placeStore: Use new Place module

Created attachment 290463 [details] [review]
placeStore: Make exists function public

Created attachment 290464 [details] [review]
placeStore: Add get function

Created attachment 290465 [details] [review]
Use Overpass to get extra info for search bubble

Review of attachment 290460 [details] [review]:

Shoud be: Add place module.

Created attachment 290478 [details]
Cast of searching for Malmö

Created attachment 290479 [details]
Cast of searching for Glassfabriken

Review of attachment 290457 [details] [review]:

I think we could push from "placeStore: Add osm-id as unique identifier" to here to streamline this.

::: src/placeStore.js
@@ +95,3 @@
+             country_code: place.contry_code,
+             continent: place.continent,
+           }

The alignment problem is still here (also, the semicolon is missed)

@@ +185,3 @@
             let jsonArray = JSON.parse(buffer);
             jsonArray.forEach((function(obj) {
+                if (!obj.place.id)

At least we need a comment here explaining that we expect exceptions in this line due DB BC break.

@@ +191,1 @@
                 this._addPlace(place, obj.type, obj.added);

Now I realize, _addPlace is calling _store, this can be performance killer or end in database corruption. We must report a bug or prepend something to this patch-set.

Review of attachment 290455 [details] [review]:

ACK

Review of attachment 290456 [details] [review]:

ACK

Created attachment 290547 [details] [review]
placeStore: Don't call _store during database loading

Created attachment 290548 [details] [review]
placeStore: Add osm-id as unique identifier

The name of a place might change in OpenStreetMap or through code
changes in geocode-glib. It is safer to use the osm-id to assert the
uniqueness of a place.

Created attachment 290549 [details] [review]
placeStore: Change updateAddTime to updatePlace

Since the name and location of place could change, make sure
we update the place on re-visit and just not the add time.

Created attachment 290550 [details] [review]
placeStore: Store all relevant place data

Created attachment 290552 [details] [review]
placeStore: Don't call _store during database loading

-----

- Make addPlace private.

Created attachment 290553 [details] [review]
placeStore: Add osm-id as unique identifier

The name of a place might change in OpenStreetMap or through code
changes in geocode-glib. It is safer to use the osm-id to assert the
uniqueness of a place.

Created attachment 290554 [details] [review]
placeStore: Change updateAddTime to updatePlace

Since the name and location of place could change, make sure
we update the place on re-visit and just not the add time.

Created attachment 290555 [details] [review]
placeStore: Store all relevant place data

Review of attachment 290462 [details] [review]:

I think you forgot to attach Rishi's place module.

Review of attachment 290465 [details] [review]:

Just a rapid review.

::: src/searchResultBubble.js
@@ +103,3 @@
+        if (place.wiki) {
+            let link = this._formatWikiLink(place.wiki);
+            info.push(_("<a href=\"%s\">Wikipedia article</a>").format(link));

You should not use markup in translation strings:

https://wiki.gnome.org/TranslationProject/DevGuidelines/Avoid%20markup%20wherever%20possible

@@ +107,3 @@
+
+        if (place.wheelchair) {
+            info.push(_("Wheelchair access: %s").format(place.wheelchair));

Is place.wheelchair translated by Overpass?

Review of attachment 290457 [details] [review]:

Nice! Some comments on code style. If you don't agree, feel free to ignore. :)

::: src/placeStore.js
@@ +62,3 @@
+
+    return new Geocode.Place(obj);
+}

I don't particularly like that this function modifies (in place) the obj that is passed to it. Maybe I'm a bit too anal here?

I'd do something like this instead:

function placeFromJSON(obj) {
    let props = { };
    for (let key in obj) {
        let prop = obj[key];
        
        switch(key) {
        case "id":
            props.osm_id = prop;
            break;

        case "location":
            props.location = new Geocode.Location(prop);
            break;

        case "bounding_box":
            obj.bounding_box = prop
                ? new Geocode.BoundingBox(prop)
                : null;
            break;

        default:
            if (obj[prop] !== null && obj[prop] !== undefined)
                props[key] = prop;
            break;
        }
    }
    return new Place(props);
}

Does that make sense?

Also I'd like this to be a static method on Place if possible. That is, inside Place.js:

Place.fromJSON = function() { /* the above function */ };

@@ +94,3 @@
+             country: place.country,
+             country_code: place.contry_code,
+             continent: place.continent,

Extra comma.

@@ +95,3 @@
+             country_code: place.contry_code,
+             continent: place.continent,
+           }

What Damián says + I'd make this a method toJSON on the Place-object itself.

@@ +184,3 @@
         try {
             let jsonArray = JSON.parse(buffer);
             jsonArray.forEach((function(obj) {

I'd patternmatch here:
 jsonArray.forEach((function({ place, type, added }) {

@@ +191,1 @@
                 this._addPlace(place, obj.type, obj.added);

...and then since place, type and added is available already: 
> this._addPlace(Place.fromJSON(place), type, added);

Review of attachment 290458 [details] [review]:

Looks good!

Review of attachment 290459 [details] [review]:

Looks good except for a small naming detail!

::: src/searchResultBubble.js
@@ +66,1 @@
+        info.forEach(function(i) {

Hm, "i" usually means "some unnameable iterative counter" so maybe rename "i" "infoText"? 
Or if you want it shorter: name the info array "infos" (plural) above and this "i" here "info" (singular).

Review of attachment 290461 [details] [review]:

Double bug reference in commit message, do we want that? Maybe yes?

Except for comments below looks good!

::: src/overpass.js
@@ +65,3 @@
+    addInfo: function(place, callback) {
+        let url = this._getQueryUrl(place.osm_id);
+        log('url: ' + url);

Utils.debug

@@ +77,3 @@
+            try {
+                let jsonObj = JSON.parse(message.response_body.data);
+                callback(true, message.status_code,

\n after "true,"

@@ +81,3 @@
+            } catch(e) {
+                throw e;
+                callback(false, message.status_code, []);

This callback will never be run because of the throw above. Maybe just remove the throw?

@@ +86,3 @@
+    },
+
+    _createPlace: function(place, jsonObj) {

I'd like something more descriptive than jsonObj here.

@@ +89,3 @@
+        let element = jsonObj.elements[0];
+
+        if (!element || !element.tags || !element.tags.name)

A little boolean logic and this becomes:
if (!(element && element.tags && element.tags.name))

Which reads easier to me at least.

@@ +95,3 @@
+
+        for (let prop in element.tags)
+            log(prop + ': ' + element.tags[prop]);

Utils.debug or just remove this.

Review of attachment 290462 [details] [review]:

Looks good except for the comments earlier about moving fromJSON to a static method on Place and toJSON as a normal method also on Place.

Review of attachment 290463 [details] [review]:

ACK (I assume you need it later.)

Review of attachment 290464 [details] [review]:

Looks good except for small nit.

::: src/placeStore.js
@@ +248,3 @@
+            let p = model.get_value(iter, Columns.PLACE);
+            if (p.osm_id === osmId)
+                place = p;

Should this loop break here as well? I'm guessing there's some return value that can be used to break the loop.

Review of attachment 290461 [details] [review]:

Also, this patch doesn't apply on master for me.

Review of attachment 290465 [details] [review]:

Looks good in general! Just some internationalization thoughts.

::: src/searchResultBubble.js
@@ +23,2 @@
 const Gtk = imports.gi.Gtk;
+

Skip this newline. The GJS guide is updated to only split on internal / external modules now and I agree with that + I fixed it to be like that in a cleanup patch recently.

@@ +56,3 @@
+            this._populate(place);
+        } else {
+            let overpass = new Overpass.Overpass({});

The Overpass module should be made to handle an empty constructor instead of this  i feel.

@@ +72,3 @@
+
+        return Format.vprintf('http://%s.wikipedia.org/wiki/%s', [ tokens[0],
+                                                                   tokens[1] ]);

Does this make the links prefer local language wikipedia? That is se.wikipedia.org if I have my locale set to SE_sv?

@@ +99,3 @@
+
+        if (place.openingHours)
+            info.push(_("Opening hours: %s").format(place.openingHours));

This should be am/pm or 24h depending on locale (or not really locale but that other thing).

@@ +103,3 @@
+        if (place.wiki) {
+            let link = this._formatWikiLink(place.wiki);
+            info.push(_("<a href=\"%s\">Wikipedia article</a>").format(link));

So basically: 
> info.push("<a href=\"%s\">%s</a>").format(link, _("Wikipedia article"));

Review of attachment 290553 [details] [review]:

pushed.

Review of attachment 290554 [details] [review]:

pushed

Review of attachment 290552 [details] [review]:

pushed.

Review of attachment 290459 [details] [review]:

::: src/searchResultBubble.js
@@ +66,1 @@
+        info.forEach(function(i) {

Yes, agreed.

Review of attachment 290461 [details] [review]:

Thanks!

::: src/overpass.js
@@ +65,3 @@
+    addInfo: function(place, callback) {
+        let url = this._getQueryUrl(place.osm_id);
+        log('url: ' + url);

Removed.

@@ +77,3 @@
+            try {
+                let jsonObj = JSON.parse(message.response_body.data);
+                callback(true, message.status_code,

Yes.

@@ +81,3 @@
+            } catch(e) {
+                throw e;
+                callback(false, message.status_code, []);

Used for debug, forgot to remove.

@@ +86,3 @@
+    },
+
+    _createPlace: function(place, jsonObj) {

Sure => overpassData

@@ +89,3 @@
+        let element = jsonObj.elements[0];
+
+        if (!element || !element.tags || !element.tags.name)

Right!

@@ +95,3 @@
+
+        for (let prop in element.tags)
+            log(prop + ': ' + element.tags[prop]);

Removed.

Review of attachment 290462 [details] [review]:

Yeah, I messed up here, place module will be included in next iteration!

Thanks to both of you for reviews, you absolutely rock!

Review of attachment 290464 [details] [review]:

::: src/placeStore.js
@@ +248,3 @@
+            let p = model.get_value(iter, Columns.PLACE);
+            if (p.osm_id === osmId)
+                place = p;

Yeah, good catch, will return true.

Review of attachment 290465 [details] [review]:

::: src/searchResultBubble.js
@@ +23,2 @@
 const Gtk = imports.gi.Gtk;
+

Yes!

@@ +56,3 @@
+            this._populate(place);
+        } else {
+            let overpass = new Overpass.Overpass({});

Agreed!

@@ +72,3 @@
+
+        return Format.vprintf('http://%s.wikipedia.org/wiki/%s', [ tokens[0],
+                                                                   tokens[1] ]);

I am not sure, it might just return a bunch of tags and we need to pick one.

I will investigate this more.

@@ +107,3 @@
+
+        if (place.wheelchair) {
+            info.push(_("Wheelchair access: %s").format(place.wheelchair));

Not sure, we might need to do some work here.

Created attachment 290579 [details] [review]
Add place module

Created attachment 290580 [details] [review]
Place: Add to/from JSON functions

Created attachment 290581 [details] [review]
placeStore: Store all relevant place data

Created attachment 290582 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 290583 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 290584 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

Created attachment 290585 [details] [review]
placeStore: Make exists function public

Created attachment 290586 [details] [review]
placeStore: Add get method

Created attachment 290587 [details] [review]
Use Overpass to get extra info for search bubble

Created attachment 290588 [details] [review]
place: Add new Overpass values to JSON method

Review of attachment 290584 [details] [review]:

Looks good, except for tiniest of nits!

::: src/overpass.js
@@ +41,3 @@
+    _init: function(params) {
+        if (!params)
+            params = { };

params = params || {}; is idiomatic js and also what rishi did below. :)

Created attachment 290590 [details] [review]
Add place module

Created attachment 290591 [details] [review]
Place: Add to/from JSON functions

Created attachment 290592 [details] [review]
placeStore: Store all relevant place data

Created attachment 290593 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 290594 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 290595 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

Created attachment 290596 [details] [review]
placeStore: Make exists function public

Created attachment 290597 [details] [review]
placeStore: Add get method

Created attachment 290598 [details] [review]
Use Overpass to get extra info for search bubble

Created attachment 290599 [details] [review]
place: Add new Overpass values to JSON method

Review of attachment 290580 [details] [review]:

Overall good. Small comment again about where the to/fromJSON should be defined.

::: src/place.js
@@ +24,3 @@
 const Lang = imports.lang;
 
+function fromJSON(obj) {

Hm, I meant it like this:
Place.fromJSON = function() { ... }

But I'm not sure, maybe this is just better?

@@ +52,3 @@
+}
+
+function toJSON(place) {

This I'm fairly sure should just be a method inside Place.

@@ +53,3 @@
+
+function toJSON(place) {
+    let bounding_box = null

Missing ;

Review of attachment 290590 [details] [review]:

Indentation and a question about a loop.

::: src/place.js
@@ +59,3 @@
+                country_code: params.place.contry_code,
+                continent: params.place.continent,
+            };

Indent these as:
params = { osm_id: ...
           ...
           continent: .. }:

@@ +63,3 @@
+            for (let prop in params)
+                if (!params[prop])
+                    delete params[prop];

Why is this loop only run if there is a place-parameter and not always?

Review of attachment 290591 [details] [review]:

Some thoughts on where from/toJSON should be.

::: src/place.js
@@ +24,3 @@
 const Lang = imports.lang;
 
+function fromJSON(obj) {

I meant for this to be like:
Place.fromJSON = function( ... ) { ... }

It would be like a static factory method / named constructor.

@@ +52,3 @@
+}
+
+function toJSON(place) {

I'm pretty sure this should just be a method on Place.

@@ +53,3 @@
+
+function toJSON(place) {
+    let bounding_box = null

Missing ;

Review of attachment 290591 [details] [review]:

Some thoughts on where from/toJSON should be.

::: src/place.js
@@ +24,3 @@
 const Lang = imports.lang;
 
+function fromJSON(obj) {

I meant for this to be like:
Place.fromJSON = function( ... ) { ... }

It would be like a static factory method / named constructor.

@@ +52,3 @@
+}
+
+function toJSON(place) {

I'm pretty sure this should just be a method on Place.

@@ +53,3 @@
+
+function toJSON(place) {
+    let bounding_box = null

Missing ;

Review of attachment 290590 [details] [review]:

::: src/place.js
@@ +59,3 @@
+                country_code: params.place.contry_code,
+                continent: params.place.continent,
+            };

Ok!

@@ +63,3 @@
+            for (let prop in params)
+                if (!params[prop])
+                    delete params[prop];

Because the GObject property setters will scream warnings of we set undefined/null properties, the javascript ones will accept it.
But it could be run for both I guess.

Review of attachment 290591 [details] [review]:

::: src/place.js
@@ +24,3 @@
 const Lang = imports.lang;
 
+function fromJSON(obj) {

Ah, ok!

@@ +52,3 @@
+}
+
+function toJSON(place) {

Yeah

@@ +53,3 @@
+
+function toJSON(place) {
+    let bounding_box = null

yep!

Created attachment 290601 [details] [review]
Add place module

Created attachment 290602 [details] [review]
Place: Add to/from JSON functions

Created attachment 290603 [details] [review]
placeStore: Store all relevant place data

Created attachment 290604 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 290605 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 290606 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

Created attachment 290607 [details] [review]
placeStore: Make exists function public

Created attachment 290608 [details] [review]
placeStore: Add get method

Created attachment 290609 [details] [review]
Use Overpass to get extra info for search bubble

Created attachment 290612 [details] [review]
Place: Add wheelchair translations

Created attachment 290614 [details] [review]
Place: Add wheelchair translations

Review of attachment 290614 [details] [review]:

So another way of doing this is to have the translation in searchResultBubble.js.
We can't do it directly on the property since then we will save it to the placeStore and then later try to translate a translated string.

Review of attachment 290601 [details] [review]:

Nice.

Just a comment about the properties: should we declare the new properties as GObject system properties and notify for changes in the setters. If that is not necessary, is necessary to have getters and setters that doesn't do anything special, maybe just public properties does the same work and reduce some lines of code.

::: src/place.js
@@ +58,3 @@
+                       country_code: params.place.contry_code,
+                       continent: params.place.continent };
+        }

Why this? in which case this constructor will receive the "place" param, I feel this as work already done by PlaceStore and fromJSON method.

Review of attachment 290601 [details] [review]:

I don't think they will change often enough to merit having notify on changes. I guess we could have them as "public" properties. Will let Mattias decide I think.

::: src/place.js
@@ +58,3 @@
+                       country_code: params.place.contry_code,
+                       continent: params.place.continent };
+        }

Well, we have a GeocodePlace that we get from GeocodeGlib, we want to add some fields to it using Overpass, right?
So we create a Place.Place object that has the extra parameters. Now we need to get the properties from the GeocodePlace into the Place.Place.
This is the way I found. We cannot iterate over GObject properties in the same way as Javascript object properties. Can you see a better way? Would love to have one.

Review of attachment 290602 [details] [review]:

Looks good for me! just a code style comment.

::: src/place.js
@@ +159,3 @@
+            if (prop !== null && prop !== undefined)
+                props[key] = prop;
+            break;

I think our style is:

switch (key) {
case 'id':
     props.osm_id = prop;
     break;
...
}

gnome-shell uses this style too in most of the cases.

Review of attachment 290603 [details] [review]:

Looks nice!

::: src/placeStore.js
@@ +138,3 @@
         try {
             let jsonArray = JSON.parse(buffer);
+            jsonArray.forEach((function({ place, type, added }) {

Ah, I didn't know we could write it like this :)

Review of attachment 290601 [details] [review]:

::: src/place.js
@@ +58,3 @@
+                       country_code: params.place.contry_code,
+                       continent: params.place.continent };
+        }

Ah, I get your point now, I thought params.place was a JSON object instead of a GeocodePlace instance.
No more objections then, maybe try to use g_object_class_list_properties with a GeocodePlaceClass as a first parameter, but not sure.

Review of attachment 290590 [details] [review]:

::: src/place.js
@@ +63,3 @@
+            for (let prop in params)
+                if (!params[prop])
+                    delete params[prop];

Ah I understand now!

Review of attachment 290601 [details] [review]:

Acketi-ack from me at least. :)

Regarding properties or fields: meh. It's fine as it is.

Review of attachment 290602 [details] [review]:

Looks good except for indent note from Damián!

::: src/place.js
@@ +159,3 @@
+            if (prop !== null && prop !== undefined)
+                props[key] = prop;
+            break;

+1 here.

Review of attachment 290603 [details] [review]:

Ack!

::: src/placeStore.js
@@ +138,3 @@
         try {
             let jsonArray = JSON.parse(buffer);
+            jsonArray.forEach((function({ place, type, added }) {

Pattern matching FTW!

Created attachment 290685 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 290686 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 290687 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

Created attachment 290688 [details] [review]
placeStore: Make exists function public

Created attachment 290689 [details] [review]
placeStore: Add get method

Created attachment 290690 [details] [review]
Use Overpass to get extra info for search bubble

Created attachment 290691 [details] [review]
Place: Add wheelchair translations

Review of attachment 290688 [details] [review]:

Looks good and I think it's safe to commit.

Just change "function" to "method" in the commit message.

Review of attachment 290689 [details] [review]:

Looks good and committable too.

Review of attachment 290691 [details] [review]:

Looks good!

Review of attachment 290685 [details] [review]:

Despite the little nit, looks good.

::: src/placeFormatter.js
@@ +51,3 @@
+
+    _update: function() {
+        let info = [];

Unused.

Created attachment 291065 [details] [review]
Add PlaceFormatter class

The purpose of this class is to get the relevant GeocodePlace properties
for the available place types. The class is implemented to be UI
agnostic, so it can be reused in many parts of Maps.

Created attachment 291066 [details] [review]
searchResultBubble: Use PlaceFormatter class

Created attachment 291067 [details] [review]
Add wrapper module for the Overpass API

Overpass QL: http://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL

Created attachment 291068 [details] [review]
placeStore: Make exists method public

Created attachment 291069 [details] [review]
placeStore: Add get method

Created attachment 291070 [details] [review]
Use Overpass to get extra info for search bubble

Created attachment 291071 [details] [review]
Place: Add wheelchair translations

Review of attachment 291066 [details] [review]:

Looks good for me!

::: src/searchResultBubble.js
@@ +73,2 @@
         this.content.add(ui.boxContent);
     },

Trailing comma :D

Review of attachment 291065 [details] [review]:

::: src/placeFormatter.js
@@ +43,3 @@
+        // We split after comma to avoid duplicating information
+        // in the title.
+        return this._place[this._titleProperty].split(',')[0];

I hope there won't be places with "country", "state", etc properties with commas.

Maybe we should only do the split when _titleProperty === 'name'.

Review of attachment 291067 [details] [review]:

::: src/overpass.js
@@ +23,3 @@
+
+const Geocode = imports.gi.GeocodeGlib;
+const Soup = imports.gi.Soup;

These four imports must be sort alphabetically and there should not be an empty line.

@@ +89,3 @@
+        let element = overpassData.elements[0];
+
+        if (!(element && element.tags && element.tags.name))

Maybe (!element || !element.tags || !element.tags.name) is more readable.

Review of attachment 291067 [details] [review]:

::: src/overpass.js
@@ +89,3 @@
+        let element = overpassData.elements[0];
+
+        if (!(element && element.tags && element.tags.name))

Sorry, Mattias already told you something about this, it seems we disagree :P

Review of attachment 291070 [details] [review]:

Looks good. Just a tiny comment.

::: src/searchResultBubble.js
@@ +51,3 @@
+        this._boxContent = ui.boxContent;
+
+        if (Application.placeStore.exists(this.place.osm_id, null)) {

We may start to think how to refresh Overpass information for old places that are present in the PlaceStore. But for now is fine :D

Review of attachment 291071 [details] [review]:

Looks good.

Review of attachment 291070 [details] [review]:

Changing state

Review of attachment 291069 [details] [review]:

Looks good.

Review of attachment 291068 [details] [review]:

Looks good.

Review of attachment 291067 [details] [review]:

::: src/overpass.js
@@ +89,3 @@
+        let element = overpassData.elements[0];
+
+        if (!(element && element.tags && element.tags.name))

Yeah, I will leave it as Mattias wanted it.

Attachment 291065 [details] pushed as 95576d3 - Add PlaceFormatter class
Attachment 291066 [details] pushed as f50ee19 - searchResultBubble: Use PlaceFormatter class
Attachment 291067 [details] pushed as 3da582b - Add wrapper module for the Overpass API
Attachment 291068 [details] pushed as bf0b505 - placeStore: Make exists method public
Attachment 291069 [details] pushed as e844a00 - placeStore: Add get method
Attachment 291070 [details] pushed as d2924bd - Use Overpass to get extra info for search bubble
Attachment 291071 [details] pushed as 2510763 - Place: Add wheelchair translations