I agree! The popovers would be a huge upgrade from our hand hacked bubble's we've made with clutte. I hope it's possible though. A potential problem might be to make the popover follow when you pan around and zoom etc (I have no idea if that's hard or not). 
Also, we'll still need /something/ to show markers on the map (think favourites as small stars, the current user location pin etc) so maybe we'll use the champlain-markers for that, but for the actual bubble the popovers seems like a perfect match. :)

(In reply to comment #1)
> I agree! The popovers would be a huge upgrade from our hand hacked bubble's
> we've made with clutte. I hope it's possible though. A potential problem might
> be to make the popover follow when you pan around and zoom etc (I have no idea
> if that's hard or not). 
> Also, we'll still need /something/ to show markers on the map (think favourites
> as small stars, the current user location pin etc) so maybe we'll use the
> champlain-markers for that, but for the actual bubble the popovers seems like a
> perfect match. :)

Yeah, i think pin better remain a champlain-marker.

We might want to give this a trial implementation. The problem is it will not be possible to add a popover to the mapView. But it would be possible to add it to the windowContent overlay.

You would set the relative-to property to the overlay. And then carve out a Cairo.rectangleInt with a suitable x and y, width and height of 1 and set the pointing-to property to that.

But as Mattias said, it might be to much work to move it around when the map moves.

Created attachment 271347 [details] [review]
experiment: Use GtkPopover to show location info

So, I'm playing right now with GtkPopover to show location info, the initial results are not bad in my opinion.

I tried to dramatically suppress the difficult part of the issue, that is, move the Popover when user pans the Map View. I was a little conviced to do that by checking Apple Map behaviour for MacOS X, that application shows the popover when user clicks in the marker and when users pan the map, the popover just dissapear.

In my opinion, that behaviour is not odd and feels natural.

I'm going to submit a patch so you can check it out. Obviously, the patch is unmergeable, is just code to get a live mockup and to get the idea. It only works with the UserLocation marker (I think that the "What's here?" marker needs a little redesign, maybe to behave more like a pin marker), when user clicks on the marker, it fires an event that reach to the MainWindow, that one gets the clicked marker instance to show the popover at proper location. When user pans the map, the popover just dissapear.

Known issues:
 - When you resize the window, the popover still there, poorly positioned.
 - The popover is modal, so is less agile to the user to get focus on other widgets when the popover is being shown.
 - The position of the popover in relation to the marker is not perfect.

Enjoy!

(In reply to comment #4)
> Created an attachment (id=271347) [details] [review]
> experiment: Use GtkPopover to show location info
> 
> So, I'm playing right now with GtkPopover to show location info, the initial
> results are not bad in my opinion.
> 
> I tried to dramatically suppress the difficult part of the issue, that is, move
> the Popover when user pans the Map View. I was a little conviced to do that by
> checking Apple Map behaviour for MacOS X, that application shows the popover
> when user clicks in the marker and when users pan the map, the popover just
> dissapear.
> 
> In my opinion, that behaviour is not odd and feels natural.
> 
> I'm going to submit a patch so you can check it out. Obviously, the patch is
> unmergeable, is just code to get a live mockup and to get the idea. It only
> works with the UserLocation marker (I think that the "What's here?" marker
> needs a little redesign, maybe to behave more like a pin marker), when user
> clicks on the marker, it fires an event that reach to the MainWindow, that one
> gets the clicked marker instance to show the popover at proper location. When
> user pans the map, the popover just dissapear.
> 
> Known issues:
>  - When you resize the window, the popover still there, poorly positioned.
>  - The popover is modal, so is less agile to the user to get focus on other
> widgets when the popover is being shown.
>  - The position of the popover in relation to the marker is not perfect.
> 
> Enjoy!

Cool stuff, thanks :)

I think it would be nice to see this on mapLocation. The userLocation is a bit of a special case. It's the mapLocation that gets shown most often. And that behavior feels more important.
So making a class for the locationPopover and using it in map- and userLocation might be a better way.

The position of the marker could be solved by using the mapLocations latitude and longitude and getting x and y by the champlain_view_longitude_to x and champlain_view_latitude_to_y functions.

For resizing and moving, listening to the latitude or longitude notify-signal might give something.

Thanks for doing this.

Also, if you think there is something missing in Champlain in order to do this, like a single or some other information. You could file a bug there and maybe we can get it in.

s/signle/signal

Do you want to keep working at it Damián? To see if we can get something pretty and functional enough to use?

Yeah, that sounds great. The problem perhaps is that I cannot dedicate fulltime to this (at least for this month), but I'll try to do my best.

Mockups: https://raw.github.com/gnome-design-team/gnome-mockups/master/maps/v2/markers-and-bubbles.png

Additional Comments from Andreas:

"My thinking was that it would hide the bubble and just show the marker on pan/zoom and then pop it up again only if you click the marker again.
With regards to hiding the bubble on traveling, I think it would be best for the bubble to only show when you arrive. Especially if we can animate it up somehow."

*** Bug 706071 has been marked as a duplicate of this bug. ***

Created attachment 272667 [details] [review]
Make PlaceStore globally accessible

(In reply to comment #13)
> Created an attachment (id=272667) [details] [review]
> Make PlaceStore globally accessible

I'm guessing this should have gone in the placestore-bug[1] instead?

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

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=272667) [details] [review] [details] [review]
> > Make PlaceStore globally accessible
> 
> I'm guessing this should have gone in the placestore-bug[1] instead?
> 
> 1: https://bugzilla.gnome.org/show_bug.cgi?id=722102

Jonas redirected me here.
I think because it doesn't belong there, moreover I think it should have a separate bug which states to synchronize placeStore data across all modules?

Comment on attachment 272667 [details] [review]
Make PlaceStore globally accessible

Attachment 272667 [details] pushed as 819c8ea - Make PlaceStore globally accessible

Created attachment 277757 [details] [review]
Add User location marker and bubble

This add only the User location marker/bubble, the new MapMarker and MapBubble classes will be helpuf classes to implement the
remaining markers/bubbles.

Review of attachment 277757 [details] [review]:

Sorry for the late review.

Some general comments:
 - Please split this up in two patches, one for map{Bubble,Marker} and one for location{Bubble,Marker}
 - Master was updated this weekend so please rebase.

Great work!

::: src/mapBubble.js
@@ +35,3 @@
+
+        this._place = params.place;
+        this._mapView = params.mapView;

I'd do something like this instead:

    this._place = params.place;
    delete params.place;
    
    this._mapView = params.mapView;
    params.relative_to = params.mapView;
    delete params.mapView;
    
    this.parent(params);

That way this class gets a bit more extendable if needed in the future.

@@ +40,3 @@
+    getPlace: function() {
+        return this._place;
+    }

get place() {
        return this._place;
    }

::: src/mapMarker.js
@@ +2,3 @@
+/* vim: set et ts=4 sw=4: */
+/*
+ * Copyright (c) 2014 Damián Nohales

Lots of code copied from the old mapLocation here, so you should keep the copyright from the old and just add yours.

@@ +18,3 @@
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Author: Damián Nohales <damiannohales@gmail.com>

Same as copyright, keep the old ones from mapLocation

@@ +21,3 @@
+ */
+
+const cairo = imports.gi.cairo;

Cairo

@@ +31,3 @@
+const Utils = imports.utils;
+const Path = imports.path;
+const _ = imports.gettext.gettext;

Please sort imports alfabetically.

@@ +33,3 @@
+const _ = imports.gettext.gettext;
+
+const _MAX_DISTANCE = 19850; // half of Earth's curcumference (km)

circumference

@@ +60,3 @@
+const MapMarker = new Lang.Class({
+    Name: "MapMarker",
+    Extends: Champlain.Marker,

Should this be Abstract btw?

@@ +65,3 @@
+    },
+
+    _init: function(params) {

If the params object shouldn't be passed on to this.parent() and the number of parameters is ≤ 4 I think you should just go with ordinary parameters.

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

...however, I think it might make sense to just filter out the params we need here and pass the rest to this.parent() here.

@@ +69,3 @@
+
+        this._place = params.place;
+        this._mapView = params.mapView;

... in which case this would become:
  this._place = params.place;
  delete params.place;

  this._mapView = params.mapView;
  delete params.mapView;
  this.parent(params);

@@ +76,3 @@
+        this.set_location(this._place.location.latitude,
+                          this._place.location.longitude);
+        this.set_selectable(true);

This is a parameter, so I would set this in this.parent()

@@ +79,3 @@
+
+        this.connect('notify::size', (this._translateMarkerPosition).bind(this));
+        this.connect("notify::selected", (this._onMarkerSelected).bind(this));

uneeded parantheses around this._translateMarkerPosition and this._onMarkerSelected

@@ +95,3 @@
+
+    _getBubblePositionTranslationData: function() {
+        return [0, 0]

missing semicolon

@@ +96,3 @@
+    _getBubblePositionTranslationData: function() {
+        return [0, 0]
+    },

What does these two methods give us? Would it make sense to just define two constants like this:

  const MARKER_TRANSLATION = [0, 0, 0];
  const BUBBLE_TRANSLATION = [0, 0];

... in the top of this file or are these meant to be overridden by sub-classes?

@@ +113,3 @@
+    getPlace: function() {
+        return this._place;
+    },

Use a getter instead.

@@ +116,3 @@
+
+    getBubble: function() {
+        if (this._bubble == null) {

If you want to check for falsiness do:
  if (!this._bubble)  // This is what I guess you actually want to do.

If you specifically want to check against just null do:
  if (this._bubble === null)

@@ +125,3 @@
+    _createBubble: function() {
+        return null;
+    },

If this is just an abstract function I'd just do:

  // Abstract
  _createBubble: function() { }

@@ +129,3 @@
+    _positionBubble: function(bubble) {
+        let [x, y] = this.get_transformed_position();
+        let translation = this._getBubblePositionTranslationData();

Might make sense to pattern match here as well.
That is:
  let [trans_x, trans_y] = ...

And then use those below. Just a little bit clearer.

@@ +136,3 @@
+            width: 1,
+            height: 1
+        });

Align like this:

let obj = new MyModule.MyClass({ p1: "",
                                 p2: null });

... where p2 is right under p1 (stupid non-monospace font).

@@ +138,3 @@
+        });
+
+        bubble.set_pointing_to(pos);

bubble.pointing_to = pos;

@@ +144,3 @@
+        let bubble = this.getBubble();
+
+        if (bubble != null) {

Same comment as above. I'm guessing you're checking for truthyness here. 
Then this becomes:
  if (bubble) {

If you're actually checking against just null (and all other values, like undefined, is ok) then:
  if (bubble !== null)

@@ +153,3 @@
+        let bubble = this.getBubble();
+
+        if (bubble != null) {

Same here.

@@ +199,3 @@
+            }
+        }
+        this._view.set_zoom_level(zoom);

this._view.zoom_level = zoom;

@@ +230,3 @@
+            latitude: this._view.get_center_latitude(),
+            longitude: this._view.get_center_longitude()
+        });

Alignment.

I realize that this is probably how it was aligned before, so not your fault in any way, but let's be consistent.

@@ +295,3 @@
+        this.showBubble();
+    }
+

Unnecessary newline

::: src/mapView.js
@@ +171,3 @@
+            place: place,
+            mapView: this
+        });

Align.

::: src/user-location-bubble.ui
@@ +11,3 @@
+    <property name="margin_bottom">15</property>
+    <child>
+      <object class="GtkImage" id="image-geolocation">

image-user-location instead maybe?

@@ +37,3 @@
+            <property name="can_focus">False</property>
+            <property name="xalign">0</property>
+            <property name="label" translatable="yes">Your current location</property>

Just "Current location"

::: src/userLocationBubble.js
@@ +38,3 @@
+            'label-accuracy',
+            'label-coordinates'
+        ]);

Align to the right of [

@@ +40,3 @@
+        ]);
+
+        ui.labelAccuracy.label = ui.labelAccuracy.label.format(MapMarker.getAccuracyDescription(this._place.location.accuracy));

This line is too long. I don't remember if we decided on 100 or 120 chars but 128 is too much at least. :)

@@ +43,3 @@
+        ui.labelCoordinates.label = this.getPlace().location.latitude
+                                  + ', '
+                                  + this.getPlace().location.longitude;

You should truncate the latitude and longitude a bit, I think 5 decimals makes sense after having read this: http://en.wikipedia.org/wiki/Decimal_degrees

::: src/userLocationMarker.js
@@ +33,3 @@
+const Utils = imports.utils;
+const Path = imports.path;
+const _ = imports.gettext.gettext;

Order alphabetically

@@ +50,3 @@
+        this.set_location(this._place.location.latitude,
+                          this._place.location.longitude);
+        this.set_reactive(false);

Set these in this.parent() instead.

@@ +54,3 @@
+
+    refreshGeometry: function(view) {
+        let zoom = view.get_zoom_level();

let zoom = view.zoom_level;

@@ +55,3 @@
+    refreshGeometry: function(view) {
+        let zoom = view.get_zoom_level();
+        let source = view.get_map_source();

let source = view.map_source;

@@ +58,3 @@
+        let metersPerPixel = source.get_meters_per_pixel(zoom,
+                                                         this.get_latitude(),
+                                                         this.get_longitude());

this.latitude and this.longitude

@@ +61,3 @@
+        let size = this._place.location.accuracy * 2 / metersPerPixel;
+        let viewWidth = view.get_width();
+        let viewHeight = view.get_height();

Skip these, and just use view.width and view.height below

@@ +67,3 @@
+            this.hide();
+        else {
+            this.set_size(size);

this.size = size

@@ +103,3 @@
+            -Math.floor(this.get_height() / 2),
+            0
+        ];

Align right of [

@@ +110,3 @@
+            Math.floor(this.get_width() / 2),
+            3
+        ];

Align

@@ +117,3 @@
+            place: this._place,
+            mapView: this._mapView
+        });

Align

> @@ +95,3 @@
> +
> +    _getBubblePositionTranslationData: function() {
> +        return [0, 0]
> 
> missing semicolon
> 
> @@ +96,3 @@
> +    _getBubblePositionTranslationData: function() {
> +        return [0, 0]
> +    },
> 
> What does these two methods give us? Would it make sense to just define two
> constants like this:
> 
>   const MARKER_TRANSLATION = [0, 0, 0];
>   const BUBBLE_TRANSLATION = [0, 0];
> 
> ... in the top of this file or are these meant to be overridden by sub-classes?

These are meant to be overridden by sub-classes, each marker could have different kind of images, and each image can have a different hot point (I mean, the point of the marker image that should be positioned over the latitude/longitude). By default, the markers are positioned with the hot point at the top-left corner. You can define how to translate the marker to place the hot point over the right point in the map. The bubble must be translated too, maybe this one may be calculated automatically, so we can avoid _getBubblePositionTranslationData.

Created attachment 278936 [details] [review]
Add MapMarker and MapBubble classes

Those are base classes intended to create markers associated with
content rich popovers.

Created attachment 278937 [details] [review]
Refactor user location marker to support bubble

Those patches also adds missing entries in POTFILES.in and remove braces in one line conditional statements bodies.

Review of attachment 278936 [details] [review]:

Apart from some small stuff below I think this looks fine. Jonas and / or Zeeshan, what do you think?

::: src/mapMarker.js
@@ +103,3 @@
+    _getBubblePositionTranslationData: function() {
+        return [0, 0];
+    },

Could these two perhaps do some advanced calculations to find out the translation?
If not I'd go for a property, otherwise a function is fine.

Also, I'd definititely drop the "Data"-suffix. Maybe also the "Position"-part (but that I'm less sure of), what do you think?

@@ +112,3 @@
+                left: place.bounding_box.left,
+                right: place.bounding_box.right
+            });

Align.

@@ +122,3 @@
+
+    get bubble() {
+        // false means "uninitialized" and null means "without bubble"

Hm, I think it makes more sense to use "undefined" as uninitialized.

Review of attachment 278937 [details] [review]:

Looks good! We need ACK from Jonas and / or Zeeshan though before committing.

::: src/mapView.js
@@ +154,3 @@
                                                     location);
 
+        let selected = this._userLocation && this._userLocation.get_selected();

Use the selected property instead.

Just realized one thing. When you resize the window the position the bubble is pointing to isn't updated. I guess listening to some champlain signal or so is needed.

Review of attachment 278936 [details] [review]:

Other than comments, this seems fine. But Mattias comment about handling re-size needs to be addressed.

::: src/mapMarker.js
@@ +103,3 @@
+    _getBubblePositionTranslationData: function() {
+        return [0, 0];
+    },

I agree that get[Marker|Bubble]Translation is prettier.

I also think it would be nice to be able to calculate this, or push the responsibility of getting the "hoptspot" right to somewhere else.

@@ +112,3 @@
+                left: place.bounding_box.left,
+                right: place.bounding_box.right
+            });

And this is a case where the multiple lines of the single statement in the if-statement might merit brackets.

@@ +135,3 @@
+
+    _positionBubble: function(bubble) {
+        let [x, y] = this.get_transformed_position();

Will this always give us the same position as using this.mapView.view.longitude_to_x and latitude_to_y using the lat/lons?

@@ +208,3 @@
+    getAccuracyDescription: function() {
+        return getAccuracyDescription(this.place.location.accuracy);
+    },

I don't see why this method needs to live here. It does not need to be a method of the marker class, as seen by it being implemented by calling a static function. And the only use of this method/function is in the next patch you post, and there it is used by a bubble subclass not a marker subclass.

Maybe it should just move to Utils?

Review of attachment 278937 [details] [review]:

If we commit this now we get a bit of an awkward state of our repo, right?

We will have src/mapLocation.js and stc/mapMarker.js that duplicates a lot of each others code.
I would prefer to wait until we can replace mapLocation with mapLocationMarker.

::: data/icons/Makefile.am
@@ +18,3 @@
 	bubble.svg				\
 	pin.svg					\
+	user-location.png		\

Align the \ please

(In reply to comment #26)
> 
> @@ +135,3 @@
> +
> +    _positionBubble: function(bubble) {
> +        let [x, y] = this.get_transformed_position();
> 
> Will this always give us the same position as using
> this.mapView.view.longitude_to_x and latitude_to_y using the lat/lons?

I think so.

(In reply to comment #27)
> Review of attachment 278937 [details] [review]:
> 
> If we commit this now we get a bit of an awkward state of our repo, right?
> 
> We will have src/mapLocation.js and stc/mapMarker.js that duplicates a lot of
> each others code.
> I would prefer to wait until we can replace mapLocation with mapLocationMarker.

Ok, I'm going to start to implement SearchResult{Marker, Bubble} so we can remove mapLocation.

> 
> ::: data/icons/Makefile.am
> @@ +18,3 @@
>      bubble.svg                \
>      pin.svg                    \
> +    user-location.png        \
> 
> Align the \ please

So you are using 8 width tabs :)

(In reply to comment #28)
> > I would prefer to wait until we can replace mapLocation with mapLocationMarker.
> 
> Ok, I'm going to start to implement SearchResult{Marker, Bubble} so we can
> remove mapLocation.

\o/

> > 
> > ::: data/icons/Makefile.am
> > @@ +18,3 @@
> >      bubble.svg                \
> >      pin.svg                    \
> > +    user-location.png        \
> > 
> > Align the \ please
> 
> So you are using 8 width tabs :)

Yeah :)

Created attachment 279069 [details] [review]
Add MapMarker and MapBubble classes - v2

- Moved getAccuracyDescription to Utils and removed getAccuracyDescription method.
- Fixed code alignments.
- Now the signals that we connect to hide the bubble are connected when we show the bubble instead of when we construct the marker, we are expecting lot of markers, making the connections when we create the markers with the bubble uninitialized seems to be a unnecesary overhead. The signals are also disconnected when the bubble is closed.
- Don't fire bubble initialization when we call hideBubble having an unitialized bubble.
- Replaced _get{Marker,Bubble}PositionTranslationData to _getMarkerHotSpot (returns the actor relative coordinates to the hot spot) and _getBubbleSpacing (allows to add an extra spacing between the edge of the marker and the bubble peak)
- Fixed bubble positioning when the marker is on the top of the map.

Created attachment 279070 [details] [review]
Refactor user location marker to support bubble - v2

- Make changes according to the previous patch.
- Fixed code alignments.
- If user activates the user location to view the bubble and the user location is changed, keep showing the bubble when we move the marker (there is a workaround related to this in mapView.js, maybe you can figure out how to do it right.)

Created attachment 279071 [details] [review]
Refactor user location marker to support bubble - v3

Remove bubble.svg.

Created attachment 279136 [details] [review]
Add MapMarker and MapBubble classes - v3

- Change the bubble position calculation by using longitude_to_x and
latitude_to_y methods, this avoid the timout workarount mapView from
the previous patch version.
- Move the goTo and zoomToFit code to a new class, MapWalker.
-_getMarkerHotSpot -> _getHotSpot

Created attachment 279137 [details] [review]
Refactor user location marker to support bubble - v4

- Add the AccuracyCircleMarker before the UserLocationMarker, if we do
it the other way around, user can not click the marker when
AccuracyCircleMarker is being show.
- Remove deprecated xalign/yalign in Glade file.

Created attachment 279138 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble

Review of attachment 279136 [details] [review]:

Thanks looks good!

In log please replace 'Those' with 'These'.

::: po/POTFILES.in
@@ +10,3 @@
 [type: gettext/glade]src/main-window.ui
 src/mapLocation.js
+src/mapMarker.js

Why does this need to go here? And why not the two other additions?

::: src/mapMarker.js
@@ +70,3 @@
+    _getHotSpot: function() {
+        return [0, 0];
+    },

Is hot spot a well known term for this? Is it obvious what hot spot means? It is not really to me. Do we need a comment or a better name?

@@ +89,3 @@
+    _isBubbleInitialized: function() {
+        // undefined means 'uninitialized' and null means 'without bubble'
+        return typeof this._bubble != 'undefined';

And this will return null if this._bubble is null?

@@ +135,3 @@
+                    }
+                }
+            }).bind(this));

This looks a bit to fancy. I think I prefer just doing it explicitly.

::: src/mapView.js
@@ -182,3 @@
-        // Animate to the center of the route bounding box
-        // goto() is currently implemented on mapLocation, so we need to go
-        // through some hoops here.

Why this comment removed?

@@ +193,2 @@
+        let walker = new MapWalker.MapWalker(place, this);
+        walker.goTo(true);

Maybe just:
    new MapWalker.MapWalker(place, this).goto(true);

::: src/mapWalker.js
@@ +35,3 @@
+const _ = imports.gettext.gettext;
+
+const _MAX_DISTANCE = 19850; // half of Earth's curcumference (km)

curcumference?

Review of attachment 279137 [details] [review]:

Thanks, looks good!

::: src/mapView.js
@@ +159,3 @@
+                                                                         mapView: this });
+        this._userLocationLayer.remove_all();
+        this._userLocation.addToLayer(this._userLocationLayer);

Couldn't this still be called show()? And have the remove_all functionality in it?
The base marker class could just call addToLayer and the sub classes that need to remove all other stuff can do a remove_all as well.

::: src/userLocationBubble.js
@@ +41,3 @@
+        ui.labelCoordinates.label = this.place.location.latitude.toFixed(5)
+                                  + ', '
+                                  + this.place.location.longitude.toFixed(5);

The '5' might be something we want as a constant somewhere. So that every time we want to print lat/lons we use the same fixed value.
Dunno where, might be a TODO.

Review of attachment 279138 [details] [review]:

Thanks looks great!

What exactly is the difference between a searchResultBubble and a poiBubble?
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/v2/markers-and-bubbles.png

The mockups seem very similar. So even tho they should not be exactly the same, maybe they should share a lot of code?
So we might find a better name that SearchResultBubble?

(Not related, but when I looked at contextMenu I wondered if the reverseGeocode-method should move to GeocodeService, thoughts?)

::: src/searchResultBubble.js
@@ +83,3 @@
+        this.add(ui.box);
+    }
+});

we need to add this file to POTFILES, right?

::: src/searchResultMarker.js
@@ +35,3 @@
+        this.parent(params);
+
+        let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg");

So this is the same pin as for userlocation?

I have only reviewed this on here, haven't tested it yet, so no UI comments. Would be nice to hear from Andreas and Mattias. Maybe you can ping them on IRC?

(In reply to comment #36)
> Review of attachment 279136 [details] [review]:
> 
> ::: src/mapMarker.js
> @@ +70,3 @@
> +    _getHotSpot: function() {
> +        return [0, 0];
> +    },
> 
> Is hot spot a well known term for this? Is it obvious what hot spot means? It
> is not really to me. Do we need a comment or a better name?

Hmmm... what do you suggest?

> 
> @@ +89,3 @@
> +    _isBubbleInitialized: function() {
> +        // undefined means 'uninitialized' and null means 'without bubble'
> +        return typeof this._bubble != 'undefined';
> 
> And this will return null if this._bubble is null?

Believe me, returns true.

print(typeof null); //prints "object"

> 
> @@ +135,3 @@
> +                    }
> +                }
> +            }).bind(this));
> 
> This looks a bit to fancy. I think I prefer just doing it explicitly.

But... it looks awesome :P

> 
> ::: src/mapView.js
> @@ -182,3 @@
> -        // Animate to the center of the route bounding box
> -        // goto() is currently implemented on mapLocation, so we need to go
> -        // through some hoops here.
> 
> Why this comment removed?
> 

MapLocation was used here to make the goTo animation, that is a bit no obvious, because MapLocation goal is to show a marker on the map, so it deserves a comment. Now we are using MapWalker in that hunk, so is more self-explanatory.

> 
> ::: src/mapWalker.js
> @@ +35,3 @@
> +const _ = imports.gettext.gettext;
> +
> +const _MAX_DISTANCE = 19850; // half of Earth's curcumference (km)
> 
> curcumference?

Déjà vu!

(In reply to comment #37)
> Review of attachment 279137 [details] [review]:
> 
> Thanks, looks good!
> 
> ::: src/mapView.js
> @@ +159,3 @@
> +                                                                        
> mapView: this });
> +        this._userLocationLayer.remove_all();
> +        this._userLocation.addToLayer(this._userLocationLayer);
> 
> Couldn't this still be called show()? And have the remove_all functionality in
> it?
> The base marker class could just call addToLayer and the sub classes that need
> to remove all other stuff can do a remove_all as well.
> 

We can not use show to add a marker to a layer, since show methods belongs to ClutterActor and we cannot change the method signature (using JS maybe we can, but we shouldn't).

I created addToLayer just for the cases of markers that has other actors related to it... UserLocationMarker has this characteristic, actually, I think is better to initialize the UserLocationMarker so this._userLocationLayer.add_marker(this._userLocation) just works.

remove_all should not be responsibility of the marker, that'd be too coupled.

> ::: src/userLocationBubble.js
> @@ +41,3 @@
> +        ui.labelCoordinates.label = this.place.location.latitude.toFixed(5)
> +                                  + ', '
> +                                  + this.place.location.longitude.toFixed(5);
> 
> The '5' might be something we want as a constant somewhere. So that every time
> we want to print lat/lons we use the same fixed value.
> Dunno where, might be a TODO.

Actually, I'm starting to think that we don't have to show coordinates at all, a "Share" button is planned (I don't know when we are going to start it) a that could have a "Copy Coordinates" feature on it.

(In reply to comment #40)
> (In reply to comment #36)
> > Review of attachment 279136 [details] [review] [details]:
> > 
> > ::: src/mapMarker.js
> > @@ +70,3 @@
> > +    _getHotSpot: function() {
> > +        return [0, 0];
> > +    },
> > 
> > Is hot spot a well known term for this? Is it obvious what hot spot means? It
> > is not really to me. Do we need a comment or a better name?
> 
> Hmmm... what do you suggest?
> 
> > 
> > @@ +89,3 @@
> > +    _isBubbleInitialized: function() {
> > +        // undefined means 'uninitialized' and null means 'without bubble'
> > +        return typeof this._bubble != 'undefined';
> > 
> > And this will return null if this._bubble is null?
> 
> Believe me, returns true.
> 
> print(typeof null); //prints "object"
> 
> > 
> > @@ +135,3 @@
> > +                    }
> > +                }
> > +            }).bind(this));
> > 
> > This looks a bit to fancy. I think I prefer just doing it explicitly.
> 
> But... it looks awesome :P
> 
> > 
If you press review instead of reply it will be easier to follow along the review-process.

> > ::: src/mapView.js
> > @@ -182,3 @@
> > -        // Animate to the center of the route bounding box
> > -        // goto() is currently implemented on mapLocation, so we need to go
> > -        // through some hoops here.
> > 
> > Why this comment removed?
> > 
> 
> MapLocation was used here to make the goTo animation, that is a bit no obvious,
> because MapLocation goal is to show a marker on the map, so it deserves a
> comment. Now we are using MapWalker in that hunk, so is more self-explanatory.
> 

Ah, right you are!

> > 
> > ::: src/mapWalker.js
> > @@ +35,3 @@
> > +const _ = imports.gettext.gettext;
> > +
> > +const _MAX_DISTANCE = 19850; // half of Earth's curcumference (km)
> > 
> > curcumference?
> 
> Déjà vu!

: )

(In reply to comment #41)
> (In reply to comment #37)
> > Review of attachment 279137 [details] [review] [details]:
> > 
> > Thanks, looks good!
> > 
> > ::: src/mapView.js
> > @@ +159,3 @@
> > +                                                                        
> > mapView: this });
> > +        this._userLocationLayer.remove_all();
> > +        this._userLocation.addToLayer(this._userLocationLayer);
> > 
> > Couldn't this still be called show()? And have the remove_all functionality in
> > it?
> > The base marker class could just call addToLayer and the sub classes that need
> > to remove all other stuff can do a remove_all as well.
> > 
> 
> We can not use show to add a marker to a layer, since show methods belongs to
> ClutterActor and we cannot change the method signature (using JS maybe we can,
> but we shouldn't).
> 
> I created addToLayer just for the cases of markers that has other actors
> related to it... UserLocationMarker has this characteristic, actually, I think
> is better to initialize the UserLocationMarker so
> this._userLocationLayer.add_marker(this._userLocation) just works.
> 
> remove_all should not be responsibility of the marker, that'd be too coupled.


Ok.

> 
> > ::: src/userLocationBubble.js
> > @@ +41,3 @@
> > +        ui.labelCoordinates.label = this.place.location.latitude.toFixed(5)
> > +                                  + ', '
> > +                                  + this.place.location.longitude.toFixed(5);
> > 
> > The '5' might be something we want as a constant somewhere. So that every time
> > we want to print lat/lons we use the same fixed value.
> > Dunno where, might be a TODO.
> 
> Actually, I'm starting to think that we don't have to show coordinates at all,
> a "Share" button is planned (I don't know when we are going to start it) a that
> could have a "Copy Coordinates" feature on it.

Well we could if my bug #712752 is accepted to Gtk+, right now there is really no way to copy to clipboard through bindings.

(In reply to comment #40)
> (In reply to comment #36)
> > Review of attachment 279136 [details] [review] [details]:
> > 
> > ::: src/mapMarker.js
> > @@ +70,3 @@
> > +    _getHotSpot: function() {
> > +        return [0, 0];
> > +    },
> > 
> > Is hot spot a well known term for this? Is it obvious what hot spot means? It
> > is not really to me. Do we need a comment or a better name?
> 
> Hmmm... what do you suggest?

Not sure, maybe a comment will do. Anyone else with better naming skills have any idea?

> 
> > 
> > @@ +89,3 @@
> > +    _isBubbleInitialized: function() {
> > +        // undefined means 'uninitialized' and null means 'without bubble'
> > +        return typeof this._bubble != 'undefined';
> > 
> > And this will return null if this._bubble is null?
> 
> Believe me, returns true.
> 
> print(typeof null); //prints "object"
> 

Righto.

(In reply to comment #38)
> Review of attachment 279138 [details] [review]:
> 
> Thanks looks great!
> 
> What exactly is the difference between a searchResultBubble and a poiBubble?
> https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/v2/markers-and-bubbles.png
> 
> The mockups seem very similar. So even tho they should not be exactly the same,
> maybe they should share a lot of code?
> So we might find a better name that SearchResultBubble?

Maybe we have a base class for those bubbles. Do we have the same information
if we got a place from Overpass compared to getting from Nominatim? Because
maybe the bubbles looks similar, but the implementations are different.
Anyways, we need to discuss it, it is obvious that we have shared code here.

> 
> (Not related, but when I looked at contextMenu I wondered if the
> reverseGeocode-method should move to GeocodeService, thoughts?)
> 

Keep features well separated is always a good idea :)

> ::: src/searchResultBubble.js
> @@ +83,3 @@
> +        this.add(ui.box);
> +    }
> +});
> 
> we need to add this file to POTFILES, right?
> 

Yes we need... my fault.

> ::: src/searchResultMarker.js
> @@ +35,3 @@
> +        this.parent(params);
> +
> +        let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR +
> "/pin.svg");
> 
> So this is the same pin as for userlocation?

There is a new icon for the userlocation.

Created attachment 279716 [details] [review]
Add MapMarker and MapBubble classes - v4

Finally, I put a comment in _getHotSpot.
Also clean imports, POTFILES.in and some other stuff.

Created attachment 279717 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v2

POTFILES.in!!!!!

Created attachment 280976 [details] [review]
Add MapMarker and MapBubble classes - v5

I'm resubmitting the patches to see if we can solve some applying problem
with Dario, and fix some trailing whitespaces in the process.

I don't know if the applying problem is due I rebased my branch to fix 
patch 1 and 3, but didn't submitted patch 2.

Created attachment 280978 [details] [review]
Refactor user location marker to support bubble - v5

Created attachment 280980 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v3

Some, Dario has started to use the markers and we have started to discover some issues.

Well, at the moment is just one but is critical for usability.


When user press on a marker, it shows the bubble, but when she press on another marker, another bubble is being show without hiding the previous bubble. Only one bubble must be show at a time. Also, when N bubbles are being showed it requires N clicks outside the bubbles to start gain focus on another window widget, that's not acceptable.

This is easy to solve when you have just one layer, because ChamplainMarkerLayer handles the selected property of the markers, so when you press on a marker, the previous selected marker is unselected, so we can "bind" the selected property with the visibility of the bubble and that's all.

The problem is when we have multiple layers, if we press a marker on a layer, the markers on the other layers won't be unselected. So can have many visible bubbles as markers of different layers we can press.

Possible solutions:

 - Before showing the bubble in showBubble method, emit a MapView or ChamplainView signal that other markers can listen and close the bubble when is triggered. This one is really simple, but, do you consider it ugly? How the signal should be called and from where?

 - Create a class that manage the markers bubble showing/hiding, like MapBubbleManager. Do you consider it unnecessary complex?


What do you think?

(In reply to comment #51)
> Some, Dario has started to use the markers and we have started to discover some
> issues.
> 
> Well, at the moment is just one but is critical for usability.
> 
> 
> When user press on a marker, it shows the bubble, but when she press on another
> marker, another bubble is being show without hiding the previous bubble. Only
> one bubble must be show at a time. Also, when N bubbles are being showed it
> requires N clicks outside the bubbles to start gain focus on another window
> widget, that's not acceptable.
> 
> This is easy to solve when you have just one layer, because
> ChamplainMarkerLayer handles the selected property of the markers, so when you
> press on a marker, the previous selected marker is unselected, so we can "bind"
> the selected property with the visibility of the bubble and that's all.
> 

We could also set the selection-mode property to SELECTION_MODE_SIGNLE:

    CHAMPLAIN_SELECTION_SINGLE:
	Only one marker can be selected.

> The problem is when we have multiple layers, if we press a marker on a layer,
> the markers on the other layers won't be unselected. So can have many visible
> bubbles as markers of different layers we can press.
> 
> Possible solutions:
> 
>  - Before showing the bubble in showBubble method, emit a MapView or
> ChamplainView signal that other markers can listen and close the bubble when is
> triggered. This one is really simple, but, do you consider it ugly? How the
> signal should be called and from where?
> 
>  - Create a class that manage the markers bubble showing/hiding, like
> MapBubbleManager. Do you consider it unnecessary complex?
> 

I think I do. We are adding a lot of classes as it is. I'd prefer to keep it simpler.

> 
> What do you think?

How about adding a "unselectAllMarkers()" method to the mapView? The method will
iterate over all layers annd call the unselect_all_markers() method. Then that mapView method could be called from showBubble?

And I guess that need to bind the visible/selected properties as well.

(In reply to comment #51)
> Some, Dario has started to use the markers and we have started to discover some
> issues.
> 
> Well, at the moment is just one but is critical for usability.
> 
> 
> When user press on a marker, it shows the bubble, but when she press on another
> marker, another bubble is being show without hiding the previous bubble. Only
> one bubble must be show at a time. Also, when N bubbles are being showed it
> requires N clicks outside the bubbles to start gain focus on another window
> widget, that's not acceptable.

Yes, I faced the exact same issue while implementing bubbles for POIs.
I had to manually close all the other POI Bubbles.

Clicking somewhere outside closed all the bubbles.

> 
> This is easy to solve when you have just one layer, because
> ChamplainMarkerLayer handles the selected property of the markers, so when you
> press on a marker, the previous selected marker is unselected, so we can "bind"
> the selected property with the visibility of the bubble and that's all.
> 
> The problem is when we have multiple layers, if we press a marker on a layer,
> the markers on the other layers won't be unselected. So can have many visible
> bubbles as markers of different layers we can press.
> 
> Possible solutions:
> 
>  - Before showing the bubble in showBubble method, emit a MapView or
> ChamplainView signal that other markers can listen and close the bubble when is
> triggered. This one is really simple, but, do you consider it ugly? How the
> signal should be called and from where?
> 
>  - Create a class that manage the markers bubble showing/hiding, like
> MapBubbleManager. Do you consider it unnecessary complex?
> 
> 
> What do you think?

(In reply to comment #52)
> (In reply to comment #51)
> > Some, Dario has started to use the markers and we have started to discover some
> > issues.
> > 
> > Well, at the moment is just one but is critical for usability.
> > 
> > 
> > When user press on a marker, it shows the bubble, but when she press on another
> > marker, another bubble is being show without hiding the previous bubble. Only
> > one bubble must be show at a time. Also, when N bubbles are being showed it
> > requires N clicks outside the bubbles to start gain focus on another window
> > widget, that's not acceptable.
> > 
> > This is easy to solve when you have just one layer, because
> > ChamplainMarkerLayer handles the selected property of the markers, so when you
> > press on a marker, the previous selected marker is unselected, so we can "bind"
> > the selected property with the visibility of the bubble and that's all.
> > 
> 
> We could also set the selection-mode property to SELECTION_MODE_SIGNLE:
> 
>     CHAMPLAIN_SELECTION_SINGLE:
>     Only one marker can be selected.
> 
> > The problem is when we have multiple layers, if we press a marker on a layer,
> > the markers on the other layers won't be unselected. So can have many visible
> > bubbles as markers of different layers we can press.
> > 
> > Possible solutions:
> > 
> >  - Before showing the bubble in showBubble method, emit a MapView or
> > ChamplainView signal that other markers can listen and close the bubble when is
> > triggered. This one is really simple, but, do you consider it ugly? How the
> > signal should be called and from where?
> > 
> >  - Create a class that manage the markers bubble showing/hiding, like
> > MapBubbleManager. Do you consider it unnecessary complex?
> > 
> 
> I think I do. We are adding a lot of classes as it is. I'd prefer to keep it
> simpler.
> 
> > 
> > What do you think?
> 
> How about adding a "unselectAllMarkers()" method to the mapView? The method
> will
> iterate over all layers annd call the unselect_all_markers() method. Then that
> mapView method could be called from showBubble?


I tried calling unselect_all_markers() for just the poiLayer but it hangs the system every time.
Instead something like this could be done for each layer:

    let markers = layer.get_selected(); // gets a list of selected markers
    markers.forEach(function(marker){
        marker.hideBubble();
    });

(In reply to comment #55)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > Some, Dario has started to use the markers and we have started to discover some
> > > issues.
> > > 
> > > Well, at the moment is just one but is critical for usability.
> > > 
> > > 
> > > When user press on a marker, it shows the bubble, but when she press on another
> > > marker, another bubble is being show without hiding the previous bubble. Only
> > > one bubble must be show at a time. Also, when N bubbles are being showed it
> > > requires N clicks outside the bubbles to start gain focus on another window
> > > widget, that's not acceptable.
> > > 
> > > This is easy to solve when you have just one layer, because
> > > ChamplainMarkerLayer handles the selected property of the markers, so when you
> > > press on a marker, the previous selected marker is unselected, so we can "bind"
> > > the selected property with the visibility of the bubble and that's all.
> > > 
> > 
> > We could also set the selection-mode property to SELECTION_MODE_SIGNLE:
> > 
> >     CHAMPLAIN_SELECTION_SINGLE:
> >     Only one marker can be selected.
> > 
> > > The problem is when we have multiple layers, if we press a marker on a layer,
> > > the markers on the other layers won't be unselected. So can have many visible
> > > bubbles as markers of different layers we can press.
> > > 
> > > Possible solutions:
> > > 
> > >  - Before showing the bubble in showBubble method, emit a MapView or
> > > ChamplainView signal that other markers can listen and close the bubble when is
> > > triggered. This one is really simple, but, do you consider it ugly? How the
> > > signal should be called and from where?
> > > 
> > >  - Create a class that manage the markers bubble showing/hiding, like
> > > MapBubbleManager. Do you consider it unnecessary complex?
> > > 
> > 
> > I think I do. We are adding a lot of classes as it is. I'd prefer to keep it
> > simpler.
> > 
> > > 
> > > What do you think?
> > 
> > How about adding a "unselectAllMarkers()" method to the mapView? The method
> > will
> > iterate over all layers annd call the unselect_all_markers() method. Then that
> > mapView method could be called from showBubble?
> 
> 
> I tried calling unselect_all_markers() for just the poiLayer but it hangs the
> system every time.

This stunds like a bug. A Nice oppurtunity to get to know the libchamplain codebase try to see what unselect_all_markera tries to do and why it fails. At least file a bug against champion.

> Instead something like this could be done for each layer:
> 
>     let markers = layer.get_selected(); // gets a list of selected markers
>     markers.forEach(function(marker){
>         marker.hideBubble();
>     });

Created attachment 281251 [details] [review]
Add MapMarker and MapBubble classes - v6

- goTo fixes on MapWalker: I moved the code of 
ensureLocationsVisible method from MapView to MapWalker since 
we are not using the code on any other side and seems to be an 
algorithm for MapWalker. And also fixed a little bug on it.
- Hide bubble when user starts to drag the marker, sadly 
Champlain selects the bubble on button press and not on 
button release, so we can see the bubble until user 
start drag, this can be solved using more low-level events 
(ClutterActor) to show the bubble, but I think that'd be .
- Only show one bubble at a time, is similar to the Rishi 
solution, that is, just hiding the bubble instead of 
unselecting: the marker that show the bubble listen on a 
MapView signal called "marker-selected" that hides the bubble 
when its triggered, another marker that is going to show the 
bubble will trigger that signal. I tried to unselect the 
marker instead of just hiding the bubble but when I change 
selected property to false, Champlain unselects all the other 
markers on the layer too and that will trigger the bubble 
hiding.
- Related to the previous item, I was showing the bubble when 
selected property changes, now I am showing/hiding the bubble 
when the marker got selected/unselected.
- Sync marker coordinates with place property coordinates, 
useful for dragged markers.

(In reply to comment #57)

Thanks!

> - Hide bubble when user starts to drag the marker, sadly 
> Champlain selects the bubble on button press and not on 
> button release, so we can see the bubble until user 
> start drag, this can be solved using more low-level events 
> (ClutterActor) to show the bubble, but I think that'd be .

^^^
Is this something that should be fixed in Champlain?


> - Only show one bubble at a time, is similar to the Rishi 
> solution, that is, just hiding the bubble instead of 
> unselecting: the marker that show the bubble listen on a 
> MapView signal called "marker-selected" that hides the bubble 
> when its triggered, another marker that is going to show the 
> bubble will trigger that signal. 

So I am not crazy about this solution. But I'm not sure what to do.

It would be nice if we could eliminate the need to explicitly call showBubble.
That the bubble is shown when the marker is selected. So that all we need to care about from mapView point of view is whether a marker is selected or not.
So the marker binds its selected property to the visibility property of its bubble. That would feel nice to me. Or have the marker listen to its selected signal and perform some actions when it goes true/false. Maybe do the emit thing on the mapView that you do.

> I tried to unselect the 
> marker instead of just hiding the bubble but when I change 
> selected property to false, Champlain unselects all the other 
> markers on the layer too and that will trigger the bubble 
> hiding.

Why is this? Has is something to do with the selection-mode property of the ChamplainMarkerLayer? Is it a bug? Should it be fixed?

(In reply to comment #58)
> (In reply to comment #57)
> 
> Thanks!
> 
> > - Hide bubble when user starts to drag the marker, sadly 
> > Champlain selects the bubble on button press and not on 
> > button release, so we can see the bubble until user 
> > start drag, this can be solved using more low-level events 
> > (ClutterActor) to show the bubble, but I think that'd be .
> 
> ^^^
> Is this something that should be fixed in Champlain?

I don't know if is necessarily a bug, the marker is selected when I start to drag it and I am listening the selected property to show/hide the bubble. Maybe it doesn't fit to my needs.

> 
> 
> > - Only show one bubble at a time, is similar to the Rishi 
> > solution, that is, just hiding the bubble instead of 
> > unselecting: the marker that show the bubble listen on a 
> > MapView signal called "marker-selected" that hides the bubble 
> > when its triggered, another marker that is going to show the 
> > bubble will trigger that signal. 
> 
> So I am not crazy about this solution. But I'm not sure what to do.
> 
> It would be nice if we could eliminate the need to explicitly call showBubble.
> That the bubble is shown when the marker is selected. So that all we need to
> care about from mapView point of view is whether a marker is selected or not.
> So the marker binds its selected property to the visibility property of its
> bubble. That would feel nice to me. Or have the marker listen to its selected
> signal and perform some actions when it goes true/false. Maybe do the emit
> thing on the mapView that you do.

That's what I'm doing (see the 4th fix and the MapMarker._onMarkerSelected method).

Also, I need to fix the patches to stop using showBubble explicitly from outside MapMarker and just use the selected property.

I'm also not crazy about this, it's like a hack, I explain it in detail below.

> 
> > I tried to unselect the 
> > marker instead of just hiding the bubble but when I change 
> > selected property to false, Champlain unselects all the other 
> > markers on the layer too and that will trigger the bubble 
> > hiding.
> 
> Why is this? Has is something to do with the selection-mode property of the
> ChamplainMarkerLayer? Is it a bug? Should it be fixed?

The goal is to have one bubble at any time, I have two cases:

 - Markers on the same layer: this is easily solvable show/hide bubble on marker select/unselect, ChamplainMarkerLayer single selection mode do the rest.

 - Markers on different layers: if I press a marker while another marker on another layer is selected, I need to unselect it to hide its bubble. Then I chose the "marker-selected signal" solution: when the marker got selected, emit the signal to unselect other markers, connect that signal and do "this.selected = false" when it's emitted by another marker. This collides with the previous case due to a Champlain bug: when I set the "selected" property to false to an unselected marker in single selection mode, Champlain unselects all the markers (just check champlain-marker-layer.c, marker_selected_cb function). So, when I select a marker while another marker in the SAME layer is selected, single selection mode unselects the second marker, but "marker-selected" signal is emitted, so it tries to unselect the second marker again and the first marker is unselected (Champlain bug), result, no bubble.

My solution to this was hide the bubble instead of unselect the marker when "marker-selected" is emitted, but now I'm thinking, is not the best one, maybe should be changed to one of this:

 1) Fix libchamplain
 2) Try to be smarter in Maps, don't unselect by hand a marker when another of the same layer is selected
 3) Do 1 and 2 <- I think this is the best one
 4) Use your solution, MapView::unselectAllMarkers method, I don't like this one, I think is not efficient and actually the method should accept a layer as parameter: we don't want to unselect all from the layer which selected marker belongs. That's like a weird method to have it in MapView class.


I hope you understand the above "spanglish" text mess xD

(In reply to comment #59)

> 
> My solution to this was hide the bubble instead of unselect the marker when
> "marker-selected" is emitted, but now I'm thinking, is not the best one, maybe
> should be changed to one of this:
> 
>  1) Fix libchamplain
>  2) Try to be smarter in Maps, don't unselect by hand a marker when another of
> the same layer is selected
>  3) Do 1 and 2 <- I think this is the best one
>  4) Use your solution, MapView::unselectAllMarkers method, I don't like this
> one, I think is not efficient and actually the method should accept a layer as
> parameter: we don't want to unselect all from the layer which selected marker
> belongs. That's like a weird method to have it in MapView class.
> 
> 
> I hope you understand the above "spanglish" text mess xD

No problems. Thanks for the comprehensive explanation and being patient with me :)

Yes I agree we should fix the libchamplain bug, it's nice to see the libraries we use as part of us. If we see a bug we should at least file a report, even if we end up doing work-arounds.


What I want to consider is a future need for 'permanent markers", we might want to put some kind of marker on the map that should remain on the map even tho other markers exist. So I guess it might be nice to have a way of having a marker that can override the behavior of being unselected when another marker is put on some layer. Am I making sense? It might already be easy with your solution. Just wanted to make sure.


(Btw, would it make sense to have some kind of super-layer class that we add our layers to and enforces common behaviors of the layers? It is probably over-engineering if there is no other behavior except this "only one marker" thing)

Review of attachment 281251 [details] [review]:

Some nits.

::: src/mapMarker.js
@@ +62,3 @@
+        // the location saved in the GeocodePlace
+        this.bind_property('latitude', this.place.location, 'latitude', 0);
+        this.bind_property('longitude', this.place.location, 'longitude', 0);

Instead of '0' could we use the actual flag name? GObject.BindingFlags.[...]

@@ +73,3 @@
+     * Returns: an array with the X and Y component of the point relative to
+     * the marker actor that should point to the specified coordinates in the
+     * map. [0, 0] represents the top-left corner of the marker actor.

This is one loong sentance. Could we split it up into several?

Created attachment 282940 [details] [review]
Add MapMarker and MapBubble classes - v7

- Fixes BindingFlags

- Add MapMarker::goToAndSelect method

- Remove addToLayer method from MapMarker, it will only used in
UserLocationMarker which is the only special case due to the
AccuracyCircleMarker. We are losing polymorfism capabilities when
adding markers by doing this, but is preferable to fix it in some other
way that only affects UserLocationMarker code.

- Keep showing bubble after zooming (this is implemented using
timeouts, we need to work in Champlain to get notified when zoom
animation finishes. TODO: markers shows over the sidebar if the new
position after zooming is over the sidebar.)

- Changes in the code that keeps one marker selected over all the
layers

Please test this along with latest Champlain master that includes

Created attachment 282941 [details] [review]
Refactor user location marker to support bubble - v6

- Use selected property instead of {show,hide}Bubble methods.

Created attachment 282942 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v4

- Use selected property instead of {show,hide}Bubble methods.

- Use goToAndSelect

- Fix bubble showing nothing when users select a recent place in the
search box.

(In reply to comment #62)
> Created an attachment (id=282940) [details] [review]
> Add MapMarker and MapBubble classes - v7
> 
> - Fixes BindingFlags
> 
> - Add MapMarker::goToAndSelect method
> 
> - Remove addToLayer method from MapMarker, it will only used in
> UserLocationMarker which is the only special case due to the
> AccuracyCircleMarker. We are losing polymorfism capabilities when
> adding markers by doing this, but is preferable to fix it in some other
> way that only affects UserLocationMarker code.
> 
> - Keep showing bubble after zooming (this is implemented using
> timeouts, we need to work in Champlain to get notified when zoom
> animation finishes. TODO: markers shows over the sidebar if the new
> position after zooming is over the sidebar.)
> 
> - Changes in the code that keeps one marker selected over all the
> layers
> 
> Please test this along with latest Champlain master that includes

This patches don't apply for me. Its not based on the latest commit.

Created attachment 283117 [details] [review]
Add MapMarker and MapBubble classes - v8

Rebase

Created attachment 283118 [details] [review]
Refactor user location marker to support bubble - v7

Rebase

Created attachment 283119 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v5

Rebase

Review of attachment 283117 [details] [review]:

This is a joint review by me and Mattias, cut short. More to come :)

::: src/mapMarker.js
@@ +49,3 @@
+        this._view = this._mapView.view;
+        this._bubble = undefined;
+        this._walker = undefined;

undefined values are implied, please remove the two lines above.

I understand that it's nice to see the member variables. But we do not do stuff like this anywhere else.
I might entertain a patch to do it maps-wide at the beginning of the 3.15 development cycle.

@@ +61,3 @@
+
+        // Some markers are draggable, with want to sync the marker location and
+        // the location saved in the GeocodePlace

'with' should be 'we' ?

@@ +63,3 @@
+        // the location saved in the GeocodePlace
+        this.bind_property('latitude', this.place.location, 'latitude', GObject.BindingFlags.DEFAULT);
+        this.bind_property('longitude', this.place.location, 'longitude', GObject.BindingFlags.DEFAULT);

Please split these up like:

    this.bind_property('latitude',
                           this.place.location, 'latitude',
                           GObject.BindingFlags.DEFAULT);

@@ +68,3 @@
+    _translateMarkerPosition: function() {
+        let [hsx, hsy] = this._getHotSpot();
+        this.set_translation(-hsx, -hsy, 0);

How about just having a getter for this, and avoid the _getHotSpot function.
There are not calculations needed for this really. So we could have a getter like:

get _anchor() {
    return { x: 0, y: 0 }
}

and then this becomes:

this.set_translations(-this._anchor.x,
                      -this._anchor.y,
                       0);

And we can remove the function below.

@@ +76,3 @@
+     * map. [0, 0] represents the top-left corner of the marker actor.
+     */
+    _getHotSpot: function() {

How about naming this _getAnchor? That seems consistent with how it is named in other libraries/applications.

And for the comment:

"The { x: x, y: y}

@@ +82,3 @@
+    _getBubbleSpacing: function() {
+        return 0;
+    },

Make this a getter, get bubbleSpacing()

@@ +89,3 @@
+
+    get bubble() {
+        if (!this._isBubbleInitialized())

This should be if (this._bubble === undefined) and then skip the isBubbleInitialized function.

@@ +108,3 @@
+        let [tx, ty, tz] = this.get_translation();
+        let [x, y] = [ this._view.longitude_to_x(this.longitude) + tx,
+                       this._view.latitude_to_y(this.latitude)   + ty ];

Just do this in two lines with let x = and let y = ...

@@ +114,3 @@
+                                           y: y - spacing,
+                                           width: this.width,
+                                           height: this.height + spacing * 2 });

Could you explain to the geometrically challenged people (like me), what adding the spacing * 2 accomplishes?

@@ +115,3 @@
+                                           width: this.width,
+                                           height: this.height + spacing * 2 });
+

Remove this empty line.

@@ +120,3 @@
+
+    showBubble: function() {
+        let bubble = this.bubble;

Just use this.bubble below.

@@ +122,3 @@
+        let bubble = this.bubble;
+
+        if (bubble !== null) {

Instead:

if (!bubble)
    return;

@@ +189,3 @@
+            bubble.show();
+        }
+    },

This function is way to big. And I think a lot of these signals are marker dependent and does not belong here. Please work on it. Might review with suggestions later on. Also should the bubble maybe be destroyed and not hidden?

@@ +198,3 @@
+
+        if (bubble !== null)
+            bubble.hide();

This function could just be:

if (this.bubble)
    bubble.hide();

@@ +228,3 @@
+    },
+
+    _onMarkerSelected: function() {

if (!this.bubble)
   return;

and then you do not need the hideBubble function, just use this.bubble.hide();

Review of attachment 283117 [details] [review]:

::: src/mapMarker.js
@@ +76,3 @@
+     * map. [0, 0] represents the top-left corner of the marker actor.
+     */
+    _getHotSpot: function() {

Do you mean call it get _anchor instead of _getAnchor?

BTW, do you prefer get _anchor or get anchor?

@@ +82,3 @@
+    _getBubbleSpacing: function() {
+        return 0;
+    },

Or maybe get _bubbleSpacing? (I think we should be consistent with the "anchor" property)

@@ +114,3 @@
+                                           y: y - spacing,
+                                           width: this.width,
+                                           height: this.height + spacing * 2 });

I apply spacing on the bottom and top of the marker actor because the bubble can adopt these positions depending on the window edges space. So the total height of the rectangle that the bubble will point to is:

actor.height + spacing_on_top + spacing_on_bottom

the spacing on both sides are the same, so total height is actually:

actor.height + spacing * 2.

@@ +120,3 @@
+
+    showBubble: function() {
+        let bubble = this.bubble;

Yeah, I tried to save the instructions of calling "get bubble" many times :)

@@ +122,3 @@
+        let bubble = this.bubble;
+
+        if (bubble !== null) {

I'm trying to distinguish between null and undefined in this case, conceptually I want to know if the marker supports a bubble instead of know if the bubble is not initialized.

@@ +189,3 @@
+            bubble.show();
+        }
+    },

Using Gtk+ and Clutter at the same time as I'm doing right now is more tricky than I expected, I'm trying to get the more natural UX with all these signaling, any suggestion will welcome.

Yes, the bubble can be destroyed I guess, but not here, maybe in the "closed" callback or in "hideBubble".

@@ +198,3 @@
+
+        if (bubble !== null)
+            bubble.hide();

I'm trying to not create a bubble if we don't need it.

@@ +228,3 @@
+    },
+
+    _onMarkerSelected: function() {

That won't work, calling this.bubble will create the bubble, this.bubble will always return an object (unless the marker doesn't support bubbles).

Also, Champlain notifies when a marker gets deselected, so, for example, if I have 300 markers in a layer and I select one, the remaining 299 will fire "notify::selected" signal to tell us that these were deselected, so we are creating 299 bubbles that we don't need.

I think the problem is that the bubble initialization is somewhat implicit.

Review of attachment 283117 [details] [review]:

Thx.

Could you also tell me if you prefer the mapWalker above just having the goto-function(s) on mapView, and if so, why?

::: src/mapMarker.js
@@ +76,3 @@
+     * map. [0, 0] represents the top-left corner of the marker actor.
+     */
+    _getHotSpot: function() {

Yes, get _anchor, as a property getter. I think get _anchor to imply it is protected (it is, right?) but it is not terribly important to me.

@@ +82,3 @@
+    _getBubbleSpacing: function() {
+        return 0;
+    },

Yeah, agreed.

@@ +114,3 @@
+                                           y: y - spacing,
+                                           width: this.width,
+                                           height: this.height + spacing * 2 });

Thx. Is a comment called for? It is already sort of a hack to point to a rectangle within mapView since we can't point it to a Clutter actor.

@@ +122,3 @@
+        let bubble = this.bubble;
+
+        if (bubble !== null) {

But the function below does not care about the difference here, right? But sure =>

if (bubble == null)
    return;

is fine as well, avoiding the extra indentation was my drift here.

@@ +189,3 @@
+            bubble.show();
+        }
+    },

Yeah. Moving the marker specific signals to _init or an _initSignals might shrink it a bit. And thinking about if the signals need to be disconnected in the closed callback. Maybe not if we destroy the bubble on hideBubble or closed.

Review of attachment 283118 [details] [review]:

::: src/user-location-bubble.ui
@@ +3,3 @@
+<interface>
+  <requires lib="gtk+" version="3.12"/>
+  <object class="GtkBox" id="box">

I think I prefer GtkGrid over GtkBox, we use Grid elsewhere in Maps. Same below.

::: src/userLocationMarker.js
@@ +81,3 @@
+        let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/user-location.png");
+        if (!iconActor)
+            return;

How could we ever catch this return? Failing in a constructor should not be possible. Remove these lines. If the file is not there it is an installation error we cannot recover from.

Review of attachment 283117 [details] [review]:

::: src/mapMarker.js
@@ +135,3 @@
+                if (this.get_parent() != closedMarker.get_parent())
+                    this.selected = false;
+            }).bind(this);

We usually do this like:

this._mapView.connect('marker-selected', ((function(mapView, closedMarker) {

[...]
}).bind(this));

@@ +157,3 @@
+            let callback = (function(mapView, closedMarker) {
+                this.selected = false;
+            }).bind(this);

Instead of opencoding this callback, couldn't we do:

this._view.connect('button-press-event',
                    this.set_selected.bind(false));

on the signals below?

Review of attachment 283119 [details] [review]:

::: src/mapView.js
@@ +171,1 @@
     },

I dislike this. Having to add a show/showNgoto for each new marker type.
It would be better, I think if we could just goto a marker, or if we could do marker.goto. The problem is with the layers I guess. Maybe a function addToLayer that takes a marker and a constant or something, or the addToLayer can determine which layer from the marker sent to it.

::: src/search-result-bubble.ui
@@ +2,3 @@
+<!-- Generated with glade 3.18.3 -->
+<interface>
+  <requires lib="gtk+" version="3.12"/>

Same comment about box => grid. There is even a note at the GtkBox documentation:
"Note that a single-row or single-column GtkGrid provides exactly the same functionality as GtkBox."

And since we use Grid elsewhere.

::: src/searchResultBubble.js
@@ +41,3 @@
+                                                             'image',
+                                                             'label-title' ]);
+

Remove this empty line.

Review of attachment 283117 [details] [review]:

::: src/mapMarker.js
@@ +122,3 @@
+        let bubble = this.bubble;
+
+        if (bubble !== null) {

Ahh... I get it now...

@@ +189,3 @@
+            bubble.show();
+        }
+    },

We will have many markers, I intentionally connect signals when bubble is only being show, connect these signals on marker constructor can overload the app (and I don't know if it will work just by moving the code anyways).

I need to explicit disconnect many signals since most are not directly related or connected to the bubble (indirectly yes, through marker deselection), so, it can't be disconnected just by destroying the bubble.

I think moving all the signaling stuff to another method _initBubbleSignals and destroying the bubble ("delete this._bubble;" I think) in "closed" signal is fine.

Review of attachment 283117 [details] [review]:

::: src/mapMarker.js
@@ +122,3 @@
+        let bubble = this.bubble;
+
+        if (bubble !== null) {

Ahh... I get it now...

@@ +189,3 @@
+            bubble.show();
+        }
+    },

We will have many markers, I intentionally connect signals when bubble is only being show, connect these signals on marker constructor can overload the app (and I don't know if it will work just by moving the code anyways).

I need to explicit disconnect many signals since most are not directly related or connected to the bubble (indirectly yes, through marker deselection), so, it can't be disconnected just by destroying the bubble.

I think moving all the signaling stuff to another method _initBubbleSignals and destroying the bubble ("delete this._bubble;" I think) in "closed" signal is fine.

Review of attachment 283119 [details] [review]:

::: src/mapView.js
@@ +171,1 @@
     },

Hmmm... yeah, this was pretty much of renaming methods, but I agree, it looks horrible.

Anyways, we don't need to have a show/showNGoto method for each kind of marker, only for this one, showSearchResult its just a helper method to update the search result marker (we wont have those methods for POIs or Turn points, that's a no sense).

We can simplify by just having "showSearchResult(place)", this method will add the marker, go to and select it (showing the bubble). We don't need showNGotoSearchResult and the showBubble parameter for now.

(In reply to comment #71)
> Could you also tell me if you prefer the mapWalker above just having the
> goto-function(s) on mapView, and if so, why?

I think MapView has pretty much non-related features in it, for example, it manages somewhat of routing, search results, user location, things that could be moved anywhere else (like layers classes), adding mapWalker code to that class it would be a mess, we will fall to a OOP antipattern having that class feature overloaded. Also, MapWalker code has many hacks that try to afford some limitations of Champlain that should be solved in the library, we obviously don't have time to fix it in Champlain for 3.14 (we didn't have time to fix it for 3.12, where this hacks are present in mapLocation.js). I prefer to have these hacks in the inherent "low level" stuff away from MapView, I higher level part of the app.

Review of attachment 283117 [details] [review]:

::: src/mapMarker.js
@@ +189,3 @@
+            bubble.show();
+        }
+    },

Ok, could you double check the difference between a normal GC'd object (through delete) and an explicit bubble.destroy() and see if we are fine with just the delete?

@@ +198,3 @@
+
+        if (bubble !== null)
+            bubble.hide();

Well at least

if (this._bubble)
     this._bubble.hide();

should be enough for this method.

Created attachment 283690 [details] [review]
Add MapMarker and MapBubble classes - v9

Created attachment 283691 [details] [review]
Refactor user location marker to support bubble - v8

Created attachment 283692 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble

Review of attachment 283690 [details] [review]:

Clicking the zoom-control while a Bubble is showing does not result in zoom taking place, can this be addressed?

::: src/mapMarker.js
@@ +76,3 @@
+     * Returns: the X and Y component of anchor point for the marker icon,
+     * relative the top left corner.
+     */

A point has an x and Y component. Just "The anchor point for the marker icon, relative to the top left corner", should be ok.

@@ +121,3 @@
+        // markers in the same layer, ChamplainMarkerLayer single selection mode
+        // does the job.
+        this._mapView.emit('marker-selected', this);

Doesn't this conceptually belong more in onSetMarkerSelected?

@@ +142,3 @@
+            }).bind(this));
+        }).bind(this));
+

All this cruft around the zoom is a bit over complicated and seems buggy for me. I think we should just destroy the bubble on zoom. At least for now.
Right now clicking the zoom-control gives different behavior from doing the zoom with ctrl-[+|-]. And this code seem very bug prone and a bit racy.

@@ +155,3 @@
+        signals.viewNotifySize = this._view.connect('notify::size',
+                                                    this._positionBubble.bind(this, this.bubble));
+

Do not align the assignments

@@ +168,3 @@
+            this._view.disconnect(signals.viewNotifySize);
+
+            this.bubble.destroy();

Shouldn't this be this._bubble ?

Review of attachment 283691 [details] [review]:

::: src/mapView.js
@@ +117,3 @@
     gotoUserLocation: function(animate) {
         this.emit('going-to-user-location');
+        Utils.once(this._userLocation, "gone-to", (function() {

Un-related change.

@@ +142,3 @@
                                                     location);
 
+        let previousSelected = this._userLocation && this._userLocation.selected;

If you do not feel strongly about this, I would prefer not to do the selected => previousSelected change.

::: src/userLocationMarker.js
@@ +88,3 @@
+                                                   this._accuracyMarker.refreshGeometry.bind(this._accuracyMarker));
+        } else
+            this._accuracyMarker = null;

No need for this else clause, just change below test to if (!this._accuracyMarker)

Review of attachment 283692 [details] [review]:

Some nits.

I feel we are soon ready to merge this series :)
Hang in there!

::: src/searchResultBubble.js
@@ +68,3 @@
+                break;
+
+            //TODO: add specific UIs for the rest of the place types

Maybe you could create a bug for this? With some details how to find out what placetypes there are and marking it gnome-love?

@@ +77,3 @@
+        ui.labelTitle.label = title;
+
+        for (let i in content) {

Should this be a forEach?

::: src/searchResultMarker.js
@@ +36,3 @@
+
+        let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg");
+        this.add_actor(iconActor);

Bit of a nit, but the variable is not really needed here.

Review of attachment 283691 [details] [review]:

::: src/mapView.js
@@ +117,3 @@
     gotoUserLocation: function(animate) {
         this.emit('going-to-user-location');
+        Utils.once(this._userLocation, "gone-to", (function() {

Yes, it is related, UserLocationMarker doesn't have "once" method.

@@ +142,3 @@
                                                     location);
 
+        let previousSelected = this._userLocation && this._userLocation.selected;

Why? if user selects the bubble and the location changes, the user will continue seeing the bubble as it moves (also, I didn't want to break the previous behaviour).

Created attachment 283836 [details] [review]
Make Sidebar to push MapView on reveal

Created attachment 283837 [details] [review]
Add MapMarker and MapBubble classes - v10

These are base classes intended to create markers associated with
content rich popovers.

Created attachment 283838 [details] [review]
Refactor user location marker to support bubble - v9

Created attachment 283839 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v7

Created attachment 283843 [details] [review]
Make Sidebar to push MapView on reveal - v2

Created attachment 283844 [details] [review]
Add MapMarker and MapBubble classes - v10

These are base classes intended to create markers associated with
content rich popovers.

Created attachment 283845 [details] [review]
Refactor user location marker to support bubble - v9

Created attachment 283846 [details] [review]
Replace MapLocation with SearchResultMarker,Bubble - v7

The latest patchset version fixes the following:

 - Bubbles are now modal: this fixes UX issues like the impossibility of interact with Gtk+ and zoom controls when bubble is visible.

 - The algorithm of update bubble position when the map view size changes is now more similar to the one that handle zoom level changes (hide when resizing, show when resizing finishes). This fixes bubbles glitches during map view resizes. Also fixes some bugs that causes incorrect position of the bubble.

 - Smart bubble positioning, taking into account the sidebar position, not only the window edge. This does not work fine when RTL is enabled and the bubble is near to the vertical edges of the window. I suppose this problem is because of this [1]

 - Now the sidebar and the map view are in a grid, this avoid the sidebar to superimpose the map view, pushing it to the left when the sidebar is revealed and viceversa. This fixes map content cut and helps to the bubble positioning.

 - Small code style fixes.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=735014

Review of attachment 283843 [details] [review]:

Thanks!

I would like a better commit message than this.

Suggestion: "Have sidebar push the map on reveal"

And something about why we want this in the body.

::: src/mainWindow.js
@@ +65,1 @@
 

Can't the grid be created in the ui file?

@@ +66,2 @@
         this.mapView = new MapView.MapView();
+        this._overlay.add(this.mapView);

Why this change?

@@ +87,3 @@
         this._overlay.add_overlay(new ZoomControl.ZoomControl(this.mapView));
+
+        grid.attach(this._overlay, 0, 0, 1, 1);

This could  be added in the ui file?

Review of attachment 283844 [details] [review]:

::: src/mapMarker.js
@@ +106,3 @@
+        let x = this._view.longitude_to_x(this.longitude);
+        let y = this._view.latitude_to_y(this.latitude);
+        let mapSize = this._mapView.get_allocation();

Wouldn't be nice to have a size method on the mapView? Even though it will just do the allocation thing?
Btw is this size different from the mapView.view.size property? Otherwise a mapView.size property could return that?

@@ +112,3 @@
+            bubble.hide();
+            return;
+        }

Do we imagine we will need to check this for other things as well? Does it make sense with a method on the mapView? like isOffMap?

@@ +125,3 @@
+            bubble.position = Gtk.PositionType.LEFT;
+        else if (x - bubble.get_preferred_size()[1].width / 2 <= 0)
+            bubble.position = Gtk.PositionType.RIGHT;

This would read a bit prettier if we add height/width properties to the bubble.

@@ +129,3 @@
+
+    _initBubbleSignals: function() {
+        let signals = {};

If the only reason to have this helper object is to avoid typing let then I think you should drop it.

@@ +136,3 @@
+        // markers in the same layer, ChamplainMarkerLayer single selection mode
+        // does the job.
+        this._mapView.onSetMarkerSelected();

Don't we need to pass the marker? The function below makes it seem that it is part of the emit.

@@ +141,3 @@
+            if (this.get_parent() != closedMarker.get_parent())
+                this.selected = false;
+        }).bind(this));

Would it be possible for mapView to handle this without us having to subscribe to this signal? Finding out what layer the marker is in and calling unselect_all_markers on the other layers?

@@ +143,3 @@
+        }).bind(this));
+
+

Two empty lines

@@ +171,3 @@
+                viewSizeTimeoutSourceId = null;
+            }).bind(this));
+        }).bind(this));

Ugh, I really do not think this is very pretty. And that "lets wait half a second for the zoom animation to complete" feels so wrong :)

First:
This function is way to big. Would it be possible to move the logic of hiding the bubble on resize and zoom to mapBubble.js?

Second:
I think we want to add an emit in Champlain. The animation-complete signal has the following description:

'The "animation-completed" signal is emitted when any animation in the view ends. This is a detailed signal. For example, if you want to be signaled only for go-to animation, you should connect to "animation-completed::go-to"'

But right now I only see it emitted when the go-to animation is completed. There is a static zoom_animation_completed function in champlain-view.c. I think it wants a  g_signal_emit_by_name (view, "animation-completed::zoom", NULL);

That might or might not simplify things. But it feels like a Right Thing To Add.

Third:
Is there a way to have the same logic for the re-size and the zoom? So that helper could be written. To mitigate the amount of hairy looking code we need here?

@@ +179,3 @@
+                                                       this.set_selected.bind(this, false));
+        signals.thisParentSet = this.connect('parent-set',
+                                             this.set_selected.bind(this, false));

Why is this needed?

@@ +181,3 @@
+                                             this.set_selected.bind(this, false));
+        signals.thisDragMotion = this.connect('drag-motion',
+                                              this.set_selected.bind(this, false));

This is needed even if the marker is not draggable?

@@ +212,3 @@
+            }
+        }).bind(this));
+    },

I do not feel comfortable commiting this while this function is so big. Please try to find way to split it up and/or send responsibilities to other modules.

::: src/mapView.js
@@ +186,3 @@
+
+    onSetMarkerSelected: function() {
+        this.emit('marker-selected');

We don't need to pass a marker to this emit?

Review of attachment 283845 [details] [review]:

Otherwise looks fine.

The log maybe could be better, maybe: "Convert user location to MapMarker"

And a short sentence in the log of the implications.

::: src/mapView.js
@@ +142,3 @@
                                                     location);
 
+        let previousSelected = this._userLocation && this._userLocation.selected;

I only meant the change from "selected" to "previousSelected" in my past comment :) But I do not feel strongly.

::: src/userLocationMarker.js
@@ +62,3 @@
+        let size = this.place.location.accuracy * 2 / metersPerPixel;
+
+        if ((view.width > 0 && view.height > 0) &&

Allowed values for a ClutterActor width/height properties are >= 0, so these checks are not needed.

Review of attachment 283845 [details] [review]:

Forgot to change the status.

Review of attachment 283846 [details] [review]:

Thanks, the same comment about log as in previous patch.

::: src/mapView.js
@@ +160,1 @@
+        this._searchResultLayer.add_marker(searchResultMarker);

The userLocationMarker hade the addToLayer method, do you think we should be consistent?
Even tho there is no auxiliary marker for this one.

::: src/search-result-bubble.ui
@@ +25,3 @@
+    </child>
+    <child>
+      <object class="GtkBox" id="box-right">

Grid :)

::: src/searchResultBubble.js
@@ +52,3 @@
+        if (this._isBrokenPlace(place)) {
+            // Fallback for places coming from PlaceStore
+            title = place.name;

The place name can be quite big. In the mockups only the proper name is included. Geocode has this problem that the name is really a display name, compounded by different properties of the place. And the name will differ depending on the amount of search results you request and get. Since it tries to only add what it needs to differentiate the result from other results. I've filed a bug about this some time a go but nothing has happened with it. A work around might be to only include the name up to the first comma. But that feels hacky. Thoughts?

I get this type of warnings in the log:

gtk_widget_size_allocate(): attempt to allocate widget with width -23 and height 129

(gnome-maps:7110): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to allocate widget with width -1 and height 456


[...]

Could you look into that as well?

Review of attachment 283843 [details] [review]:

::: src/mainWindow.js
@@ +65,1 @@
 

Yes, it can be created in the ui file, but the grid children must be added programmatically, we won't have access to the sidebar nor overlay in GtkBuilder.

@@ +66,2 @@
         this.mapView = new MapView.MapView();
+        this._overlay.add(this.mapView);

To be consistent, we are using this._overlay instead of overlay variable below.

@@ +87,3 @@
         this._overlay.add_overlay(new ZoomControl.ZoomControl(this.mapView));
+
+        grid.attach(this._overlay, 0, 0, 1, 1);

Nanay, the creation of this overlay it's a bit ugly because NotificationManager depends on it (see application.js), so, we cannot just create the overlay and add it through GtkBuilder.

Review of attachment 283844 [details] [review]:

::: src/mapMarker.js
@@ +106,3 @@
+        let x = this._view.longitude_to_x(this.longitude);
+        let y = this._view.latitude_to_y(this.latitude);
+        let mapSize = this._mapView.get_allocation();

A method to obtain the size in mapView? why over-encapsulate? get_allocation method is already quite simple to use.
MapView is a Gtk+ widget, it does not have a "size" property, I opted to use MapView to do size calculation because both the bubble and mapView are Gtk+ widgets, so it looks more consistent, using view.size property should be the same I think.

@@ +112,3 @@
+            bubble.hide();
+            return;
+        }

Yes, it could be moved to a method, but I don't imagine if it'd be useful, I think we can move it when we actually need it.

Changing the subject, this code should be moved to another method and called from "showBubble" since is unrelated to set the position of the bubble, but to check if the bubble should be shown, what do you think?

@@ +125,3 @@
+            bubble.position = Gtk.PositionType.LEFT;
+        else if (x - bubble.get_preferred_size()[1].width / 2 <= 0)
+            bubble.position = Gtk.PositionType.RIGHT;

Hmmm... I don't feel good by hacking the way to use Gtk+, maybe moving the bubble.get_preferred_size()[1] to a variable is enough to prettify the conditionals.

@@ +129,3 @@
+
+    _initBubbleSignals: function() {
+        let signals = {};

It doesn't avoid typing... anyways, if I drop this object, I will name the signal handlers as signalViewNotifySize, which is pretty much the same.

@@ +136,3 @@
+        // markers in the same layer, ChamplainMarkerLayer single selection mode
+        // does the job.
+        this._mapView.onSetMarkerSelected();

Ooops!

@@ +141,3 @@
+            if (this.get_parent() != closedMarker.get_parent())
+                this.selected = false;
+        }).bind(this));

Yes, it can be done, but it will look more complicated IMO

@@ +143,3 @@
+        }).bind(this));
+
+

Intentional, separator :)

@@ +171,3 @@
+                viewSizeTimeoutSourceId = null;
+            }).bind(this));
+        }).bind(this));

Using GtkPopover as map markers should feel the Champlain designer so wrong too, using GtkPopover brings more possibilities to make more useful UIs easier, but I think is not a way that Champlain developer has planned to use his library, so, hacks like these are expected.

First:
I can think about move some of the parts of these hacks to mapBubble, but the result can be two:
 - Have hacks in mapBubble and mapMarker
 - Have all the hacks in mapBubble
Both cases are worst or the same as the current state of this patch-set.

Second:
Yes, that can simplify the things but I cannot guarantee that it won't look hacky (anyways, I need to think about it a little bit more).

Third:
Let me think I little bit and I'll try to shrink it a little bit more.

Again... this feature needs a lot of interaction of between Champlain/Clutter and Gtk+ and also uses GtkPopover in many ways that Gtk+ developers didn't expected and also implements markers in a way that Champlain developers didn't expected. A lot of hacks are expected to get a natural and predictable UX (actually, there still issues, like bubble not reappearing in panning finish), having this hacks encapsulated in a single method is fine to me. I'll try to shrink it a bit, but I don't feel right scattering hacky code to other parts of the app and I'm sure that at this moment, I cannot makes this hacks to just disappear. I personally have priority to UX, if we can work together with Champlain to make this code less chaotic, perfect! for 3.14, I don't know if we have time to do it (more than implementing animation-completed::zoom signal).

@@ +179,3 @@
+                                                       this.set_selected.bind(this, false));
+        signals.thisParentSet = this.connect('parent-set',
+                                             this.set_selected.bind(this, false));

If the marker disappear or change of layer, I think the safer thing to do is to destroy the bubble.

@@ +181,3 @@
+                                             this.set_selected.bind(this, false));
+        signals.thisDragMotion = this.connect('drag-motion',
+                                              this.set_selected.bind(this, false));

Maybe, I don't know if a marker can have the requirements of suddenly becomes draggable when the bubble is visible.

@@ +212,3 @@
+            }
+        }).bind(this));
+    },

Read above.

::: src/mapView.js
@@ +186,3 @@
+
+    onSetMarkerSelected: function() {
+        this.emit('marker-selected');

Ooops bis!

Review of attachment 283846 [details] [review]:

::: src/mapView.js
@@ +160,1 @@
+        this._searchResultLayer.add_marker(searchResultMarker);

I prefer to think a way to get rid of addToLayer from UserLocationMarker.

::: src/search-result-bubble.ui
@@ +25,3 @@
+    </child>
+    <child>
+      <object class="GtkBox" id="box-right">

Hmmm... this is intentional, using GtkGrid here will unnecessarily complicate the code of the bubble class and I don't feel right with that.

::: src/searchResultBubble.js
@@ +52,3 @@
+        if (this._isBrokenPlace(place)) {
+            // Fallback for places coming from PlaceStore
+            title = place.name;

Yes, the name property in Geocode is a bit broken (Argentina name property is "Argentina, Argentina" :(), to fix this, what about word-wrap the title label?

Review of attachment 283843 [details] [review]:

::: src/mainWindow.js
@@ +65,1 @@
 

Yes, that I know but the grid itself should go in the UI file.

@@ +66,2 @@
         this.mapView = new MapView.MapView();
+        this._overlay.add(this.mapView);

Ok, consider it to go in a patch of its own then.

@@ +87,3 @@
         this._overlay.add_overlay(new ZoomControl.ZoomControl(this.mapView));
+
+        grid.attach(this._overlay, 0, 0, 1, 1);

Ah, right you are. Thanks.

Review of attachment 283844 [details] [review]:

::: src/mapMarker.js
@@ +112,3 @@
+            bubble.hide();
+            return;
+        }

Sounds sane, sort of if (this._inView()) this.showBubble() ?

@@ +125,3 @@
+            bubble.position = Gtk.PositionType.LEFT;
+        else if (x - bubble.get_preferred_size()[1].width / 2 <= 0)
+            bubble.position = Gtk.PositionType.RIGHT;

Or the bubble.get_preferred_size()[1].width / 2 statement to a variable that describes what it is.

@@ +129,3 @@
+
+    _initBubbleSignals: function() {
+        let signals = {};

I meant avoid typing 'let' :)

And I think you can call them:
zoomId, goingToId, etc where they come from is clear from the connect line and I do not think it will be confusing.
So please, drop the signals object.

@@ +141,3 @@
+            if (this.get_parent() != closedMarker.get_parent())
+                this.selected = false;
+        }).bind(this));

Ok.

@@ +171,3 @@
+                viewSizeTimeoutSourceId = null;
+            }).bind(this));
+        }).bind(this));

Yeah I get that. Please do what you can do make this pretty and easy to grok. And at the end of the day we will have to live with the Conflated Clear Cut Cluttering Clutter Conspiracy

@@ +181,3 @@
+                                             this.set_selected.bind(this, false));
+        signals.thisDragMotion = this.connect('drag-motion',
+                                              this.set_selected.bind(this, false));

Then maybe that kind of marker can do the special casing?

Review of attachment 283846 [details] [review]:

::: src/mapView.js
@@ +160,1 @@
+        this._searchResultLayer.add_marker(searchResultMarker);

Sure.

::: src/searchResultBubble.js
@@ +52,3 @@
+        if (this._isBrokenPlace(place)) {
+            // Fallback for places coming from PlaceStore
+            title = place.name;

Dunno, will have to consult UI people. Maybe we should just live with this and try to fix Geocode.

Review of attachment 283846 [details] [review]:

::: src/searchResultMarker.js
@@ +41,3 @@
+    get anchor() {
+        return { x: Math.floor(this.width / 2),
+                 y: this.height - 3 };

Btw, why this.height - 3? A bit of magic number feel over it.

Review of attachment 283846 [details] [review]:

::: src/searchResultMarker.js
@@ +41,3 @@
+    get anchor() {
+        return { x: Math.floor(this.width / 2),
+                 y: this.height - 3 };

The pin.svg image has a shadow and some margin at the bottom, so to correct positioning this image we need to use we need to move the image a little bit to the top.

Created attachment 283988 [details] [review]
Have sidebar push the map on reveal - v3

This avoid cuts of visible parts of the map and helps to
implement better GtkPopover positioning over the map view.

Created attachment 283989 [details] [review]
Add MapMarker and MapBubble classes - v11

These are base classes intended to create markers associated with
content rich GtkPopover based bubbles.

Created attachment 283990 [details] [review]
Convert UserLocation to UserLocationMarker,Bubble - v10

The user location marker is now based on MapMarker class,
supporting MapBubble to show a GtkPopover based UI for the
user location information. This removes UserLocation which is
based on MapLocation.

Created attachment 283991 [details] [review]
Convert MapLocation to SearchResultMarker,Bubble - v8

The search result marker now is based in the MapMarker and
MapBubble classes. This improves the UI of the information about
searched place, adapting it to the place type. Adding complete
support for most Geocode place types is a pending task.

This also removes MapLocation class.

Review of attachment 283988 [details] [review]:

This change causes a bunch of warnings like:

(gnome-maps:8757): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to allocate widget with width -1 and height 394

(gnome-maps:8757): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to allocate widget with width -23 and height 129

(gnome-maps:8757): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to allocate widget with width -1 and height 394

(gnome-maps:8757): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to allocate widget with width -23 and height 129

Which is very annoying.

Review of attachment 283989 [details] [review]:

Great job!
Ack.

Review of attachment 283990 [details] [review]:

Ack.

Review of attachment 283991 [details] [review]:

Ack.

Created attachment 284083 [details] [review]
Add MapMarker and MapBubble classes - v12

- Disable zoom animation non animated goTo

Review of attachment 283988 [details] [review]:

I don't see the warnings on my laptop and noone else sees them.
If the appear later on we will have to fix them then.

Review of attachment 284083 [details] [review]:

Ack.

Epic \o/ Well done!

Yaaaay!!! Thanks!!!

Review of attachment 283990 [details] [review]:

::: src/userLocationMarker.js
@@ +72,3 @@
+
+const UserLocationMarker = new Lang.Class({
+    Name: 'UserLocation',

Note to self: this should be 'UserLocationMarker'

Review of attachment 283991 [details] [review]:

::: src/mapView.js
@@ +163,1 @@
+        return searchResultMarker;

Note to self: This does not need to return?

(In reply to comment #88)
> Created an attachment (id=283837) [details] [review]
> Add MapMarker and MapBubble classes - v10

+   <property name="label" translatable="yes">Accuracy %s</property>

As a translator, what does the %s stand for and how to properly translate this? Also see https://wiki.gnome.org/TranslationProject/DevGuidelines/Use%20comments

Created attachment 284439 [details] [review]
Add MapMarker and MapBubble classes - v13

- _('...') -> _("...") in getAccuracyDescription
 - _(" km²") -> _("%f km²")

Created attachment 284440 [details] [review]
Convert UserLocation to UserLocationMarker,Bubble - v11

- Add comment to translators in "label-accuracy"
 - Fix UserLocationMarker's Name property

Review of attachment 284439 [details] [review]:

Ack.

Review of attachment 284440 [details] [review]:

Ack