Created attachment 293061 [details] [review]
Unify favoritesPopover/placeEntry filter code

Review of attachment 293061 [details] [review]:

Seems fine for me.

I know Mattias dislikes utils as a dumping ground for all sorts of code :)
But I also suspect he wants to do something about it on a grander scale, so dumping a big more should be fine.

Review of attachment 293061 [details] [review]:

::: src/utils.js
@@ +386,3 @@
+
+function matchPlace(place, searchString)
+{

The curly brace should be on the same line as the function definition.

Hm. Yeah I don't like what utils.js has grown into at all.

We don't happen to be so lucky that the only places you want to run this code
on are actually instances of Place (in Place.js)? If so I'd prefer to add a
match-method to that instead.

(In reply to comment #4)
> We don't happen to be so lucky that the only places you want to run this code
> on are actually instances of Place (in Place.js)? If so I'd prefer to add a
> match-method to that instead.

Yeah, I have the same idea and preference while coding this. But as you say, we have no guarantee to have Place instances everywhere.

Maybe we can start doing something from that point, using Place class everywhere.

Well since the placeStore gives place.js Places, the only place where we do not get them is geocodeService,js, and since you can instanciate a place.js Place from a GeocodePlace it would be quite simple to make sure, right?

Hm yeah, so either that or just a function in place.js.

Btw, since everything we are comparing here is from the place store we are guarenteed to have place.js places.

(In reply to comment #8)
> Btw, since everything we are comparing here is from the place store we are
> guarenteed to have place.js places.

Ok so a method on Place then? Does that make sense Damián?

(In reply to comment #9)
> Ok so a method on Place then? Does that make sense Damián?

Yup, going to submit the patch in a moment.

Created attachment 293180 [details] [review]
Use Place instead of Geocode.Place

---

This maybe is not necessary to the place filter changes
but I think is better to guarantee that a place object
is always an instance of Place in every part of Maps.

Created attachment 293181 [details] [review]
Unify favoritesPopover/placeEntry filter code

The code is moved to a method in Place class.

Review of attachment 293181 [details] [review]:

::: src/place.js
@@ +181,3 @@
+
+    match: function(searchString) {
+        let name = this.name;

One thing that is a bit worrysome is that "this.name" is not the same that the user see, right?
Since we use the searchFormatter. But that is the same in the old way we did this as well.

So I guess this is fine, and maybe we can rethink the filtering later to make it more awesome.

@@ +192,3 @@
+
+        if (searchString.length === 0)
+            return true;

Maybe move this to the top of the function, since everything else is useless if this condition is met?

::: src/placeEntry.js
@@ +166,1 @@
+        if (place !== null)

Can place be null here?

Review of attachment 293180 [details] [review]:

Humehum, so I think I am fine with this.

Some thoughts.

* In the places we are switching from Geocode to Place, is the imports line for Geocode still necessary?
* In overpass.js maybe we do not need to create a new place in addInfo? But instead fill in the passed one.

(In reply to comment #13)
> Review of attachment 293181 [details] [review]:
> 
> ::: src/place.js
> @@ +181,3 @@
> +
> +    match: function(searchString) {
> +        let name = this.name;
> 
> One thing that is a bit worrysome is that "this.name" is not the same that the
> user see, right?
> Since we use the searchFormatter. But that is the same in the old way we did
> this as well.
> 
> So I guess this is fine, and maybe we can rethink the filtering later to make
> it more awesome.
> 
> @@ +192,3 @@
> +
> +        if (searchString.length === 0)
> +            return true;
> 
> Maybe move this to the top of the function, since everything else is useless if
> this condition is met?
> 
> ::: src/placeEntry.js
> @@ +166,1 @@
> +        if (place !== null)
> 
> Can place be null here?

s/searchFormatter/placeFormatter/

Review of attachment 293180 [details] [review]:

::: src/geocodeService.js
@@ +55,3 @@
+                    places = places.map(function(p) {
+                        return new Place.Place({ place: p });
+                    });

I would like braces around multiline statements like these, to help readability.

::: src/searchResultBubble.js
@@ +63,3 @@
             overpass.addInfo(this.place, (function(status, code, place) {
                 if (!status)
+                    place = this.place;

If we modify addInfo to only addInfo and not create a new place, then this becomes useless, and we can use this.place throughout, right?

Review of attachment 293181 [details] [review]:

::: src/place.js
@@ +181,3 @@
+
+    match: function(searchString) {
+        let name = this.name;

I kept the same logic as before, but you are right.

Maybe we should filter the by searching in title and subtitle as is shown in search popup and also make the subtitle bold for the matched part.

@@ +192,3 @@
+
+        if (searchString.length === 0)
+            return true;

Ok, moving it after GLib.utf8_normalize of searchString

::: src/placeEntry.js
@@ +166,1 @@
+        if (place !== null)

Indeed, it happens when I insert or remove an element from the place store. Maybe this from the gtk_tree_model_filter_set_visible_func documentation is related: "Note that func is called whenever a row is inserted, when it may still be empty. The visible function should therefore take special care of empty rows, like in the example below."

Also, we were doing almost the same check in the "if (name === null)" line.

Review of attachment 293181 [details] [review]:

::: src/place.js
@@ +181,3 @@
+
+    match: function(searchString) {
+        let name = this.name;

Yeah, maybe. And ideally we would have a concept of "relevance".

::: src/placeEntry.js
@@ +166,1 @@
+        if (place !== null)

Yeah, you are right.

(In reply to comment #18)
> Review of attachment 293181 [details] [review]:
> 
> ::: src/place.js
> @@ +181,3 @@
> +
> +    match: function(searchString) {
> +        let name = this.name;
> 
> Yeah, maybe. And ideally we would have a concept of "relevance".
> 
> ::: src/placeEntry.js
> @@ +166,1 @@
> +        if (place !== null)
> 
> Yeah, you are right.

But I would accept this for now, and we can re-visit if we have time for 3.16.

Created attachment 293184 [details] [review]
Use Place instead of Geocode.Place

Created attachment 293185 [details] [review]
Unify favoritesPopover/placeEntry filter code

The code is moved to a method in Place class.

Review of attachment 293184 [details] [review]:

Thanks!

Review of attachment 293185 [details] [review]:

Yesbox!

Attachment 293184 [details] pushed as e9b9d00 - Use Place instead of Geocode.Place
Attachment 293185 [details] pushed as 0f881a1 - Unify favoritesPopover/placeEntry filter code