Yes, agreed!

For whomever wants to work on this, check in src/sidebar.js where we set the visible child of the instructionStack to instructionSpinner...

Right now we show it if query.isValid(), maybe we should also check that the path array of the route has a length that is !== 0? Or is there a more elegant solution?

Created attachment 322553 [details] [review]
Patch to avoid infinte display intsruction spinner

If we enter source and destination to be the same place
or if route request fails for some reason then we need to
reset route to avoid infinite display of instruction spinner

Created attachment 322649 [details] [review]
route-not-found:revert to previous route if new one not found

if any new entry added to exixting results in route not found
then revert back to previous route and remove the point from
route list. Add latest field to routeQuery which keeps the
latest point added. and in routeService if route is not found
remove latest point from the points list.

Review of attachment 322649 [details] [review]:

Thanks!

Some comments below!

Also for shortlog, maybe:

"Routing: Remove latest place on failure" ?

::: src/routeQuery.js
@@ +106,3 @@
     },
+    
+    get latest(){

space before brace!

@@ +111,3 @@
+
+    set latest(point){
+        this._latest=point;

Spaces! :)

@@ +117,3 @@
         this.parent(args);
         this._points = [];
+        this._latest=null;

No need for this, it can be undefined from start.

@@ +133,3 @@
         this.notify('points');
         this.emit('point-added', point, index);
+        this._latest=point;

this._latest = point; (and maybe set this before emit and notify.

@@ +140,3 @@
         let removedPoints = this._points.splice(index, 1);
         let point = removedPoints ? removedPoints[0] : null;
+        if(point===this._latest){

You need spaces!

if (point === this._latest) {
^^^^    ^^^^^^^^^^           ^^^
here    and here             also here

@@ +141,3 @@
         let point = removedPoints ? removedPoints[0] : null;
+        if(point===this._latest){
+            this._latest=null;

this._latest = null;

@@ +164,3 @@
             point.place = null;
         });
+        this._latest=null;

spaces...

::: src/routeService.js
@@ +101,3 @@
+                    else{
+                        this.route.reset();
+                                break; 

No, I do not like this!

We should _not_ remove the point. I want:

if (this.query.latest)
    this.query.latest.place = null;
else
    this.route.reset();

@@ +108,3 @@
             } catch(e) {
                 Application.notificationManager.showMessage(_("Route request failed."));
                 log(e);

And I want the same code here.

Created attachment 322661 [details] [review]
Routing: Remove latest place on failure

If a new entry is added to exixting route which results in
route not found then revert back to previous route and set the place
of the point to null. To implement this Add a latest field to routeQuery
which keeps the latest point added. and in routeService if route
is not found set latest.place to null.

Review of attachment 322661 [details] [review]:

Thanks!

Other than small comments, looks great!

::: src/routeQuery.js
@@ +113,3 @@
+        this._latest = point;
+        this.notify('points');
+    },

Two things,

1) This is never used?
2) I do not see the need to notify here

@@ +130,2 @@
         point.connect('notify::place', (function() {
             this.notify('points');

We should set ...

this._latest = point;

... here as well, to catch latest place added.

::: src/routeService.js
@@ +89,3 @@
                 if (!result) {
                     Application.notificationManager.showMessage(_("No route found."));
+                    if(this.query.latest) 

if (

@@ +100,3 @@
             } catch(e) {
                 Application.notificationManager.showMessage(_("Route request failed."));
                 log(e);

if (

Created attachment 322695 [details] [review]
Routing: Remove latest place on failure

If a new entry is added to exixting route which results in
route not found then revert back to previous route and set the place
of the point to null. To implement this Add a latest field to routeQuery
which keeps the latest point added. and in routeService if route
is not found set latest.place to null.

Review of attachment 322695 [details] [review]:

Thanks!

Looks great! Please run git diff --check before commiting! Or git diff HEAD~1 --check after commiting! This adds whitespace damage, and I will not apply it in this state.

$ git diff HEAD~1 --check
src/routeQuery.js:110: trailing whitespace.
+    
src/routeQuery.js:152: trailing whitespace.
+        if (point === this._latest) 
src/routeQuery.js:154: trailing whitespace.
+        
src/routeService.js:91: trailing whitespace.
+                    if(this.query.latest) 
src/routeService.js:93: trailing whitespace.
+                    else 
src/routeService.js:95: trailing whitespace.
+                    
src/routeService.js:103: trailing whitespace.
+                if(this.query.latest) 
src/routeService.js:105: trailing whitespace.
+                else

::: src/routeQuery.js
@@ +112,3 @@
+    set latest(point) {
+        this._latest = point;
+    },

Remove this if unused.

Created attachment 322724 [details] [review]
Routing: Remove latest place on failure

If a new entry is added to exixting route which results in
route not found then revert back to previous route and set the place
of the point to null. To implement this Add a latest field to routeQuery
which keeps the latest point added. and in routeService if route
is not found set latest.place to null.

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

Review of attachment 322724 [details] [review]:

Thanks looks awesome!

::: src/routeService.js
@@ +92,3 @@
+                        this.query.latest.place = null;
+                        if (!this.query.isValid())
+                            this.route.reset();

I think is not needed, and instead we can do a small fix in sidebar.js

Review of attachment 322724 [details] [review]:

pushed