GNOME Bugzilla – Bug 755808
Implement support for public transportation routing
Last modified: 2018-03-26 12:56:44 UTC
This one issue that has been itching me for a while. So, I have started looking at alternatives for providing support for this. I think OpenTripPlanner looks promising for providing this support. I have even started a little side-project for downloading GTFS feeds and updating a local OpenTripPlanner instance: https://github.com/mlundblad/otp-updater Currently I'm running an instance with around 30 feeds (some large ones like the national Swedish and Dutch ones, Berlin, and some smaller ones). The OTP process is consuming around 6.3 GB RAM (this is idling, so I'm not sure how it performes under real load). Looking a bit at the REST API and playing with queries in the browser seems to indicate it would be feasable to implement routing support based on this.
Hello Marcus, I am interested in Doing this. Where should I start?
Created attachment 338855 [details] [review] routeQuery: Add support for setting time and arrival/departure Adds a trip time and arrival/departure parameter, indicating if the time means "arrive no later than the time specified" or "depart not before the time specified". This would be used for transit routing.
Created attachment 338856 [details] [review] transitOptions: Add class holding options for transit routing Currently has options to show all route types (modes of transportation) or restrict to a selected set of modes.
Created attachment 338857 [details] [review] routeQuery: Add ability to set options for transit routing Adds the ability to set an options object specifying options when performing a public transit route search. Currently contains options for setting desired transportation modes.
Created attachment 338858 [details] [review] routeQuery: Move routequery to the Application module Let the Application module handle the route query singleton instance. This is needed so that the OpenTripPlanner module can access the query.
Created attachment 338859 [details] [review] routeService: Enable querying GraphHopper externally Add a way to do a route query without triggering route signals. This will be used by the OpenTripPlanner module to use the existing GraphHopper services to calculate walking (and maybe pontentially in the future car and bike park-and-ride) segments in an intinerary.
Created attachment 338860 [details] [review] Add module to handle HVT codes (hierachical vehicle types) This modules contains defintions for the HVT TPEG-derived vehicle codes: See: https://support.google.com/transitpartners/answer/3520902
Created attachment 338907 [details] [review] Break out reading of the service file This will be needed to use the service definition file to read service URL for OpenTripPlanner.
Created attachment 338908 [details] [review] mapSource: Use general service handler
Created attachment 340075 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 340076 [details] [review] routeQuery: Add support for transit mode
Created attachment 340077 [details] [review] application: Add initialization of the OpenTripPlanner instance
Created attachment 340078 [details] [review] Add transit mode icons
Created attachment 340079 [details] [review] utils: Add color luminance and contrast functions This is needed to compute good contrasting colors for transit route labels.
Created attachment 340080 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 340081 [details] [review] Add list box row class for representing transit itineraries This list box class is used to render the overview items showing the list of found itineraries when performing a transit route search.
Created attachment 340082 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 340083 [details] [review] Add a list box row to show the arrival of a transit itinerary
Created attachment 340084 [details] [review] Add list box row for loading more transit results This adds a GtkListBoxRow implementation used as clickable placeholder for loading later/earlier transit alternatives.
Created attachment 340198 [details] [review] application: Add initialization of the OpenTripPlanner instance
Created attachment 340474 [details] [review] application: Add a function to switch to/from transit route mode Add a utility function in the Application module to switch routing "engine", so that modules such as the sidebar won't have to handle it themselves.
Created attachment 340475 [details] [review] mapBubble: Ensure non-transit when routing When using the route button, the mode will default to car, so make sure the GraphHopper service is listening to route query events.
Created attachment 340718 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 340860 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
Created attachment 340861 [details] [review] Add map marker to mark start of walking transit legs
Created attachment 340862 [details] [review] Add map marker to mark end of transit itineraries
Created attachment 340863 [details] [review] Add map marker for marking boarding points in transit itineraries
Created attachment 340864 [details] [review] Add list box row class for transit stops List box row showing intermediate stops.
Created attachment 340865 [details] [review] turnPointMarker: Add support for showing markers for transit stops
Created attachment 340937 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 340938 [details] [review] turnPointMarker: Add support for showing markers for transit stops
Created attachment 340944 [details] [review] turnPointMarker: Add support for showing markers for transit stops
Created attachment 340949 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 340950 [details] [review] mapView: Add a view property to indicate showing a route Adds a new boolean property that is set to true when an actual route is rendered on the map. This will allow only showing the print button when a transit itinerary is "dived into".
Created attachment 340951 [details] [review] mainWindow: Show the print button when a route is rendered Only show the print button when a route is rendered on the map. This way, the print button is hidden until an itinerary is selected when in transit mode. Also, only activate printing with the accelerator when actually showing a route.
Created attachment 340998 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 340999 [details] [review] Add a list box row to show the arrival of a transit itinerary
Created attachment 341000 [details] [review] Add print layout for transit Adds a printing layout implementation for transit itineraries. It will print the instructions for transit switches during a trip. Also includes minimaps for walking sections.
Created attachment 341001 [details] [review] printOperation: Use transit print layout when requested Use the transit print layout when a transit itinerary was selected.
Created attachment 341002 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 341088 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 341177 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
Created attachment 341178 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 341511 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 341695 [details] [review] Add list box row class for representing transit itineraries This list box class is used to render the overview items showing the list of found itineraries when performing a transit route search.
Created attachment 341780 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 341781 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 342043 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 342044 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 342239 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 342353 [details] [review] utils: Add color luminance and contrast functions This is needed to compute good contrasting colors for transit route labels.
Created attachment 342665 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 342985 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 343392 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Review of attachment 338855 [details] [review]: LGTM
Review of attachment 338856 [details] [review]: routeType => transitType No need for "show", it gives no extra information => addRouteTypeToShow => addTransitType ::: src/transitOptions.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad. 2017 @@ +35,3 @@ + + /* when set to true, show any mode of transportation, else only show modes + * added with addRouteTypeToShow() */ /* * This is a comment */ @@ +41,3 @@ + }, + + /* add an explicit transport mode to show */ /* Add
Review of attachment 338857 [details] [review]: LGTM
Review of attachment 338858 [details] [review]: Switch to using a delegator pattern for routeService to be able to better handle otp and graphhopper seemlessly ::: src/mainWindow.js @@ -140,0 +138,2 @@ + Application.routeQuery.connect('notify', + this._setRevealSidebar.bind(this, true)); Indentations issues, also: check if break is still needed ::: src/routeService.js @@ +48,3 @@ this.storedRoute = null; + this._query = Application.routeQuery; + this.parent(); Switch to taking the query as a construct time parameter
Review of attachment 338859 [details] [review]: LGTM but might change a bit if we change the pattern of routeService
Review of attachment 338860 [details] [review]: Other than nits LGTM ::: src/hvt.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA We do not want to track the address of the FSF, look at other modules for better formulation. @@ +150,3 @@ +const SHUTTLE_AIR_SERVICE = 1105; +const INTERCONTINENTAL_CHARTER_AIR_SERVICE = 1106; +const INTERNATIONAL_CHARTER_AIR_SERVICE = 1107; Align or not align, pick one
Review of attachment 338907 [details] [review]: LGTM ::: src/service.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad. 2017
Review of attachment 338908 [details] [review]: LGTM
Review of attachment 340076 [details] [review]: LGTM
Review of attachment 340078 [details] [review]: LGTM
Review of attachment 342985 [details] [review]: neat ::: data/gnome-maps.css @@ +104,3 @@ + background-color: transparent; + -gtk-outline-radius: 14px; +} Investigate if this can be done with standard GTK circlular-button class ::: src/transitLegRow.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Remove address, look at other modules @@ +64,3 @@ + delete params.print; + + this.parent(); this.parent(params); @@ +70,3 @@ + if (this._leg.from) { + /* Translators: this is a format string indicating instructions + * starting a journey at the address given as the parameter */ nit: /* * This is a comment */ @@ +84,3 @@ + if (this._leg.transit) { + let routeLabel = + new TransitRouteLabel.TransitRouteLabel({ leg: this._leg }); ... TransitRouteLabel({ leg: this._leg }); might be a prettier break @@ +86,3 @@ + new TransitRouteLabel.TransitRouteLabel({ leg: this._leg }); + + routeLabel.margin_end = 3; Does this number have a deeper significane? Can we add a comment or in another way shed light on this number 3? @@ +87,3 @@ + + routeLabel.margin_end = 3; + routeLabel.hexpand = false; Add these routeLabel properties to constructor construct. Or if thety really are constant, add them to transitroutelabel ui file @@ +91,3 @@ + this._routeGrid.add(routeLabel); + + this._agencyLabel.visible = true; always visible? add to ui file @@ +110,3 @@ + + if (!this._leg.transit || this._leg.headsign) { + let maxWidthChars = this._print ? -1 : 20; a bit terse, untersify or add a comment @@ +116,3 @@ + max_width_chars: maxWidthChars, + ellipsize: Pango.EllipsizeMode.END, + halign: Gtk.Align.START }); one prop per row @@ +122,3 @@ + } else if (!this._leg.transit) { + /* Translators: this is a format string indicating walking a certain + * distance, with the distance expression being the %s placeholder */ comment format @@ +129,3 @@ + } + + headsignLabel.get_style_context().add_class('dim-label'); investigate if a small class exists and if this class handling can be added to ui file @@ +136,3 @@ + this._timeLabel.label = this._leg.prettyPrintDepartureTime(); + else + this._timeLabel.label = this._leg.prettyPrintTimeInterval(); Maybe this can be one row if prettyPrint changes a bit @@ +145,3 @@ + this._expandButton.connect('clicked', (function() { + this._expand(); + }).bind(this)); this.expandButton.connect('clicked', this._expand.bind(this)); @@ +149,3 @@ + this._collapsButton.connect('clicked', (function() { + this._collaps(); + }).bind(this)); Same as above. @@ +163,3 @@ + this._onEvent(event); + return true; + }).bind(this)); Either pick more verbose names for the handlers or add a comment @@ +194,3 @@ + /* collaps the time label down to just show the start time when + * revealing intermediate stop times, as the arrival time is displayed + * at the last stop */ comment format @@ +223,3 @@ + } else { + /* don't output the starting and ending instructions from the walk + * route, since these are explicitely added by the itinerary */ comment format
Review of attachment 340084 [details] [review]: Otherwise lgtm ::: src/transitMoreRow.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA No address
Created attachment 345015 [details] [review] Add module to handle HVT codes (hierachical vehicle types) This modules contains defintions for the HVT TPEG-derived vehicle codes: See: https://support.google.com/transitpartners/answer/3520902
(In reply to Jonas Danielsson from comment #61) > Review of attachment 338860 [details] [review] [review]: > > Other than nits LGTM > > ::: src/hvt.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Fixed > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > We do not want to track the address of the FSF, look at other modules for > better formulation. Fixed > > @@ +150,3 @@ > +const SHUTTLE_AIR_SERVICE = 1105; > +const INTERCONTINENTAL_CHARTER_AIR_SERVICE = 1106; > +const INTERNATIONAL_CHARTER_AIR_SERVICE = 1107; > > Align or not align, pick one Choose not to align, looks prettier actually, and the numbers aren't the important thing in themselfs anyway. Also changed some one-line /* */ to use // style comments
Created attachment 345018 [details] [review] transitOptions: Add class holding options for transit routing Currently has options to show all route types (modes of transportation) or restrict to a selected set of modes.
Created attachment 345019 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 345020 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
Created attachment 345021 [details] [review] Add list box row for loading more transit results This adds a GtkListBoxRow implementation used as clickable placeholder for loading later/earlier transit alternatives.
(In reply to Jonas Danielsson from comment #68) > Review of attachment 340084 [details] [review] [review]: > > Otherwise lgtm > > ::: src/transitMoreRow.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > No address Yep, fixed
Review of attachment 345015 [details] [review]: LGTM
Review of attachment 345018 [details] [review]: LGTM
Review of attachment 345021 [details] [review]: LGTM
Comment on attachment 338907 [details] [review] Break out reading of the service file Attachment 338907 [details] pushed as ad4efcc - Break out reading of the service file
Comment on attachment 338908 [details] [review] mapSource: Use general service handler Attachment 338908 [details] pushed as f6562ae - mapSource: Use general service handler
Review of attachment 340864 [details] [review]: some nits ::: src/transitStopRow.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +17,3 @@ + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * adress @@ +41,3 @@ + delete params.final; + + this.parent(); should take params @@ +48,3 @@ + this._timeLabel.label = this.stop.prettyPrintArrivalTime(); + else + this._timeLabel.label = this.stop.prettyPrintDepartureTime(); this.stop.prettyPrint(!this._final) so prettyPrint would be a function like prettyPrint(iSDepartureTime);
Created attachment 345055 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
Created attachment 345056 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 345057 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
(In reply to Jonas Danielsson from comment #67) > Review of attachment 342985 [details] [review] [review]: > > neat > > ::: data/gnome-maps.css > @@ +104,3 @@ > + background-color: transparent; > + -gtk-outline-radius: 14px; > +} > > Investigate if this can be done with standard GTK circlular-button class > > ::: src/transitLegRow.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Sure > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > Remove address, look at other modules Sure > > @@ +64,3 @@ > + delete params.print; > + > + this.parent(); > > this.parent(params); Fixed > > @@ +70,3 @@ > + if (this._leg.from) { > + /* Translators: this is a format string indicating > instructions > + * starting a journey at the address given as the parameter > */ > > nit: > > /* > * This is a comment > */ > Sure > @@ +84,3 @@ > + if (this._leg.transit) { > + let routeLabel = > + new TransitRouteLabel.TransitRouteLabel({ leg: this._leg }); > > ... TransitRouteLabel({ > leg: this._leg > }); > > > might be a prettier break Makes sense > > @@ +86,3 @@ > + new TransitRouteLabel.TransitRouteLabel({ leg: this._leg }); > + > + routeLabel.margin_end = 3; > > Does this number have a deeper significane? Can we add a comment or in > another way shed light on this number 3? I moved this (and the other contruct params) to the UI file and added a comment on the rationale (we want a padding after the route (i.e. line number) label for transit legs, while for walking legs the headsign label is the first in the grid row, and is flush-on the left side. > > @@ +87,3 @@ > + > + routeLabel.margin_end = 3; > + routeLabel.hexpand = false; > > Add these routeLabel properties to constructor construct. Or if thety really > are constant, add them to transitroutelabel ui file > > @@ +91,3 @@ > + this._routeGrid.add(routeLabel); > + > + this._agencyLabel.visible = true; > > always visible? add to ui file Actually, agency lable is only visible for transit legs, not walking. > > @@ +110,3 @@ > + > + if (!this._leg.transit || this._leg.headsign) { > + let maxWidthChars = this._print ? -1 : 20; > > a bit terse, untersify or add a comment Added a comment here about setting max width to constrain side bar, but unlimited for printing. > > @@ +116,3 @@ > + max_width_chars: > maxWidthChars, > + ellipsize: > Pango.EllipsizeMode.END, > + halign: Gtk.Align.START }); > > one prop per row Sure > > @@ +122,3 @@ > + } else if (!this._leg.transit) { > + /* Translators: this is a format string indicating walking > a certain > + * distance, with the distance expression being the %s > placeholder */ > > comment format Fixed > > @@ +129,3 @@ > + } > + > + headsignLabel.get_style_context().add_class('dim-label'); > > investigate if a small class exists and if this class handling can be added > to ui file I didn't yet find out a way to do this with standard styles, but I wanted to refresh this patch anyway to avoid having WIP stashes when starting the "routing engine delegator" refactoring. > > @@ +136,3 @@ > + this._timeLabel.label = this._leg.prettyPrintDepartureTime(); > + else > + this._timeLabel.label = this._leg.prettyPrintTimeInterval(); > > Maybe this can be one row if prettyPrint changes a bit Yes, introduces a new prettyPrintTime() taking a params object where "isStart" indicates if it's a first-in-an-itinerary leg doing this logic in there. > > @@ +145,3 @@ > + this._expandButton.connect('clicked', (function() { > + this._expand(); > + }).bind(this)); > > this.expandButton.connect('clicked', this._expand.bind(this)); Yep > > @@ +149,3 @@ > + this._collapsButton.connect('clicked', (function() { > + this._collaps(); > + }).bind(this)); > > Same as above. Yep > > @@ +163,3 @@ > + this._onEvent(event); > + return true; > + }).bind(this)); > > Either pick more verbose names for the handlers or add a comment Changes this to "_handleEventBox" > > @@ +194,3 @@ > + /* collaps the time label down to just show the start time when > + * revealing intermediate stop times, as the arrival time is > displayed > + * at the last stop */ > > comment format > > @@ +223,3 @@ > + } else { > + /* don't output the starting and ending instructions from the > walk > + * route, since these are explicitely added by the itinerary */ > > comment format Yep
Created attachment 345059 [details] [review] Add list box row class for transit stops List box row showing intermediate stops.
Created attachment 345060 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 345061 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
(In reply to Jonas Danielsson from comment #81) > Review of attachment 340864 [details] [review] [review]: > > some nits > > ::: src/transitStopRow.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Sure > > @@ +17,3 @@ > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > > adress > Sure > @@ +41,3 @@ > + delete params.final; > + > + this.parent(); > > should take params > Fixed > @@ +48,3 @@ > + this._timeLabel.label = this.stop.prettyPrintArrivalTime(); > + else > + this._timeLabel.label = this.stop.prettyPrintDepartureTime(); > > this.stop.prettyPrint(!this._final) so prettyPrint would be a function like > prettyPrint(iSDepartureTime); Changed to use a prettyPrint() in TransitPlan.Stop taking a param object, using a isFinal field to select the arrival or depature to for the stop.
Created attachment 345063 [details] [review] Add map marker to mark start of walking transit legs
Review of attachment 345063 [details] [review]: Thanks! ::: src/transitWalkMarker.js @@ +36,3 @@ + delete params.leg; + + this._previousLeg = params.previousLeg; is this temp member variable really needed? We could just use params.previousLeg below, or failing that this could just be let prevLeg = @@ +46,3 @@ + let point = this._previousLeg ? + this._previousLeg.polyline[this._previousLeg.polyline.length - 1] : + this._leg.polyline[0]; I think I would prefer this as an if/else-statement, no strong feelings though @@ +49,3 @@ + let location = + new Location.Location({ latitude: point.get_latitude(), + longitude: point.get_longitude() }); can we break it as: let location = new Location.Location({ latitude: point.get_latitude(), longitude: point.get_longitude(), }); ? And also what class is point? Is latitude/longitude not properties so this could be point.latitude?
Review of attachment 342353 [details] [review]: Nice! I think this deserves its own module instead of piling onto Utils though! Maybe contrast.js?
Review of attachment 340862 [details] [review]: thanks! ::: src/transitArrivalMarker.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA address @@ +40,3 @@ + let location = + new Location.Location({ latitude: lastPoint.get_latitude(), + longitude: lastPoint.get_longitude() }); Can we break with ({ property: value, ... }) instead? @@ +46,3 @@ + this.parent(params); + + let color = new Gdk.RGBA({ red: 0, green: 0, blue: 0, alpha: 1.0 }); new line for each prop
Review of attachment 340863 [details] [review]: nice! ::: src/transitBoardMarker.js @@ +3,3 @@ +/* + * Copyright (c) 2016 Marcus Lundblad + * 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA address @@ +51,3 @@ + let location = + new Location.Location({ latitude: firstPoint.get_latitude(), + longitude: firstPoint.get_longitude() }); Break style @@ +58,3 @@ + let actor = this._createActor(); + + this.add_actor(actor); this.add_Actor(this._createActor()); @@ +60,3 @@ + this.add_actor(actor); + }, + Could we get a comment for the quite large function below? What is it doing and why?
Review of attachment 340999 [details] [review]: Otherwise LGTM ::: src/transitArrivalRow.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA address
Review of attachment 345059 [details] [review]: hm ::: src/transitLegRow.js @@ +27,3 @@ const GLib = imports.gi.GLib; const Gtk = imports.gi.Gtk; +const Pango = imports.gi.Pango; mis-rebase?
Created attachment 345147 [details] [review] Add map marker to mark start of walking transit legs
(In reply to Jonas Danielsson from comment #91) > Review of attachment 345063 [details] [review] [review]: > > Thanks! > > ::: src/transitWalkMarker.js > @@ +36,3 @@ > + delete params.leg; > + > + this._previousLeg = params.previousLeg; > > is this temp member variable really needed? We could just use > params.previousLeg below, or failing that this could just be let prevLeg = > Indeed, changed to just use the params properties to contruct the point directly. > @@ +46,3 @@ > + let point = this._previousLeg ? > + > this._previousLeg.polyline[this._previousLeg.polyline.length - 1] : > + this._leg.polyline[0]; > > I think I would prefer this as an if/else-statement, no strong feelings > though Sure, I have no strong feeling about it either, so might as well go for a outspoken if/else here. > > @@ +49,3 @@ > + let location = > + new Location.Location({ latitude: point.get_latitude(), > + longitude: point.get_longitude() }); > > can we break it as: > > let location = new Location.Location({ > latitude: point.get_latitude(), > longitude: point.get_longitude(), > }); > > ? Sure thing > > And also what class is point? Is latitude/longitude not properties so this > could be point.latitude? Yep, actually it is a Champlain.Coordinate, so it has these as properties.
Created attachment 345148 [details] [review] Add color luminance and contrast functions Add a new module with utility functions for computing lumninance and contrast for background and foreground colors. This is needed to compute good contrasting colors for transit route labels.
(In reply to Jonas Danielsson from comment #92) > Review of attachment 342353 [details] [review] [review]: > > Nice! > > I think this deserves its own module instead of piling onto Utils though! > Maybe contrast.js? Yep, makes good sense. I moved it to a new contrast.js (seems to be a prudent name :) ) I also re-worded the commit message a bit (as it is now a new module and all… BTW, there is some fallout on this, so will come a few follow-up patch refreshes.
Created attachment 345149 [details] [review] Add map marker for marking boarding points in transit itineraries
Created attachment 345150 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 345151 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 345153 [details] [review] Add map marker for marking boarding points in transit itineraries
Created attachment 345154 [details] [review] Add map marker to mark end of transit itineraries
Created attachment 345155 [details] [review] Add map marker to mark end of transit itineraries
(In reply to Jonas Danielsson from comment #93) > Review of attachment 340862 [details] [review] [review]: > > thanks! > > ::: src/transitArrivalMarker.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Fixed > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > address Fixed > > @@ +40,3 @@ > + let location = > + new Location.Location({ latitude: lastPoint.get_latitude(), > + longitude: lastPoint.get_longitude() }); > > Can we break with > > ({ > property: value, > ... > }) > > instead? > Yep > @@ +46,3 @@ > + this.parent(params); > + > + let color = new Gdk.RGBA({ red: 0, green: 0, blue: 0, alpha: 1.0 }); > > new line for each prop Yep Also got rid of the _leg instance variable here (and use params.leg directly to create the the location object) and also use the .latitude and .longitude properties directly, like in a previous patch.
Created attachment 345156 [details] [review] Add map marker for marking boarding points in transit itineraries
(In reply to Jonas Danielsson from comment #94) > Review of attachment 340863 [details] [review] [review]: > > nice! Thanks! > > ::: src/transitBoardMarker.js > @@ +3,3 @@ > +/* > + * Copyright (c) 2016 Marcus Lundblad > + * > > 2017 Yep > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > address Fixed > > @@ +51,3 @@ > + let location = > + new Location.Location({ latitude: firstPoint.get_latitude(), > + longitude: firstPoint.get_longitude() > }); > > Break style > Fixed > @@ +58,3 @@ > + let actor = this._createActor(); > + > + this.add_actor(actor); > > this.add_Actor(this._createActor()); Yep (and the _createActor takes a leg parameter now, to avoid setting a _leg instance variable like in another patch). > > @@ +60,3 @@ > + this.add_actor(actor); > + }, > + > > Could we get a comment for the quite large function below? What is it doing > and why? Added a comment about this function creating a Clutter actor depicting the transit type for boarding, drawn in colors from the transit data and with foreground-colored outline to improve legibility when the background of the marker is light.
Created attachment 345157 [details] [review] Add map marker for marking boarding points in transit itineraries
Created attachment 345159 [details] [review] Add a list box row to show the arrival of a transit itinerary
(In reply to Jonas Danielsson from comment #95) > Review of attachment 340999 [details] [review] [review]: > > Otherwise LGTM > > ::: src/transitArrivalRow.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Sure > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > address Sure
Created attachment 345160 [details] [review] Add list box row class for transit stops List box row showing intermediate stops.
Created attachment 345161 [details] [review] Add list box row to display a leg of a transit itinerary This list box class is used to render a "leg" of an itinerary when "diving in" to a particular itinerary. I.e. part of a journey such as "Walk 500 m" or "Bus 42 leaving at 12:00".
(In reply to Jonas Danielsson from comment #96) > Review of attachment 345059 [details] [review] [review]: > > hm > > ::: src/transitLegRow.js > @@ +27,3 @@ > const GLib = imports.gi.GLib; > const Gtk = imports.gi.Gtk; > +const Pango = imports.gi.Pango; > > mis-rebase? Yeah, must have been a Franken-rebase :) Re-aligned the patches, should be right now.
Review of attachment 345155 [details] [review]: just a nit, could be ignored- ::: src/transitArrivalMarker.js @@ +48,3 @@ + blue: 0, + alpha: 1.0 + }); alpha: 1.0 });
Review of attachment 345157 [details] [review]: LGTM
Review of attachment 345159 [details] [review]: LGTM
Review of attachment 345160 [details] [review]: LGTM
Review of attachment 345161 [details] [review]: LGTM
Review of attachment 345159 [details] [review]: set
Review of attachment 345160 [details] [review]: set
Review of attachment 345161 [details] [review]: set
Created attachment 345370 [details] [review] routeQuery: Move routequery to the Application module Let the Application module handle the route query singleton instance. This is needed so that the OpenTripPlanner module can access the query. Also change the GraphHopper object to take the query as a contruct parameter to avoid accessing globals in Application.
Created attachment 345371 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
Created attachment 345372 [details] [review] Rename the RouteService module to GraphHopper Since we are going to add a delegator abstraction to handle delegating routing requests to the different routing implementations, need a more specific name for this.
Created attachment 345373 [details] [review] Add module to delegate routing requests. Adds a new module implementing a delegator that delegates routing requests to either GraphHopper or OpenTripPlanner based on the selected mode.
Created attachment 345374 [details] [review] mapBubble: Ensure non-transit when routing When using the route button, the mode will default to car, so make sure the GraphHopper service is listening to route query events.
Created attachment 345375 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
Created attachment 345376 [details] [review] mapView: Add a view property to indicate showing a route Adds a new boolean property that is set to true when an actual route is rendered on the map. This will allow only showing the print button when a transit itinerary is "dived into".
Created attachment 345377 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 345378 [details] [review] Add print layout for transit Adds a printing layout implementation for transit itineraries. It will print the instructions for transit switches during a trip. Also includes minimaps for walking sections.
(In reply to Jonas Danielsson from comment #60) > Review of attachment 338859 [details] [review] [review]: > > LGTM but might change a bit if we change the pattern of routeService The routing delegator is now in place, and this patch is unaffected. A later patch renamed the module to GraphHopper, though.
Created attachment 345379 [details] [review] routeQuery: Move routequery to the Application module Let the Application module handle the route query singleton instance. This is needed so that the OpenTripPlanner module can access the query. Also change the GraphHopper object to take the query as a contruct parameter to avoid accessing globals in Application.
(In reply to Jonas Danielsson from comment #59) > Review of attachment 338858 [details] [review] [review]: > > Switch to using a delegator pattern for routeService to be able to better > handle otp and graphhopper seemlessly Yep, introduced now in a new patch! > > ::: src/mainWindow.js > @@ -140,0 +138,2 @@ > + Application.routeQuery.connect('notify', > + > this._setRevealSidebar.bind(this, true)); > > Indentations issues, also: check if break is still needed > Fixed and the line was overflowing 80 columns by quite a few, so I kept the break. > ::: src/routeService.js > @@ +48,3 @@ > this.storedRoute = null; > + this._query = Application.routeQuery; > + this.parent(); > > Switch to taking the query as a construct time parameter The query is now passed in the param object (after the renaming of the module to GraphHopper).
Created attachment 345383 [details] [review] Add module to delegate routing requests. Adds a new module implementing a delegator that delegates routing requests to either GraphHopper or OpenTripPlanner based on the selected mode.
Created attachment 345384 [details] [review] printOperation: Use transit print layout when requested Use the transit print layout when a transit itinerary was selected.
Created attachment 345385 [details] [review] Add module to delegate routing requests. Adds a new module implementing a delegator that delegates routing requests to either GraphHopper or OpenTripPlanner based on the selected mode.
Review of attachment 340944 [details] [review]: LGTM
Review of attachment 341695 [details] [review]: nits ::: src/transitItineraryRow.js @@ +2,3 @@ +/* vim: set et ts=4 sw=4: */ +/* + * Copyright (c) 2016 Marcus Lundblad 2017 @@ +16,3 @@ + * You should have received a copy of the GNU General Public License along + * with GNOME Maps; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA no address @@ +58,3 @@ + * itinerary */ + let useContractedLabels = this._itinerary.legs.length > 5; + for (let i = 0; i < this._itinerary.legs.length; i++) { no way of doing this with an this._itinerary.legs.forEach( ... type of construct?
Review of attachment 345147 [details] [review]: lgtm
Review of attachment 345148 [details] [review]: \o/
Review of attachment 345150 [details] [review]: lgtm ::: src/transitRouteLabel.js @@ +111,3 @@ + * and getting rounded corner just using CSS styles, so doing a custom + * Cairo drawing of a "roundrect" + */ Removing the background-image, setting backround-color and using border-radius does not work?
Review of attachment 345371 [details] [review]: So, this all looks very nice! But it is hard for me to follow, would it be possible with a larger comment block on top of openTripPlanner.js that talks a bit about general code flow and function? ::: src/openTripPlanner.js @@ +449,3 @@ + + let params = { fromPlace: stops[0].id, + toPlace: stops[stops.length - 1].id }; we are stating to have a lot of this pattern, getting the last element of an array. How about we add: if (!Array.prototype.last){ Array.prototype.last = function(){ return this[this.length - 1]; }; }; Somewhere? We already do String.prototype.format = Format.format; in application.js, not sure if that belongs there though. Turning all those awkward constructs from stops[stops.length - 1]; to stops.last; is appealing. @@ +702,3 @@ + duration: route.time / 1000, + distance: route.distance, + walkingInstructions: route.turnPoints }); I am not crazy about this formatting, but not sure if an alternative is better. Can it be de-complexed? Creating the from/to-coordinate on their own lines maybe? let fromCoord = [from.place.location.latitude, from.place.location.longitude]; let toCoord = [to.place.location.latitude, to.place.location.longitude]; And then the constructor will be more normal looking, and maybe we skip the alignment? ::: src/transitPlan.js @@ +399,3 @@ + delete params.tripShortName; + + this.parent(); what parent?
Review of attachment 345372 [details] [review]: Does this not have code fallout as well? Is this commit buildable?
Review of attachment 340951 [details] [review]: lgtm
Review of attachment 345374 [details] [review]: Ok. At some poing we should stop defaulting to car. Cars are uncivilized.
Review of attachment 345375 [details] [review]: This looks nice! But sidebar.js is growing! Do you see anyway of breaking out functionality from this class? ::: src/sidebar.js @@ +85,3 @@ + 'trainCheckButton', + 'subwayCheckButton', + 'transitParametersMenuButton', Holy wall of widgets, Batman! @@ +108,3 @@ + */ + this._modeTransitToggle.sensitive = + Application.routingDelegator.openTripPlanner.enabled; I wonder if this should be this._modeTransitTogge.visible = ... Can we get Andreas opinion? @@ +515,3 @@ + return null; + } + this._transitTimeEntry.connect('activate', This is a pure function right, maybe move to Utils or if we have a bunch of similar helpers, to a [something with time].js
Review of attachment 345376 [details] [review]: ::: src/mapView.js @@ +84,3 @@ GObject.ParamFlags.WRITABLE, + false), + /* this property is true when a route is being shown on the map */ Are the comments above the wrong way around? Or are we really bad at naming?
Review of attachment 345377 [details] [review]: o// ::: src/mapView.js @@ +176,3 @@ + lineColor ? parseInt(lineColor.substring(2, 4), 16) : 0; + let blue = + lineColor ? parseInt(lineColor.substring(4, 6), 16) : 255; this pattern is used in other places? Maybe time for a helper? function parseColor(string, offset, length, default) { ... } let red = parseColor(linecolor, offset, length, 0); Or similar? Will that let us avoid the silly breaks and hard to parse trinary operators? @@ +588,3 @@ + let previousLeg = itinerary.legs[index - 1]; + let lastPoint = + let outlineRouteLayer; If we had the last prototype: let lastPoing = previousLeg.polyline.last; @@ +624,3 @@ + new TransitBoardMarker.TransitBoardMarker({ leg: leg, + mapView: this}); + } no way to avoid this break at assignment? startMarker = new TransitBoardMarker.TransitBoardMarker({ ... ... ... }); ? Or renaming to just "start"? @@ +633,3 @@ + let arrivalMarker = + new TransitArrivalMarker.TransitArrivalMarker({ leg: lastLeg, + leg.polyline.forEach(routeLayer.add_node.bind(routeLayer)); same break question here
Review of attachment 345378 [details] [review]: Yes.
Review of attachment 345379 [details] [review]: ok
Review of attachment 345384 [details] [review]: \o/
Review of attachment 345385 [details] [review]: nice! nits inside! ::: src/application.js @@ +249,3 @@ + routeQuery = new RouteQuery.RouteQuery(); + routingDelegator = new RoutingDelegator.RoutingDelegator( + { query: routeQuery }); this is a silly break, is it needed? If so try to make it more standard: new RoutingDelegator.RoutingDelegator({ query: routeQuery }); ::: src/routingDelegator.js @@ +38,3 @@ + this._openTripPlanner = new OpenTripPlanner.OpenTripPlanner( + { query: this._query, + graphHopper: this._graphHopper }); silly break again
Created attachment 345402 [details] [review] Add list box row class for representing transit itineraries This list box class is used to render the overview items showing the list of found itineraries when performing a transit route search.
(In reply to Jonas Danielsson from comment #141) > Review of attachment 341695 [details] [review] [review]: > > nits > > ::: src/transitItineraryRow.js > @@ +2,3 @@ > +/* vim: set et ts=4 sw=4: */ > +/* > + * Copyright (c) 2016 Marcus Lundblad > > 2017 Yep > > @@ +16,3 @@ > + * You should have received a copy of the GNU General Public License along > + * with GNOME Maps; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > no address Fixed > > @@ +58,3 @@ > + * itinerary */ > + let useContractedLabels = this._itinerary.legs.length > 5; > + for (let i = 0; i < this._itinerary.legs.length; i++) { > > no way of doing this with an this._itinerary.legs.forEach( ... type of > construct? Yeah, changed this to use forEach(function(leg, i) { Should work with ES5-compatability, so should be safe, I think.
(In reply to Jonas Danielsson from comment #148) > Review of attachment 345374 [details] [review] [review]: > > Ok. At some poing we should stop defaulting to car. Cars are uncivilized. :-) One cool idea would be to have it context-based, so rural area with low transit-coverage: car, short distance: walk or bike, cities: transit etc.
Created attachment 345403 [details] [review] Add module to delegate routing requests. Adds a new module implementing a delegator that delegates routing requests to either GraphHopper or OpenTripPlanner based on the selected mode. Also change the name of the RouteService module to GraphHopper to make it more clear that these are different service interfaces.
(In reply to Jonas Danielsson from comment #155) > Review of attachment 345385 [details] [review] [review]: > > nice! nits inside! :) > > ::: src/application.js > @@ +249,3 @@ > + routeQuery = new RouteQuery.RouteQuery(); > + routingDelegator = new RoutingDelegator.RoutingDelegator( > + { query: routeQuery }); > > this is a silly break, is it needed? If so try to make it more standard: > I moved it up (there were some equally-long lines already in the visinity). > new RoutingDelegator.RoutingDelegator({ > query: routeQuery > }); > > ::: src/routingDelegator.js > @@ +38,3 @@ > + this._openTripPlanner = new OpenTripPlanner.OpenTripPlanner( > + { query: this._query, > + graphHopper: this._graphHopper }); > > silly break again Yep, re-arranged a bit I also merged this patch with the one renaming routeService.js so it should be in a consistent state along commits.
Created attachment 345404 [details] [review] main: Add array prototype to get last element Add an array prototype function to get the last element. This saves having to do some array[array.length - 1] contructs later on. TODO: if we grow more prototype definitions further along this could be moved to a separate module.
Review of attachment 345404 [details] [review]: LGTM, Mattias?
(In reply to Jonas Danielsson from comment #150) > Review of attachment 345376 [details] [review] [review]: > > ::: src/mapView.js > @@ +84,3 @@ > > GObject.ParamFlags.WRITABLE, > + false), > + /* this property is true when a route is being shown on the map */ > > Are the comments above the wrong way around? Or are we really bad at naming? Actually, the "routeVisible" property is what's there currently. This is bound to the route query, so that when the query fires notify this propery is set to true if the query is in a valid state. Actually, there is a short "race condition" today, where the print button is visible before the route is displayed (while the spinner is showing). Perhaps unlikely the user would manage to get a print of an inconsistent state, though. So, I added the new "routeActive" property that is set when the route is actually finished, or in transit, when the user "dives into" a particular itininerary. But, yeah, the naming is probably a bit iffy.
(In reply to Marcus Lundblad from comment #163) > (In reply to Jonas Danielsson from comment #150) > > Review of attachment 345376 [details] [review] [review] [review]: > > > > ::: src/mapView.js > > @@ +84,3 @@ > > > > GObject.ParamFlags.WRITABLE, > > + false), > > + /* this property is true when a route is being shown on the map */ > > > > Are the comments above the wrong way around? Or are we really bad at naming? > > Actually, the "routeVisible" property is what's there currently. > This is bound to the route query, so that when the query fires notify this > propery is set to true if the query is in a valid state. > Actually, there is a short "race condition" today, where the print button is > visible before the route is displayed (while the spinner is showing). > Perhaps unlikely the user would manage to get a print of an inconsistent > state, though. > > So, I added the new "routeActive" property that is set when the route is > actually finished, or in transit, when the user "dives into" a particular > itininerary. > > But, yeah, the naming is probably a bit iffy. The naming is a showstopper. This is very confusing. The comment above routeVisible mentions active the comment above routeActive mentions shown.
Review of attachment 345403 [details] [review]: LGTM
Created attachment 345447 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 345448 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 345449 [details] [review] turnPointMarker: Add support for showing markers for transit stops
Created attachment 345450 [details] [review] Add label widget for route labels This adds a label widget for representing parts of transit journeys. The label can use "long" or "compact" mode, where the latter is suited for presenting in an overview and the former for a detailed view of an itinerary.
Created attachment 345451 [details] [review] Add color utils Add a new module with utility functions for computing lumninance and contrast for background and foreground colors and for parsing color components from hex-encoded color strings. This is needed to compute good contrasting colors for transit route labels.
(In reply to Jonas Danielsson from comment #151) > Review of attachment 345377 [details] [review] [review]: > > o// > > ::: src/mapView.js > @@ +176,3 @@ > + lineColor ? parseInt(lineColor.substring(2, 4), 16) : 0; > + let blue = > + lineColor ? parseInt(lineColor.substring(4, 6), 16) : 255; > > this pattern is used in other places? Maybe time for a helper? > > function parseColor(string, offset, length, default) { > ... > } > > let red = parseColor(linecolor, offset, length, 0); > > Or similar? Will that let us avoid the silly breaks and hard to parse > trinary operators? > Moved this to a util functions in the (renamed) Color module. Some fallout patches have been refreshed as a consequence. > @@ +588,3 @@ > + let previousLeg = itinerary.legs[index - 1]; > + let lastPoint = > + let outlineRouteLayer; > > If we had the last prototype: > > let lastPoing = previousLeg.polyline.last; Yep, now using the last() proto. > > @@ +624,3 @@ > + new TransitBoardMarker.TransitBoardMarker({ leg: leg, > + mapView: > this}); > + } > > no way to avoid this break at assignment? > > startMarker = new TransitBoardMarker.TransitBoardMarker({ > ... > ... > ... > }); ? > > Or renaming to just "start"? > > @@ +633,3 @@ > + let arrivalMarker = > + new TransitArrivalMarker.TransitArrivalMarker({ leg: lastLeg, > + leg.polyline.forEach(routeLayer.add_node.bind(routeLayer)); > > same break question here Renamed those and got rid of breaks.
Review of attachment 345448 [details] [review]: Looks nice! There is a lot of magic numbers tho, that seems connected with the transit route style. Can they be consolidated? So that we have transitLeg.XYZ_COLOR, transitLeg.DASH_STEP or similar? (I dunno if transitLeg is the correct class for this) Or if we should have a new module. ::: src/mapView.js @@ +573,3 @@ + /* draw an outline by drawing a background path layer if needed + * TODO: maybe we should add support for outlined path layers in + let color = leg.color; Can this TODO result in a bug on the libchamplain product and then maybe be removed? @@ +615,3 @@ + } else { + start = new TransitBoardMarker.TransitBoardMarker({ leg: leg, + routeLayer.add_node(lastPoint); space before }
Review of attachment 345449 [details] [review]: Nitty McNitface ::: src/turnPointMarker.js @@ +89,3 @@ + let blue = Color.parseColor(color, 2, 0x9C); + + let color = this._transitLeg ? this._transitLeg.color : null; nitty, could this be: return new Gdk.RGBA({ red: Color.parseColor(color, 0, 0x21), green: ... }); And btw, what is the hex stuff?
Review of attachment 345450 [details] [review]: lgtm
Review of attachment 345451 [details] [review]: lgtm
Review of attachment 345402 [details] [review]: lgtm
Created attachment 345487 [details] [review] turnPointMarker: Add support for showing markers for transit stops
Created attachment 345488 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
Created attachment 345490 [details] [review] mapView: Implement showing transit routes Also added functionallity to allow showing dynamically created route layer segments, optionally using a dashed style. This is used by transit routes to show walking and transit legs of journeys.
(In reply to Jonas Danielsson from comment #172) > Review of attachment 345448 [details] [review] [review]: > > Looks nice! > > There is a lot of magic numbers tho, that seems connected with the transit > route style. Can they be consolidated? > So that we have transitLeg.XYZ_COLOR, transitLeg.DASH_STEP or similar? (I > dunno if transitLeg is the correct class for this) Or if we should have a > new module. There's the color used for non-transit route lines (blue). I added that as a constant in mapView.js. So, now that color is used as a parameter to the _createRouteLayer() function in the non-transit case when creating a route line instead of dealing with that special case inside _createRouteLayer(). This is probably cleaner, I think. Just as in the transit case, where the color comes from the transit data (or using a transit-specific fallback color). Also, I'm not sure that the dash line parameters really belongs in transitPlan.js, I think it's more a matter of rendition. Not sure if having a separate module is really worth it for that. I added this as constants in here as well (one for the filled part's length and one for the gap between dahshes), what do you think? > > ::: src/mapView.js > @@ +573,3 @@ > + /* draw an outline by drawing a background path layer if needed > + * TODO: maybe we should add support for outlined path layers in > + let color = leg.color; > > Can this TODO result in a bug on the libchamplain product and then maybe be > removed? > Yeah, sounds good! I'll file a bug! > @@ +615,3 @@ > + } else { > + start = new TransitBoardMarker.TransitBoardMarker({ leg: > leg, > + routeLayer.add_node(lastPoint); > > space before } Yep
Created attachment 345491 [details] [review] turnPointMarker: Add support for showing markers for transit stops
(In reply to Marcus Lundblad from comment #180) > (In reply to Jonas Danielsson from comment #172) > > Review of attachment 345448 [details] [review] [review] [review]: > > > > Looks nice! > > > > There is a lot of magic numbers tho, that seems connected with the transit > > route style. Can they be consolidated? > > So that we have transitLeg.XYZ_COLOR, transitLeg.DASH_STEP or similar? (I > > dunno if transitLeg is the correct class for this) Or if we should have a > > new module. > > There's the color used for non-transit route lines (blue). I added that as a > constant in mapView.js. So, now that color is used as a parameter to the > _createRouteLayer() function in the non-transit case when creating a route > line instead of dealing with that special case inside _createRouteLayer(). > This is probably cleaner, I think. Just as in the transit case, where the > color comes from the transit data (or using a transit-specific fallback > color). Also, I'm not sure that the dash line parameters really belongs in > transitPlan.js, I think it's more a matter of rendition. Not sure if having > a separate module is really worth it for that. I added this as constants in > here as well (one for the filled part's length and one for the gap between > dahshes), what do you think? > > > > > ::: src/mapView.js > > @@ +573,3 @@ > > + /* draw an outline by drawing a background path layer if needed > > + * TODO: maybe we should add support for outlined path layers in > > + let color = leg.color; > > > > Can this TODO result in a bug on the libchamplain product and then maybe be > > removed? > > > Yeah, sounds good! I'll file a bug! > > > @@ +615,3 @@ > > + } else { > > + start = new TransitBoardMarker.TransitBoardMarker({ leg: > > leg, > > + routeLayer.add_node(lastPoint); > > > > space before } > > Yep Oh, and I found a bug in my previous patch, since Clutter uses integers (0-255) for the color components, the components from the Color.parseColor needs offsetting (everyting came of black).
(In reply to Jonas Danielsson from comment #173) > Review of attachment 345449 [details] [review] [review]: > > Nitty McNitface > > ::: src/turnPointMarker.js > @@ +89,3 @@ > + let blue = Color.parseColor(color, 2, 0x9C); > + > + let color = this._transitLeg ? this._transitLeg.color : null; > > nitty, could this be: > > return new Gdk.RGBA({ > red: Color.parseColor(color, 0, 0x21), > green: ... > }); Sure! > > And btw, what is the hex stuff? Actually, those are the original colors used for the turn-point marker (as the comment goes: "a GNOME:ish blue color). I made it so that these markers are kept that color for the non-transit stuff and use the transit leg color for transit. But I had hexstringyfied it before. But now changed it back to use the decimal divided by 255 notation as before (and actually the hex values where not correct here in my previous patch, as the Gdk.Color expects floating point 0.0..1.0, so it was a good nit, since I found the regression :))
Created attachment 345540 [details] [review] mapView: Add a view property to indicate showing a route Adds a new boolean property that is set to true when an actual route is rendered on the map. This will allow only showing the print button when a transit itinerary is "dived into". Also rename the current "routeVisible" property to "routingOpen" to avoid confusion, since the new property is what's set when a route is actually rendered on the map, the former property is set when routing is ongoing (and the routing headerbar button is pressed).
Created attachment 345541 [details] [review] mainWindow: Show the print button when a route is rendered Only show the print button when a route is rendered on the map. This way, the print button is hidden until an itinerary is selected when in transit mode. Also, only activate printing with the accelerator when actually showing a route.
(In reply to Jonas Danielsson from comment #150) > Review of attachment 345376 [details] [review] [review]: > > ::: src/mapView.js > @@ +84,3 @@ > > GObject.ParamFlags.WRITABLE, > + false), > + /* this property is true when a route is being shown on the map */ > > Are the comments above the wrong way around? Or are we really bad at naming? I have now renamed the old "routeVisible" property to "routingOpen" to reflect the toogle button being connecting to this property. And the new property is now "routeShowing" for when a route is actually rendered on the map (this will be triggering the print button now, so that printing is only enabled when selecting an individual itinerary in transit mode, this is done in a following patch).
Created attachment 345545 [details] [review] Add module to query an OpenTripPlanner instance Adds an openTripPlanner module, containing functionallity for interfacing against an OpenTripPlanner service. Also has functionallity to augment itineraries obtained with walk routing from GraphHopper (as used by the other routing modes). Furthermore, the transitPlan module contains interfacing classes modelling transit data Plan: Populated by a list of itineraries when performing a transit query. Itinerary: Represents one particular trip option in a search result. Contains one or more transit legs. Leg: Represents one distinct part of a trip, such as "take tram #5 departuring at 10:00 from Foo Street, get off at Bar Street, arrival at 10:12". Stop: Represents an intermediate stop along a transit leg (a place where the vehicle stops, but the itinerary doesn't board or alight the vehicle.
(In reply to Jonas Danielsson from comment #145) > Review of attachment 345371 [details] [review] [review]: > > So, this all looks very nice! But it is hard for me to follow, would it be > possible with a larger comment block on top of openTripPlanner.js that talks > a bit about general code flow and function? > Sure! I added a comment block, I tried to explain the flow in large parts without going into details about every single function. > ::: src/openTripPlanner.js > @@ +449,3 @@ > + > + let params = { fromPlace: stops[0].id, > + toPlace: stops[stops.length - 1].id }; > > we are stating to have a lot of this pattern, getting the last element of an > array. > > How about we add: > > if (!Array.prototype.last){ > Array.prototype.last = function(){ > return this[this.length - 1]; > }; > }; > > Somewhere? We already do String.prototype.format = Format.format; in > application.js, not sure if that belongs there though. > > Turning all those awkward constructs from stops[stops.length - 1]; to > stops.last; is appealing. > Done now, witch the aid of the new prototype added in main.js. > @@ +702,3 @@ > + duration: route.time / 1000, > + distance: route.distance, > + walkingInstructions: route.turnPoints > }); > > I am not crazy about this formatting, but not sure if an alternative is > better. > Can it be de-complexed? Creating the from/to-coordinate on their own lines > maybe? > > let fromCoord = [from.place.location.latitude, > from.place.location.longitude]; > let toCoord = [to.place.location.latitude, to.place.location.longitude]; > > And then the constructor will be more normal looking, and maybe we skip the > alignment? Yep. I did this and also elimenated the alignments. I did it an addional step by introducing temporaries for from.place.location and to.place.location to keep line lengths in check. > > ::: src/transitPlan.js > @@ +399,3 @@ > + delete params.tripShortName; > + > + this.parent(); > > what parent? Fixed now by correctly chaining up params.
Created attachment 345586 [details] [review] Add module with time utility functions Currently contains a function to parse an entered time into a normalized HH:MM string format.
Created attachment 345587 [details] [review] Add widget to select transit options This is used from within the routing sidebar when transit mode is selected.
Created attachment 345588 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
(In reply to Jonas Danielsson from comment #149) > Review of attachment 345375 [details] [review] [review]: > > This looks nice! But sidebar.js is growing! Do you see anyway of breaking > out functionality from this class? Indeed! I broke out the transit options "toolbar", that shows when transit is selected, into its own TransitOptionsPanel class. I also moved out the rather large function that parses the entered time into a normalized HH:MM string into a separate Time module. > > ::: src/sidebar.js > @@ +85,3 @@ > + 'trainCheckButton', > + 'subwayCheckButton', > + 'transitParametersMenuButton', > > Holy wall of widgets, Batman! :-) Should be a bit more manageble now, I think! > > @@ +108,3 @@ > + */ > + this._modeTransitToggle.sensitive = > + Application.routingDelegator.openTripPlanner.enabled; > > I wonder if this should be this._modeTransitTogge.visible = ... > > Can we get Andreas opinion? I think the consensus was to hide it, so I did that now. > > @@ +515,3 @@ > + return null; > + } > + this._transitTimeEntry.connect('activate', > > This is a pure function right, maybe move to Utils or if we have a bunch of > similar helpers, to a [something with time].js Yep! I moved it to a new time.js (even if it's just one function there for now, it's rather big so…)
Created attachment 345592 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
(In reply to Marcus Lundblad from comment #192) > (In reply to Jonas Danielsson from comment #149) > > Review of attachment 345375 [details] [review] [review] [review]: > > > I wonder if this should be this._modeTransitTogge.visible = ... > > > > Can we get Andreas opinion? > > I think the consensus was to hide it, so I did that now. > > I'll have to take that back. Actually it didn't work (maybe because the buttons used a linked style?), so I switched back to sensitive/insesitive for now.
Review of attachment 345490 [details] [review]: neat!
Review of attachment 345491 [details] [review]: yep
Review of attachment 345540 [details] [review]: ok
Review of attachment 345541 [details] [review]: mhm
Review of attachment 345545 [details] [review]: Nice! I am not a big fan of TODO:s, can you review them and see if some of them can be turned in to bugzilla bugs instead?
Review of attachment 345586 [details] [review]: mis-rebase ::: src/org.gnome.Maps.src.gresource.xml @@ -78,2 +78,4 @@ <file>storedRoute.js</file> <file>togeojson/togeojson.js</file> + <file>time.js</file> + <file>transitArrivalMarker.js</file> o_O ::: src/time.js @@ +23,3 @@ + * hour:min + * TODO: maybe try to use some library to get better locale handling, + * or push for something in GLib */ Could we get some example strings that this code parses here?
Review of attachment 345587 [details] [review]: lgtm
Review of attachment 345592 [details] [review]: some nits ::: src/sidebar.js @@ +96,3 @@ + */ + this._transitOptionsPanel = + this._modeTransitToggle.sensitive = Can this visible be set in the ui-file and this fit on one line? @@ +232,3 @@ + } else { + if (this._query.transportation === + RouteQuery.Transportation.TRANSIT) { this break hurs me, can something be done? I think I prefer > 80 chars to this @@ +296,3 @@ + if (prev) + row.set_header(new Gtk.Separator()); + }).bind(this)); no need to bind
(In reply to Jonas Danielsson from comment #202) > Review of attachment 345592 [details] [review] [review]: > > some nits > > ::: src/sidebar.js > @@ +96,3 @@ > + */ > + this._transitOptionsPanel = > + this._modeTransitToggle.sensitive = > > Can this visible be set in the ui-file and this fit on one line? > Something like setting sensible="False" in the UI and then: if (Application.routingDelegator.openTripPlanner.enabled) this._modeTransitToggle.sensitive = true; I guess. > @@ +232,3 @@ > + } else { > + if (this._query.transportation === > + RouteQuery.Transportation.TRANSIT) { > > this break hurs me, can something be done? I think I prefer > 80 chars to > this > > @@ +296,3 @@ > + if (prev) > + row.set_header(new Gtk.Separator()); > + }).bind(this)); > > no need to bind
Created attachment 345668 [details] [review] Add module with time utility functions Currently contains a function to parse an entered time into a normalized HH:MM string format.
Created attachment 345669 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
(In reply to Jonas Danielsson from comment #203) > Review of attachment 345592 [details] [review] [review]: > > some nits > > ::: src/sidebar.js > @@ +96,3 @@ > + */ > + this._transitOptionsPanel = > + this._modeTransitToggle.sensitive = > > Can this visible be set in the ui-file and this fit on one line? > Yep. Changed to set sensitive False in the UI and then programmatically set it to true when the if-condition on .enabled is true. > @@ +232,3 @@ > + } else { > + if (this._query.transportation === > + RouteQuery.Transportation.TRANSIT) { > > this break hurs me, can something be done? I think I prefer > 80 chars to > this > Yeah, it was only a couple of columns, so just moved it up. > @@ +296,3 @@ > + if (prev) > + row.set_header(new Gtk.Separator()); > + }).bind(this)); > > no need to bind Sure
(In reply to Jonas Danielsson from comment #200) > Review of attachment 345586 [details] [review] [review]: > > mis-rebase > > ::: src/org.gnome.Maps.src.gresource.xml > @@ -78,2 +78,4 @@ > <file>storedRoute.js</file> > <file>togeojson/togeojson.js</file> > + <file>time.js</file> > + <file>transitArrivalMarker.js</file> > > o_O Yeah, woops! Should be sorted now. > > ::: src/time.js > @@ +23,3 @@ > + * hour:min > + * TODO: maybe try to use some library to get better locale handling, > + * or push for something in GLib */ > > Could we get some example strings that this code parses here? Yep. Added some samples of input → output pairs.
Review of attachment 345491 [details] [review]: mark
Review of attachment 345668 [details] [review]: ok
Review of attachment 345669 [details] [review]: lgtm
Created attachment 345763 [details] [review] sidebar: Add functionallity for transit routing Adds a new mode button (enabled when the service file indicates an OpenTripPlanner instance, or if explicitly pointed out via an env variable). Adds showing transit itineraries in addition to regular turn-by-turn-based routes, and options for transit route search.
Since the consensus seems to be it's better to hide the transit mode button when no service is available, I updated the sidebar patch for this. Unfortunatly I couldn't get it working in the most obvious way, by setting visible = false in the template and programmatically make it visible when needed (nor the other way around), maybe some peculiarity with GtkRadioButtons and being part of a group? So, as a workaround just destroy the widget if it's not be shown (added a comment about why this was done as well)…
Review of attachment 345763 [details] [review]: ok
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-maps/issues/25.