diff --git a/AGENTS.md b/AGENTS.md index fd558628..cb710e3c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,10 @@ OSRM Frontend is the browser-based routing interface served at https://map.proje - All new functionality must have unit test coverage. +### Naming + +- **Use descriptive, functional names for variables and functions.** Avoid cute, playful, or whimsical names (e.g. no `makeRoute`, `switchTo`, `handleIt`). Names should describe what the thing is or does, not how the author feels about it. + ### Notes - Commit generated artifacts (`bundle.js`, `bundle.js.map`, `dist/`) only when necessary. diff --git a/src/index.js b/src/index.js index 04fbcd9c..a9e9fa6c 100644 --- a/src/index.js +++ b/src/index.js @@ -43,6 +43,7 @@ var localization = require('./localization'); var initialLayers = require('./initial_layers'); var layerUtils = require('./layer_utils'); var routeZoom = require('./route_zoom'); +var resolveInitialAlternative = require('./route_alternative'); require('./polyfill'); var parsedOptions = urlState.parse(window.location.search.slice(1)); @@ -721,6 +722,23 @@ lrmControl.on('alternateChosen', function(e) { lrmControl.on('routeselected', function(e) { var route = e.route || {}; + // On the initial route load, the LRM always selects route[0] first. + // If the URL requested a different alternative, switch to it now. + // Falls back to route[0] when the requested alternative no longer exists. + if (shouldFitRoute) { + var switchTo = resolveInitialAlternative(route, e.alternatives, state.options.alternative); + if (switchTo) { + // Re-fire on lrmControl so the LRM-internal listeners + // (_routeSelected, _selectAlt) also process the correct route. + lrmControl.fire('routeselected', switchTo); + return; + } + } + + // Track selected alternative in URL state and persist to URL + state.options.alternative = route.routesIndex; + state.update(); + // Route export: build GeoJSON and hand it to the tools control for GPX download var routeGeoJSON = { type: 'Feature', diff --git a/src/route_alternative.js b/src/route_alternative.js new file mode 100644 index 00000000..2d766c89 --- /dev/null +++ b/src/route_alternative.js @@ -0,0 +1,32 @@ +'use strict'; + +/** + * Decides whether to switch from the current route to a different alternative + * based on a desired index (e.g. read from URL state on initial page load). + * + * The LRM always selects route[0] first. If the caller wants a different + * alternative, this function returns the re-arranged event payload so the + * caller can re-fire `routeselected` with the correct route. + * + * @param {Object} route – the currently selected route (must have .routesIndex) + * @param {Array} alternatives – other route objects (may be empty / undefined) + * @param {*} desiredAlt – the 0-based index of the desired alternative + * @returns {Object|null} – {route, alternatives} to re-fire, or null when + * no switch is needed (desiredAlt is 0, missing, + * already matches, or out of bounds) + */ +module.exports = function resolveInitialAlternative(route, alternatives, desiredAlt) { + var index = Number(desiredAlt); + if (!index || index < 0 || index % 1 !== 0) return null; // 0, NaN, undefined, null, "", negative, non-integer + if (route.routesIndex === index) return null; // already on the desired route + + var allRoutes = [route].concat(alternatives || []); + if (index >= allRoutes.length) return null; // desiredAlt no longer exists + + return { + route: allRoutes[index], + alternatives: allRoutes.filter(function(_, i) { + return i !== index; + }) + }; +}; diff --git a/src/state.js b/src/state.js index 97a8660e..58186fe7 100644 --- a/src/state.js +++ b/src/state.js @@ -18,10 +18,6 @@ var State = L.Class.extend({ this.set(default_options); - this._lrm.on('routeselected', function(e) { - this.options.alternative = e.route.routesIndex; - }, this); - this._lrm.getPlan().on('waypointschanged', function() { this.options.waypoints = this._lrm.getWaypoints(); this.update({ push: true }); }.bind(this)); diff --git a/test/route_alternative.test.js b/test/route_alternative.test.js new file mode 100644 index 00000000..b19ad47f --- /dev/null +++ b/test/route_alternative.test.js @@ -0,0 +1,170 @@ +'use strict'; + +var resolveInitialAlternative = require('../src/route_alternative'); + +function makeRoute(routesIndex) { + return { name: 'Route ' + routesIndex, coordinates: [], routesIndex: routesIndex }; +} + +describe('resolveInitialAlternative', function() { + + describe('returns null (no switch) when', function() { + + test('desiredAlt is 0', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], 0); + expect(result).toBeNull(); + }); + + test('desiredAlt is undefined', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], undefined); + expect(result).toBeNull(); + }); + + test('desiredAlt is null', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], null); + expect(result).toBeNull(); + }); + + test('desiredAlt is NaN (unparseable string)', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], 'abc'); + expect(result).toBeNull(); + }); + + test('desiredAlt is an empty string', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], ''); + expect(result).toBeNull(); + }); + + test('route.routesIndex already matches desiredAlt', function() { + var route = makeRoute(1); + var result = resolveInitialAlternative(route, [makeRoute(0), makeRoute(2)], 1); + expect(result).toBeNull(); + }); + + test('desiredAlt exceeds available routes (out of bounds)', function() { + var route = makeRoute(0); + // Only 3 routes total (indices 0,1,2) — desiredAlt 5 doesn't exist + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 5); + expect(result).toBeNull(); + }); + + test('desiredAlt equals length (off by one)', function() { + var route = makeRoute(0); + // 3 routes, index 3 is past the end + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 3); + expect(result).toBeNull(); + }); + + test('there are no alternatives and desiredAlt > 0', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [], 1); + expect(result).toBeNull(); + }); + + test('alternatives is undefined', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, undefined, 1); + expect(result).toBeNull(); + }); + + test('alternatives is null', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, null, 1); + expect(result).toBeNull(); + }); + + test('desiredAlt is negative', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1)], -1); + expect(result).toBeNull(); + }); + + test('desiredAlt is a non-integer float (e.g. "1.5")', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], 1.5); + expect(result).toBeNull(); + }); + + test('desiredAlt is a float string ("1.5")', function() { + var route = makeRoute(0); + var result = resolveInitialAlternative(route, [makeRoute(1), makeRoute(2)], '1.5'); + expect(result).toBeNull(); + }); + }); + + describe('returns a switch payload when', function() { + + test('desiredAlt matches a middle alternative', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 1); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toHaveLength(2); + expect(result.alternatives).toContain(route0); + expect(result.alternatives).toContain(route2); + }); + + test('desiredAlt is the last alternative', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 2); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route2); + expect(result.alternatives).toEqual([route0, route1]); + }); + + test('desiredAlt is a string ("1") that coerces to number', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + // This mimics the URL parser returning strings + var result = resolveInitialAlternative(route0, [route1, route2], '1'); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toContain(route0); + expect(result.alternatives).toContain(route2); + }); + + test('there is exactly one alternative and it is the target', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var result = resolveInitialAlternative(route0, [route1], 1); + + expect(result).not.toBeNull(); + expect(result.route).toBe(route1); + expect(result.alternatives).toEqual([route0]); + }); + }); + + describe('returns referentially correct objects', function() { + + test('the returned route is the exact same object from the input', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var result = resolveInitialAlternative(route0, [route1], 1); + + expect(result.route).toBe(route1); + }); + + test('returned alternatives are the exact same objects', function() { + var route0 = makeRoute(0); + var route1 = makeRoute(1); + var route2 = makeRoute(2); + var result = resolveInitialAlternative(route0, [route1, route2], 1); + + expect(result.alternatives[0]).toBe(route0); + expect(result.alternatives[1]).toBe(route2); + }); + }); +});