After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 731068 - Add support for via points
Add support for via points
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on: 722871 728695
Blocks:
 
 
Reported: 2014-06-01 09:15 UTC by Dario Di Nucci
Modified: 2014-09-03 05:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
routing: GraphHopper interface change for allowing viapoints (5.36 KB, patch)
2014-06-01 09:34 UTC, Dario Di Nucci
none Details | Review
routing: GraphHopper interface change for allowing viapoints (4.12 KB, patch)
2014-06-03 11:05 UTC, Dario Di Nucci
none Details | Review
Updated GraphHopper interface for allowing viapoints (11.01 KB, patch)
2014-07-13 14:42 UTC, Dario Di Nucci
none Details | Review
Updated GraphHopper interface for allowing viapoints (11.01 KB, patch)
2014-07-13 14:46 UTC, Dario Di Nucci
none Details | Review
Update GraphHopper interface for allowing viapoints (11.01 KB, patch)
2014-07-13 14:47 UTC, Dario Di Nucci
needs-work Details | Review
Changes to routing service interface for via points support (6.84 KB, patch)
2014-07-31 21:40 UTC, Dario Di Nucci
none Details | Review
Add via points support to sidebar (10.92 KB, patch)
2014-07-31 21:41 UTC, Dario Di Nucci
none Details | Review
Add indications about distance and time in the sidebar (4.88 KB, patch)
2014-07-31 21:41 UTC, Dario Di Nucci
none Details | Review
Changed path's width and color (1.14 KB, patch)
2014-07-31 21:41 UTC, Dario Di Nucci
none Details | Review
New turnpoint markers (11.54 KB, patch)
2014-07-31 21:41 UTC, Dario Di Nucci
none Details | Review
On instraction selection show turnpoint marker (4.99 KB, patch)
2014-07-31 21:42 UTC, Dario Di Nucci
none Details | Review
Small graphical changes to the sidebar (4.58 KB, patch)
2014-07-31 21:42 UTC, Dario Di Nucci
none Details | Review
Changes to routing service interface for via points support (8.86 KB, patch)
2014-07-31 21:59 UTC, Dario Di Nucci
none Details | Review
Changes to routing service interface for via points support (8.86 KB, patch)
2014-07-31 22:31 UTC, Dario Di Nucci
none Details | Review
Add via points support to sidebar (10.35 KB, patch)
2014-07-31 22:31 UTC, Dario Di Nucci
none Details | Review
Add indications about distance and time in the sidebar (4.88 KB, patch)
2014-07-31 22:31 UTC, Dario Di Nucci
none Details | Review
Change path's width and color (1.14 KB, patch)
2014-07-31 22:31 UTC, Dario Di Nucci
reviewed Details | Review
New turnpoint markers (12.05 KB, patch)
2014-07-31 22:31 UTC, Dario Di Nucci
none Details | Review
On instruction selection show turnpoint marker (4.99 KB, patch)
2014-07-31 22:32 UTC, Dario Di Nucci
none Details | Review
Small graphical changes to the sidebar (4.58 KB, patch)
2014-07-31 22:32 UTC, Dario Di Nucci
none Details | Review
Changes to routing service interface for via points support (8.86 KB, patch)
2014-07-31 23:26 UTC, Dario Di Nucci
needs-work Details | Review
Add via points support to sidebar (10.35 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
needs-work Details | Review
Add indications about distance and time in the sidebar (4.89 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
needs-work Details | Review
Changed path's width and color (1.14 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
reviewed Details | Review
New turnpoint markers (12.06 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
none Details | Review
On instruction selection show turnpoint marker (4.54 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
reviewed Details | Review
Small graphical changes to the sidebar (4.37 KB, patch)
2014-07-31 23:27 UTC, Dario Di Nucci
needs-work Details | Review
Changes to routing service interface for via points support (7.13 KB, patch)
2014-08-04 14:43 UTC, Dario Di Nucci
needs-work Details | Review
Add via points support to sidebar (10.12 KB, patch)
2014-08-04 14:44 UTC, Dario Di Nucci
needs-work Details | Review
Add indications about distance and time in the sidebar (4.90 KB, patch)
2014-08-04 14:44 UTC, Dario Di Nucci
needs-work Details | Review
New turnpoint markers (11.91 KB, patch)
2014-08-04 14:47 UTC, Dario Di Nucci
needs-work Details | Review
route: add support for via points (7.17 KB, patch)
2014-08-04 17:04 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: add via points support (10.12 KB, patch)
2014-08-04 17:04 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: add indications about distance and time (5.15 KB, patch)
2014-08-04 17:05 UTC, Dario Di Nucci
needs-work Details | Review
New turnpoint markers (12.29 KB, patch)
2014-08-04 17:06 UTC, Dario Di Nucci
needs-work Details | Review
On instruction selection show turnpoint marker (3.57 KB, patch)
2014-08-04 17:06 UTC, Dario Di Nucci
needs-work Details | Review
Small graphical changes to the sidebar (4.39 KB, patch)
2014-08-04 17:06 UTC, Dario Di Nucci
none Details | Review
route: add support for via points (6.85 KB, patch)
2014-08-16 15:41 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: add via points support (9.52 KB, patch)
2014-08-16 15:42 UTC, Dario Di Nucci
none Details | Review
sidebar: add indications about distance and time (4.92 KB, patch)
2014-08-16 15:43 UTC, Dario Di Nucci
needs-work Details | Review
New turnpoint markers (12.16 KB, patch)
2014-08-16 15:44 UTC, Dario Di Nucci
needs-work Details | Review
route: add support for via points (6.81 KB, patch)
2014-08-18 14:14 UTC, Dario Di Nucci
none Details | Review
sidebar: add via points support (9.64 KB, patch)
2014-08-18 14:16 UTC, Dario Di Nucci
none Details | Review
sidebar: add indications about distance and time (4.92 KB, patch)
2014-08-18 14:17 UTC, Dario Di Nucci
reviewed Details | Review
New turnpoint markers (14.82 KB, patch)
2014-08-18 14:17 UTC, Dario Di Nucci
needs-work Details | Review
On instruction selection show turnpoint marker (2.46 KB, patch)
2014-08-18 14:18 UTC, Dario Di Nucci
needs-work Details | Review
route: add support for via points (6.82 KB, patch)
2014-08-18 14:41 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: add via points support (10.29 KB, patch)
2014-08-18 14:42 UTC, Dario Di Nucci
needs-work Details | Review
route: add support for via points (6.77 KB, patch)
2014-08-19 12:34 UTC, Dario Di Nucci
none Details | Review
route: add support for via points (6.77 KB, patch)
2014-08-19 12:36 UTC, Dario Di Nucci
needs-work Details | Review
route: add support for via points (6.76 KB, patch)
2014-08-19 12:45 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.71 KB, patch)
2014-08-19 12:51 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add via points support (10.22 KB, patch)
2014-08-19 13:49 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.68 KB, patch)
2014-08-19 13:56 UTC, Dario Di Nucci
none Details | Review
route: New markers for turn point (15.53 KB, patch)
2014-08-19 15:38 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (15.53 KB, patch)
2014-08-19 15:42 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (15.53 KB, patch)
2014-08-19 16:04 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1.62 KB, patch)
2014-08-19 16:06 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (3.16 KB, patch)
2014-08-19 16:09 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.68 KB, patch)
2014-08-20 22:29 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (10.92 KB, patch)
2014-08-20 22:29 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-20 22:30 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-20 22:31 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.38 KB, patch)
2014-08-20 22:31 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-20 22:31 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-20 22:32 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.68 KB, patch)
2014-08-21 07:09 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (10.86 KB, patch)
2014-08-21 07:10 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 07:10 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 07:10 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.52 KB, patch)
2014-08-21 07:10 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 07:11 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 07:11 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.68 KB, patch)
2014-08-21 07:24 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (10.86 KB, patch)
2014-08-21 07:24 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 07:25 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 07:26 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.47 KB, patch)
2014-08-21 07:26 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 07:27 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 07:27 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.72 KB, patch)
2014-08-21 07:47 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Add via points support (10.85 KB, patch)
2014-08-21 07:48 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 07:48 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 07:49 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.47 KB, patch)
2014-08-21 07:50 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 07:50 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 07:50 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.67 KB, patch)
2014-08-21 08:11 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (10.85 KB, patch)
2014-08-21 08:11 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 08:12 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 08:12 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.47 KB, patch)
2014-08-21 08:12 UTC, Dario Di Nucci
reviewed Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 08:13 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 08:13 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.67 KB, patch)
2014-08-21 10:20 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (11.13 KB, patch)
2014-08-21 10:21 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 10:21 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 10:21 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.06 KB, patch)
2014-08-21 10:22 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 10:22 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 10:22 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.67 KB, patch)
2014-08-21 15:53 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (11.07 KB, patch)
2014-08-21 15:53 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 15:53 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (6.67 KB, patch)
2014-08-21 15:54 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (11.07 KB, patch)
2014-08-21 15:54 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-21 15:54 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-21 15:54 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.18 KB, patch)
2014-08-21 15:54 UTC, Dario Di Nucci
needs-work Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-21 15:55 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-21 15:55 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (9.39 KB, patch)
2014-08-22 14:56 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add via points support (11.06 KB, patch)
2014-08-22 14:56 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.69 KB, patch)
2014-08-22 14:56 UTC, Dario Di Nucci
reviewed Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-22 14:56 UTC, Dario Di Nucci
reviewed Details | Review
routing: New markers for turnpoint (14.13 KB, patch)
2014-08-22 14:57 UTC, Dario Di Nucci
needs-work Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-22 14:57 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-22 14:57 UTC, Dario Di Nucci
none Details | Review
route: Add support for via points (9.39 KB, patch)
2014-08-25 15:27 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (14.88 KB, patch)
2014-08-25 15:27 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.70 KB, patch)
2014-08-25 15:27 UTC, Dario Di Nucci
none Details | Review
mapView: change route path width and color (1.27 KB, patch)
2014-08-25 15:28 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.12 KB, patch)
2014-08-25 15:28 UTC, Dario Di Nucci
reviewed Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-25 15:28 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-25 15:28 UTC, Dario Di Nucci
none Details | Review
sidebar: Add Via points support - [jonas] (18.72 KB, patch)
2014-08-26 13:06 UTC, Jonas Danielsson
none Details | Review
sidebar: Add Via points support - [jonas] (18.72 KB, patch)
2014-08-26 13:12 UTC, Jonas Danielsson
none Details | Review
route: Add via points support (7.09 KB, patch)
2014-08-26 18:23 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add via points support (16.33 KB, patch)
2014-08-26 18:25 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.70 KB, patch)
2014-08-26 18:25 UTC, Dario Di Nucci
reviewed Details | Review
mapView: Change route path width and color (1.27 KB, patch)
2014-08-26 18:26 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.76 KB, patch)
2014-08-26 18:26 UTC, Dario Di Nucci
reviewed Details | Review
routing: Show turnpoint marker on instruction selection (1011 bytes, patch)
2014-08-26 18:26 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.89 KB, patch)
2014-08-26 18:26 UTC, Dario Di Nucci
none Details | Review
route: Add via points support (8.03 KB, patch)
2014-08-28 12:04 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (21.84 KB, patch)
2014-08-28 12:04 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.87 KB, patch)
2014-08-28 12:05 UTC, Dario Di Nucci
none Details | Review
mapView: Change route path width and color (1.27 KB, patch)
2014-08-28 12:05 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.91 KB, patch)
2014-08-28 12:05 UTC, Dario Di Nucci
reviewed Details | Review
routing: Show turnpoint marker on instruction selection (1001 bytes, patch)
2014-08-28 12:05 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.83 KB, patch)
2014-08-28 12:06 UTC, Dario Di Nucci
none Details | Review
route: Add via points support (8.03 KB, patch)
2014-08-28 14:18 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (21.84 KB, patch)
2014-08-28 14:18 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (4.87 KB, patch)
2014-08-28 14:19 UTC, Dario Di Nucci
needs-work Details | Review
mapView: Change route path width and color (1.27 KB, patch)
2014-08-28 14:19 UTC, Dario Di Nucci
reviewed Details | Review
routing: New markers for turnpoint (15.29 KB, patch)
2014-08-28 14:19 UTC, Dario Di Nucci
needs-work Details | Review
routing: Show turnpoint marker on instruction selection (1001 bytes, patch)
2014-08-28 14:19 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Small graphical changes (2.83 KB, patch)
2014-08-28 14:20 UTC, Dario Di Nucci
reviewed Details | Review
Sidebar: Add spinner to instruction list (4.13 KB, patch)
2014-08-28 18:55 UTC, Jonas Danielsson
needs-work Details | Review
route: Add via points support (7.84 KB, patch)
2014-08-29 19:14 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Add via points support (22.85 KB, patch)
2014-08-29 19:14 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (4.80 KB, patch)
2014-08-29 19:15 UTC, Dario Di Nucci
none Details | Review
mapView: Change route path width and color (1.17 KB, patch)
2014-08-29 19:15 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.36 KB, patch)
2014-08-29 19:15 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (928 bytes, patch)
2014-08-29 19:16 UTC, Dario Di Nucci
none Details | Review
sidebar: Small graphical changes (2.83 KB, patch)
2014-08-29 19:16 UTC, Dario Di Nucci
none Details | Review
sidebar: Add spinner to instructionList (4.33 KB, patch)
2014-09-01 07:23 UTC, Jonas Danielsson
needs-work Details | Review
route: Add via points support (7.83 KB, patch)
2014-09-01 12:58 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Add via points support (26.69 KB, patch)
2014-09-01 12:58 UTC, Dario Di Nucci
needs-work Details | Review
sidebar: Add indications about distance and time (5.70 KB, patch)
2014-09-01 12:59 UTC, Dario Di Nucci
none Details | Review
mapView: Change route path width and color (1.17 KB, patch)
2014-09-01 12:59 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.36 KB, patch)
2014-09-01 12:59 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (928 bytes, patch)
2014-09-01 13:00 UTC, Dario Di Nucci
none Details | Review
route: Add via points support (7.83 KB, patch)
2014-09-01 14:02 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (25.60 KB, patch)
2014-09-01 14:02 UTC, Dario Di Nucci
reviewed Details | Review
sidebar: Add indications about distance and time (3.99 KB, patch)
2014-09-01 14:02 UTC, Dario Di Nucci
none Details | Review
mapView: Change route path width and color (1.17 KB, patch)
2014-09-01 14:03 UTC, Dario Di Nucci
reviewed Details | Review
routing: New markers for turnpoint (14.30 KB, patch)
2014-09-01 14:03 UTC, Dario Di Nucci
reviewed Details | Review
routing: Show turnpoint marker on instruction selection (962 bytes, patch)
2014-09-01 14:03 UTC, Dario Di Nucci
none Details | Review
sidebar: Add spinner to instructionList (5.51 KB, patch)
2014-09-01 19:30 UTC, Dario Di Nucci
none Details | Review
route: Add via points support (7.83 KB, patch)
2014-09-01 19:30 UTC, Dario Di Nucci
none Details | Review
sidebar: Add via points support (23.15 KB, patch)
2014-09-01 19:30 UTC, Dario Di Nucci
none Details | Review
sidebar: Add indications about distance and time (6.71 KB, patch)
2014-09-01 19:31 UTC, Dario Di Nucci
none Details | Review
mapView: Change route path width and color (1.17 KB, patch)
2014-09-01 19:31 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (14.30 KB, patch)
2014-09-01 19:31 UTC, Dario Di Nucci
none Details | Review
routing: Show turnpoint marker on instruction selection (962 bytes, patch)
2014-09-01 19:31 UTC, Dario Di Nucci
none Details | Review
route: Add via points support (8.21 KB, patch)
2014-09-02 09:37 UTC, Dario Di Nucci
none Details | Review
mapView: New icons for destination points (51.37 KB, patch)
2014-09-02 10:41 UTC, Dario Di Nucci
none Details | Review
routing: New markers for turnpoint (13.92 KB, patch)
2014-09-02 10:42 UTC, Dario Di Nucci
none Details | Review
sidebar: Add spinner to instructionList (5.52 KB, patch)
2014-09-02 13:31 UTC, Jonas Danielsson
committed Details | Review
route: Add via points support (7.88 KB, patch)
2014-09-02 13:32 UTC, Jonas Danielsson
committed Details | Review
sidebar: Add via points support (23.17 KB, patch)
2014-09-02 13:33 UTC, Jonas Danielsson
committed Details | Review
sidebar: Add indications about distance and time (6.52 KB, patch)
2014-09-02 13:33 UTC, Jonas Danielsson
committed Details | Review
mapView: Change route path width and color (1.17 KB, patch)
2014-09-02 13:33 UTC, Jonas Danielsson
committed Details | Review
Add new icons for destination points (53.88 KB, patch)
2014-09-02 13:34 UTC, Jonas Danielsson
accepted-commit_after_freeze Details | Review
routing: New markers for turnpoint (14.37 KB, patch)
2014-09-02 13:34 UTC, Jonas Danielsson
committed Details | Review
routing: Show marker on instruction selection (958 bytes, patch)
2014-09-02 13:34 UTC, Jonas Danielsson
committed Details | Review
Add new icons for destination points (74.37 KB, patch)
2014-09-02 13:53 UTC, Jonas Danielsson
committed Details | Review

Description Dario Di Nucci 2014-06-01 09:15:06 UTC
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).
Comment 1 Dario Di Nucci 2014-06-01 09:34:16 UTC
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.
Comment 2 Zeeshan Ali 2014-06-01 11:37:00 UTC
(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.
Comment 3 Mattias Bengtsson 2014-06-02 00:21:59 UTC
(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.
Comment 4 Mattias Bengtsson 2014-06-02 00:43:32 UTC
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?
Comment 5 Zeeshan Ali 2014-06-02 11:26:15 UTC
(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.
Comment 6 Mattias Bengtsson 2014-06-02 12:45:48 UTC
(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
Comment 7 Zeeshan Ali 2014-06-02 13:24:31 UTC
(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?
Comment 8 Dario Di Nucci 2014-06-03 11:00:25 UTC
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.
Comment 9 Dario Di Nucci 2014-06-03 11:05:24 UTC
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.
Comment 10 Dario Di Nucci 2014-07-13 14:42:53 UTC
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.
Comment 11 Dario Di Nucci 2014-07-13 14:46:24 UTC
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.
Comment 12 Dario Di Nucci 2014-07-13 14:47:07 UTC
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.
Comment 13 Jonas Danielsson 2014-07-14 10:39:21 UTC
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.
Comment 14 Dario Di Nucci 2014-07-31 21:40:59 UTC
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.
Comment 15 Dario Di Nucci 2014-07-31 21:41:19 UTC
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.
Comment 16 Dario Di Nucci 2014-07-31 21:41:29 UTC
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.
Comment 17 Dario Di Nucci 2014-07-31 21:41:41 UTC
Created attachment 282210 [details] [review]
Changed path's width and color

The routing path now is blue and the width is setted to 5px.
Comment 18 Dario Di Nucci 2014-07-31 21:41:51 UTC
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.
Comment 19 Dario Di Nucci 2014-07-31 21:42:01 UTC
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.
Comment 20 Dario Di Nucci 2014-07-31 21:42:09 UTC
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.
Comment 21 Dario Di Nucci 2014-07-31 21:59:49 UTC
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.
Comment 22 Dario Di Nucci 2014-07-31 22:31:13 UTC
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.
Comment 23 Dario Di Nucci 2014-07-31 22:31:24 UTC
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.
Comment 24 Dario Di Nucci 2014-07-31 22:31:38 UTC
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.
Comment 25 Dario Di Nucci 2014-07-31 22:31:49 UTC
Created attachment 282219 [details] [review]
Change path's width and color

The routing path now is blue and the width is setted to 5px.
Comment 26 Dario Di Nucci 2014-07-31 22:31:56 UTC
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.
Comment 27 Dario Di Nucci 2014-07-31 22:32:07 UTC
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.
Comment 28 Dario Di Nucci 2014-07-31 22:32:16 UTC
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.
Comment 29 Dario Di Nucci 2014-07-31 23:26:59 UTC
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.
Comment 30 Dario Di Nucci 2014-07-31 23:27:07 UTC
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.
Comment 31 Dario Di Nucci 2014-07-31 23:27:15 UTC
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.
Comment 32 Dario Di Nucci 2014-07-31 23:27:22 UTC
Created attachment 282227 [details] [review]
Changed path's width and color

The routing path now is blue and the width is setted to 5px.
Comment 33 Dario Di Nucci 2014-07-31 23:27:33 UTC
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.
Comment 34 Dario Di Nucci 2014-07-31 23:27:41 UTC
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.
Comment 35 Dario Di Nucci 2014-07-31 23:27:51 UTC
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.
Comment 36 Jonas Danielsson 2014-08-02 21:37:16 UTC
Review of attachment 282219 [details] [review]:

Thx!

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

And "setted" to "set".
Comment 37 Jonas Danielsson 2014-08-02 21:51:28 UTC
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.
Comment 38 Jonas Danielsson 2014-08-02 21:51:37 UTC
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.
Comment 39 Jonas Danielsson 2014-08-02 21:55:20 UTC
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.
Comment 40 Jonas Danielsson 2014-08-02 21:55:52 UTC
Review of attachment 282227 [details] [review]:

Forgot to obsolete the previous one? Same comments applies.
Comment 41 Dario Di Nucci 2014-08-04 14:43:58 UTC
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.
Comment 42 Dario Di Nucci 2014-08-04 14:44:11 UTC
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.
Comment 43 Dario Di Nucci 2014-08-04 14:44:24 UTC
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.
Comment 44 Dario Di Nucci 2014-08-04 14:47:04 UTC
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.
Comment 45 Jonas Danielsson 2014-08-04 14:56:36 UTC
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?
Comment 46 Jonas Danielsson 2014-08-04 15:02:11 UTC
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.
Comment 47 Jonas Danielsson 2014-08-04 15:10:33 UTC
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
Comment 48 Jonas Danielsson 2014-08-04 15:17:00 UTC
I am having trouble appling these patches, could you please rebase to master and indicate what order the patches should go?
Comment 49 Jonas Danielsson 2014-08-04 15:21:24 UTC
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){

...
Comment 50 Jonas Danielsson 2014-08-04 15:22:42 UTC
Review of attachment 282229 [details] [review]:

Why do we need to extend from GObject?
Comment 51 Jonas Danielsson 2014-08-04 15:24:58 UTC
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?
Comment 52 Jonas Danielsson 2014-08-04 15:25:47 UTC
Review of attachment 282230 [details] [review]:

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

Space before bracket on all your changes in this file.
Comment 53 Dario Di Nucci 2014-08-04 16:29:55 UTC
(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?
Comment 54 Dario Di Nucci 2014-08-04 17:04:38 UTC
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.
Comment 55 Dario Di Nucci 2014-08-04 17:04:59 UTC
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.
Comment 56 Dario Di Nucci 2014-08-04 17:05:19 UTC
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.
Comment 57 Dario Di Nucci 2014-08-04 17:06:04 UTC
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.
Comment 58 Dario Di Nucci 2014-08-04 17:06:15 UTC
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.
Comment 59 Dario Di Nucci 2014-08-04 17:06:26 UTC
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.
Comment 60 Dario Di Nucci 2014-08-04 17:11:02 UTC
Review of attachment 282229 [details] [review]:

It is needed for using TurnPoint as parameter when emitting a signal (sidebar.js @196).
Comment 61 Dario Di Nucci 2014-08-04 17:16:39 UTC
(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
Comment 62 Jonas Danielsson 2014-08-16 13:00:09 UTC
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?
Comment 63 Jonas Danielsson 2014-08-16 13:04:33 UTC
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
Comment 64 Jonas Danielsson 2014-08-16 13:07:48 UTC
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?
Comment 65 Jonas Danielsson 2014-08-16 13:15:09 UTC
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.
Comment 66 Dario Di Nucci 2014-08-16 14:04:23 UTC
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
Comment 67 Dario Di Nucci 2014-08-16 14:04:25 UTC
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
Comment 68 Dario Di Nucci 2014-08-16 15:08:59 UTC
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.
Comment 69 Dario Di Nucci 2014-08-16 15:15:57 UTC
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.
Comment 70 Dario Di Nucci 2014-08-16 15:41:32 UTC
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.
Comment 71 Dario Di Nucci 2014-08-16 15:42:46 UTC
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.
Comment 72 Dario Di Nucci 2014-08-16 15:43:38 UTC
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.
Comment 73 Dario Di Nucci 2014-08-16 15:44:14 UTC
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.
Comment 74 Jonas Danielsson 2014-08-18 06:23:34 UTC
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?
Comment 75 Jonas Danielsson 2014-08-18 06:28:50 UTC
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.
Comment 76 Jonas Danielsson 2014-08-18 06:31:06 UTC
Review of attachment 283600 [details] [review]:

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

No underscore.

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

No underscore.
Comment 77 Jonas Danielsson 2014-08-18 06:39:00 UTC
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 });
Comment 78 Jonas Danielsson 2014-08-18 06:41:03 UTC
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?
Comment 79 Dario Di Nucci 2014-08-18 08:57:55 UTC
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.
Comment 80 Dario Di Nucci 2014-08-18 10:09:57 UTC
Review of attachment 283600 [details] [review]:

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

Fixed.

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

Fixed.
Comment 81 Dario Di Nucci 2014-08-18 13:30:14 UTC
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. :)
Comment 82 Dario Di Nucci 2014-08-18 14:10:25 UTC
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.
Comment 83 Dario Di Nucci 2014-08-18 14:14:35 UTC
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.
Comment 84 Dario Di Nucci 2014-08-18 14:16:36 UTC
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.
Comment 85 Dario Di Nucci 2014-08-18 14:17:05 UTC
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.
Comment 86 Dario Di Nucci 2014-08-18 14:17:32 UTC
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.
Comment 87 Dario Di Nucci 2014-08-18 14:18:10 UTC
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.
Comment 88 Dario Di Nucci 2014-08-18 14:41:37 UTC
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.
Comment 89 Dario Di Nucci 2014-08-18 14:42:06 UTC
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.
Comment 90 Jonas Danielsson 2014-08-19 06:08:55 UTC
Review of attachment 283750 [details] [review]:

Looks fine!
Comment 91 Jonas Danielsson 2014-08-19 06:18:28 UTC
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: [...]

?
Comment 92 Jonas Danielsson 2014-08-19 06:20:21 UTC
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.
Comment 93 Jonas Danielsson 2014-08-19 06:22:34 UTC
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?
Comment 94 Jonas Danielsson 2014-08-19 06:26:51 UTC
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.
Comment 95 Jonas Danielsson 2014-08-19 06:32:15 UTC
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.
Comment 96 Dario Di Nucci 2014-08-19 12:30:10 UTC
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.
Comment 97 Dario Di Nucci 2014-08-19 12:34:10 UTC
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.
Comment 98 Dario Di Nucci 2014-08-19 12:36:09 UTC
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.
Comment 99 Jonas Danielsson 2014-08-19 12:41:15 UTC
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.
Comment 100 Jonas Danielsson 2014-08-19 12:43:44 UTC
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?
Comment 101 Dario Di Nucci 2014-08-19 12:45:44 UTC
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.
Comment 102 Dario Di Nucci 2014-08-19 12:51:39 UTC
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.
Comment 103 Jonas Danielsson 2014-08-19 12:57:16 UTC
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.
Comment 104 Dario Di Nucci 2014-08-19 13:12:55 UTC
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?
Comment 105 Jonas Danielsson 2014-08-19 13:23:11 UTC
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.
Comment 106 Dario Di Nucci 2014-08-19 13:27:30 UTC
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.
Comment 107 Jonas Danielsson 2014-08-19 13:29:31 UTC
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.
Comment 108 Dario Di Nucci 2014-08-19 13:48:20 UTC
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.
Comment 109 Dario Di Nucci 2014-08-19 13:49:15 UTC
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.
Comment 110 Dario Di Nucci 2014-08-19 13:56:15 UTC
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.
Comment 111 Dario Di Nucci 2014-08-19 15:38:02 UTC
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.
Comment 112 Dario Di Nucci 2014-08-19 15:42:52 UTC
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.
Comment 113 Dario Di Nucci 2014-08-19 16:04:41 UTC
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.
Comment 114 Dario Di Nucci 2014-08-19 16:05:01 UTC
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.
Comment 115 Dario Di Nucci 2014-08-19 16:06:04 UTC
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... :)
Comment 116 Dario Di Nucci 2014-08-19 16:06:19 UTC
Created attachment 283910 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 117 Dario Di Nucci 2014-08-19 16:09:42 UTC
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.
Comment 118 Jonas Danielsson 2014-08-20 05:12:27 UTC
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 = [...]
Comment 119 Jonas Danielsson 2014-08-20 06:06:22 UTC
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?
Comment 120 Jonas Danielsson 2014-08-20 06:07:11 UTC
Could you please rebase this on latest master and on latest Damian work?
Comment 121 Dario Di Nucci 2014-08-20 21:58:14 UTC
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.
Comment 122 Dario Di Nucci 2014-08-20 22:23:34 UTC
Review of attachment 282227 [details] [review]:

Fixed.
Comment 123 Dario Di Nucci 2014-08-20 22:29:24 UTC
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.
Comment 124 Dario Di Nucci 2014-08-20 22:29:59 UTC
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.
Comment 125 Dario Di Nucci 2014-08-20 22:30:29 UTC
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.
Comment 126 Dario Di Nucci 2014-08-20 22:31:02 UTC
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.
Comment 127 Dario Di Nucci 2014-08-20 22:31:22 UTC
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.
Comment 128 Dario Di Nucci 2014-08-20 22:31:53 UTC
Created attachment 284021 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 129 Dario Di Nucci 2014-08-20 22:32:12 UTC
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.
Comment 130 Dario Di Nucci 2014-08-20 22:36:19 UTC
(In reply to comment #120)
> Could you please rebase this on latest master and on latest Damian work?
Done :)
Comment 131 Jonas Danielsson 2014-08-21 06:42:24 UTC
(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?
Comment 132 Dario Di Nucci 2014-08-21 07:09:42 UTC
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.
Comment 133 Dario Di Nucci 2014-08-21 07:10:00 UTC
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.
Comment 134 Dario Di Nucci 2014-08-21 07:10:20 UTC
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.
Comment 135 Dario Di Nucci 2014-08-21 07:10:38 UTC
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.
Comment 136 Dario Di Nucci 2014-08-21 07:10:54 UTC
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.
Comment 137 Dario Di Nucci 2014-08-21 07:11:25 UTC
Created attachment 284039 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 138 Dario Di Nucci 2014-08-21 07:11:41 UTC
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.
Comment 139 Dario Di Nucci 2014-08-21 07:15:46 UTC
(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.
Comment 140 Dario Di Nucci 2014-08-21 07:24:10 UTC
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.
Comment 141 Dario Di Nucci 2014-08-21 07:24:36 UTC
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.
Comment 142 Dario Di Nucci 2014-08-21 07:25:00 UTC
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.
Comment 143 Dario Di Nucci 2014-08-21 07:26:01 UTC
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.
Comment 144 Dario Di Nucci 2014-08-21 07:26:23 UTC
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.
Comment 145 Dario Di Nucci 2014-08-21 07:27:00 UTC
Created attachment 284046 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 146 Dario Di Nucci 2014-08-21 07:27:16 UTC
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.
Comment 147 Dario Di Nucci 2014-08-21 07:33:43 UTC
Just fixed a small error during rebase :)
Comment 148 Jonas Danielsson 2014-08-21 07:35:01 UTC
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?
Comment 149 Jonas Danielsson 2014-08-21 07:35:55 UTC
Also go: git diff [master] --check and fix the whitespace damage that the patches causes.
Comment 150 Dario Di Nucci 2014-08-21 07:47:58 UTC
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.
Comment 151 Dario Di Nucci 2014-08-21 07:48:16 UTC
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.
Comment 152 Dario Di Nucci 2014-08-21 07:48:36 UTC
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.
Comment 153 Dario Di Nucci 2014-08-21 07:49:11 UTC
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.
Comment 154 Dario Di Nucci 2014-08-21 07:50:06 UTC
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.
Comment 155 Dario Di Nucci 2014-08-21 07:50:26 UTC
Created attachment 284053 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 156 Dario Di Nucci 2014-08-21 07:50:42 UTC
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.
Comment 157 Dario Di Nucci 2014-08-21 07:54:14 UTC
(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.
Comment 158 Dario Di Nucci 2014-08-21 07:56:13 UTC
(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 :)
Comment 159 Jonas Danielsson 2014-08-21 08:05:37 UTC
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?
Comment 160 Dario Di Nucci 2014-08-21 08:11:26 UTC
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.
Comment 161 Dario Di Nucci 2014-08-21 08:11:53 UTC
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.
Comment 162 Dario Di Nucci 2014-08-21 08:12:12 UTC
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.
Comment 163 Dario Di Nucci 2014-08-21 08:12:29 UTC
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.
Comment 164 Dario Di Nucci 2014-08-21 08:12:47 UTC
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.
Comment 165 Dario Di Nucci 2014-08-21 08:13:06 UTC
Created attachment 284060 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 166 Dario Di Nucci 2014-08-21 08:13:26 UTC
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.
Comment 167 Dario Di Nucci 2014-08-21 08:14:17 UTC
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.
Comment 168 Jonas Danielsson 2014-08-21 08:30:38 UTC
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?
Comment 169 Jonas Danielsson 2014-08-21 08:32:47 UTC
Review of attachment 284059 [details] [review]:

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

{...

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

{...
Comment 170 Jonas Danielsson 2014-08-21 08:34:38 UTC
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?
Comment 171 Dario Di Nucci 2014-08-21 08:41:52 UTC
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?
Comment 172 Jonas Danielsson 2014-08-21 08:45:32 UTC
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?
Comment 173 Dario Di Nucci 2014-08-21 08:53:19 UTC
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.
Comment 174 Dario Di Nucci 2014-08-21 09:08:32 UTC
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.
Comment 175 Dario Di Nucci 2014-08-21 10:17:09 UTC
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.
Comment 176 Dario Di Nucci 2014-08-21 10:20:57 UTC
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.
Comment 177 Dario Di Nucci 2014-08-21 10:21:16 UTC
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.
Comment 178 Dario Di Nucci 2014-08-21 10:21:31 UTC
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.
Comment 179 Dario Di Nucci 2014-08-21 10:21:46 UTC
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.
Comment 180 Dario Di Nucci 2014-08-21 10:22:08 UTC
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.
Comment 181 Dario Di Nucci 2014-08-21 10:22:24 UTC
Created attachment 284075 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 182 Dario Di Nucci 2014-08-21 10:22:42 UTC
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.
Comment 183 Jonas Danielsson 2014-08-21 10:22:51 UTC
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.
Comment 184 Jonas Danielsson 2014-08-21 10:26:45 UTC
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.
Comment 185 Jonas Danielsson 2014-08-21 10:27:31 UTC
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
Comment 186 Dario Di Nucci 2014-08-21 10:35:09 UTC
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]?
Comment 187 Jonas Danielsson 2014-08-21 10:39:13 UTC
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?
Comment 188 Dario Di Nucci 2014-08-21 10:42:17 UTC
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.
Comment 189 Jonas Danielsson 2014-08-21 10:46:41 UTC
Review of attachment 284071 [details] [review]:

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

Sure.
Comment 190 Dario Di Nucci 2014-08-21 10:57:19 UTC
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?
Comment 191 Jonas Danielsson 2014-08-21 11:04:27 UTC
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.
Comment 192 Jonas Danielsson 2014-08-21 11:16:46 UTC
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?
Comment 193 Jonas Danielsson 2014-08-21 11:24:21 UTC
Could you also see if it looks better if we remove the zoomToFit from mapWalker if animation is off?
Comment 194 Dario Di Nucci 2014-08-21 11:26:18 UTC
(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. :)
Comment 195 Dario Di Nucci 2014-08-21 11:27:34 UTC
(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?
Comment 196 Jonas Danielsson 2014-08-21 11:41:01 UTC
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.
Comment 197 Dario Di Nucci 2014-08-21 15:09:18 UTC
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.
Comment 198 Dario Di Nucci 2014-08-21 15:15:03 UTC
(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.
Comment 199 Damián Nohales 2014-08-21 15:28:14 UTC
(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.
Comment 200 Dario Di Nucci 2014-08-21 15:53:17 UTC
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.
Comment 201 Dario Di Nucci 2014-08-21 15:53:28 UTC
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.
Comment 202 Dario Di Nucci 2014-08-21 15:53:39 UTC
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.
Comment 203 Dario Di Nucci 2014-08-21 15:54:08 UTC
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.
Comment 204 Dario Di Nucci 2014-08-21 15:54:18 UTC
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.
Comment 205 Dario Di Nucci 2014-08-21 15:54:32 UTC
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.
Comment 206 Dario Di Nucci 2014-08-21 15:54:45 UTC
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.
Comment 207 Dario Di Nucci 2014-08-21 15:54:58 UTC
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.
Comment 208 Dario Di Nucci 2014-08-21 15:55:13 UTC
Created attachment 284108 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 209 Dario Di Nucci 2014-08-21 15:55:25 UTC
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.
Comment 210 Jonas Danielsson 2014-08-21 18:08:40 UTC
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.
Comment 211 Jonas Danielsson 2014-08-21 18:14:22 UTC
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?
Comment 212 Dario Di Nucci 2014-08-22 08:19:58 UTC
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.
Comment 213 Dario Di Nucci 2014-08-22 10:30:37 UTC
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.
Comment 214 Jonas Danielsson 2014-08-22 11:32:48 UTC
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.
Comment 215 Dario Di Nucci 2014-08-22 14:30:18 UTC
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.
Comment 216 Dario Di Nucci 2014-08-22 14:56:01 UTC
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.
Comment 217 Dario Di Nucci 2014-08-22 14:56:15 UTC
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.
Comment 218 Dario Di Nucci 2014-08-22 14:56:28 UTC
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.
Comment 219 Dario Di Nucci 2014-08-22 14:56:41 UTC
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.
Comment 220 Dario Di Nucci 2014-08-22 14:57:03 UTC
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.
Comment 221 Dario Di Nucci 2014-08-22 14:57:18 UTC
Created attachment 284225 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 222 Dario Di Nucci 2014-08-22 14:57:29 UTC
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.
Comment 223 Jonas Danielsson 2014-08-25 05:23:09 UTC
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' ?
Comment 224 Jonas Danielsson 2014-08-25 05:27:38 UTC
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?
Comment 225 Jonas Danielsson 2014-08-25 05:28:53 UTC
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?
Comment 226 Jonas Danielsson 2014-08-25 05:29:30 UTC
Review of attachment 284223 [details] [review]:

Ack
Comment 227 Jonas Danielsson 2014-08-25 05:48:01 UTC
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?
Comment 228 Jonas Danielsson 2014-08-25 05:48:23 UTC
Review of attachment 284224 [details] [review]:

Set status.
Comment 229 Jonas Danielsson 2014-08-25 05:48:49 UTC
Review of attachment 284225 [details] [review]:

Ack
Comment 230 Jonas Danielsson 2014-08-25 05:59:42 UTC
Clicking the instructions and getting the marker is very slow for me, is it for you as well?
Comment 231 Dario Di Nucci 2014-08-25 14:33:44 UTC
(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.
Comment 232 Dario Di Nucci 2014-08-25 15:19:40 UTC
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.
Comment 233 Dario Di Nucci 2014-08-25 15:27:26 UTC
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.
Comment 234 Dario Di Nucci 2014-08-25 15:27:42 UTC
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.
Comment 235 Dario Di Nucci 2014-08-25 15:27:56 UTC
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.
Comment 236 Dario Di Nucci 2014-08-25 15:28:08 UTC
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.
Comment 237 Dario Di Nucci 2014-08-25 15:28:23 UTC
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.
Comment 238 Dario Di Nucci 2014-08-25 15:28:38 UTC
Created attachment 284420 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 239 Dario Di Nucci 2014-08-25 15:28:54 UTC
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.
Comment 240 Dario Di Nucci 2014-08-25 15:30:43 UTC
(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.
Comment 241 Jonas Danielsson 2014-08-26 13:06:45 UTC
Created attachment 284504 [details] [review]
sidebar: Add Via points support - [jonas]

How about something like this?

It moves a lot out to ui files.
Comment 242 Jonas Danielsson 2014-08-26 13:12:10 UTC
Created attachment 284505 [details] [review]
sidebar: Add Via points support - [jonas]

A version that works.
Comment 243 Jonas Danielsson 2014-08-26 13:13:11 UTC
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.
Comment 244 Jonas Danielsson 2014-08-26 13:17:48 UTC
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.
Comment 245 Jonas Danielsson 2014-08-26 13:30:42 UTC
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?
Comment 246 Dario Di Nucci 2014-08-26 17:32:04 UTC
(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.
Comment 247 Dario Di Nucci 2014-08-26 17:34:56 UTC
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.
Comment 248 Dario Di Nucci 2014-08-26 18:23:01 UTC
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.
Comment 249 Dario Di Nucci 2014-08-26 18:25:27 UTC
Created attachment 284528 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 250 Dario Di Nucci 2014-08-26 18:25:42 UTC
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.
Comment 251 Dario Di Nucci 2014-08-26 18:26:02 UTC
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.
Comment 252 Dario Di Nucci 2014-08-26 18:26:18 UTC
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.
Comment 253 Dario Di Nucci 2014-08-26 18:26:36 UTC
Created attachment 284532 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 254 Dario Di Nucci 2014-08-26 18:26:52 UTC
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.
Comment 255 Jonas Danielsson 2014-08-27 05:08:18 UTC
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?
Comment 256 Jonas Danielsson 2014-08-27 05:11:24 UTC
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?
Comment 257 Jonas Danielsson 2014-08-27 05:14:14 UTC
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.
Comment 258 Jonas Danielsson 2014-08-27 05:19:06 UTC
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?
Comment 259 Damián Nohales 2014-08-27 19:11:34 UTC
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.
Comment 260 Dario Di Nucci 2014-08-27 19:50:39 UTC
(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.
Comment 261 Dario Di Nucci 2014-08-27 19:56:03 UTC
(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.
Comment 262 Dario Di Nucci 2014-08-27 20:04:01 UTC
(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.
Comment 263 Damián Nohales 2014-08-27 20:24:39 UTC
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.
Comment 264 Damián Nohales 2014-08-27 20:24:51 UTC
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?
Comment 265 Dario Di Nucci 2014-08-27 20:28:10 UTC
(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.
Comment 266 Dario Di Nucci 2014-08-27 20:44:26 UTC
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
Comment 267 Dario Di Nucci 2014-08-27 20:54:48 UTC
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.
Comment 268 Dario Di Nucci 2014-08-27 21:14:56 UTC
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.
Comment 269 Dario Di Nucci 2014-08-27 22:25:28 UTC
(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.
Comment 270 Damián Nohales 2014-08-27 23:09:44 UTC
(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?
Comment 271 Jonas Danielsson 2014-08-28 05:53:54 UTC
(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.
Comment 272 Jonas Danielsson 2014-08-28 05:56:33 UTC
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?
Comment 273 Jonas Danielsson 2014-08-28 06:01:06 UTC
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.
Comment 274 Jonas Danielsson 2014-08-28 06:07:44 UTC
(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.
Comment 275 Jonas Danielsson 2014-08-28 06:14:19 UTC
(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?
Comment 276 Dario Di Nucci 2014-08-28 08:47:03 UTC
(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.
Comment 277 Dario Di Nucci 2014-08-28 09:04:34 UTC
Review of attachment 284527 [details] [review]:

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

Fixed. Setting point.place = null.
Comment 278 Dario Di Nucci 2014-08-28 10:14:31 UTC
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.
Comment 279 Dario Di Nucci 2014-08-28 11:39:46 UTC
(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.
Comment 280 Dario Di Nucci 2014-08-28 12:04:42 UTC
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.
Comment 281 Dario Di Nucci 2014-08-28 12:04:57 UTC
Created attachment 284687 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 282 Dario Di Nucci 2014-08-28 12:05:11 UTC
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.
Comment 283 Dario Di Nucci 2014-08-28 12:05:28 UTC
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.
Comment 284 Dario Di Nucci 2014-08-28 12:05:43 UTC
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.
Comment 285 Dario Di Nucci 2014-08-28 12:05:57 UTC
Created attachment 284691 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 286 Dario Di Nucci 2014-08-28 12:06:12 UTC
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.
Comment 287 Jonas Danielsson 2014-08-28 12:18:25 UTC
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?
Comment 288 Jonas Danielsson 2014-08-28 12:23:05 UTC
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?
Comment 289 Jonas Danielsson 2014-08-28 12:23:34 UTC
Review of attachment 284690 [details] [review]:

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

this.parent() I mean.
Comment 290 Jonas Danielsson 2014-08-28 12:25:28 UTC
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.
Comment 291 Dario Di Nucci 2014-08-28 13:30:13 UTC
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.
Comment 292 Dario Di Nucci 2014-08-28 14:14:08 UTC
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.
Comment 293 Dario Di Nucci 2014-08-28 14:18:34 UTC
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.
Comment 294 Dario Di Nucci 2014-08-28 14:18:50 UTC
Created attachment 284700 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 295 Dario Di Nucci 2014-08-28 14:19:04 UTC
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.
Comment 296 Dario Di Nucci 2014-08-28 14:19:19 UTC
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.
Comment 297 Dario Di Nucci 2014-08-28 14:19:34 UTC
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.
Comment 298 Dario Di Nucci 2014-08-28 14:19:48 UTC
Created attachment 284704 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 299 Dario Di Nucci 2014-08-28 14:20:04 UTC
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.
Comment 300 Damián Nohales 2014-08-28 18:35:40 UTC
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.
Comment 301 Jonas Danielsson 2014-08-28 18:55:01 UTC
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.
Comment 302 Jonas Danielsson 2014-08-28 18:55:49 UTC
(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
Comment 303 Jonas Danielsson 2014-08-28 18:56:23 UTC
And if you do not see spinners: jhbuild build adwaita-icon-theme
Comment 304 Damián Nohales 2014-08-28 18:59:56 UTC
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 :)
Comment 305 Damián Nohales 2014-08-28 19:06:52 UTC
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
Comment 306 Jonas Danielsson 2014-08-28 19:17:47 UTC
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.
Comment 307 Damián Nohales 2014-08-28 19:20:43 UTC
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"
Comment 308 Jonas Danielsson 2014-08-28 19:23:53 UTC
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."
Comment 309 Jonas Danielsson 2014-08-28 19:25:14 UTC
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.
Comment 310 Damián Nohales 2014-08-28 19:47:18 UTC
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
Comment 311 Damián Nohales 2014-08-28 19:50:17 UTC
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 :)
Comment 312 Damián Nohales 2014-08-28 20:02:22 UTC
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?
Comment 313 Jonas Danielsson 2014-08-29 05:03:19 UTC
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)
Comment 314 Jonas Danielsson 2014-08-29 05:10:25 UTC
(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?
Comment 315 Dario Di Nucci 2014-08-29 10:43:59 UTC
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. :)
Comment 316 Dario Di Nucci 2014-08-29 10:49:32 UTC
Review of attachment 284702 [details] [review]:

::: src/mapView.js
@@ +98,3 @@
+                                                        alpha: 100
+                                                    })
+        });

Fixed in some way.
Comment 317 Dario Di Nucci 2014-08-29 10:50:53 UTC
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. :)
Comment 318 Dario Di Nucci 2014-08-29 10:52:10 UTC
Review of attachment 284705 [details] [review]:

::: src/sidebar.js
@@ +159,2 @@
             route.turnPoints.forEach((function(turnPoint) {
+                let row = new InstructionRow({ visible:   true,

Fixed.
Comment 319 Dario Di Nucci 2014-08-29 16:26:01 UTC
Review of attachment 284704 [details] [review]:

::: src/sidebar.js
@@ +167,3 @@
+                    this._mapView.showTurnPoint(row.turnPoint);
+            }).bind(this));
+

Fixed.
Comment 320 Dario Di Nucci 2014-08-29 19:14:37 UTC
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.
Comment 321 Dario Di Nucci 2014-08-29 19:14:53 UTC
Created attachment 284834 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 322 Dario Di Nucci 2014-08-29 19:15:08 UTC
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.
Comment 323 Dario Di Nucci 2014-08-29 19:15:24 UTC
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.
Comment 324 Dario Di Nucci 2014-08-29 19:15:44 UTC
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.
Comment 325 Dario Di Nucci 2014-08-29 19:16:01 UTC
Created attachment 284839 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 326 Dario Di Nucci 2014-08-29 19:16:16 UTC
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.
Comment 327 Zeeshan Ali 2014-08-29 23:28:40 UTC
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.
Comment 328 Zeeshan Ali 2014-08-29 23:29:11 UTC
Review of attachment 284837 [details] [review]:

Just to be clear, I meant in the commit logs. :)
Comment 329 Damián Nohales 2014-08-30 18:27:06 UTC
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.
Comment 330 Damián Nohales 2014-08-30 19:00:10 UTC
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.
Comment 331 Damián Nohales 2014-08-30 19:00:38 UTC
Review of attachment 284833 [details] [review]:

::: src/routeQuery.js
@@ +107,3 @@
     },
 
+    getFilledPoints: function() {

Maybe "get filledPoints()"?
Comment 332 Damián Nohales 2014-08-30 19:01:41 UTC
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.
Comment 333 Jonas Danielsson 2014-09-01 07:02:41 UTC
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)
Comment 334 Jonas Danielsson 2014-09-01 07:23:24 UTC
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.
Comment 335 Dario Di Nucci 2014-09-01 12:58:05 UTC
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.
Comment 336 Dario Di Nucci 2014-09-01 12:58:37 UTC
Created attachment 285007 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 337 Dario Di Nucci 2014-09-01 12:59:01 UTC
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.
Comment 338 Dario Di Nucci 2014-09-01 12:59:38 UTC
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.
Comment 339 Dario Di Nucci 2014-09-01 12:59:57 UTC
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.
Comment 340 Dario Di Nucci 2014-09-01 13:00:17 UTC
Created attachment 285011 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 341 Jonas Danielsson 2014-09-01 13:01:15 UTC
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.
Comment 342 Jonas Danielsson 2014-09-01 13:05:58 UTC
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
Comment 343 Dario Di Nucci 2014-09-01 14:02:13 UTC
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.
Comment 344 Dario Di Nucci 2014-09-01 14:02:32 UTC
Created attachment 285016 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 345 Dario Di Nucci 2014-09-01 14:02:49 UTC
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.
Comment 346 Dario Di Nucci 2014-09-01 14:03:14 UTC
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.
Comment 347 Dario Di Nucci 2014-09-01 14:03:31 UTC
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.
Comment 348 Dario Di Nucci 2014-09-01 14:03:52 UTC
Created attachment 285020 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 349 Damián Nohales 2014-09-01 17:06:24 UTC
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?
Comment 350 Damián Nohales 2014-09-01 17:32:00 UTC
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.
Comment 351 Damián Nohales 2014-09-01 17:37:22 UTC
Review of attachment 285019 [details] [review]:

Good work!
Comment 352 Damián Nohales 2014-09-01 17:37:24 UTC
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 :-[
Comment 353 Damián Nohales 2014-09-01 17:51:19 UTC
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?
Comment 354 Dario Di Nucci 2014-09-01 18:09:19 UTC
Review of attachment 285018 [details] [review]:

::: src/mapView.js
@@ +92,2 @@
     _initLayers: function() {
+        let stroke_color = new Clutter.Color({ red: 0,

Fixed.
Comment 355 Dario Di Nucci 2014-09-01 18:09:26 UTC
Review of attachment 285016 [details] [review]:

::: src/sidebar.js
@@ +165,3 @@
         }).bind(this));
 
+        query.connect('notify::points', (function() {

Fixed.
Comment 356 Dario Di Nucci 2014-09-01 18:14:40 UTC
Review of attachment 285017 [details] [review]:

Fixed.
Comment 357 Dario Di Nucci 2014-09-01 19:30:04 UTC
Created attachment 285059 [details] [review]
sidebar: Add spinner to instructionList
Comment 358 Dario Di Nucci 2014-09-01 19:30:23 UTC
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.
Comment 359 Dario Di Nucci 2014-09-01 19:30:44 UTC
Created attachment 285061 [details] [review]
sidebar: Add via points support

Add support for dynamicly add and remove via points to
the route search interface.
Comment 360 Dario Di Nucci 2014-09-01 19:31:01 UTC
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.
Comment 361 Dario Di Nucci 2014-09-01 19:31:17 UTC
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.
Comment 362 Dario Di Nucci 2014-09-01 19:31:34 UTC
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.
Comment 363 Dario Di Nucci 2014-09-01 19:31:51 UTC
Created attachment 285065 [details] [review]
routing: Show turnpoint marker on instruction selection
Comment 364 Dario Di Nucci 2014-09-02 09:37:27 UTC
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.
Comment 365 Dario Di Nucci 2014-09-02 10:41:42 UTC
Created attachment 285127 [details] [review]
mapView: New icons for destination points
Comment 366 Dario Di Nucci 2014-09-02 10:42:25 UTC
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.
Comment 367 Jonas Danielsson 2014-09-02 13:31:51 UTC
Created attachment 285137 [details] [review]
sidebar: Add spinner to instructionList
Comment 368 Jonas Danielsson 2014-09-02 13:32:23 UTC
Created attachment 285138 [details] [review]
route: Add via points support
Comment 369 Jonas Danielsson 2014-09-02 13:33:00 UTC
Created attachment 285139 [details] [review]
sidebar: Add via points support
Comment 370 Jonas Danielsson 2014-09-02 13:33:22 UTC
Created attachment 285140 [details] [review]
sidebar: Add indications about distance and time
Comment 371 Jonas Danielsson 2014-09-02 13:33:56 UTC
Created attachment 285141 [details] [review]
mapView: Change route path width and color
Comment 372 Jonas Danielsson 2014-09-02 13:34:14 UTC
Created attachment 285142 [details] [review]
Add new icons for destination points
Comment 373 Jonas Danielsson 2014-09-02 13:34:30 UTC
Created attachment 285143 [details] [review]
routing: New markers for turnpoint
Comment 374 Jonas Danielsson 2014-09-02 13:34:51 UTC
Created attachment 285145 [details] [review]
routing: Show marker on instruction selection
Comment 375 Jonas Danielsson 2014-09-02 13:35:28 UTC
Review of attachment 285137 [details] [review]:

Ack.
Comment 376 Jonas Danielsson 2014-09-02 13:35:55 UTC
Review of attachment 285138 [details] [review]:

Ack.
Comment 377 Jonas Danielsson 2014-09-02 13:36:22 UTC
Review of attachment 285139 [details] [review]:

Ack.
Comment 378 Jonas Danielsson 2014-09-02 13:36:35 UTC
Review of attachment 285140 [details] [review]:

Ack.
Comment 379 Jonas Danielsson 2014-09-02 13:36:43 UTC
Review of attachment 285141 [details] [review]:

Ack.
Comment 380 Jonas Danielsson 2014-09-02 13:36:54 UTC
Review of attachment 285142 [details] [review]:

Ack.
Comment 381 Jonas Danielsson 2014-09-02 13:37:00 UTC
Review of attachment 285143 [details] [review]:

Ack.
Comment 382 Jonas Danielsson 2014-09-02 13:37:08 UTC
Review of attachment 285145 [details] [review]:

Ack.
Comment 383 Jonas Danielsson 2014-09-02 13:53:12 UTC
Created attachment 285149 [details] [review]
Add new icons for destination points

(Added the icons)
Comment 384 Jonas Danielsson 2014-09-02 15:46:48 UTC
Attachment 285137 [details] pushed as 227c90d - sidebar: Add spinner to instructionList
Attachment 285138 [details] pushed as e181196 - route: Add via points support
Attachment 285139 [details] pushed as 0eafc48 - sidebar: Add via points support
Attachment 285140 [details] pushed as ccb9f03 - sidebar: Add indications about distance and time
Attachment 285141 [details] pushed as 2570ed3 - mapView: Change route path width and color
Attachment 285143 [details] pushed as aa60ff6 - routing: New markers for turnpoint
Attachment 285145 [details] pushed as 4a2a8aa - routing: Show marker on instruction selection
Attachment 285149 [details] pushed as 088a4c4 - Add new icons for destination points