diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index 9c1757b12..5c10577c7 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -18,7 +18,13 @@ const mapMock = vi.hoisted(() => ({ on: vi.fn(), off: vi.fn(), panBy: vi.fn(), - latLngToContainerPoint: vi.fn(() => ({ x: 0, y: 0, distanceTo: () => 1000 })), + // A flat projection, 1000 px per degree: far enough apart that every + // booking line clears its declutter floor, which is measured along the + // projected line (#2275) rather than read off a canned distanceTo. + latLngToContainerPoint: vi.fn(([lat, lng]: [number, number]) => ({ + x: lng * 1000, y: lat * 1000, + distanceTo(other: { x: number; y: number }) { return Math.hypot(lng * 1000 - other.x, lat * 1000 - other.y) }, + })), // Panes: jsdom has none, so keep them in a map the pane tests can read back. panes: new Map(), getPane: vi.fn(function (this: void, name: string) { return mapMock.panes.get(name) }), diff --git a/client/src/components/Map/ReservationOverlay.test.tsx b/client/src/components/Map/ReservationOverlay.test.tsx index a0dd6c068..f19904dea 100644 --- a/client/src/components/Map/ReservationOverlay.test.tsx +++ b/client/src/components/Map/ReservationOverlay.test.tsx @@ -1,4 +1,4 @@ -// FE-COMP-RESOVERLAY-001 to FE-COMP-RESOVERLAY-024 +// FE-COMP-RESOVERLAY-001 to FE-COMP-RESOVERLAY-045 import React from 'react' import { describe, it, expect, vi, afterEach } from 'vitest' import { render, screen } from '../../../tests/helpers/render' @@ -389,6 +389,43 @@ describe('ReservationOverlay declutter floors and road routes', () => { expect(JSON.parse(lines()[0].getAttribute('data-points') || '[]')).toEqual([[48, 16], [51.5, 16]]) }) + it('FE-COMP-RESOVERLAY-043: a routed drive whose ends project close together still draws (#2275)', () => { + // Endpoints 50 px apart, under the 80 px car floor, so the straight hop would + // hide. The road between them loops out 0.3° (300 px) and back, which is what + // is actually drawn — and what the person was missing on their map. + const road: [number, number][] = [[48, 16], [48.3, 16.1], [48.3, 16.3], [48.05, 16]] + const { unmount } = renderOverlay({ reservations: [hop('car', 7, 0.05)] }) + expect(lines()).toHaveLength(0) + unmount() + + renderOverlay({ reservations: [hop('car', 7, 0.05)], roadRoutes: new Map([[7, road]]) }) + expect(lines()).toHaveLength(1) + expect(JSON.parse(lines()[0].getAttribute('data-points') || '[]')).toEqual(road) + }) + + it('FE-COMP-RESOVERLAY-044: a drive with a stop on the way is measured along its legs, not its ends', () => { + // From and to sit 20 px apart; the stop is 400 px away, so the two legs + // together draw 800 px of line. + renderOverlay({ + reservations: [booking({ + id: 8, type: 'car', + endpoints: [ + endpoint({ role: 'from', sequence: 0, name: 'Home', lat: 48, lng: 16 }), + endpoint({ role: 'stop', sequence: 1, name: 'Lake', lat: 48.4, lng: 16 }), + endpoint({ role: 'to', sequence: 2, name: 'Back', lat: 48.02, lng: 16 }), + ], + })], + }) + expect(lines()).toHaveLength(2) + }) + + it('FE-COMP-RESOVERLAY-045: a routed drive that really is tiny on screen stays decluttered', () => { + // Every point of the road projects within 30 px: nothing worth drawing yet. + const road: [number, number][] = [[48, 16], [48.01, 16.01], [48.02, 16.02], [48.03, 16]] + renderOverlay({ reservations: [hop('car', 7, 0.03)], roadRoutes: new Map([[7, road]]) }) + expect(lines()).toHaveLength(0) + }) + it('FE-COMP-RESOVERLAY-036: a booking without an endpoints array is skipped', () => { renderOverlay({ reservations: [booking({ endpoints: undefined })] }) expect(lines()).toHaveLength(0) diff --git a/client/src/components/Map/ReservationOverlay.tsx b/client/src/components/Map/ReservationOverlay.tsx index 22e380dd2..8a34cc99d 100644 --- a/client/src/components/Map/ReservationOverlay.tsx +++ b/client/src/components/Map/ReservationOverlay.tsx @@ -8,6 +8,7 @@ import { getTransitMapSegments, type TransitMapSegment } from './transitGeometry import { geodesicArcs } from './flightGeodesy' import { cleanEndpointName } from './reservationName' import { useSettingsStore } from '../../store/settingsStore' +import { hopIsVisible, labelFloorPx } from '../../utils/reservationRoutes' import type { Reservation, ReservationEndpoint } from '../../types' const ENDPOINT_PANE = 'reservation-endpoints' @@ -122,6 +123,12 @@ function computeDuration(from: ReservationEndpoint, to: ReservationEndpoint, fal return h > 0 ? `${h}h ${m}m` : `${m}m` } +/** What a hop will draw: the road it was routed along when there is one, else its arcs. */ +function linesFor(item: TransportItem, roadRoutes: Map | undefined): [number, number][][] { + const road = roadRoutes?.get(item.res.id) + return road && road.length >= 2 ? [road] : item.arcs +} + interface TransportItem { res: Reservation from: ReservationEndpoint @@ -200,26 +207,25 @@ export default function ReservationOverlay({ reservations, showConnections, onEn }, [reservations]) const visibleItems = useMemo(() => { + const project = (p: readonly [number, number]) => map.latLngToContainerPoint([p[0], p[1]]) return items.filter(item => { // A transit journey draws its real rail/bus alignment, not a straight from->to // line, so the endpoint-proximity declutter (which exists to hide tiny no-op // straight connectors) must not suppress it. Otherwise a zoomed-out day — e.g. one // with no other places to tighten the map onto — hides the whole route (#1570). if (item.transitSegs.length > 0) return true - const fromPx = map.latLngToContainerPoint([item.from.lat, item.from.lng]) - const toPx = map.latLngToContainerPoint([item.to.lat, item.to.lng]) - const minPx = item.type === 'flight' ? 50 : item.type === 'cruise' ? 150 : item.type === 'car' ? 80 : 200 - return fromPx.distanceTo(toPx) >= minPx + // Measured along what is drawn, not between the ends: a routed drive can + // cover half the screen while its endpoints sit close together (#2275). + return hopIsVisible(item.type, linesFor(item, roadRoutes), project) }) - }, [items, zoom, map]) + }, [items, zoom, map, roadRoutes]) const labelVisibleIds = useMemo(() => { const set = new Set() for (const item of visibleItems) { const fromPx = map.latLngToContainerPoint([item.from.lat, item.from.lng]) const toPx = map.latLngToContainerPoint([item.to.lat, item.to.lng]) - const minPx = item.type === 'flight' ? 50 : item.type === 'cruise' ? 300 : item.type === 'car' ? 150 : item.type === 'transit' ? 900 : 400 - if (fromPx.distanceTo(toPx) >= minPx) set.add(item.res.id) + if (fromPx.distanceTo(toPx) >= labelFloorPx(item.type)) set.add(item.res.id) } return set }, [visibleItems, zoom, map]) @@ -248,9 +254,7 @@ export default function ReservationOverlay({ reservations, showConnections, onEn )) } // Prefer the real road route (car/bus/taxi/bicycle) over the straight arc. - const road = roadRoutes?.get(item.res.id) - const lines = road && road.length >= 2 ? [road] : item.arcs - return lines.map((seg, segIdx) => ( + return linesFor(item, roadRoutes).map((seg, segIdx) => ( { attach(wide, labelled).update([hop(2, 'cruise', 0.4)], labelled) expect(glMarkers.map(m => m.el.textContent)).toEqual(['GOA', 'PMI']) }) + + it('FE-COMP-RESGL-043: a routed drive whose ends project close together still draws (#2275)', () => { + // 50 px between the ends, under the 80 px car floor; the road loops out 300 px. + const road: [number, number][] = [[48, 16], [48.3, 16.1], [48.3, 16.3], [48.05, 16]] + const bare = freshMap() + attach(bare).update([hop(7, 'car', 0.05)], opts) + expect(features(bare)).toHaveLength(0) + + const routed = freshMap() + attach(routed).update([hop(7, 'car', 0.05)], opts, new Map([[7, road]])) + expect(features(routed)).toHaveLength(1) + expect(features(routed)[0].geometry.coordinates).toEqual(road.map(([lat, lng]) => [lng, lat])) + }) + + it('FE-COMP-RESGL-044: a routed drive that really is tiny on screen stays decluttered', () => { + const road: [number, number][] = [[48, 16], [48.01, 16.01], [48.02, 16.02], [48.03, 16]] + const map = freshMap() + attach(map).update([hop(7, 'car', 0.03)], opts, new Map([[7, road]])) + expect(features(map)).toHaveLength(0) + }) }) describe('ReservationMapboxOverlay waypoint edge cases', () => { diff --git a/client/src/components/Map/reservationsMapbox.ts b/client/src/components/Map/reservationsMapbox.ts index 8b6e64a63..5972e737d 100644 --- a/client/src/components/Map/reservationsMapbox.ts +++ b/client/src/components/Map/reservationsMapbox.ts @@ -13,6 +13,7 @@ import { Plane, Train, Ship, Car, Bus, Sailboat, Bike, CarTaxiFront, Route, Tram import { getTransitMapSegments } from './transitGeometry' import { geodesicArcs } from './flightGeodesy' import { cleanEndpointName } from './reservationName' +import { hopIsVisible, labelFloorPx } from '../../utils/reservationRoutes' import { escapeHtml } from '@trek/shared' import type { Reservation, ReservationEndpoint } from '../../types' @@ -259,27 +260,31 @@ export class ReservationMapboxOverlay { }) } + /** What a hop will draw: the road it was routed along when there is one, else its arcs. */ + private linesFor(item: TransportItem): [number, number][][] { + const road = this.roadRoutes.get(item.res.id) + return road && road.length >= 2 ? [road] : item.arcs + } + private render() { const map = this.map if (!this.map.getSource(RESERVATION_SOURCE_ID)) return const show = this.opts.showConnections - // Visible filter: require the on-screen pixel distance between - // endpoints to exceed a type-specific minimum, same as the Leaflet - // overlay, so tiny no-op transport lines don't clutter the map. + // Visible filter: a hop draws once what it will draw is long enough on + // screen to be worth it, same as the Leaflet overlay, so tiny no-op + // transport lines don't clutter the map. Measured along the drawn line, + // not between the ends: a routed drive can cover half the screen while + // its endpoints sit close together (#2275). + const project = (p: readonly [number, number]) => map.project([p[1], p[0]]) const visibleItems = show ? this.items.filter(item => { try { // A transit journey draws its real alignment, not a straight from->to line, so // don't let the endpoint-proximity declutter hide it when the map is zoomed out // (#1570). Mirrors the Leaflet overlay. if (item.type === 'transit' && getTransitMapSegments(item.res).length > 0) return true - const fromPx = map.project([item.from.lng, item.from.lat]) - const toPx = map.project([item.to.lng, item.to.lat]) - const dx = fromPx.x - toPx.x, dy = fromPx.y - toPx.y - const dist = Math.sqrt(dx * dx + dy * dy) - const minPx = item.type === 'flight' ? 50 : item.type === 'cruise' ? 150 : item.type === 'car' ? 80 : 200 - return dist >= minPx + return hopIsVisible(item.type, this.linesFor(item), project) } catch { return true } }) : [] @@ -293,8 +298,7 @@ export class ReservationMapboxOverlay { const toPx = map.project([item.to.lng, item.to.lat]) const dx = fromPx.x - toPx.x, dy = fromPx.y - toPx.y const dist = Math.sqrt(dx * dx + dy * dy) - const minPx = item.type === 'flight' ? 50 : item.type === 'cruise' ? 300 : item.type === 'car' ? 150 : item.type === 'transit' ? 900 : 400 - if (dist >= minPx) labelVisibleIds.add(item.res.id) + if (dist >= labelFloorPx(item.type)) labelVisibleIds.add(item.res.id) } catch { /* ignore */ } } } @@ -320,9 +324,7 @@ export class ReservationMapboxOverlay { })) } // Prefer the real road route (car/bus/taxi/bicycle) over the straight arc. - const road = this.roadRoutes.get(item.res.id) - const lines = road && road.length >= 2 ? [road] : item.arcs - return lines.map(seg => ({ + return this.linesFor(item).map(seg => ({ type: 'Feature' as const, properties: { resId: item.res.id, diff --git a/client/src/utils/reservationRoutes.test.ts b/client/src/utils/reservationRoutes.test.ts index 1f0be69ec..38405e767 100644 --- a/client/src/utils/reservationRoutes.test.ts +++ b/client/src/utils/reservationRoutes.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { isRoutableReservation, visibleRouteReservations } from './reservationRoutes' +import { hopIsVisible, isRoutableReservation, labelFloorPx, lineFloorPx, visibleRouteReservations } from './reservationRoutes' import type { Day, Reservation, ReservationEndpoint } from '../types' function endpoint(role: 'from' | 'to', lat: number, lng: number): ReservationEndpoint { @@ -116,3 +116,41 @@ describe('visibleRouteReservations with a selected day', () => { expect(visibleRouteReservations([r], { visibleConnectionIds: [], showTransitRoutes: true, selectedDayId: null, days })).toEqual([r]) }) }) + +describe('hopIsVisible (#2275)', () => { + // A flat projection: one unit of lat or lng is one pixel. + const px = (p: readonly [number, number]) => ({ x: p[1], y: p[0] }) + + it('keeps the type floors of the old endpoint check', () => { + expect(lineFloorPx('car')).toBe(80) + expect(lineFloorPx('flight')).toBe(50) + expect(lineFloorPx('cruise')).toBe(150) + expect(lineFloorPx('train')).toBe(200) + expect(labelFloorPx('car')).toBe(150) + expect(labelFloorPx('transit')).toBe(900) + expect(labelFloorPx('bus')).toBe(400) + }) + + it('measures a straight two-point line exactly as the endpoint gap', () => { + expect(hopIsVisible('car', [[[0, 0], [0, 79]]], px)).toBe(false) + expect(hopIsVisible('car', [[[0, 0], [0, 80]]], px)).toBe(true) + }) + + it('walks a routed line, so a loop between close ends counts its full length', () => { + const loop: [number, number][] = [[0, 0], [30, 0], [30, 30], [0, 30], [0, 5]] + expect(hopIsVisible('car', [loop], px)).toBe(true) + expect(hopIsVisible('train', [loop], px)).toBe(false) + }) + + it('adds the legs of a multi-stop booking together', () => { + const legs: [number, number][][] = [[[0, 0], [0, 50]], [[0, 50], [0, 100]]] + expect(hopIsVisible('car', legs, px)).toBe(false) + expect(hopIsVisible('car', [[[0, 0], [0, 50], [0, 100]]], px)).toBe(true) + }) + + it('draws nothing for an empty or one-point line', () => { + expect(hopIsVisible('car', [], px)).toBe(false) + expect(hopIsVisible('car', [[]], px)).toBe(false) + expect(hopIsVisible('car', [[[0, 0]]], px)).toBe(false) + }) +}) diff --git a/client/src/utils/reservationRoutes.ts b/client/src/utils/reservationRoutes.ts index 3f5cc5f2f..94645d0ae 100644 --- a/client/src/utils/reservationRoutes.ts +++ b/client/src/utils/reservationRoutes.ts @@ -52,3 +52,63 @@ export function visibleRouteReservations(reservations: Reservation[], options: R set.has(r.id) ) } + +/** The type-specific floors, in screen pixels, under which a hop is not worth drawing. */ +const LINE_FLOOR_PX: Record = { flight: 50, cruise: 150, car: 80 } +const LINE_FLOOR_DEFAULT_PX = 200 + +/** Under this floor the endpoint labels stay off, so short hops do not stack text. */ +const LABEL_FLOOR_PX: Record = { flight: 50, cruise: 300, car: 150, transit: 900 } +const LABEL_FLOOR_DEFAULT_PX = 400 + +/** How far the line is allowed to disappear before its endpoints cannot be told apart. */ +export function lineFloorPx(type: string): number { + return LINE_FLOOR_PX[type] ?? LINE_FLOOR_DEFAULT_PX +} + +export function labelFloorPx(type: string): number { + return LABEL_FLOOR_PX[type] ?? LABEL_FLOOR_DEFAULT_PX +} + +/** + * Whether a hop is worth drawing at the current zoom. + * + * The declutter exists for a tiny straight connector, the kind that would sit + * under its own endpoint markers and add nothing but clutter. It used to be + * decided from the two endpoints alone, which was fine while every hop was a + * straight line: the line was never longer than the gap it bridged. + * + * A routed car booking is not that line. It follows the road, and a road that + * leaves a town, loops round a lake and comes back to the next town along + * can run across half the screen while its two ends project a handful of + * pixels apart. The old check then hid the whole drive, and the person looking + * at their trip saw a gap in the route until they zoomed in far enough for + * the endpoints to separate (#2275). A booking with stops on the way has the + * same shape: the ends may be close, the drawn path is not. + * + * So the decision looks at what will actually be drawn. A hop stays hidden + * only while every polyline it would draw is shorter on screen than the + * floor, walked point to point. A straight two-point line measures the same + * as before, so every existing floor holds for the case it was written for. + * + * `project` is whichever renderer's own projection: Leaflet's + * latLngToContainerPoint or Mapbox's project, wrapped to take [lat, lng]. + */ +export function hopIsVisible( + type: string, + lines: readonly (readonly [number, number][])[], + project: (point: readonly [number, number]) => { x: number; y: number }, +): boolean { + const floor = lineFloorPx(type) + for (const line of lines) { + let length = 0 + let prev = line.length > 0 ? project(line[0]) : null + for (let i = 1; i < line.length && prev; i++) { + const next = project(line[i]) + length += Math.hypot(next.x - prev.x, next.y - prev.y) + if (length >= floor) return true + prev = next + } + } + return false +}