From b75569cdfe8526bb3d8e202a6fd34756cb16919c Mon Sep 17 00:00:00 2001 From: Robert Stoll Date: Fri, 17 Dec 2021 23:07:51 +0100 Subject: [PATCH] #491 use map.center for loc as jumpTo requires it moreover: - also use the first load strategy for ?loc= (move into inner actOnFirstLoad function) - don't navigate in router.component.ts if neither the city nor the query params have changed --- src/app/city/map.service.ts | 30 +++--------- src/app/map/map.component.ts | 75 ++++++++++++++++------------- src/app/router/router.component.ts | 38 +++++++++------ src/app/services/routing.service.ts | 2 +- 4 files changed, 73 insertions(+), 72 deletions(-) diff --git a/src/app/city/map.service.ts b/src/app/city/map.service.ts index 5606945f..730bc155 100644 --- a/src/app/city/map.service.ts +++ b/src/app/city/map.service.ts @@ -97,7 +97,6 @@ export interface MapState { location: LngLat; zoom: number | 'auto'; } -export type MapStateOmitCalculatedFields = Omit; @Injectable() export class MapService { @@ -126,26 +125,21 @@ export class MapService { this.updateState({ bounds: bounds, + location: sharedLocation.location, zoom: sharedLocation.zoom, city: this.parseCity(cityIdOrAlias), }); } - updateState(mapState: MapStateOmitCalculatedFields): void { - const calculcatedMapState: MapState = this.calculateFields(mapState); - - if (!_.isEqual(calculcatedMapState, this.stateSubject.value)) { + updateState(newMapState: MapState): void { + if (!_.isEqual(newMapState, this.stateSubject.value)) { if (!environment.production) { - console.log('updating map state to', calculcatedMapState); + console.log('updating map state to', newMapState, 'loc', newMapState.location); } - this.stateSubject.next(calculcatedMapState); + this.stateSubject.next(newMapState); } } - calculateFields(mapState: MapStateOmitCalculatedFields): MapState { - return { ...mapState, location: getCentre(mapState.bounds) }; - } - get currentCity(): City | undefined { return this.stateSubject.value?.city; } @@ -158,15 +152,6 @@ export class MapService { ); } - get sharedLocation(): Observable { - return ( - this.state - .map(s => this.mapStateToSharedLocation(s)) - // we don't want to propagate a change if e.g. the bounds change (due to resizing of the browser for instance) - .pipe(distinctUntilChanged((x, y) => _.isEqual(x, y))) - ); - } - private mapStateToSharedLocation(state: MapState): SharedLocation { return { location: state.location, zoom: state.zoom }; } @@ -176,10 +161,11 @@ export class MapService { } setCity(city: City) { + const bounds = getCityBounds(city); this.updateState({ city: city, - bounds: getCityBounds(city), - // if we have not yet zoomed, then we want to fit to bounds + bounds: bounds, + location: getCentre(bounds), zoom: 'auto', }); } diff --git a/src/app/map/map.component.ts b/src/app/map/map.component.ts index 638f715c..7affc91b 100644 --- a/src/app/map/map.component.ts +++ b/src/app/map/map.component.ts @@ -18,7 +18,7 @@ import { DirectionsService } from '../directions/directions.service'; import { FountainService } from '../fountain/fountain.service'; import { City } from '../locations'; import { Bounds, Fountain, FountainCollection, LngLat } from '../types'; -import { MapService, MapState, MapStateOmitCalculatedFields } from '../city/map.service'; +import { MapService, MapState } from '../city/map.service'; import { MapConfig } from './map.config'; import { UserLocationService } from './user-location.service'; import _ from 'lodash'; @@ -87,8 +87,8 @@ export class MapComponent implements OnInit { this.mapLocationChangeSubject .pipe( - // don't trigger multiple change events in case of an animation - debounceTime(50) + // don't trigger multiple change events shortly after each other (e.g. when resizing window) + debounceTime(100) ) .subscribe(_ => this.handleMapLocationChange()), @@ -186,53 +186,62 @@ export class MapComponent implements OnInit { private firstStateChange = true; private adjustMapToStateChange(newState: MapState) { - const currentState = this.mapService.calculateFields(this.toMapStateOmitCaluclatedFields(newState.city)); + const self = this; + + const currentState = this.toMapState(newState.city); if (this.firstStateChange || !_.isEqual(currentState, newState)) { if (this.firstStateChange) { // we need to resize the map once as it is slightly displaced otherwise this.map.resize(); } - if (newState.zoom === 'auto') { - try { - // only refit city bounds if not zoomed into a fountain and still same city - if (this._mode === 'map' && this.mapService.currentCity === newState.city) { - const bounds = newState.bounds; - const options = { - maxDuration: 500, - pitch: 0, - bearing: 0, - offset: [0, 0] as [number, number], - }; - this.map.fitBounds([bounds.min, bounds.max], options); - if (this.firstStateChange) { - const afterFirstStyleLoad = () => { - // on first load, it happens that fitBounds does not trigger `moveend` all the time - // so we trigger one manually - this.mapLocationChangeSubject.next(); - this.map.off('styledata', afterFirstStyleLoad); + // only refit city bounds or jump to location if not zommed into fountain + if (this._mode === 'map') { + if (newState.zoom === 'auto') { + try { + // only refit city bounds if still same city + if (this.mapService.currentCity === newState.city) { + const options = { + maxDuration: 500, + pitch: 0, + bearing: 0, }; - this.map.on('styledata', afterFirstStyleLoad); + this.map.fitBounds([newState.bounds.min, newState.bounds.max], options); + actOnFirstLoad(); } + } catch (e: unknown) { + console.error(e); } - } catch (e: unknown) { - console.error(e); + } else { + this.map.jumpTo({ + center: newState.location, + zoom: newState.zoom, + around: newState.location, + }); + actOnFirstLoad(); } - } else { - this.map.jumpTo({ - center: [newState.location.lng, newState.location.lat], - zoom: newState.zoom, - }); } this.firstStateChange = false; } + + function actOnFirstLoad() { + if (self.firstStateChange) { + const afterFirstStyleLoad = () => { + // on first load, it happens that fitBounds and jumpTo do not trigger `moveend` (at least not always) + // so we trigger one manually + self.mapLocationChangeSubject.next(); + self.map.off('styledata', afterFirstStyleLoad); + }; + self.map.on('styledata', afterFirstStyleLoad); + } + } } - private toMapStateOmitCaluclatedFields(city: City | undefined): MapStateOmitCalculatedFields { + private toMapState(city: City | undefined): MapState { const mapboxBounds = this.map.getBounds(); const sw = mapboxBounds.getSouthWest(); const ne = mapboxBounds.getNorthEast(); - return { bounds: Bounds(sw, ne), city: city, zoom: this.map.getZoom() }; + return { bounds: Bounds(sw, ne), location: this.map.getCenter(), city: city, zoom: this.map.getZoom() }; } private zoomOut(): void { @@ -530,7 +539,7 @@ export class MapComponent implements OnInit { } private handleMapLocationChange() { - const state = this.toMapStateOmitCaluclatedFields(this.mapService.currentCity); + const state = this.toMapState(this.mapService.currentCity); this.mapService.updateState(state); } diff --git a/src/app/router/router.component.ts b/src/app/router/router.component.ts index 19e45bd2..19905898 100644 --- a/src/app/router/router.component.ts +++ b/src/app/router/router.component.ts @@ -11,11 +11,14 @@ import { ActivatedRoute, Router } from '@angular/router'; import { combineLatest } from 'rxjs/index'; import { SubscriptionService } from '../core/subscription.service'; import { MapService, MapState } from '../city/map.service'; -import { first, switchMap } from 'rxjs/operators'; +import { distinctUntilChanged, first, map, switchMap } from 'rxjs/operators'; import { FountainService } from '../fountain/fountain.service'; import { FountainSelector } from '../types'; import { LanguageService } from '../core/language.service'; import { RoutingService } from '../services/routing.service'; +import { environment } from '../../environments/environment'; +import _ from 'lodash'; +import { City } from '../locations'; @Component({ selector: 'app-router', @@ -37,35 +40,38 @@ export class RouterComponent implements OnInit { ngOnInit(): void { this.subscriptionService.registerSubscriptions( combineLatest([this.route.paramMap, this.route.queryParamMap]) - .pipe( - // we only want to know about the first url (i.e. the url the user entered into the addressbar) - // and not programmatic changes to the url - first(), - // need to use pipe(switchMap(...)) as WEBPACK somehow messes up everything and cannot find the extension method - switchMap(([routeParam, queryParam]) => this.routeValidator.handleUrlChange(routeParam, queryParam)) - ) + // TODO after webpack update: currently we need to use pipe(switchMap(...)) as WEBPACK somehow messes up + // everything and cannot find the extension method (tries to call another function) + .pipe(switchMap(([routeParam, queryParam]) => this.routeValidator.handleUrlChange(routeParam, queryParam))) .subscribe(_ => undefined /* nothing to do as side effect (navigating) occurs in handleUrlChange */), // TODO @ralf.hauser - navigating to a fountain currently happens in two route changes (which is not nice IMO). // first it changes to https://old.water-fountains.org/ch-zh?l=de and then it changes to https://old.water-fountains.org/ch-zh?l=de&i=Q27229673 // this is because city and fountainSelector are independent states (subscribed independently) // Yet, they cannot really change independently and should be consolidated in one state - combineLatest([this.mapService.state, this.fountainService.fountainSelector]).subscribe(([state, selector]) => { - this.navigateToNewRoute(state, selector); - }) + combineLatest([this.mapService.state, this.fountainService.fountainSelector]) + .pipe( + map( + ([state, selector]) => [state.city, this.toQueryParams(state, selector)] as [City | undefined, QueryParams] + ), + distinctUntilChanged((x, y) => _.isEqual(x, y)) + ) + .subscribe(([city, queryParams]) => { + console.log('navigate to', city, queryParams); + this.router.navigate([`/${city ? city : ''}`], { + queryParams: queryParams, + }); + }) ); } - private navigateToNewRoute(state: MapState, fountainSelector: FountainSelector | undefined): void { + private toQueryParams(state: MapState, fountainSelector: FountainSelector | undefined): QueryParams { const q: Omit = fountainSelector ? this.getQueryParamsForSelector(fountainSelector) : {}; - const queryParams: QueryParams = { + return { loc: `${state.location.lat},${state.location.lng},` + (state.zoom === 'auto' ? 'auto' : `${state.zoom}z`), ...q, l: this.languageService.currentLang, }; - this.router.navigate([`/${state.city ? state.city : ''}`], { - queryParams: queryParams, - }); } private getQueryParamsForSelector(fountainSelector: FountainSelector): Omit { diff --git a/src/app/services/routing.service.ts b/src/app/services/routing.service.ts index 4a2909f8..087e9f85 100644 --- a/src/app/services/routing.service.ts +++ b/src/app/services/routing.service.ts @@ -106,9 +106,9 @@ export class RoutingService { const { lngLat, database, updateId } = data; const city = this.getCityByLngLat(lngLat); if (city !== undefined) { - this.updateFromId(database, updateId); //TODO @ralf.hauser this function currently depends on that a fountain is in a city this.layoutService.flyToCity(city); + this.updateFromId(database, updateId); } return true; } else {