Created attachment 277664 [details] [review]
routing: GraphHopper interface change for allowing viapoints

Change routing query and interface with GraphHopper service for
adding viapoints support.
Remove query properties to and from and add a new property viapoints
that is a coordinates array.
A query is legal if there are at least two viapoints.

(In reply to comment #0)
> There is already a branch dedicated to routing feature, but one of the issue
> with the current behaviour is that the people can’t add intermediate points
> (e.g going from point A to C passing through B).

Thanks for working on this. I know that this is your SoC project but I'd suggest to temporarily focus on helping Mattias finish his branch first. Just a suggestion.

(In reply to comment #2)
> (In reply to comment #0)
> > There is already a branch dedicated to routing feature, but one of the issue
> > with the current behaviour is that the people can’t add intermediate points
> > (e.g going from point A to C passing through B).
> 
> Thanks for working on this. I know that this is your SoC project but I'd
> suggest to temporarily focus on helping Mattias finish his branch first. Just a
> suggestion.

This was sent up for review per my suggestion actually. Dario is working on alternate routes in GraphHopper currently and also a bit on the viapoint stuff that only depends on the COMMIT_NOW status patches in the routing bug. I think that's fine for now. When there's actual blocking going on I think Dario should just take over from me, but until that happens (and I hope it doesn't) I still have some time to finish the remaining little bits.

Review of attachment 277664 [details] [review]:

Some questions and code comments.

::: src/mapLocation.js
@@ +136,3 @@
     },
 
     _onHereButtonClicked: function() {

This code is based off of a patch that actually isn't meant for inclusion in the routing bug. 
Until the new popovers or the sidebar has landed that code makes sense for debugging though I guess. My suggestion is that you split off the changes to this file in its own patch and don't publish it here. :)

::: src/routeQuery.js
@@ +52,3 @@
+                                              GObject.ParamFlags.READABLE |
+                                              GObject.ParamFlags.WRITABLE,
+                                              GObject.Object),

Is GObject.Object how you do this with lists? I thought you needed to create some wrapper class that inherited from a gobject to make this work?

@@ +71,3 @@
 
+    reset: function() {this._changeSignalId = this.connect('notify',
+                                            this.emit.bind(this, 'change'));

Why do you connect here?

@@ +78,3 @@
     setMany: function(obj) {
         this.disconnect(this._changeSignalId);
 

Why remove the comment?

@@ +80,3 @@
 
+        ["viapoints", "transportation"].forEach((function(prop) {
+            if (obj.hasOwnProperty(prop)){

Space before "{"

@@ +83,1 @@
                 this[prop] = obj[prop];

Will this work for the viapoints-property btw? Like of you set that to a js list will that actually work given that the property is a general GObject.Object?

@@ +83,2 @@
                 this[prop] = obj[prop];
+                }

This looks badly indented and why do you wrap this up in {} ?

::: src/routeService.js
@@ +58,3 @@
+            this._query.viapoints.forEach((function(viapoint){
+                viapoints.push(viapoint);
+            }).bind(this));

Why do you create a new array here?

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > There is already a branch dedicated to routing feature, but one of the issue
> > > with the current behaviour is that the people can’t add intermediate points
> > > (e.g going from point A to C passing through B).
> > 
> > Thanks for working on this. I know that this is your SoC project but I'd
> > suggest to temporarily focus on helping Mattias finish his branch first. Just a
> > suggestion.
> 
> This was sent up for review per my suggestion actually.

I wasn't talking about him putting up this bug or patches but priorities.

> Dario is working on
> alternate routes in GraphHopper currently and also a bit on the viapoint stuff
> that only depends on the COMMIT_NOW status patches in the routing bug.

That is fine. I've setup the dependency on routing bug so there shouldn't be any confusion.

> I think
> that's fine for now. When there's actual blocking going on I think Dario should
> just take over from me, but until that happens (and I hope it doesn't) I still
> have some time to finish the remaining little bits.

Its not about his development being blocked (although there is conflicts and waste of time as obvious from one of your review comments) but rather getting things done.

All I was saying was that it would be nice to get basic routing in first before we care about non-essential (although important) features on top.

(In reply to comment #5)
> (In reply to comment #3)
> > I think that's fine for now. When there's actual blocking going on I think Dario should
> > just take over from me, but until that happens (and I hope it doesn't) I still
> > have some time to finish the remaining little bits.
> 
> Its not about his development being blocked (although there is conflicts and
> waste of time as obvious from one of your review comments) but rather getting
> things done.

Hm yeah. I see what you mean. My thought was that we could work in parallel here, but since I'm struggling to both find time and energy it might make sense to hand this over to Dario soon.

There's so annoyingly little left and I would really like to get this finished for real (and by my own hand) but at some point you have to admit defeat I guess. 
I'll continue working on this for another week and hand all of the routing over to Dario if I'm not done by next Tuesday (when we have our weekly meetings). How does that sound?

> All I was saying was that it would be nice to get basic routing in first before
> we care about non-essential (although important) features on top.

Sure

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > I think that's fine for now. When there's actual blocking going on I think Dario should
> > > just take over from me, but until that happens (and I hope it doesn't) I still
> > > have some time to finish the remaining little bits.
> > 
> > Its not about his development being blocked (although there is conflicts and
> > waste of time as obvious from one of your review comments) but rather getting
> > things done.
> 
> Hm yeah. I see what you mean. My thought was that we could work in parallel
> here, but since I'm struggling to both find time and energy it might make sense
> to hand this over to Dario soon.

I don't think it has to be either you or Dario working on it. Make a list of things to be fixed and then give some of the tasks to Dario?

Review of attachment 277664 [details] [review]:

::: src/mapLocation.js
@@ +136,3 @@
     },
 
     _onHereButtonClicked: function() {

This was needed by me for having some tests... removing from this patch right now. :)

::: src/routeQuery.js
@@ +52,3 @@
+                                              GObject.ParamFlags.READABLE |
+                                              GObject.ParamFlags.WRITABLE,
+                                              GObject.Object),

I could create a wrapper class, but actually it works.
The point is that I should create a new class with just an array and some methods for pushing and take elements. Then I should also create a class like a java bean for a single viapoint.
I believed was too much noise... but if needed I could do it :)

@@ +71,3 @@
 
+    reset: function() {this._changeSignalId = this.connect('notify',
+                                            this.emit.bind(this, 'change'));

Fixed with last rebasing on your code.

@@ +78,3 @@
     setMany: function(obj) {
         this.disconnect(this._changeSignalId);
 

It's back. :)

@@ +80,3 @@
 
+        ["viapoints", "transportation"].forEach((function(prop) {
+            if (obj.hasOwnProperty(prop)){

Fixed.

@@ +83,1 @@
                 this[prop] = obj[prop];

Javascript magic... it just works :) 
As told before I could do it more complicated, it's like typify js...

@@ +83,2 @@
                 this[prop] = obj[prop];
+                }

Fixed

::: src/routeService.js
@@ +58,3 @@
+            this._query.viapoints.forEach((function(viapoint){
+                viapoints.push(viapoint);
+            }).bind(this));

Removed.

Created attachment 277792 [details] [review]
routing: GraphHopper interface change for allowing viapoints

Change routing query and interface with GraphHopper service for
adding viapoints support.
Remove query properties to and from and add a new property viapoints
that is a coordinates array.
A query is legal if there are at least two viapoints.

Created attachment 280590 [details] [review]
Updated GraphHopper interface for allowing viapoints

Changes to routing query and interface with GraphHopper service for
adding viapoints support.
Remove query properties to and from and add a new property viapoints
that is a placeWrapper array.
PlaceWrapper class is needed for handling the bindings among sidebar
entries and query viapoints.
A query is legal if each place inside placeWrapper is not null.

Created attachment 280591 [details] [review]
Updated GraphHopper interface for allowing viapoints

Changes to routing query and interface with GraphHopper service for
adding viapoints support.
Remove query properties to and from and add a new property viapoints
that is a placeWrapper array.
PlaceWrapper class is needed for handling the bindings among sidebar
entries and query viapoints.
A query is legal if each place inside placeWrapper is not null.

Created attachment 280592 [details] [review]
Update GraphHopper interface for allowing viapoints

Changes to routing query and interface with GraphHopper service for
adding viapoints support.
Remove query properties to and from and add a new property viapoints
that is a placeWrapper array.
PlaceWrapper class is needed for handling the bindings among sidebar
entries and query viapoints.
A query is legal if each place inside placeWrapper is not null.

Review of attachment 280592 [details] [review]:

Hi, thanks for the patches!

I do not like that wrapper class _at all_, please try to find a way of accomplishing this without it.
Also, must we think of the route as an array of via points? For me via points are the points between "from" and "to".
With this code routeQuery.viapoints is the way to get the whole route, right? For me it feels awkward, but that might just be me.

Also, please fixup the commit message some. Maybe the headline can be:
Add viapoint support to route service

And try to explain what and why in the log.

Thanks!

::: src/routeQuery.js
@@ +24,3 @@
 const GObject = imports.gi.GObject;
+const PlaceWrapper = imports.placeWrapper;
+const Application = imports.application;

Where is Application used?

@@ -26,3 @@
 const Lang = imports.lang;
 
-const Utils = imports.utils;

If this should be removed then do it in a separate patch with its own justification.

@@ +49,3 @@
+        'updated': { },
+        'viapoint-added': { param_types: [ PlaceWrapper.PlaceWrapper ] },
+        'viapoint-removed': { }

What are these signals and where are they used?

@@ +56,3 @@
+                                              '',
+                                              GObject.ParamFlags.READWRITE,
+                                              GObject.Object),

I kind of dislike this. In my view via points are the points on the route between the end and the start, hence the "via".

With this I guess from will always be this._viapoints[0] end to will be this._viapoints[-1]?

@@ +95,3 @@
+        //this.connect('notify', function(place,idx){
+        //    this._viapoints[idx]=place; //Change the place binded to idx
+        //});

Remove these comments.

::: src/routeService.js
@@ +56,3 @@
         this.query.connect('updated', (function() {
+            if ( ( this._query.viapoints[0].place != null ) && ( this._query.viapoints[1].place != null ) ) {
+

I don't understand this. Before the code checked to and from to see that we had a route. And no we check the first two points on the route? Why the first two? are we sure that viapoints[0|1] i always non-null? This might blow up otherwise, right?

::: src/sidebar.js
@@ +105,3 @@
+        let placeWrapper = new PlaceWrapper.PlaceWrapper(new GObject.Object,index);
+        entry.bind_property('place', placeWrapper, 'place', GObject.BindingFlags.BIDIRECTIONAL);
+

No, I will not have that placeWrapper object included. Why do you feel the need for it?
Only for the bind_property thing? If doing something so ugly is the price for doing a neat trick like bind_property than it doesn't feel worth it.

Created attachment 282207 [details] [review]
Changes to routing service interface for via points support

Changes to routing query and service for adding support to viapoints.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'viapoints' that is a placeWrapper array.
PlaceWrapper contains just a property called 'place' and it is needed for
bindings with the sidebar search entries.
RouteQuery allows to do all the basic operations on the via points
array (add, modify, remove).
A query is legal if the places inside the wrappers are not null.

Created attachment 282208 [details] [review]
Add via points support to sidebar

Replaced the sidebar form's grid with listBox
Removed the statical entries called 'to' and 'from'.
Using the new function called 'create_row' is possible to add new
row to the sidebar form.
Every row contains a label and a PlaceEntry, that is binded with
the PlaceWrapper.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 282209 [details] [review]
Add indications about distance and time in the sidebar

A row in the sidebar contains informations about the estimated time
for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 282210 [details] [review]
Changed path's width and color

The routing path now is blue and the width is setted to 5px.

Created attachment 282211 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This is kind of markers is showed as the new route is shown if
the turnpoint is a start, via or end.
The marker are draggable and on after dragging a new route is
calculated and shown.

Created attachment 282212 [details] [review]
On instraction selection show turnpoint marker

If an instruction is selected a signal is emitted.
For allowing this, route now extends GObject.Object
The signal is caught by the mainWindows and mapView show the turnpoint.

Created attachment 282213 [details] [review]
Small graphical changes to the sidebar

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.
Some other minor tweaks.

Created attachment 282214 [details] [review]
Changes to routing service interface for via points support

Changes to routing query and service for adding support to viapoints.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'viapoints' that is a placeWrapper array.
PlaceWrapper contains just a property called 'place' and it is needed for
bindings with the sidebar search entries.
RouteQuery allows to do all the basic operations on the via points
array (add, modify, remove).
A query is legal if the places inside the wrappers are not null.

Created attachment 282216 [details] [review]
Changes to routing service interface for via points support

Changes to routing query and service for adding support to viapoints.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'viapoints' that is a placeWrapper array.
PlaceWrapper contains just a property called 'place' and it is needed for
bindings with the sidebar search entries.
RouteQuery allows to do all the basic operations on the via points
array (add, modify, remove).
A query is legal if the places inside the wrappers are not null.

Created attachment 282217 [details] [review]
Add via points support to sidebar

Replaced the sidebar form's grid with listBox
Removed the statical entries called 'to' and 'from'.
Using the new function called 'create_row' is possible to add new
row to the sidebar form.
Every row contains a label and a PlaceEntry, that is binded with
the PlaceWrapper.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 282218 [details] [review]
Add indications about distance and time in the sidebar

A row in the sidebar contains informations about the estimated time
for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 282219 [details] [review]
Change path's width and color

The routing path now is blue and the width is setted to 5px.

Created attachment 282220 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This is kind of markers is showed as the new route is shown if
the turnpoint is a start, via or end.
The marker are draggable and on after dragging a new route is
calculated and shown.

Created attachment 282221 [details] [review]
On instruction selection show turnpoint marker

If an instruction is selected a signal is emitted.
For allowing this, route now extends GObject.Object
The signal is caught by the mainWindows and mapView show the turnpoint.

Created attachment 282222 [details] [review]
Small graphical changes to the sidebar

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.
Some other minor tweaks.

Created attachment 282224 [details] [review]
Changes to routing service interface for via points support

Changes to routing query and service for adding support to viapoints.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'viapoints' that is a placeWrapper array.
PlaceWrapper contains just a property called 'place' and it is needed for
bindings with the sidebar search entries.
RouteQuery allows to do all the basic operations on the via points
array (add, modify, remove).
A query is legal if the places inside the wrappers are not null.

Created attachment 282225 [details] [review]
Add via points support to sidebar

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that is binded with
the PlaceWrapper.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 282226 [details] [review]
Add indications about distance and time in the sidebar

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 282227 [details] [review]
Changed path's width and color

The routing path now is blue and the width is setted to 5px.

Created attachment 282228 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This kind of markers is showed as the new route is shown if the
turnpoint is a start, via or end.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 282229 [details] [review]
On instruction selection show turnpoint marker

If an instruction is selected, a signal is emitted.
For allowing this, route now extends GObject.Object.
The signal is caught by mainWindows and mapView shows the turnpoint.

Created attachment 282230 [details] [review]
Small graphical changes to the sidebar

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.
Some other minor tweaks.

Review of attachment 282219 [details] [review]:

Thx!

In the log, suggestion for headline: "Change route path width and color"

And "setted" to "set".

Review of attachment 282224 [details] [review]:

No I do not like that placeWrapper class, lets just use an array of places.

::: src/routeQuery.js
@@ +56,3 @@
+                                              '',
+                                              GObject.ParamFlags.READWRITE,
+                                              GObject.Object),

I dislike the name 'viapoints', what we get with this property is all the "points" of the route. So I would prefer 'points' or even 'route'. Viapoints is implied.

Review of attachment 282225 [details] [review]:

Thx!

::: src/sidebar.js
@@ +117,3 @@
+
+        let placeWrapper = new PlaceWrapper.PlaceWrapper();
+        entry.bind_property('place', placeWrapper, 'place', GObject.BindingFlags.BIDIRECTIONAL);

I really dislike the placeWrapper hack. Creating an object just to have an object with a "place" property so that we can use bind_property. I do not like it.

Maybe instead add a notify signal to the PlaceEntry 'place' property?

And do something like:
entry.connecy('notify::place', function() { query.addViapoint(entry.place) }):

@@ +130,3 @@
+        let rowGrid = new Gtk.Grid();
+
+        if (index == 0){

Space before bracket, and use ===

@@ +136,3 @@
+              rowGrid.attach(addViapointButton, 2, 0 , 1, 1);
+        } else {
+            if ((query._viapoints.length > 2) && (index != query._viapoints.length-1)){

Space before bracket, space around - sign, use !==

@@ +153,3 @@
+            let rowToRemove = ui.sidebarForm.get_row_at_index(index);
+            ui.sidebarForm.remove(rowToRemove);
+            query.removeViapoint(index-1);

Space before and after minus sign.

@@ +156,3 @@
+        }).bind(this));
+
+        ui.sidebarForm.insert(rowGrid, index+1);

Space before and after plus sign.

Review of attachment 282226 [details] [review]:

Thx!

::: src/sidebar.js
@@ +178,3 @@
                 this._instructionList.add(row);
             }).bind(this));
+            let labelledTime = "Estimated time " + Utils._timeToLabelledTime(route.time);

Needs the gettext thingie to indicate translatable.

_("Estimated time ")

::: src/utils.js
@@ +287,3 @@
+        let labelledDistance;
+        if (km > 0)
+            labelledDistance = km + " km";

If km is meant to be translated it should be _" km", otherwise we use single quotes. ' km'.

See: https://wiki.gnome.org/Projects/Gjs/StyleGuide#Translatable_Strings

Same above with hours and minutes and below with meters.

Review of attachment 282227 [details] [review]:

Forgot to obsolete the previous one? Same comments applies.

Created attachment 282446 [details] [review]
Changes to routing service interface for via points support

Changes to routing query and service for adding support to viapoints.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'viapoints' that is a placeWrapper array.
PlaceWrapper contains just a property called 'place' and it is needed for
bindings with the sidebar search entries.
RouteQuery allows to do all the basic operations on the via points
array (add, modify, remove).
A query is legal if the places inside the wrappers are not null.

Created attachment 282447 [details] [review]
Add via points support to sidebar

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that is binded with
the PlaceWrapper.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 282448 [details] [review]
Add indications about distance and time in the sidebar

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 282449 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This kind of markers is showed as the new route is shown if the
turnpoint is a start, via or end.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Review of attachment 282446 [details] [review]:

Thx!

Maybe the commit headline: "route: add support for via points" ?

And the log still contain references to placeWrapper, that needs to be updated.

::: src/routeService.js
@@ +65,3 @@
+            this._query.points.forEach(function(point) {
+                locations.push(point.location);
+            });

We use a map function below to do something similar to this, maybe this should be a map as well?
Not sure.

let locations = this._query.points.map(function(place) { return place.location });

Is this neater?

Review of attachment 282447 [details] [review]:

Thx!

Headline: "sidebar: Add via points support"

And remove placeWrapper references.

::: src/sidebar.js
@@ +119,3 @@
+        let index = query._points.length;
+
+        if (query._points.length < 2){

Space before bracket.

@@ +126,3 @@
+                rowGrid.attach(addPointButton, 2, 0 , 1, 1);
+            }
+            if (index === 1) {

else if?

@@ +148,3 @@
+        });
+
+        addPointButton.connect('clicked', (function(){

Space before bracket.

@@ +159,3 @@
+        }).bind(this));
+
+        if (query._points.length < 2){

Space before bracket.

Review of attachment 282448 [details] [review]:

Thx.

::: src/sidebar.js
@@ +188,3 @@
             }).bind(this));
+
+            let labelledTime = _("Estimated time ") + Utils._timeToLabelledTime(route.time);

You need to import gettext thingie:

import _ = imports.gettext.gettext;

Otherwise you get:
 Gjs-WARNING **: JS ERROR: Exception in callback for signal: update: ReferenceError: _ is not defined

I am having trouble appling these patches, could you please rebase to master and indicate what order the patches should go?

Review of attachment 282449 [details] [review]:

Thx!

::: src/mapView.js
@@ +103,3 @@
         this.view.add_layer(this._searchResultLayer);
 
+        this._destinationMarkerLayer = new Champlain.MarkerLayer();

Could you please explain to me why we need a marker layer for the destination markers. Why can't they live on the instructionMarkerLayer?

@@ +208,3 @@
+        let pointIndex = 0;
+        route.turnPoints.forEach(function(turnPoint) {
+            if (turnPoint.isDestination(turnPoint)){

Space before bracket! :)

::: src/turnPointBubble.js
@@ +28,3 @@
+const MapBubble = imports.mapBubble;
+const Utils = imports.utils;
+const _ = imports.gettext.gettext;

Unused?

::: src/turnPointMarker.js
@@ +31,3 @@
+const Application = imports.application;
+
+const _ = imports.gettext.gettext;

Unused?

@@ +72,3 @@
+    },
+
+    _onMarkerDrag: function(index){

...

Review of attachment 282229 [details] [review]:

Why do we need to extend from GObject?

Review of attachment 282447 [details] [review]:

::: src/sidebar.js
@@ +146,3 @@
+            let index = rowGrid.get_parent().get_index();
+            query.modifyPoint(entry.place, index - 1);
+        });

Before we had the bidirectional bind_property, so changes to the route changed the placeEntry as well. Does this affect us now that we do not have that bidirectional quality anymore?

Review of attachment 282230 [details] [review]:

::: data/gnome-maps.css
@@ +115,2 @@
 }
 

Space before bracket on all your changes in this file.

(In reply to comment #51)
> Review of attachment 282447 [details] [review]:
> 
> ::: src/sidebar.js
> @@ +146,3 @@
> +            let index = rowGrid.get_parent().get_index();
> +            query.modifyPoint(entry.place, index - 1);
> +        });
> 
> Before we had the bidirectional bind_property, so changes to the route changed
> the placeEntry as well. Does this affect us now that we do not have that
> bidirectional quality anymore?

Now, on dragging, it is not possible to update the placeEntry of the dragged marker. Any idea?

Created attachment 282466 [details] [review]
route: add support for via points

Changes to routing query and service for adding support to via points.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'points' that is a place array.
RouteQuery allows to do all the basic operations on the points
array (add, modify, remove).
A query is legal if the places are not null.

Created attachment 282467 [details] [review]
sidebar: add via points support

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that on change
modify the query properties 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 282468 [details] [review]
sidebar: add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 282469 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This kind of markers is showed as the new route is shown if the
turnpoint is a start, via or end.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 282470 [details] [review]
On instruction selection show turnpoint marker

If an instruction is selected, a signal is emitted.
For allowing this, route now extends GObject.Object.
The signal is caught by mainWindows and mapView shows the turnpoint.

Created attachment 282471 [details] [review]
Small graphical changes to the sidebar

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.
Some other minor tweaks.

Review of attachment 282229 [details] [review]:

It is needed for using TurnPoint as parameter when emitting a signal (sidebar.js @196).

(In reply to comment #48)
> I am having trouble appling these patches, could you please rebase to master
> and indicate what order the patches should go?

I just rebased on the master. Before applying my code you should apply the Damiàn patches concerning markers.

In details:
f02a50eb1e385ce2ac7580fc96024be57eb78ddd Add MapMarker and MapBubble classes
4b9041baaaad3b2b91980dd803695a4fe1004051 Refactor user location marker to support bubble
d7c55f5ba94eae49da92ff640787474b84160b5e Replace MapLocation with SearchResultMarker,Bubble
411ecd10832f7f4b10dc9bc7ea500a158079bef6 route: add support for via points
ea226dadfeedcc4f6486a32a76577238db5c3925 sidebar: add via points support
224d999119fa1cb7840ea8c5b29d11c7c54a6c7d sidebar: add indications about distance and time
22eedca64ee8b49c4cc63ff32915a3f11f0ccbf0 Change route path width and color
7473ae46805ea914cd751af165fdeaf30e2fa64e New turnpoint markers
ed36837d100e8309671a15dfbac958b3a5b7f209 On instruction selection show turnpoint marker
0ba70931f5358651302d5e574b232583ea19752f Small graphical changes to the sidebar

Review of attachment 282466 [details] [review]:

This patch seem corrupted somehow?

::: src/routeService.js
@@ +59,3 @@
+                if ( this._query.points[i] == null ){
+                    return;
+                }

for (let point in this._query.points) {
    if (!point)
        return;
}

@@ +65,3 @@
+            this._query.points.forEach(function(point) {
+                locations.push(point.location);
+            });

This could be a map function, right?

@@ +95,3 @@
+        //let coordinates = points.map(function({ latitude, longitude }) {
+        //    return [latitude, longitude].join(',');
+        //});

What has happend here?

Review of attachment 282467 [details] [review]:

Thanks!

::: src/sidebar.js
@@ +112,3 @@
+                                                width_request: 220,
+                                                mapView:       mapView });
+

Don't align these properties.

@@ +136,3 @@
+            rowGrid.attach(removePointButton, 2, 0, 1, 1);
+            rowGrid.show_all();
+        }

Maybe it would simpler to not have the from/to/via/labels. Just have a start icon at the first and a finish icon at the last? Same symbols or similar as when we show the route?

@@ +140,3 @@
+        query.connect('reset', (function() {
+            entry.set_text('');
+        }).bind(this));

No need to bind to this if you do not access any member variables in the block.

@@ +156,3 @@
+            ui.sidebarForm.remove(rowToRemove);
+            query.removePoint(index - 1);
+        }).bind(this));

Same here the bind is not necessary.

@@ +162,3 @@
+        } else {
+            ui.sidebarForm.insert(rowGrid, index);
+        }

No need for brackets when there are only one statement.

::: src/sidebar.ui
@@ +120,3 @@
         </child>
         <child>
+          <object class="GtkBox" id="sidebar-route-info">

Please use GtkGrid instead of GtkBox

Review of attachment 282468 [details] [review]:

::: src/sidebar.js
@@ +223,3 @@
+        if (this.turnPoint.distance > 0) {
+            let labelledDistance = Utils._distanceToLabelledDistance(this.turnPoint.distance);
+            ui.distanceLabel.label = labelledDistance;

Maybe do this in one statement, and then remove the brackets as well.

::: src/utils.js
@@ +265,3 @@
 }
+
+function _timeToLabelledTime(time) {

prettyTime?

@@ +280,3 @@
+}
+
+function _distanceToLabelledDistance(distance) {

prettyDistance?

Review of attachment 282469 [details] [review]:

::: src/mapView.js
@@ +108,3 @@
+
+        this._userLocationLayer = new Champlain.MarkerLayer();
+        this._userLocationLayer.set_selection_mode(Champlain.SelectionMode.SINGLE);

You have mangled this patch some, or mis-merged it. See the diff how the userLocation is supposed to be declared, and do the same for the instructionMArkerLayer, you can set the selection_mode in the constructor.

@@ +173,3 @@
+        turnPointMarker.goTo(true);
+    },
+

See my review of Damians patches, I do not like having to add a function to mapView for every marker type we come up with. This could change when Damians patches change.

@@ +229,3 @@
+
+    _createTurnPointMarker: function(turnPoint, pointIndex, draggable) {
+        let route = Application.routeService.route;

Where is the route used?

::: src/route.js
@@ +23,3 @@
 const Lang = imports.lang;
 const Champlain = imports.gi.Champlain;
+const TurnPointMarker = imports.turnPointMarker;

Where is TurnPointMarker used?

::: src/turnPointMarker.js
@@ +47,3 @@
+        this.parent(params);
+
+        this.connect('drag-finish', (function(){

Space before braces.

@@ +53,3 @@
+        let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg");
+        if (!iconActor)
+            return;

Remove this, we can't handle this failing.

@@ +71,3 @@
+
+    _onMarkerDrag: function(index) {
+        let query = Application.routeService._query;

Use the property getter. routeService.query. Member variables starting with _ are private.

Review of attachment 282466 [details] [review]:

::: src/routeService.js
@@ +59,3 @@
+                if ( this._query.points[i] == null ){
+                    return;
+                }

I'm trying it but it doesn't work :(

@@ +65,3 @@
+            this._query.points.forEach(function(point) {
+                locations.push(point.location);
+            }

I refactored a bit the code and now I don't need locations anylong :)

@@ +95,3 @@
+        //let coordinates = points.map(function({ latitude, longitude }) {
+        //    return [latitude, longitude].join(',');
+        //});

Ops... removed

Review of attachment 282466 [details] [review]:

::: src/routeService.js
@@ +59,3 @@
+                if ( this._query.points[i] == null ){
+                    return;
+                }

I'm trying it but it doesn't work :(

@@ +65,3 @@
+            this._query.points.forEach(function(point) {
+                locations.push(point.location);
+            }

I refactored a bit the code and now I don't need locations anylong :)

@@ +95,3 @@
+        //let coordinates = points.map(function({ latitude, longitude }) {
+        //    return [latitude, longitude].join(',');
+        //});

Ops... removed

Review of attachment 282467 [details] [review]:

::: src/sidebar.js
@@ +112,3 @@
+                                                width_request: 220,
+                                                mapView:       mapView });
+                                    halign:        Gtk.Align.END,

why? what do you mean?

@@ +136,3 @@
+            rowGrid.attach(removePointButton, 2, 0, 1, 1);
+            rowGrid.show_all();
+        let removePointButton = Gtk.Button.new_from_icon_name("list-remove-symbolic", 1);

Yeah it would be great! Waiting for designing and most of all icons. ;)

@@ +140,3 @@
+        query.connect('reset', (function() {
+            entry.set_text('');
+        let index = query._points.length;

Fixed.

@@ +156,3 @@
+            ui.sidebarForm.remove(rowToRemove);
+            query.removePoint(index - 1);
+            } else if (index === 1) {

Fixed.

@@ +162,3 @@
+        } else {
+            ui.sidebarForm.insert(rowGrid, index);
+                rowGrid.attach(entry, 1, 0, 1, 1);

Fixed.

::: src/sidebar.ui
@@ +120,3 @@
         </child>
         <child>
+          <object class="GtkBox" id="sidebar-route-info">

Fixed.

Review of attachment 282468 [details] [review]:

::: src/sidebar.js
@@ +223,3 @@
+        if (this.turnPoint.distance > 0) {
+            let labelledDistance = Utils._distanceToLabelledDistance(this.turnPoint.distance);
+            ui.distanceLabel.label = labelledDistance;

Fixed.

::: src/utils.js
@@ +265,3 @@
 }
+
+function _timeToLabelledTime(time) {

Fixed.

@@ +280,3 @@
+}
+
+

Fixed.

Created attachment 283598 [details] [review]
route: add support for via points

Changes to routing query and service for adding support to via points.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'points' that is a place array.
RouteQuery allows to do all the basic operations on the points
array (add, modify, remove).
A query is legal if the places are not null.

Created attachment 283599 [details] [review]
sidebar: add via points support

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that on change
modify the query properties 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 283600 [details] [review]
sidebar: add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 283601 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This kind of markers is showed as the new route is shown if the
turnpoint is a start, via or end.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Review of attachment 282470 [details] [review]:

Thx!

::: src/mainWindow.js
@@ +73,3 @@
+        this._sidebar.connect('instruction-selected', (function(sidebar, turnPoint) {
+            this.mapView.showTurnPoint(turnPoint);
+        }).bind(this));

Could this be done in sidebar.js?

::: src/mapView.js
@@ +109,3 @@
+        this._instructionMarkerLayer = new Champlain.MarkerLayer();
+        this._instructionMarkerLayer.set_selection_mode(Champlain.SelectionMode.SINGLE);
+        this.view.add_layer(this._instructionMarkerLayer);

Is this really based on the master branch? We do the setting of selection-mode property in the constructor master AFAIK.

::: src/sidebar.js
@@ +191,3 @@
                 this._instructionList.add(row);
+                this._instructionList.connect('row-activated',(function(listbox, row) {
+                   this.emit('instruction-selected', row.turnPoint);

Couldn't this just call the showTurnpoint method on mapView? Why go through signals?

Review of attachment 283598 [details] [review]:

::: src/routeQuery.js
@@ +23,3 @@
 const Geocode = imports.gi.GeocodeGlib;
 const GObject = imports.gi.GObject;
+const Application = imports.application;

Where is Application used?

::: src/routeService.js
@@ +57,3 @@
         this.query.connect('updated', (function() {
+            for (let i = 0; i < this._query.points.length; i++) {
+                 if ( this._query.points[i] == null )

===

No spaces around the expression.

Review of attachment 283600 [details] [review]:

::: src/utils.js
@@ +265,3 @@
 }
+
+function _prettyTime(time) {

No underscore.

@@ +280,3 @@
+}
+
+function _prettyDistance(distance) {

No underscore.

Review of attachment 283601 [details] [review]:

::: src/mapView.js
@@ +209,3 @@
         });
 
+        this._showDestinationTurnpoint();

Why is this needed?

@@ +232,3 @@
+        let place = Geocode.Place.new_with_location("turn-point",
+                                                    Geocode.PlaceType.UNKNOWN,
+                                                    location);

If the place name is never to be shown then you could create this like:

let place = new Geocode.Place({ location: location });

And you could create the location when setting the property to avoid the temporary variable.

::: src/turnPointMarker.js
@@ +74,3 @@
+        let place = Geocode.Place.new_with_location("Custom place",
+                                                    Geocode.PlaceType.UNKNOWN,
+                                                    location);

If the place name is never to be shown then you could create this like:

let place = new Geocode.Place({ location: location });

Review of attachment 283601 [details] [review]:

::: src/mapView.js
@@ +166,3 @@
 
+    showTurnPoint: function(turnPoint) {
+        this._showDestinationTurnpoint();

Why does this need to be called for every turnpoint?

Review of attachment 283598 [details] [review]:

::: src/routeQuery.js
@@ +23,3 @@
 const Geocode = imports.gi.GeocodeGlib;
 const GObject = imports.gi.GObject;
+const Application = imports.application;

Fixed.

::: src/routeService.js
@@ +57,3 @@
         this.query.connect('updated', (function() {
+            for (let i = 0; i < this._query.points.length; i++) {
+                 if ( this._query.points[i] == null )

Both fixed.

Review of attachment 283600 [details] [review]:

::: src/utils.js
@@ +265,3 @@
 }
+
+function _prettyTime(time) {

Fixed.

@@ +280,3 @@
+}
+
+

Fixed.

Review of attachment 283601 [details] [review]:

::: src/mapView.js
@@ +166,3 @@
 
+    showTurnPoint: function(turnPoint) {
+        this._showDestinationTurnpoint();

Probably I found a fix. Posting a new patch soon.

@@ +209,3 @@
         });
 
+        this._showDestinationTurnpoint();

Fixed.

@@ +232,3 @@
+        let place = Geocode.Place.new_with_location("turn-point",
+                                                    Geocode.PlaceType.UNKNOWN,
+            }

Fixed.

::: src/turnPointMarker.js
@@ +74,3 @@
+        let place = Geocode.Place.new_with_location("Custom place",
+                                                    Geocode.PlaceType.UNKNOWN,
+                                                    location);

Fixed and created the location when setting the property to avoid the temporary variable. :)

Review of attachment 282470 [details] [review]:

::: src/mainWindow.js
@@ +73,3 @@
+        this._sidebar.connect('instruction-selected', (function(sidebar, turnPoint) {
+            this.mapView.showTurnPoint(turnPoint);
+        }).bind(this));

Fixed.

::: src/mapView.js
@@ +109,3 @@
+        this._instructionMarkerLayer = new Champlain.MarkerLayer();
+        this._instructionMarkerLayer.set_selection_mode(Champlain.SelectionMode.SINGLE);
+        this.view.add_layer(this._instructionMarkerLayer);

should be fixed now. :)
I also put this code in the new turnpointMarker patch.

::: src/sidebar.js
@@ +191,3 @@
                 this._instructionList.add(row);
+                this._instructionList.connect('row-activated',(function(listbox, row) {
+                   this.emit('instruction-selected', row.turnPoint);

I tried it for a while, but it doesn't work.
For now I'll leave it as it is and I will try to change it later.

Created attachment 283741 [details] [review]
route: add support for via points

Changes to routing query and service for adding support to via points.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'points' that is a place array.
RouteQuery allows to do all the basic operations on the points
array (add, modify, remove).
A query is legal if the places are not null.

Created attachment 283744 [details] [review]
sidebar: add via points support

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that on change
modify the query properties 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 283750 [details] [review]
sidebar: add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 283756 [details] [review]
New turnpoint markers

There are two new classes for managing turnpoint's marker and bubble.
This kind of markers is showed as the new route is shown if the
turnpoint is a start, via or end.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 283757 [details] [review]
On instruction selection show turnpoint marker

If an instruction is selected, a signal is emitted.
For allowing this, route now extends GObject.Object.
The signal is caught by mainWindows and mapView shows the turnpoint.

Created attachment 283759 [details] [review]
route: add support for via points

Changes to routing query and service for adding support to via points.
In RouteQuery there are no more the query properties 'to' and 'from'
and there is a new property 'points' that is a place array.
RouteQuery allows to do all the basic operations on the points
array (add, modify, remove).
A query is legal if the places are not null.

Created attachment 283760 [details] [review]
sidebar: add via points support

Replace the sidebar form's grid with listBox
Remove the statical entries called 'to' and 'from'.
Using the new function called 'create_row', it is possible to
add a new row to the sidebar form.
Every row contains a label and a PlaceEntry, that on change
modify the query properties 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Review of attachment 283750 [details] [review]:

Looks fine!

Review of attachment 283756 [details] [review]:

Please try to improve the log message a bit as well. The headline needs some love.

::: src/mapView.js
@@ +112,3 @@
     _connectRouteSignals: function(route) {
         route.connect('update', this.showRoute.bind(this, route));
+        route.connect('reset', (function(){

Space before bracket. Come on, please check this before sending patches, maybe use jslint or something.

@@ +165,3 @@
 
+    showTurnPoint: function(turnPoint) {
+        this._instructionMarkerLayer.get_markers().forEach((function(marker){

Space before bracket.

@@ +166,3 @@
+    showTurnPoint: function(turnPoint) {
+        this._instructionMarkerLayer.get_markers().forEach((function(marker){
+            if (marker._index === -1)

Here we are accessing a property marked as private, do not do that. And if its public do a getter for it in the marker class. And consider if index is the correct name.

@@ +180,3 @@
 
+        searchResultMarker.addToLayer(this._searchResultLayer);
+

Remove this empty line.

@@ +212,3 @@
+        let pointIndex = 0;
+        this._instructionMarkerLayer.remove_all();
+        route.turnPoints.forEach(function(turnPoint) {

Do we need to loop, isn't the destination always the last one?
so destination = route.turnPoints[route.turnPoints.length - 1] ?

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

Grid

::: src/turnPointBubble.js
@@ +48,3 @@
+        
+        ui.image.resource = icon;
+

Remove empty line

::: src/turnPointMarker.js
@@ +69,3 @@
+        let query = Application.routeService.query;
+        let place = new Geocode.Place({ location: new Geocode.Location({ latitude: this.latitude,
+                                                                         longitude: this.longitude }) });

Will this line be shorter if you format as:

new Geocode.Place({
    location: [...]

?

Review of attachment 283757 [details] [review]:

Thanks!

Maybe:
routing: Show turnpoint marker on instruction selection

And maybe the message body is unnecessary. Please check the body on all your patches and try to see if writing about the implementation details really is needed.

Review of attachment 283757 [details] [review]:

Is turning the route to a GObject still needed?

::: src/sidebar.js
@@ +197,3 @@
+                this._instructionList.connect('row-activated',(function(listbox, row) {
+                   this.emit('instruction-selected', row.turnPoint);
+                }).bind(this));

What did not work with doing the showTurnPoint method here? Instead of the emit?

Review of attachment 283759 [details] [review]:

route: Add support for via points.

Please work on the message body.

Also, please try to ask Mattias if he agrees with the property name "points" on the route.

::: src/routeService.js
@@ +60,3 @@
+                    return;
+            }
+

Remove empty line.

Review of attachment 283760 [details] [review]:

sidebar: Add via points support

And the message more about what and less about implementation details.

::: src/sidebar.js
@@ +68,3 @@
 
+        ui = this._createRow(ui);
+        ui = this._createRow(ui);

This looks odd, overwriting the variable you just assigned. Do you really need to return and assign ui? Why?

Can't the original two boxes be created in ui file and this function just used to add extra rows?

@@ +120,3 @@
+
+        let addPointButton = Gtk.Button.new_from_icon_name("list-add-symbolic", 1);
+

Remove empty line.

@@ +143,3 @@
+            rowGrid.show_all();
+        }
+

Same comment about using icons. What exactly is holding this up?

@@ +146,3 @@
+        query.connect('reset', (function() {
+            entry.set_text('');
+        }));

Remove extra parenthesis.

@@ +162,3 @@
+            ui.sidebarForm.remove(rowToRemove);
+            query.removePoint(index - 1);
+        }));

Remove extra parenthesis.

Review of attachment 283759 [details] [review]:

I changed the message body and Mattias agreed with me about the property name.

::: src/routeService.js
@@ +60,3 @@
+                    return;
+            }
+

Fixed.

Created attachment 283874 [details] [review]
route: add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 283875 [details] [review]
route: add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Review of attachment 283875 [details] [review]:

::: src/routeService.js
@@ +57,3 @@
         this.query.connect('updated', (function() {
+            for (let i = 0; i < this._query.points.length; i++) {
+                if (typeof(this._query.points[i]) === 'undefined')

if (this._query.points[i] === undefined) should be ok.

Review of attachment 283875 [details] [review]:

nit: "add support [...]" = "Add support [...]" in message header

::: src/routeQuery.js
@@ +132,1 @@
     },

Looking closer, does anything actually use setMany anymore?

Created attachment 283876 [details] [review]
route: add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 283878 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Review of attachment 283878 [details] [review]:

Almost there! :)

::: src/routeQuery.js
@@ +23,3 @@
 const Geocode = imports.gi.GeocodeGlib;
 const GObject = imports.gi.GObject;
+const Utils = imports.utils;

Move this to under const Lang.
So we difer from our own modules and the libraries.

@@ +68,3 @@
     },
+    set points(points) {
+        this._points = points;

Shouldn't we have a this.notify('points'); here?

And while you are at it, change all the other notify calls from double(") to single(') quotes.

Review of attachment 283760 [details] [review]:

::: src/sidebar.js
@@ +68,3 @@
 
+        ui = this._createRow(ui);
+        ui = this._createRow(ui);

How can I use PlaceEntry and refer to this.mapView form the ui file?

Review of attachment 283760 [details] [review]:

::: src/sidebar.js
@@ +68,3 @@
 
+        ui = this._createRow(ui);
+        ui = this._createRow(ui);

You can't so I guess that is not possible. Just feels so hacky to call that twice to get the original two boxes. Think about how it can improve.

Review of attachment 283760 [details] [review]:

::: src/sidebar.js
@@ +143,3 @@
+            rowGrid.show_all();
+        }
+        let removePointButton = Gtk.Button.new_from_icon_name("list-remove-symbolic", 1);

This is needed if we don't find a better solution for "from" and "to" rows.

Review of attachment 283760 [details] [review]:

::: src/sidebar.js
@@ +143,3 @@
+            rowGrid.show_all();
+        }
+

I think I want an icon on the first and one on the last regardless. Can we use the same as the ones indicating start/end in the list or on the map? It would help clear up the mess that these nested conditionals make as well.

Please try to accomplish this.

Review of attachment 283760 [details] [review]:

::: src/sidebar.js
@@ +120,3 @@
+
+        let addPointButton = Gtk.Button.new_from_icon_name("list-add-symbolic", 1);
+                                    margin_right:  5,

Fixed.

@@ +143,3 @@
+            rowGrid.show_all();
+        }
+        let removePointButton = Gtk.Button.new_from_icon_name("list-remove-symbolic", 1);

Done. I used the instruction icon for start and end.

@@ +162,3 @@
+            ui.sidebarForm.remove(rowToRemove);
+            query.removePoint(index - 1);
+                rowGrid.attach(addPointButton, 2, 0 , 1, 1);

Fixed.

Created attachment 283888 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 283892 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 283907 [details] [review]
route: New markers for turn point

These markers are showed if a turn point is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 283908 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 283909 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Review of attachment 283756 [details] [review]:

::: src/mapView.js
@@ +112,3 @@
     _connectRouteSignals: function(route) {
         route.connect('update', this.showRoute.bind(this, route));
+        route.connect('reset', (function(){

Sorry :(

@@ +165,3 @@
 
+    showTurnPoint: function(turnPoint) {
+        this._instructionMarkerLayer.get_markers().forEach((function(marker){

Fixed.

@@ +166,3 @@
+    showTurnPoint: function(turnPoint) {
+        this._instructionMarkerLayer.get_markers().forEach((function(marker){
+            if (marker._index === -1)

index now is stopNumber property and has setter/getter.

@@ +180,3 @@
 
+        searchResultMarker.addToLayer(this._searchResultLayer);
+

Fixed.

@@ +212,3 @@
+        let pointIndex = 0;
+        this._instructionMarkerLayer.remove_all();
+        route.turnPoints.forEach(function(turnPoint) {

For destination I meant a stop (start point, via point or end point).
I changed it a bit. :)

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

Fixed.

::: src/turnPointBubble.js
@@ +48,3 @@
+        
+        ui.image.resource = icon;
+

Fixed.

::: src/turnPointMarker.js
@@ +69,3 @@
+        let query = Application.routeService.query;
+        let place = new Geocode.Place({ location: new Geocode.Location({ latitude: this.latitude,
+                                                                         longitude: this.longitude }) });

Fixed.

Review of attachment 283757 [details] [review]:

::: src/sidebar.js
@@ +197,3 @@
+                this._instructionList.connect('row-activated',(function(listbox, row) {
+                   this.emit('instruction-selected', row.turnPoint);
+                }).bind(this));

Fixed working more on it... ops... :)

Created attachment 283910 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 283911 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 282227 [details] [review]:

::: src/mapView.js
@@ +91,3 @@
     _initLayers: function() {
         this._routeLayer = new Champlain.PathLayer();
+        this._routeLayer.set_stroke_width(5.0);

This could set from the constructor:

this._routeLayer = new Champlain.PathLayer({ stroke_width: 5.0 });

Same with the line below really, but dunno if it makes it prettier, even if you do not convert the below to set at init, it should be this._routeLayer.stroke_color = [...]

Review of attachment 283909 [details] [review]:

::: src/gnome-maps.data.gresource.xml
@@ +36,3 @@
     <file alias="direction-start">../data/media/direction-checkpoint.png</file>
     <file alias="direction-end">../data/media/direction-checkpoint.png</file>
+    <file alias="direction-via">../data/media/direction-checkpoint.png</file>

I am curious why does start/end/via all point to the same png?

::: src/gnome-maps.js.gresource.xml
@@ +28,3 @@
+    <file>searchResultMarker.js</file>
+    <file>turnPointMarker.js</file>
+    <file>turnPointBubble.js</file>

Please keep this file in alphabetical order.

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

You can make the lines short if you go ({
             propname: property,
[...]

For these two.

@@ +234,3 @@
+                                                                    draggable: draggable,
+                                                                    mapView: this });
+        return turnPointMarker;

And you could return the marker directly and avoid the temporary variable.

::: src/turnPointMarker.js
@@ +44,3 @@
+                                            100,
+                                            -1)
+    },

Why does this need to be a GObject property?

Could you please rebase this on latest master and on latest Damian work?

Review of attachment 283909 [details] [review]:

::: src/gnome-maps.data.gresource.xml
@@ +36,3 @@
     <file alias="direction-start">../data/media/direction-checkpoint.png</file>
     <file alias="direction-end">../data/media/direction-checkpoint.png</file>
+    <file alias="direction-via">../data/media/direction-checkpoint.png</file>

I'm still waiting for icons from Andreas (a bug has already been opened).
As soon as I have the new icons, I will change them. :)

::: src/gnome-maps.js.gresource.xml
@@ +28,3 @@
+    <file>searchResultMarker.js</file>
+    <file>turnPointMarker.js</file>
+    <file>turnPointBubble.js</file>

Fixed.

::: src/mapView.js
@@ +233,3 @@
+                                                                    place: place,
+                                                                    draggable: draggable,
+                stopNumber++;

hope I figure out what you mean ;)

@@ +234,3 @@
+                                                                    draggable: draggable,
+                                                                    mapView: this });
+            }

Fixed.

::: src/turnPointMarker.js
@@ +44,3 @@
+                                            100,
+                                            -1)
+    },

Fixed.

Review of attachment 282227 [details] [review]:

Fixed.

Created attachment 284016 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284017 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284018 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284019 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284020 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284021 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284022 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

(In reply to comment #120)
> Could you please rebase this on latest master and on latest Damian work?
Done :)

(In reply to comment #130)
> (In reply to comment #120)
> > Could you please rebase this on latest master and on latest Damian work?
> Done :)

Still does not apply cleanly for me :( Could you make sure?

Created attachment 284034 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284035 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284036 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284037 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284038 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284039 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284040 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

(In reply to comment #131)
> (In reply to comment #130)
> > (In reply to comment #120)
> > > Could you please rebase this on latest master and on latest Damian work?
> > Done :)
> 
> Still does not apply cleanly for me :( Could you make sure?

Sorry. :(
Maybe before I made a mistake. Btw I just created a new master branch, uploaded Damiàn patches and then mines. They all applied well.
It should work with the last set of patches.

Created attachment 284041 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284042 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284043 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284044 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284045 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284046 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284047 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Just fixed a small error during rebase :)

I get errors like these while playing around:

(gnome-maps:14836): Gjs-WARNING **: JS ERROR: TypeError: place is null
GraphHopper<._buildURL/locations<@resource:///org/gnome/maps/routeService.js:89
GraphHopper<._buildURL@resource:///org/gnome/maps/routeService.js:88
wrapper@resource:///org/gnome/gjs/modules/lang.js:169

Could you look into that?

Also go: git diff [master] --check and fix the whitespace damage that the patches causes.

Created attachment 284048 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284049 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284050 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284051 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284052 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284053 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284054 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

(In reply to comment #148)
> I get errors like these while playing around:
> 
> (gnome-maps:14836): Gjs-WARNING **: JS ERROR: TypeError: place is null
> GraphHopper<._buildURL/locations<@resource:///org/gnome/maps/routeService.js:89
> GraphHopper<._buildURL@resource:///org/gnome/maps/routeService.js:88
> wrapper@resource:///org/gnome/gjs/modules/lang.js:169
> 
> Could you look into that?

Just fixed.

(In reply to comment #149)
> Also go: git diff [master] --check and fix the whitespace damage that the
> patches causes.

No white damage any longer :)

Review of attachment 284048 [details] [review]:

::: src/routeService.js
@@ +57,3 @@
         this.query.connect('updated', (function() {
+            for (let i = 0; i < this._query.points.length; i++) {
+                if ((this._query.points[i] === undefined) || (this._query.points[i] === null))

I think this is the same as: if (!this._query.points[i]), right?

Created attachment 284055 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284056 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284057 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284058 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284059 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284060 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284061 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284048 [details] [review]:

::: src/routeService.js
@@ +57,3 @@
         this.query.connect('updated', (function() {
+            for (let i = 0; i < this._query.points.length; i++) {
+                if ((this._query.points[i] === undefined) || (this._query.points[i] === null))

Fixed.

Review of attachment 284056 [details] [review]:

::: src/sidebar.js
@@ +122,3 @@
+        let removePointButton = Gtk.Button.new_from_icon_name("list-remove-symbolic", 1);
+
+        let index = query._points.length;

All these shold be query.points not query._points;

@@ +150,3 @@
+            query.modifyPoint(entry.place, index - 1);
+        });
+

Should we also have a query.connect('notify::points') that updates any placeEntries if the corresponding place has updated? Where should it be connected in that case, and does it need disconnecting?

Review of attachment 284059 [details] [review]:

::: src/turnPointMarker.js
@@ +36,3 @@
+    Extends: MapMarker.MapMarker,
+
+    get stopNumber(){

{...

@@ +40,3 @@
+    },
+
+    set stopNumber(stopNumber){

{...

Review of attachment 284059 [details] [review]:

I am having a hard time to understand the code in mapView, with the stopNumber (what is that?) and other stuff, could you try to explain what is going on? And does it need to be that complicated?

::: src/mapView.js
@@ +184,3 @@
+        searchResultMarker.addToLayer(this._searchResultLayer);
+        this._searchResultLayer.add_marker(searchResultMarker);
+        searchResultMarker.goToAndSelect(true);

What is this? mismerge?

Review of attachment 284056 [details] [review]:

::: src/sidebar.js
@@ +122,3 @@
+        let removePointButton = Gtk.Button.new_from_icon_name("list-remove-symbolic", 1);
+
+                                    width_request: 32 });

Fixed.

@@ +150,3 @@
+            query.modifyPoint(entry.place, index - 1);
+        });
+        if (query._points.length < 2) {

Could do I do that on _onMarkerDrag?

Review of attachment 284056 [details] [review]:

::: src/sidebar.js
@@ +150,3 @@
+            query.modifyPoint(entry.place, index - 1);
+        });
+

That might work... but hmm how?
You don't have access to the sidebar or placeEntries in the marker class, right?
onMarkerDrag will do modifyPoint and that will notify on "points" so maybe it is best to listen for that notify here and determine if anything changed in the callback?

Review of attachment 284059 [details] [review]:

::: src/mapView.js
@@ +184,3 @@
+        searchResultMarker.addToLayer(this._searchResultLayer);
+        this._searchResultLayer.add_marker(searchResultMarker);
+        searchResultMarker.goToAndSelect(true);

Fixed.

::: src/turnPointMarker.js
@@ +36,3 @@
+    Extends: MapMarker.MapMarker,
+
+    get stopNumber(){

Fixed.

@@ +40,3 @@
+    },
+
+    set stopNumber(stopNumber){

Fixed.

Review of attachment 284059 [details] [review]:

stopNumber is the index of the turnPoint that is a placeEntry.
So for example, with two nodes, the start placeEntry has a turn point with stopNumber = 0
and the end placeEntry has a turn point with stopNumber = 1.

These index are needed for showing just the turn point destinations (stop) or for dragging
the turn point and  modify the right point in the route query.

Review of attachment 284056 [details] [review]:

::: src/sidebar.js
@@ +150,3 @@
+            query.modifyPoint(entry.place, index - 1);
+        });
+        if (query._points.length < 2) {

Uhm maybe I found a solution could I write just 'Custom place' instead of the new location name.
Don't know why but the location name of the dragged marker is everytime null.

Created attachment 284070 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284071 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284072 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284073 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284074 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284075 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284076 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284056 [details] [review]:

::: src/sidebar.js
@@ +150,3 @@
+            query.modifyPoint(entry.place, index - 1);
+        });
+

Well of course they are null, you create the new place without adding a name, right?
What we could do is perform a reverse lookup with geocode to try to get the name, but that is not guaranteed to be the right one either.

It's a pickle. Maybe just a name sort of "[latitude: lat, longitude: lon]" is the best we can do?
With a fixed number of decimals.

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+        });

The name should be added in the turnpointMarker code then you do need this check. Right now you crate the place object without supplying a name property.

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+        });

_do not_ of course

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+            if (index === 0) {

The point is that if I avoid this check then as I add a new placeEntry the text is "Custom place" or so.

So I could just put a string as for example, [43.389, 43.389]?

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+        });

There should be no special casing in regards to name here. Add a name in turnpointMarker.js. A name that best describes the point that the marker is dragged to.

You could try a geocode reverse search like in contextMenu.js. But that will be slow and not guaranteed to be correct.

So I think the best description is to supply the latitude and longitude that the marker is dragged too. And try to come up with a nice string containing that as a name,

Do you agree?

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+            if (index === 0) {

Oh yes of course.
My point was to print in the entry, something like "[43.389, 12.042]".
"[latitude: 43.389, longitude: 12.042]" is for me a too long string.

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+        });

Sure.

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+            if (index === 0) {

I don't understand why I should add a property to turnPointMarker.js.
The turnpoint marker generally has not a name.
Why I can't directly edit the placeEntry?

Review of attachment 284071 [details] [review]:

::: src/sidebar.js
@@ +151,3 @@
+                if ((entry.text !== query.points[index].name))
+                    entry.text = _("Custom place");
+        });

You are not adding a property on turnPointMarker. You are setting a name on the place that you drag a marker to.

General comments:

I think that we do not need to have an icon for the turnpointMarker if it's not draggable.

In order words: I think it's kind of annoying to have to first click the instruction and then a marker to get the bubble. I think we should show the bubble when we click the instruction.

So in code do not add the icon to the actor if it is not draggable. And try to find a way to select the marker when we have shown it. I suggest replacing turnPointerMarker.goTo(true) with turnPointMarker.goToAndSelect(false).

Note the false for no animation.

Also I keep getting warnings in the log:
(gnome-maps:23580): libsoup-CRITICAL **: soup_connection_disconnect: assertion 'SOUP_IS_CONNECTION (conn)' failed


Could you look into that?

Could you also see if it looks better if we remove the zoomToFit from mapWalker if animation is off?

(In reply to comment #191)
> Review of attachment 284071 [details] [review]:
> 
> ::: src/sidebar.js
> @@ +151,3 @@
> +                if ((entry.text !== query.points[index].name))
> +                    entry.text = _("Custom place");
> +        });
> 
> You are not adding a property on turnPointMarker. You are setting a name on the
> place that you drag a marker to.

Fixed. coming soon. :)

(In reply to comment #192)
> General comments:
> 
> I think that we do not need to have an icon for the turnpointMarker if it's not
> draggable.
> 
> In order words: I think it's kind of annoying to have to first click the
> instruction and then a marker to get the bubble. I think we should show the
> bubble when we click the instruction.
> 
> So in code do not add the icon to the actor if it is not draggable. And try to
> find a way to select the marker when we have shown it. I suggest replacing
> turnPointerMarker.goTo(true) with turnPointMarker.goToAndSelect(false).
> 
> Note the false for no animation.
> 
> Also I keep getting warnings in the log:
> (gnome-maps:23580): libsoup-CRITICAL **: soup_connection_disconnect: assertion
> 'SOUP_IS_CONNECTION (conn)' failed
> 
> 
> Could you look into that?

Applyed your suggestion, now it looks really better. coming soon. :)
Don't know what this error is related to. When it happen?

Review of attachment 284074 [details] [review]:

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

This y: should be this.height - 3 like for the searchResultMarker.

Review of attachment 284074 [details] [review]:

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

Fixed.

(In reply to comment #193)
> Could you also see if it looks better if we remove the zoomToFit from mapWalker
> if animation is off?

I think that zoomToFit should stay. It's nicer if it's activated.
So I propose to have animation off and zoomToFit on.

(In reply to comment #198)
> (In reply to comment #193)
> > Could you also see if it looks better if we remove the zoomToFit from mapWalker
> > if animation is off?
> 
> I think that zoomToFit should stay. It's nicer if it's activated.
> So I propose to have animation off and zoomToFit on.

I made a little modification in the latest patch related to it, we keep zoomToFit when animation is off, but the zoom is not animated. I tried it by using goToAndSelect(false) on instruction list click and looks good and usable to me.

Created attachment 284100 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284101 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284102 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284103 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284104 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284105 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284106 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284107 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284108 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284109 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284104 [details] [review]:

::: src/sidebar.js
@@ +149,3 @@
+            let index = rowGrid.get_parent().get_index() - 1;
+            if (query.points[index])
+                entry.text = query.points[index].name;

Well this is wrong right? It must be entry.place = query.points[index], right? Otherwise we might set a place with the name from one place and the location of another?

And this is not very pretty to do this for every row. It would be better if we could connect this once for the whole sidebar in some way.

Mattias, could you help us out? I feel this whole approach of createRow needs looking into.

Review of attachment 284107 [details] [review]:

::: src/mapView.js
@@ +171,3 @@
+            if (marker.stopNumber === -1)
+                this._instructionMarkerLayer.remove_marker(marker)
+        }).bind(this));

Couldn't we have the markers that are not draggable remove themself when the bubble closes? on the 'closed' signal perhaps? Or in the notify::selected if selected is false.

::: src/turnPointMarker.js
@@ +79,3 @@
+        let query = Application.routeService.query;
+        let place = new Geocode.Place({
+                        name: '[' + this.latitude.toFixed(3) + ', ' + this.longitude.toFixed(3) + ']',

Maybe have a let name = ... variable above is easier to read?

Review of attachment 284107 [details] [review]:

::: src/mapView.js
@@ +171,3 @@
+            if (marker.stopNumber === -1)
+                this._instructionMarkerLayer.remove_marker(marker)
+        }).bind(this));

Fixed, catching the bubble closed signal.

::: src/turnPointMarker.js
@@ +79,3 @@
+        let query = Application.routeService.query;
+        let place = new Geocode.Place({
+                        name: '[' + this.latitude.toFixed(3) + ', ' + this.longitude.toFixed(3) + ']',

Fixed.

Review of attachment 284104 [details] [review]:

::: src/sidebar.js
@@ +149,3 @@
+            let index = rowGrid.get_parent().get_index() - 1;
+            if (query.points[index])
+        if (query.points.length < 2) {

Any idea?
Putting entry.place = query.points[index] doesn't work cause it goes to a loop and I can't bind.

Review of attachment 284104 [details] [review]:

::: src/sidebar.js
@@ +149,3 @@
+            let index = rowGrid.get_parent().get_index() - 1;
+            if (query.points[index])
+                entry.text = query.points[index].name;

Ah, you are right :/

Tricky Tricky Tricky. I guess this was why the bind_property was implemented :)
Is there any thing more we could add to a potential QueryPoint class?

If there was just something else that it having a property called 'place' with the place it would make me feel happier with adding it.

But yeah, add a class QueryPoint inte the routeQuery.js and let it have a property called place.
Then go back to using the bind_property like you did before. And please try to think if any functionality could move to that class.

Review of attachment 284104 [details] [review]:

::: src/sidebar.js
@@ +149,3 @@
+            let index = rowGrid.get_parent().get_index() - 1;
+            if (query.points[index])
+        if (query.points.length < 2) {

Fixed. Btw I didn't find anything to add to QueryPoint, cause most of things is good that stay in routeQuery.

Created attachment 284220 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284221 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284222 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284223 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284224 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284225 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284226 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284220 [details] [review]:

Thanks!

I think I told you that I wanted the queryPoint class inside of the routeQuery.js file, could you please fix that?

::: src/queryPoint.js
@@ +44,3 @@
+        this._place = p;
+        this.notify('place');
+    },

Add a new line here.

@@ +58,3 @@
+            this.emit('updated');
+        }).bind(this));
+    },

Why do this? The 'notify::place' is the same thing as this signal.

::: src/routeQuery.js
@@ +77,3 @@
+        point.connect('updated', (function() {
+            this.notify('points');
+        }).bind(this));

Why is the updated signal needed here? And if it really is needed, why not connect to 'notify::place' ?

Review of attachment 284221 [details] [review]:

::: src/sidebar.js
@@ +172,1 @@
     },

This functions is very big and does a lot of things. Can you think of a way to split it up? And if possible move something to ui files?

Review of attachment 284222 [details] [review]:

::: src/utils.js
@@ +276,3 @@
+        labelledTime += hours + ' h ';
+    if (minutes > 0)
+        labelledTime += minutes + ' min';

These does not need translation?

Review of attachment 284223 [details] [review]:

Ack

Review of attachment 284224 [details] [review]:

Thanks.

Please also work on the commit message.

::: src/mapView.js
@@ +203,3 @@
     },
 
+    _showDestinationTurnpoint: function() {

Should it be Turnpoints maybe? It could be more than one, right?

@@ +205,3 @@
+    _showDestinationTurnpoint: function() {
+        let route = Application.routeService.route;
+        let stopNumber = 0;

New line here I think

@@ +208,3 @@
+        this._instructionMarkerLayer.remove_all();
+        route.turnPoints.forEach(function(turnPoint) {
+            if (turnPoint.isStop(turnPoint)) {

isStop does not take an argument

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

This empty line could be removed.

::: src/turnPointMarker.js
@@ +57,3 @@
+
+        this.bubble.connect('closed', function() {
+            if (this._stopNumber == -1)

Should be === and we are using the draggable property for this kind of check below. Maybe we should use the same here?

Review of attachment 284224 [details] [review]:

Set status.

Review of attachment 284225 [details] [review]:

Ack

Clicking the instructions and getting the marker is very slow for me, is it for you as well?

(In reply to comment #230)
> Clicking the instructions and getting the marker is very slow for me, is it for
> you as well?

After changing the query and having new instructions, getting the marker after clicking, becomes slower. No idea how to fix it.

Review of attachment 284221 [details] [review]:

::: src/sidebar.js
@@ +172,1 @@
     },

I tried to split it a bit and it's really difficult cause all the components are really related and the refactored functions are worse than the original one.

I don't think I could move something to ui file, because all the widgets are dynamical to the contest. (i.e. the add button is just on the first line and the remove one is just on the via points). It's also difficult to put PlaceEntry there.

Created attachment 284415 [details] [review]
route: Add support for via points

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284416 [details] [review]
sidebar: Add via points support

Remove the statical entries called 'to' and 'from'.
It is now possible to add and remove rows to the sidebar form.
Every row contains an image label and a PlaceEntry,
that on change modify the query property 'points'.
The first row has a button for adding new rows.
The via point's rows have a button for removing them.

Created attachment 284417 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284418 [details] [review]
mapView: change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284419 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284420 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284421 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

(In reply to comment #231)
> (In reply to comment #230)
> > Clicking the instructions and getting the marker is very slow for me, is it for
> > you as well?
> 
> After changing the query and having new instructions, getting the marker after
> clicking, becomes slower. No idea how to fix it.

Maybe we could avoid to create the markers when an instruction is selected and
create them on querying and then just display.

Created attachment 284504 [details] [review]
sidebar: Add Via points support - [jonas]

How about something like this?

It moves a lot out to ui files.

Created attachment 284505 [details] [review]
sidebar: Add Via points support - [jonas]

A version that works.

I have only given this approach light testing. But it seems to work.
But you should look it over since I did it in quite a hurry.

Also please clean-up the queryPoint.js mess :)

And check for whitespace damage for the whole series.

Review of attachment 284505 [details] [review]:

::: src/sidebar.js
@@ +135,3 @@
+    _initRouteEntry: function(container, pointIndex) {
+        let entry = this._createPlaceEntry();
+        container.add(entry);

Maybe the entry variable is useless.

@@ +142,2 @@
                             GObject.BindingFlags.BIDIRECTIONAL);
+

Empty line might be unnecessary.

Review of attachment 284419 [details] [review]:

Maybe we should a a destinationMarker that inherits from turnPointMarker and adds an pointIndex property (instead of stopNumber).
It would also be draggable and have the icon added that turnPointMarker does not need.

It would clean the code up a bit I feel.


What do you think?

(In reply to comment #241)
> Created an attachment (id=284504) [details] [review]
> sidebar: Add Via points support - [jonas]
> 
> How about something like this?
> 
> It moves a lot out to ui files.


Great! I'm going to fix it a bit and commit it again right now.

Review of attachment 284505 [details] [review]:

::: src/sidebar.js
@@ +135,3 @@
+    _initRouteEntry: function(container, pointIndex) {
+        let entry = this._createPlaceEntry();
+        let insertIndex = 1; // Always insert after 'From'

Fixed.

@@ +142,2 @@
                             GObject.BindingFlags.BIDIRECTIONAL);
+

Fixed.

Created attachment 284527 [details] [review]
route: Add via points support

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if all the places are defined.

Created attachment 284528 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 284529 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284530 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284531 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284532 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284533 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284527 [details] [review]:

::: src/routeQuery.js
@@ +77,3 @@
+        point.connect('updated', (function() {
+            this.notify('points');
+        }).bind(this));

Tell me again why this notify needs to be in this callback?

Review of attachment 284528 [details] [review]:

::: src/routeQuery.js
@@ +130,3 @@
+    Signals: {
+        'updated': { },
+    },

Where is this signal emitted?

::: src/sidebar.js
@@ +122,3 @@
+        let insertIndex = 2; // Always insert after 'From'
+        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
+        listbox.insert(ui.viaGrid, insertIndex);

Why is the index for the points and the listbox different? It seemed to work for me. What did I miss?

Review of attachment 284529 [details] [review]:

::: src/utils.js
@@ +291,3 @@
+            labelledDistance = m + ' m';
+        return labelledDistance;
+}

If you look at Damians marker-patches he switched to another style of formatting with gettext in cases lake this:

"return _("%f km²").format(area);"

https://bugzilla.gnome.org/review?bug=722871&attachment=284439

Maybe the same is better here? Ask Damian if you are uncertain.

Review of attachment 284531 [details] [review]:

::: src/turnPointMarker.js
@@ +48,3 @@
+            if (this.draggable)
+                this.destroy();
+        });

Is this really correct? Shouldn't this signal always be here?
You could possibly set this uncondionally to some kind of Id and disconnect it after this.parent(params) in the DestinationMarker, maybe?

@@ +82,3 @@
+        this.parent(params);
+
+        this.draggable = true;

Isn't all destinations markers draggable? Is this needed?

Since Jonas is the only one reviewing this, I'm going to try to do my best to help him, since I'm idle in my tasks for now.

First I'm going to test the app as a regular user, then I'm going to review the patches.

My test results are the following (please note that this is a general testing, some errors are unrelated to this ticket but related to the sidebar inclusion):

a) I don't have any feedback of when the routes are loading, only when route is located or not found. If you ask me, I would put a spinner in the instructions list to know if the route is loading.

b) No error handling. I only see "No route found." notification whenever the route is actually not found or when I lost my Internet connection.

c) If I drag the destination point to another place and the route search fails, the the old path remains visible with the destination pin in other place, away from the path. I don't know how this can be solved, maybe is not a bug and it's fine to keep this behavior, but it cached my attention.

d) When I drag the destination point to another custom place, the string in the place entry becomes "[<lat>, <long>]", if I change these numbers and press enter, Maps cannot interpret it as a coordinate. I think supporting custom coordinates like Google Maps in PlaceEntry is not a hard thing to do, what do you think?

e) I always can see an horizontal scrollbar in the instructions list (when it has results).

f) You surely know this, but we are overusing pin.svg in markers. For the via point case, I would use a pin with space to put a letter on it, so we can have pins with the letter "A", "B", "C" and so on, for each via point. This surely can be done for 3.16 since we are a little out of time for this release.

g) Wasn't the via points sortable (DnD place entries)?

h) 1) Open the sidebar, write start point and ending point
   2) Wait for route search to finish
   3) Press "+" to add a via point, don't do anything, just press the button
   4) Drag the destination in the map view to another place
   5) Via point entry is changed instead of final destination

i) After search for a route, when I press an instruction list row, the bubble delays a lot to appear and I see the same amount of this lines in output "Gjs-Message: JS LOG: DEBUG: Going to null" as instructions rows I have. I mean, if I have 20 rows in the instructions list, I see 20 lines with the mentioned content when I click a row before actually seeing the bubble. If I perform the same route search again, I see 40 lines of logs (2 times the previous one).

j) Using the instructions list with keyboard arrows doesn't updates the bubbles.

k) I think widget tab order must be tweaked so I can change focus between text entries with a single tab press ("add" and "remove" buttons tab order can be after text entries).

l) Maybe there is no need to perform the search again if any via points changed (I mean, user writes down the same place in the same entry).

m) 1) Open the sidebar, write start point and ending point
   2) Wait for route search to finish
   3) Press "+" to add a via point, don't do anything, just press the button
   4) Drag the origin in the map view to another place
   5) Route not refreshed
   6) Try to fill the via point entry added in (3), route not refreshed

n) 1) Open the sidebar, write start point and ending point
   2) Wait for route search to finish
   3) Press "+" to add a via point, fill the new entry
   4) Wait for the search to finish
   5) Delete the created via point
   6) Now route goes from origin to deleted via point, so the route is inconsistent with the sidebar information

o) Pressing the entry's clear button should update the route?


I think that's it, if I find more I will include it in future comments, continuing this enumeration. I may refer to this items during the patch reviews using it corresponding letter.

(In reply to comment #255)
> Review of attachment 284527 [details] [review]:
> 
> ::: src/routeQuery.js
> @@ +77,3 @@
> +        point.connect('updated', (function() {
> +            this.notify('points');
> +        }).bind(this));
> 
> Tell me again why this notify needs to be in this callback?

This is needed because the service must be connected to the query.

Btw I clean it a bit.

(In reply to comment #256)
> Review of attachment 284528 [details] [review]:
> 
> ::: src/routeQuery.js
> @@ +130,3 @@
> +    Signals: {
> +        'updated': { },
> +    },
> 
> Where is this signal emitted?

Fixed.

> 
> ::: src/sidebar.js
> @@ +122,3 @@
> +        let insertIndex = 2; // Always insert after 'From'
> +        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
> +        listbox.insert(ui.viaGrid, insertIndex);
> 
> Why is the index for the points and the listbox different? It seemed to work
> for me. What did I miss?

Because on the top of the sidebar form you need to have the mode chooser. This is a easy way for solving the issue.

(In reply to comment #257)
> Review of attachment 284529 [details] [review]:
> 
> ::: src/utils.js
> @@ +291,3 @@
> +            labelledDistance = m + ' m';
> +        return labelledDistance;
> +}
> 
> If you look at Damians marker-patches he switched to another style of
> formatting with gettext in cases lake this:
> 
> "return _("%f km²").format(area);"
> 
> https://bugzilla.gnome.org/review?bug=722871&attachment=284439
> 
> Maybe the same is better here? Ask Damian if you are uncertain.

Fixed.

Review of attachment 284527 [details] [review]:

Hmmm... I'm suspecting you had problems while rebasing this one since I cannot see QueryPoint class, isn't that class related to this changes?

::: src/gnome-maps.js.gresource.xml
@@ +21,3 @@
     <file>placeEntry.js</file>
     <file>placeStore.js</file>
+    <file>queryPoint.js</file>

Unrelated?

::: src/routeQuery.js
@@ +77,3 @@
+        point.connect('updated', (function() {
+            this.notify('points');
+        }).bind(this));

Actually, as Jonas pointed out in the next patch, 'updated' signal is never emitted (I'm supposing that "point" is a QueryPoint instance)

@@ +99,2 @@
         this.reset();
     },

This comes from previous changes, but, why _init is not the first method?

@@ +115,3 @@
+            point = null;
+        });
+        this.emit('reset');

Maybe you should emit "notify::points" too.

Review of attachment 284528 [details] [review]:

::: src/gnome-maps.js.gresource.xml
@@ +21,2 @@
     <file>placeEntry.js</file>
     <file>placeStore.js</file>

Why this removal? how is this related with the previous patch.

::: src/routeQuery.js
@@ +71,1 @@
     set points(points) {

Unrelated, please do it in the previous patch

@@ +76,3 @@
     addPoint: function(point, index) {
         this._points.splice(index, 0, point);
+        point.connect('notify::place', (function() {

Why this was "updated" in the previous patch anyways?

@@ +147,3 @@
+        return this._place;
+    }
+});

Does this class belongs to "route: Add via points support", probably in a separated file? (see my comment at "gnome-maps.js.gresource.xml" in that patch)

::: src/sidebar.js
@@ +122,3 @@
+        let insertIndex = 2; // Always insert after 'From'
+        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
+        listbox.insert(ui.viaGrid, insertIndex);

I also don't understand this, shouldn't be 1 on both cases?

(In reply to comment #258)
> Review of attachment 284531 [details] [review]:
> 
> ::: src/turnPointMarker.js
> @@ +48,3 @@
> +            if (this.draggable)
> +                this.destroy();
> +        });
> 
> Is this really correct? Shouldn't this signal always be here?
> You could possibly set this uncondionally to some kind of Id and disconnect it
> after this.parent(params) in the DestinationMarker, maybe?

This cannot work because local members are not inherited in javascript.

> 
> @@ +82,3 @@
> +        this.parent(params);
> +
> +        this.draggable = true;
> 
> Isn't all destinations markers draggable? Is this needed?

This is why this is needed. TurnPointMarker aren't draggable so here I must define the marker as draggable.

Review of attachment 284527 [details] [review]:

::: src/gnome-maps.js.gresource.xml
@@ -23,2 @@
     <file>route.js</file>
     <file>routeQuery.js</file>

Fixed.

::: src/routeQuery.js
@@ +77,3 @@
+        point.connect('updated', (function() {
+            this.notify('points');
+        }).bind(this));

Fixed.

@@ +99,2 @@
         this.reset();
     },

Fixed.

@@ +115,3 @@
+            point = null;
+        });
+        this.emit('reset');

This is not need because as this._points is updated "notify::points" is emitted.

Btw I changed the forEach to this._points = []; for simplifying

Review of attachment 284528 [details] [review]:

::: src/gnome-maps.js.gresource.xml
@@ +20,3 @@
     <file>path.js</file>
     <file>placeEntry.js</file>
     <file>placeStore.js</file>

Not needed at all. I made some errors rebasing. I'll check it again and then attach a new patch set.

::: src/routeQuery.js
@@ +71,1 @@
     set points(points) {

Fixed.

@@ +76,3 @@
     addPoint: function(point, index) {
         this._points.splice(index, 0, point);
+        point.connect('notify::place', (function() {

I made some changes to the emitted signals. Now it's easier to understand.

@@ +130,3 @@
+    Signals: {
+        'updated': { },
+    },

Fixed.

@@ +147,3 @@
+        return this._place;
+    }
+                                          GObject.ParamFlags.READWRITE,

Yes of course RouteQuery should not be here.

::: src/sidebar.js
@@ +122,3 @@
+        let insertIndex = 2; // Always insert after 'From'
+        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
+    },

No, cause we want the mode-choose to be before the place entries.
This is a really easy way for having it.

Review of attachment 284527 [details] [review]:

::: src/routeQuery.js
@@ +115,3 @@
+            point = null;
+        });
+        this.emit('reset');

I went back to forEach, there are some issues not updating every point.

(In reply to comment #259)
> Since Jonas is the only one reviewing this, I'm going to try to do my best to
> help him, since I'm idle in my tasks for now.
> 
> First I'm going to test the app as a regular user, then I'm going to review the
> patches.

Thanks!

> 
> My test results are the following (please note that this is a general testing,
> some errors are unrelated to this ticket but related to the sidebar inclusion):
> 
> a) I don't have any feedback of when the routes are loading, only when route is
> located or not found. If you ask me, I would put a spinner in the instructions
> list to know if the route is loading.

This has been in my mind for a while. I don't think to have time to do it now, but I plan to do it after my MSc thesis discussion (end of September).

> 
> b) No error handling. I only see "No route found." notification whenever the
> route is actually not found or when I lost my Internet connection.

Mattias worked on this. It should already working. Maybe you should open a bug.

> 
> c) If I drag the destination point to another place and the route search fails,
> the the old path remains visible with the destination pin in other place, away
> from the path. I don't know how this can be solved, maybe is not a bug and it's
> fine to keep this behavior, but it cached my attention.

I could reset the marker to the old position or leave as it is.

> 
> d) When I drag the destination point to another custom place, the string in the
> place entry becomes "[<lat>, <long>]", if I change these numbers and press
> enter, Maps cannot interpret it as a coordinate. I think supporting custom
> coordinates like Google Maps in PlaceEntry is not a hard thing to do, what do
> you think?

As my code is committed, I could open a bug and add this new feature or we could use a reverse geocoding service.

> 
> e) I always can see an horizontal scrollbar in the instructions list (when it
> has results).

I have this behaviour since I can't use expand property, I'll try to remove some pixels from the instruction label.

> 
> f) You surely know this, but we are overusing pin.svg in markers. For the via
> point case, I would use a pin with space to put a letter on it, so we can have
> pins with the letter "A", "B", "C" and so on, for each via point. This surely
> can be done for 3.16 since we are a little out of time for this release.

Andreas is going to draw new icons for start, via and end markers.

> 
> g) Wasn't the via points sortable (DnD place entries)?

This isn't so easy as it seems. Mattias spoke with Jasper at Guadec and he told us something like: "It's a mess, try not to use it". Btw I could do it but this needs a lot of time.

> 
> h) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, don't do anything, just press the button
>    4) Drag the destination in the map view to another place
>    5) Via point entry is changed instead of final destination

Fixed, but I would like to test it more.

> 
> i) After search for a route, when I press an instruction list row, the bubble
> delays a lot to appear and I see the same amount of this lines in output
> "Gjs-Message: JS LOG: DEBUG: Going to null" as instructions rows I have. I
> mean, if I have 20 rows in the instructions list, I see 20 lines with the
> mentioned content when I click a row before actually seeing the bubble. If I
> perform the same route search again, I see 40 lines of logs (2 times the
> previous one).

The problem seems that on 'row-activated' the turnPoint is showed multiple times. I'm getting crazy with this, help would be appreciated.

> 
> j) Using the instructions list with keyboard arrows doesn't updates the
> bubbles.

You should press enter after selecting, otherwise the signal is not emitted.

> 
> k) I think widget tab order must be tweaked so I can change focus between text
> entries with a single tab press ("add" and "remove" buttons tab order can be
> after text entries).

Mattias already know about this. It should be related to placeEntry popover.

> 
> l) Maybe there is no need to perform the search again if any via points changed
> (I mean, user writes down the same place in the same entry).

Yes, this could be done easily. Please open a bug. It should be related to PlaceEntry.

> 
> m) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, don't do anything, just press the button
>    4) Drag the origin in the map view to another place
>    5) Route not refreshed
>    6) Try to fill the via point entry added in (3), route not refreshed

Fixed.

> 
> n) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, fill the new entry
>    4) Wait for the search to finish
>    5) Delete the created via point
>    6) Now route goes from origin to deleted via point, so the route is
> inconsistent with the sidebar information

Fixed.

> 
> o) Pressing the entry's clear button should update the route?

Fixed. But the new route is calculated if you have at least two points.

> 
> 
> I think that's it, if I find more I will include it in future comments,
> continuing this enumeration. I may refer to this items during the patch reviews
> using it corresponding letter.

(In reply to comment #269)
> (In reply to comment #259)
> > i) After search for a route, when I press an instruction list row, the bubble
> > delays a lot to appear and I see the same amount of this lines in output
> > "Gjs-Message: JS LOG: DEBUG: Going to null" as instructions rows I have. I
> > mean, if I have 20 rows in the instructions list, I see 20 lines with the
> > mentioned content when I click a row before actually seeing the bubble. If I
> > perform the same route search again, I see 40 lines of logs (2 times the
> > previous one).
> 
> The problem seems that on 'row-activated' the turnPoint is showed multiple
> times. I'm getting crazy with this, help would be appreciated.

Got it!

You're connecting the listbox 'row-activated' signal in the forEach, you need to connect this signal just one time, because this signal is for the whole listbox, not just the row.

Also, if you use 'row-selected' instead of 'row-activated' you can solve the item (j), I think is more comfortable to not press ENTER to update the bubble, what do you think?

(In reply to comment #270)
> (In reply to comment #269)
> > (In reply to comment #259)
> > > i) After search for a route, when I press an instruction list row, the bubble
> > > delays a lot to appear and I see the same amount of this lines in output
> > > "Gjs-Message: JS LOG: DEBUG: Going to null" as instructions rows I have. I
> > > mean, if I have 20 rows in the instructions list, I see 20 lines with the
> > > mentioned content when I click a row before actually seeing the bubble. If I
> > > perform the same route search again, I see 40 lines of logs (2 times the
> > > previous one).
> > 
> > The problem seems that on 'row-activated' the turnPoint is showed multiple
> > times. I'm getting crazy with this, help would be appreciated.
> 
> Got it!
> 
> You're connecting the listbox 'row-activated' signal in the forEach, you need
> to connect this signal just one time, because this signal is for the whole
> listbox, not just the row.
> 
> Also, if you use 'row-selected' instead of 'row-activated' you can solve the
> item (j), I think is more comfortable to not press ENTER to update the bubble,
> what do you think?

Nice!

I vote for row-selected I think.

Review of attachment 284527 [details] [review]:

::: src/routeQuery.js
@@ +115,3 @@
+            point = null;
+        });
+        this.emit('reset');

Are you sure this is causing the notify to fire?

Review of attachment 284528 [details] [review]:

::: src/sidebar.js
@@ +122,3 @@
+        let insertIndex = 2; // Always insert after 'From'
+        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
+        listbox.insert(ui.viaGrid, insertIndex);

I do not get this at all :)

The mode chooser is below the place entries, right? It is last in the GtkListBox (Does it need to be in the listbox at all?)

So entering something at position 1 (0 being the fromEntry) should only push it further down, right? And we want that, right? Since we are adding something before it. So I still think it should be 1 in both cases.

(In reply to comment #265)
> (In reply to comment #258)
> > Review of attachment 284531 [details] [review] [details]:
> > 


Please use review when replying.


> > ::: src/turnPointMarker.js
> > @@ +48,3 @@
> > +            if (this.draggable)
> > +                this.destroy();
> > +        });
> > 
> > Is this really correct? Shouldn't this signal always be here?
> > You could possibly set this uncondionally to some kind of Id and disconnect it
> > after this.parent(params) in the DestinationMarker, maybe?
> 
> This cannot work because local members are not inherited in javascript.


Well, ok, but shouldn't it be if (!this.draggable) ?
It is the instructionMarkers we want to destroy when the bubble dissapear, right? The ones without an icon. And we do not want to destroy the marker when the bubble closes if it _is_ draggable. Or am I crazy?

> 
> > 
> > @@ +82,3 @@
> > +        this.parent(params);
> > +
> > +        this.draggable = true;
> > 
> > Isn't all destinations markers draggable? Is this needed?
> 
> This is why this is needed. TurnPointMarker aren't draggable so here I must
> define the marker as draggable.

(In reply to comment #269)
> (In reply to comment #259)
> > Since Jonas is the only one reviewing this, I'm going to try to do my best to
> > help him, since I'm idle in my tasks for now.
> > 
> > First I'm going to test the app as a regular user, then I'm going to review the
> > patches.
> 
> Thanks!
> 


Yes thanks a lot for this Damian!



> > 
> > a) I don't have any feedback of when the routes are loading, only when route is
> > located or not found. If you ask me, I would put a spinner in the instructions
> > list to know if the route is loading.
> 
> This has been in my mind for a while. I don't think to have time to do it now,
> but I plan to do it after my MSc thesis discussion (end of September).
> 

I think this would be really nice, I can maybe try to add a patch that does this. I think it would be nice for 3.14.


> > 
> > b) No error handling. I only see "No route found." notification whenever the
> > route is actually not found or when I lost my Internet connection.
> 
> Mattias worked on this. It should already working. Maybe you should open a bug.
> 

Yeah, maybe this is something we could improve later. If you don't have a good idea of what and how to add better error handling.

> > 
> > c) If I drag the destination point to another place and the route search fails,
> > the the old path remains visible with the destination pin in other place, away
> > from the path. I don't know how this can be solved, maybe is not a bug and it's
> > fine to keep this behavior, but it cached my attention.
> 
> I could reset the marker to the old position or leave as it is.
> 

I think maybe reset would be best. Not sure tho.

> > 
> > d) When I drag the destination point to another custom place, the string in the
> > place entry becomes "[<lat>, <long>]", if I change these numbers and press
> > enter, Maps cannot interpret it as a coordinate. I think supporting custom
> > coordinates like Google Maps in PlaceEntry is not a hard thing to do, what do
> > you think?
> 
> As my code is committed, I could open a bug and add this new feature or we
> could use a reverse geocoding service.
> 


What do you mean with 'my code is committed'?
Damian: want to take a shot of adding support to the PlaceEntry of adding custom coordinates?


> > 
> > l) Maybe there is no need to perform the search again if any via points changed
> > (I mean, user writes down the same place in the same entry).
> 
> Yes, this could be done easily. Please open a bug. It should be related to
> PlaceEntry.

Maybe only notify if the new place differs from the old place in the placeEntry?
That would solve it right? So we compare what? lat/lon + name or only lat/lon maybe?

if (p.location && p.location.latitude == place.location.latitude ...
Something like that?

(In reply to comment #274)
> (In reply to comment #265)
> > (In reply to comment #258)
> > > Review of attachment 284531 [details] [review] [details] [details]:
> > > 
> 
> 
> Please use review when replying.
> 
> 
> > > ::: src/turnPointMarker.js
> > > @@ +48,3 @@
> > > +            if (this.draggable)
> > > +                this.destroy();
> > > +        });
> > > 
> > > Is this really correct? Shouldn't this signal always be here?
> > > You could possibly set this uncondionally to some kind of Id and disconnect it
> > > after this.parent(params) in the DestinationMarker, maybe?
> > 
> > This cannot work because local members are not inherited in javascript.
> 
> 
> Well, ok, but shouldn't it be if (!this.draggable) ?
> It is the instructionMarkers we want to destroy when the bubble dissapear,
> right? The ones without an icon. And we do not want to destroy the marker when
> the bubble closes if it _is_ draggable. Or am I crazy?
> 

Fixed.

> > 
> > > 
> > > @@ +82,3 @@
> > > +        this.parent(params);
> > > +
> > > +        this.draggable = true;
> > > 
> > > Isn't all destinations markers draggable? Is this needed?
> > 
> > This is why this is needed. TurnPointMarker aren't draggable so here I must
> > define the marker as draggable.

(In reply to comment #274)
> (In reply to comment #265)
> > (In reply to comment #258)
> > > Review of attachment 284531 [details] [review] [details] [details]:
> > > 
> 
> 
> Please use review when replying.
> 
> 
> > > ::: src/turnPointMarker.js
> > > @@ +48,3 @@
> > > +            if (this.draggable)
> > > +                this.destroy();
> > > +        });
> > > 
> > > Is this really correct? Shouldn't this signal always be here?
> > > You could possibly set this uncondionally to some kind of Id and disconnect it
> > > after this.parent(params) in the DestinationMarker, maybe?
> > 
> > This cannot work because local members are not inherited in javascript.
> 
> 
> Well, ok, but shouldn't it be if (!this.draggable) ?
> It is the instructionMarkers we want to destroy when the bubble dissapear,
> right? The ones without an icon. And we do not want to destroy the marker when
> the bubble closes if it _is_ draggable. Or am I crazy?
> 
> > 
> > > 
> > > @@ +82,3 @@
> > > +        this.parent(params);
> > > +
> > > +        this.draggable = true;
> > > 
> > > Isn't all destinations markers draggable? Is this needed?
> > 
> > This is why this is needed. TurnPointMarker aren't draggable so here I must
> > define the marker as draggable.

Review of attachment 284527 [details] [review]:

::: src/routeQuery.js
@@ +115,3 @@
+            point = null;
+        });
+        this.emit('reset');

Fixed. Setting point.place = null.

Review of attachment 284528 [details] [review]:

::: src/sidebar.js
@@ +122,3 @@
+        let insertIndex = 2; // Always insert after 'From'
+        this._initRouteEntry(ui.viaEntryGrid, insertIndex - 1);
+    },

As in the mockup
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/v2/routing.png
the mode chooser should be above the place entries.
For this reason in my last patch set now it is a grid and it is above the entries.
I think it's better if the new row is created before the 'To' and not after the 'From' as it is now. In my last patch it should be more clear.

(In reply to comment #275)
> (In reply to comment #269)
> > (In reply to comment #259)
> > > Since Jonas is the only one reviewing this, I'm going to try to do my best to
> > > help him, since I'm idle in my tasks for now.
> > > 
> > > First I'm going to test the app as a regular user, then I'm going to review the
> > > patches.
> > 
> > Thanks!
> > 
> 
> 
> Yes thanks a lot for this Damian!
> 
> 
> 
> > > 
> > > a) I don't have any feedback of when the routes are loading, only when route is
> > > located or not found. If you ask me, I would put a spinner in the instructions
> > > list to know if the route is loading.
> > 
> > This has been in my mind for a while. I don't think to have time to do it now,
> > but I plan to do it after my MSc thesis discussion (end of September).
> > 
> 
> I think this would be really nice, I can maybe try to add a patch that does
> this. I think it would be nice for 3.14.
> 
> 
> > > 
> > > b) No error handling. I only see "No route found." notification whenever the
> > > route is actually not found or when I lost my Internet connection.
> > 
> > Mattias worked on this. It should already working. Maybe you should open a bug.
> > 
> 
> Yeah, maybe this is something we could improve later. If you don't have a good
> idea of what and how to add better error handling.
> 
> > > 
> > > c) If I drag the destination point to another place and the route search fails,
> > > the the old path remains visible with the destination pin in other place, away
> > > from the path. I don't know how this can be solved, maybe is not a bug and it's
> > > fine to keep this behavior, but it cached my attention.
> > 
> > I could reset the marker to the old position or leave as it is.
> > 
> 
> I think maybe reset would be best. Not sure tho.
> 

Fixed, resetting the query.

> > > 
> > > d) When I drag the destination point to another custom place, the string in the
> > > place entry becomes "[<lat>, <long>]", if I change these numbers and press
> > > enter, Maps cannot interpret it as a coordinate. I think supporting custom
> > > coordinates like Google Maps in PlaceEntry is not a hard thing to do, what do
> > > you think?
> > 
> > As my code is committed, I could open a bug and add this new feature or we
> > could use a reverse geocoding service.
> > 
> 
> 
> What do you mean with 'my code is committed'?
> Damian: want to take a shot of adding support to the PlaceEntry of adding
> custom coordinates?
> 

My idea is to do this after 3.14.

> 
> > > 
> > > l) Maybe there is no need to perform the search again if any via points changed
> > > (I mean, user writes down the same place in the same entry).
> > 
> > Yes, this could be done easily. Please open a bug. It should be related to
> > PlaceEntry.
> 
> Maybe only notify if the new place differs from the old place in the
> placeEntry?
> That would solve it right? So we compare what? lat/lon + name or only lat/lon
> maybe?
> 
> if (p.location && p.location.latitude == place.location.latitude ...
> Something like that?

Fixed.

Created attachment 284686 [details] [review]
route: Add via points support

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if at least two places are defined.

Created attachment 284687 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 284688 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284689 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284690 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284691 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284692 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284687 [details] [review]:

::: src/placeEntry.js
@@ +45,2 @@
     set place(p) {
+        if (p != null) {

if (p) or if (p !== null)
I think I prefer if (p) here, then you can drop the check below with the ?-operator.

@@ +46,3 @@
+        if (p != null) {
+            if (!(this.place) ||
+                !(this.place.location.latitude == p.location.latitude && this.place.location.longitude == p.location.longitude)) {

This must be able to be written in a prettier way :)

Maybe you can add a helper function or at least format this better.



if (!this.place ||
    (this.place.location.latitude != p.location.latitude ||
    (this.place.location.longitude != p.location.longitude) {
    ...
}

or with helper:

locationEqual = function(locationA, locationB) {
    return locationA.latitude == locationB.latitude &&
           locationA.longitude == locationB.longitude;
} 

if (!this.place || locationsEqual(p.location, this.place.location)) {
    ...
}

Is this prettier?

@@ +48,3 @@
+                !(this.place.location.latitude == p.location.latitude && this.place.location.longitude == p.location.longitude)) {
+                this._place = p;
+                this.text   = p ? p.name : "";

We know that p is not falsy here.

@@ +53,3 @@
+        } else {
+            this._place = p;
+            this.text   = p ? p.name : "";

Same here, we know p is falsy.

::: src/sidebar.js
@@ +120,3 @@
+                                                      'via-remove-button',
+                                                      'via-entry-grid' ]);
+        let insertIndex = Application.routeService.query.points.length - 1; // Always insert before 'To'

Maybe have the comment above shorten the line?

Review of attachment 284690 [details] [review]:

::: src/turnPointMarker.js
@@ +83,3 @@
+
+        this.draggable = true;
+

Well this sets the draggable after the call to this.params, will this mean that the destinationmarker will be destroyed on bubble closed?

Review of attachment 284690 [details] [review]:

::: src/turnPointMarker.js
@@ +83,3 @@
+
+        this.draggable = true;
+

this.parent() I mean.

Review of attachment 284690 [details] [review]:

::: src/turnPointMarker.js
@@ +98,3 @@
+                        location: new Geocode.Location({ latitude: this.latitude,
+                                                         longitude: this.longitude }) });
+        if (query.points[pointIndex].place != null) {

!== or just if (query.points[poinyIndex].place)

@@ +101,3 @@
+            query.points[pointIndex].place = place;
+        } else {
+            query.points[pointIndex + 1].place = place;

Tell me what's going on here. What if it's the last pointIndex we are dragging? Why is this safe? And even if it's safe it needs a comment since it seems like magic.

Review of attachment 284687 [details] [review]:

::: src/placeEntry.js
@@ +45,2 @@
     set place(p) {
+        if (p != null) {

Fixed.

@@ +46,3 @@
+        if (p != null) {
+            if (!(this.place) ||
+                !(this.place.location.latitude == p.location.latitude && this.place.location.longitude == p.location.longitude)) {

Fixed.

if (!this.place ||
    (this.place.location.latitude != p.location.latitude ||
     this.place.location.longitude != p.location.longitude)) {

@@ +48,3 @@
+                !(this.place.location.latitude == p.location.latitude && this.place.location.longitude == p.location.longitude)) {
+                this._place = p;
+                this.text   = p ? p.name : "";

Fixed.

@@ +53,3 @@
+        } else {
+            this._place = p;
+                this.text   = p ? p.name : "";

Fixed.

::: src/sidebar.js
@@ +120,3 @@
+                                                      'via-remove-button',
+                                                      'via-entry-grid' ]);
+                                           mapView: this._mapView });

Fixed.

Review of attachment 284690 [details] [review]:

::: src/turnPointMarker.js
@@ +83,3 @@
+
+        this.draggable = true;
+

I solved this problem creating a new class InstructionMarker that inherits from TurnPointMarker and is destroyed as the bubble is closed.

@@ +98,3 @@
+                        location: new Geocode.Location({ latitude: this.latitude,
+                                                         longitude: this.longitude }) });
+        if (query.points[pointIndex].place != null) {

Fixed.

@@ +101,3 @@
+            query.points[pointIndex].place = place;
+        } else {
+            query.points[pointIndex + 1].place = place;

Comment added.

//This check is needed for updating the correct query point after 
//that a new viapoint row has been added. In details, it's needed for
//fixing the behaviour when a viapoint row has been added but it's empty.

Created attachment 284699 [details] [review]
route: Add via points support

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if at least two places are defined.

Created attachment 284700 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 284701 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284702 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284703 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284704 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284705 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284704 [details] [review]:

::: src/sidebar.js
@@ +167,3 @@
+                    this._mapView.showTurnPoint(row.turnPoint);
+            }).bind(this));
+

You should put this outside the callback, if not, any time route is updated you will reconnect that signal. You need to connect this just one time for the entire app lifetime.

Created attachment 284734 [details] [review]
Sidebar: Add spinner to instruction list

This adds a spinner to the instruction list. It is based on Damiáns marker patchers. Maybe you could rebase your series on this?

You also need to rebase on master since i commited a css fix to make the spinner spin against the theme background as well.

(In reply to comment #301)
> Created an attachment (id=284734) [details] [review]
> Sidebar: Add spinner to instruction list
> 
> This adds a spinner to the instruction list. It is based on Damiáns marker
> patchers. Maybe you could rebase your series on this?
> 
> You also need to rebase on master since i commited a css fix to make the
> spinner spin against the theme background as well.

You could also check how this is implemented and do something similar in a patch after you have made your changes to the sidebar.ui

And if you do not see spinners: jhbuild build adwaita-icon-theme

Review of attachment 284701 [details] [review]:

::: src/sidebar.js
@@ +163,3 @@
             }).bind(this));
+
+            this._timeInfo.label = _("Estimated time ") + Utils.prettyTime(route.time);

This string should be "Estimated time: %s", maybe we can skip the colon, but you surely need to put the time as a printf-like placeholder.

Maybe, you should add a comment to translators to explain what %s, something like:

/* Translators: %s is a time expression with the format "%f h" or "%f min" */

::: src/utils.js
@@ +274,3 @@
+    let labelledTime = "";
+    if (hours > 0)
+        labelledTime += _("%f h ").format(hours);

Trailing space can be problematic during translation if you put it inside the translatable string, it'd be better if you put the space away from translatable string:

_("%f h").format(hours) + ' ';

@@ +277,3 @@
+    if (minutes > 0)
+        labelledTime += _("%f min").format(minutes);
+    return labelledTime;

I'm not sure if this is the correct way to make this translatable or is better to have three translatable strings for the three possible cases here:

 - "%f h"
 - "%f min"
 - "%f h %f min"

Again, I'm not sure, we could ask it to someone with knowledge in i18n.

@@ +289,3 @@
+        else if (m > 0)
+            return _("%f m").format(m);
+}

Double indentation here :)

Review of attachment 284702 [details] [review]:

::: src/mapView.js
@@ +98,3 @@
+                                                        alpha: 100
+                                                    })
+        });

Arghh... I cannot see a way to use the Maps alignment style here, maybe moving the stroke color to a variable.

I do not want to be annoying, sorry :D

Review of attachment 284699 [details] [review]:

::: src/routeService.js
@@ +61,3 @@
+                    validPoints.push(this._query.points[i]);
+            }
+            if (validPoints.length >= 2)

> 1 is clearer I think.

Review of attachment 284699 [details] [review]:

::: src/routeService.js
@@ +61,3 @@
+                    validPoints.push(this._query.points[i]);
+            }
+            if (validPoints.length >= 2)

You say? I think is clearer >= 2 since we want to check if "we have at least 2 points to make a route"

Review of attachment 284703 [details] [review]:

::: src/turnPointMarker.js
@@ +109,3 @@
+        //This check is needed for updating the correct query point after
+        //that a new viapoint row has been added. In details, it's needed for
+        //fixing the behaviour when a viapoint row has been added but it's empty.

I still don't get this fully.

What if two empty rows has been added? Does this catch all cases?
And the comment still doesn't explain why we want to go to the next point
if this one is empty. And what if that one is empty as well?


If it's just me being dense at least fix the formating of this comment.
Add space after the // on each row. And just say 'row' instead of 'viapoint row'.
And maybe "when an empty row has been added" instead of "when a viapoint row has been added but it's empty."

Review of attachment 284699 [details] [review]:

::: src/routeService.js
@@ +61,3 @@
+                    validPoints.push(this._query.points[i]);
+            }
+            if (validPoints.length >= 2)

:) I don't feel strongly. But yeah, cause I see it as:

We want to check that "we have to have more than one point to make a route".

But either is fine I guess.

Review of attachment 284703 [details] [review]:

::: src/turnPointMarker.js
@@ +109,3 @@
+        //This check is needed for updating the correct query point after
+        //that a new viapoint row has been added. In details, it's needed for
+        //fixing the behaviour when a viapoint row has been added but it's empty.

Jonas is right, actually, a bug is reproducible:

1) Open sidebar, search for a route (just two points)
2) Add 2 via points, don't fill these
3) Drag the destination to a new place
4) 3rd entry is updated instead of destination entry

Review of attachment 284705 [details] [review]:

::: src/sidebar.js
@@ +159,2 @@
             route.turnPoints.forEach((function(turnPoint) {
+                let row = new InstructionRow({ visible:   true,

Don't align values, just one space after colon :)

One more concern.

p) I don't feel right on how reset works. First, there is a little bug, the via points are not deleted when sidebar is closed, so, if I search a route with 4 via points and hide then show sidebar, 4 empty place entries are show, I think that if are resetting, we need to show just 2 entries as happening during first sidebar reveal.
OTOH, I feel the route should not be reset on sidebar close, that can be annoying, the behavior should be:

function onSidebarHide() {
    hide route-related layers.
}

function onSidebarShow() {
    show route-related layers.

    if (route exists)
        goto route.
}

Isn't this most practical? What do you think?

Review of attachment 284703 [details] [review]:

::: src/turnPointMarker.js
@@ +109,3 @@
+        //This check is needed for updating the correct query point after
+        //that a new viapoint row has been added. In details, it's needed for
+        //fixing the behaviour when a viapoint row has been added but it's empty.

I think the fix for this should be easy actually. I think we want the init-function for the turnPointMarker to take the actual queryPoint that represents the marker.
Then you use the point.place to init the MapMarker parent. And then when you update the point.place then the binding will still be to the original entry.

Sounds easy enough? And then you will not have to create a place in the mapView.
You just have to refactor a bit to have the turnpoint and destination markers to take the queryPoint (or the pointIndex I guess and get the queryPoint in the init function)

(In reply to comment #312)
> One more concern.
> 
> p) I don't feel right on how reset works. First, there is a little bug, the via
> points are not deleted when sidebar is closed, so, if I search a route with 4
> via points and hide then show sidebar, 4 empty place entries are show, I think
> that if are resetting, we need to show just 2 entries as happening during first
> sidebar reveal.
> OTOH, I feel the route should not be reset on sidebar close, that can be
> annoying, the behavior should be:
> 
> function onSidebarHide() {
>     hide route-related layers.
> }
> 
> function onSidebarShow() {
>     show route-related layers.
> 
>     if (route exists)
>         goto route.
> }
> 
> Isn't this most practical? What do you think?

I agree with the most of this. I am not sure if we want to go to the route when we show the sidebar. If we are looking at an area and want to check a route around there. We open the sidebar and we get swept away to an old route we checked some time ago. I don't know. It might be the best, but I am not sure.

btw (I think that a routeExist-helper function might be a nice thing to have. Right now it is a open coded loop. But I think it might be needed to do the the spinner in the instructionList as well. So a helper function that performs that loop or check would be nice. Maybe on the routeService object?

Review of attachment 284701 [details] [review]:

::: src/sidebar.js
@@ +163,3 @@
             }).bind(this));
+
+            this._timeInfo.label = _("Estimated time ") + Utils.prettyTime(route.time);

Fixed.

::: src/utils.js
@@ +274,3 @@
+    let labelledTime = "";
+    if (hours > 0)
+    seconds = seconds % 60;

Fixed.

@@ +277,3 @@
+    if (minutes > 0)
+        labelledTime += _("%f min").format(minutes);
+    minutes = minutes % 60;

For now I'm going to leave it as it is.

@@ +289,3 @@
+        else if (m > 0)
+            return _("%f m").format(m);
+        labelledTime += _("%f min").format(minutes);

Argh... fixed. :)

Review of attachment 284702 [details] [review]:

::: src/mapView.js
@@ +98,3 @@
+                                                        alpha: 100
+                                                    })
+        });

Fixed in some way.

Review of attachment 284699 [details] [review]:

::: src/routeService.js
@@ +61,3 @@
+                    validPoints.push(this._query.points[i]);
+            }
+            if (validPoints.length >= 2)

I'm going to leave as it is. :)

Review of attachment 284705 [details] [review]:

::: src/sidebar.js
@@ +159,2 @@
             route.turnPoints.forEach((function(turnPoint) {
+                let row = new InstructionRow({ visible:   true,

Fixed.

Review of attachment 284704 [details] [review]:

::: src/sidebar.js
@@ +167,3 @@
+                    this._mapView.showTurnPoint(row.turnPoint);
+            }).bind(this));
+

Fixed.

Created attachment 284833 [details] [review]
route: Add via points support

Route query and service have now support to via points.
The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.
The routing query allows all the basic operations on points.
A query is legal if at least two places are defined.

Created attachment 284834 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 284835 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.
Every instruction containes information about the distance.
There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 284836 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 284837 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.
The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 284839 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 284840 [details] [review]
sidebar: Small graphical changes

Add a new separator between the sidebar form and
the instruction list.
Remove a glitch on mouse hover on the form.
The sidebar form is now transparent.

Review of attachment 284837 [details] [review]:

just a minor nitpick but I see it in a few other patches of yours too so I thought maybe I should point out: Please keep empty lines between paragraphs.

Review of attachment 284837 [details] [review]:

Just to be clear, I meant in the commit logs. :)

I tried to reproduce the issues I reported in comment#259, these are the results.

(In reply to comment #259)
> a) I don't have any feedback of when the routes are loading, only when route is
> located or not found. If you ask me, I would put a spinner in the instructions
> list to know if the route is loading.

There is a little problem with this one. When I search a route, the first time works fine, but when I change the route in a way that doesn't require a reset (like changing transportation method or adding a via point), I need to scroll down to see the spinner. It seems that we need to clear the instruction list before doing anything.

> 
> c) If I drag the destination point to another place and the route search fails,
> the the old path remains visible with the destination pin in other place, away
> from the path. I don't know how this can be solved, maybe is not a bug and it's
> fine to keep this behavior, but it cached my attention.

Same as before. I think nobody is going to die be keep it as is. From my point of view the ideal would be to hide the path but not the markers and leave the query as is when the route isn't found.

> 
> e) I always can see an horizontal scrollbar in the instructions list (when it
> has results).

Same as before.

> h) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, don't do anything, just press the button
>    4) Drag the destination in the map view to another place
>    5) Via point entry is changed instead of final destination

Fixed.

> 
> i) After search for a route, when I press an instruction list row, the bubble
> delays a lot to appear and I see the same amount of this lines in output
> "Gjs-Message: JS LOG: DEBUG: Going to null" as instructions rows I have. I
> mean, if I have 20 rows in the instructions list, I see 20 lines with the
> mentioned content when I click a row before actually seeing the bubble. If I
> perform the same route search again, I see 40 lines of logs (2 times the
> previous one).

Fixed.

> 
> j) Using the instructions list with keyboard arrows doesn't updates the
> bubbles.

Fixed.

> 
> k) I think widget tab order must be tweaked so I can change focus between text
> entries with a single tab press ("add" and "remove" buttons tab order can be
> after text entries).

Same as before.

> 
> l) Maybe there is no need to perform the search again if any via points changed
> (I mean, user writes down the same place in the same entry).

I think is fixed.

> 
> m) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, don't do anything, just press the button
>    4) Drag the origin in the map view to another place
>    5) Route not refreshed
>    6) Try to fill the via point entry added in (3), route not refreshed

Fixed.

> 
> n) 1) Open the sidebar, write start point and ending point
>    2) Wait for route search to finish
>    3) Press "+" to add a via point, fill the new entry
>    4) Wait for the search to finish
>    5) Delete the created via point
>    6) Now route goes from origin to deleted via point, so the route is
> inconsistent with the sidebar information

Fixed.

> 
> o) Pressing the entry's clear button should update the route?

Fixed


q) The via point removal is quite broken.
   - Open the sidebar (at startup), don't search anything, add two via points, then remove one, all place entries disappear except "From" place entry.
   - I think this one was mentioned by Dario: open the sidebar (at startup), add one or more via points, close the sidebar, open it again, everything disappear execpt "From" place entry.
I'm going to try to figure out what's happening and offer some suggestion.

r) After fill a place entry or by opening -> closing -> opening sidebar for the first time at startup, the UI container that holds total estimated time and total distance disappear. The result is that I cannot see this info at all.

Review of attachment 284734 [details] [review]:

In the commit log, Sidebar must be capitalized?

::: src/sidebar.js
@@ +114,3 @@
+
+        this._instructionStack.visible_child = listBox;
+        listBox.forall(listBox.remove.bind(listBox));

Maybe this method is too much, and since is called _showInstructions, deleting all rows of the list seems confusing.

@@ +124,3 @@
+
+        query.connect('updated', (function() {
+            if (query.from && query.to)

I think we need to clear the instruction list here to solve the vertical scroll issue that I've mentioned in comment#329 or as you suggest, put the spinner outside the scrolledwindow.

Review of attachment 284833 [details] [review]:

::: src/routeQuery.js
@@ +107,3 @@
     },
 
+    getFilledPoints: function() {

Maybe "get filledPoints()"?

Review of attachment 284834 [details] [review]:

::: src/sidebar.js
@@ +170,3 @@
+                    this._sidebarForm.remove(child);
+            }).bind(this));
+        }).bind(this));

Are you deleting place entries when the route is resetted, why? maybe you misspelled route with query?

@@ +174,1 @@
+        query.connect('notify::points', (function() {

What happen if I change the transportation method?
Maybe removing updated signal from RouteQuery wasn't a good idea.

Review of attachment 284834 [details] [review]:

::: src/sidebar.js
@@ +169,3 @@
+                if ((child.get_index() != 0) && (child.get_index() != lastRowIndex))
+                    this._sidebarForm.remove(child);
+            }).bind(this));

This is messy. Are we sure it is safe?

Wouldn't the best way to make sure that only the via-point entries are in this listbox? Either by putting the from above and the to below the GtkListBox, or having an additional listbox for the via-point entries?
Then we could just remove all the children. And it might simplify code elsewhere as weöö?

Also, the documentation specifies that calling destroy on the child is better than doing container.remove:

"If you don’t want to use widget again it’s usually more efficient to simply destroy it directly using gtk_widget_destroy() since this will remove it from the container and help break any circular reference count cycles."

https://developer.gnome.org/gtk3/stable/GtkContainer.html#gtk-container-remove

So in this case it would be child.destroy() instead of this._sidebarForm.remove(child)

Created attachment 284976 [details] [review]
sidebar: Add spinner to instructionList

Updated after Damians comments, the stack now has the scrolled window and the spinner as children.

Created attachment 285006 [details] [review]
route: Add via points support

Route query and service have now support to via points.

The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.

The routing query allows all the basic operations on points.

A query is legal if at least two places are defined.

Created attachment 285007 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 285008 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.

Every instruction containes information about the distance.

There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 285009 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 285010 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.

The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 285011 [details] [review]
routing: Show turnpoint marker on instruction selection

Review of attachment 285006 [details] [review]:

::: src/routeQuery.js
@@ +116,1 @@
     },

I liked Damiáns suggestion of making this a getter:

get filledPoints() {
[...]
},

Then we access it with query.filledPoints in other modules and this.filledPoints.length below.

Review of attachment 285007 [details] [review]:

Something went wrong with the merging of this one :)
Please test before attaching to bugzilla.

also: No spinner?

::: src/sidebar.ui
@@ +149,3 @@
+            <property name="visible">True</property>
+            <property name="can_focus">False</property>
+<<<<<<< HEAD

mismerge

@@ +168,2 @@
             </child>
+=======

mismerge

Created attachment 285015 [details] [review]
route: Add via points support

Route query and service have now support to via points.

The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.

The routing query allows all the basic operations on points.

A query is legal if at least two places are defined.

Created attachment 285016 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 285017 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.

Every instruction containes information about the distance.

There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 285018 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 285019 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.

The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 285020 [details] [review]
routing: Show turnpoint marker on instruction selection

Review of attachment 284976 [details] [review]:

I think we need another child for the stack that doesn't show any widgets at all, this child should be show as a initial state of the sidebar and when the route is not found (or even better, a child with a label "Route not found" for the second case). I suggesting this since it cached my attention that Maps keep showing the spinner after the route isn't found.

::: src/sidebar.ui
@@ +149,3 @@
+        <style>
+          <class name="maps-stack"/>
+        </style>

I don't get this, why do we want to change the stack background color, for some reason, this is also causing the removal of left border of the GtkStack, can you see that glitch?

Review of attachment 285016 [details] [review]:

After applying this patch I cannot see the left border of the pedestrian mode button.

http://imgur.com/uocWJUg

::: src/sidebar.js
@@ +165,3 @@
         }).bind(this));
 
+        query.connect('notify::points', (function() {

We still not showing spinner during transportation mode changes...

::: src/sidebar.ui
@@ +188,3 @@
+    <child>
+      <object class="GtkSeparator">
+        <property name="visible">True</property>

Errr... dat separator... I was trying to get rid of it and change it by GtkStack top border plus a bit of top margin, but I was unsuccessful.

Review of attachment 285019 [details] [review]:

Good work!

Review of attachment 285018 [details] [review]:

Looks ok otherwise.

::: src/mapView.js
@@ +92,2 @@
     _initLayers: function() {
+        let stroke_color = new Clutter.Color({ red: 0,

Oh no, the variable uses underscore style naming :-[

Review of attachment 285017 [details] [review]:

Hmmm... I think you are hauling a commit error here: "time-info" and "distance-info" are supposed to be added in sidebar.ui in this commit instead of "sidebar: Add via points support", am I right?

Review of attachment 285018 [details] [review]:

::: src/mapView.js
@@ +92,2 @@
     _initLayers: function() {
+        let stroke_color = new Clutter.Color({ red: 0,

Fixed.

Review of attachment 285016 [details] [review]:

::: src/sidebar.js
@@ +165,3 @@
         }).bind(this));
 
+        query.connect('notify::points', (function() {

Fixed.

Review of attachment 285017 [details] [review]:

Fixed.

Created attachment 285059 [details] [review]
sidebar: Add spinner to instructionList

Created attachment 285060 [details] [review]
route: Add via points support

Route query and service have now support to via points.

The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.

The routing query allows all the basic operations on points.

A query is legal if at least two places are defined.

Created attachment 285061 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.

Created attachment 285062 [details] [review]
sidebar: Add indications about distance and time

Add a new row in the sidebar that contains information about the
estimated time for reaching the destination and the distance to it.

Every instruction containes information about the distance.

There are two new utility functions for writing in the correct format
the distances and the estimated time.

Created attachment 285063 [details] [review]
mapView: Change route path width and color

The routing path now is blue and the width is set to 5px.

Created attachment 285064 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.

The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 285065 [details] [review]
routing: Show turnpoint marker on instruction selection

Created attachment 285124 [details] [review]
route: Add via points support

Route query and service have now support to via points.

The routing query has no more 'from' and 'to' properties but
it has a new property called 'points' that is an array of places.

The routing query allows all the basic operations on points.

A query is legal if at least two places are defined.

Created attachment 285127 [details] [review]
mapView: New icons for destination points

Created attachment 285128 [details] [review]
routing: New markers for turnpoint

These markers are showed if a turnpoint is a stop.

The markers are draggable and after drag is finished a new route is
calculated and shown.

Created attachment 285137 [details] [review]
sidebar: Add spinner to instructionList

Created attachment 285138 [details] [review]
route: Add via points support

Created attachment 285139 [details] [review]
sidebar: Add via points support