From 56d36de3388854925558466792d2848eb83ee420 Mon Sep 17 00:00:00 2001 From: manmorjim Date: Mon, 10 Aug 2020 14:12:22 +0200 Subject: [PATCH 1/4] Use the feature geom instead of the pixel clicked Use the feature geometry when it is a point in order to improve the precision of the popup position --- src/viz/popups/Popup.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/viz/popups/Popup.ts b/src/viz/popups/Popup.ts index 3751546f..c1474033 100644 --- a/src/viz/popups/Popup.ts +++ b/src/viz/popups/Popup.ts @@ -159,18 +159,15 @@ export class Popup { const popupContent: string = generatePopupContent(elements, features); this.open(); this.setContent(popupContent); + let popupCoordinates = coordinates; // to be more accurate on points we use the feature // coordinates instead of the coordinates where the user clicked - // if (features[0].geometry.type === 'Point') { - // const featureCoordinates = pixels2coordinates( - // features[0].geometry.coordinates, - // this._deckInstance - // ); - // this.setCoordinates(featureCoordinates); - // } else { - this.setCoordinates(coordinates); - // } + if (features[0].geometry.type === 'Point') { + popupCoordinates = features[0].geometry.coordinates; + } + + this.setCoordinates(popupCoordinates); } else { this.close(); } From f372991079f36c4563afec7e0060b58521b6dca3 Mon Sep 17 00:00:00 2001 From: manmorjim Date: Mon, 10 Aug 2020 14:55:49 +0200 Subject: [PATCH 2/4] add type and coordinates to the mocked fetaures --- .../__tests__/interactivity-popups.spec.ts | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/viz/__tests__/interactivity-popups.spec.ts b/src/viz/__tests__/interactivity-popups.spec.ts index 3accd262..0bae6770 100644 --- a/src/viz/__tests__/interactivity-popups.spec.ts +++ b/src/viz/__tests__/interactivity-popups.spec.ts @@ -3,7 +3,11 @@ import { InteractivityEvent, LayerInteractivity } from '../layer/LayerInteractiv import { Popup } from '../popups/Popup'; const FAKE_COORDS = [0, 0]; -const FEATURE = { type: 'feature', properties: { cartodb_id: '15', pop: 10435000 } }; +const FEATURE = { + type: 'feature', + properties: { cartodb_id: '15', pop: 10435000 }, + geometry: { type: 'Point', coordinates: [0, 0] } +}; let layer: Layer; let setContentMockClick: jasmine.Spy; @@ -14,12 +18,36 @@ beforeEach(() => { const sourceData = [ FEATURE, - { type: 'feature', properties: { cartodb_id: '3', pop: 30 } }, - { type: 'feature', properties: { cartodb_id: '4', pop: 40 } }, - { type: 'feature', properties: { cartodb_id: '5', pop: null } }, - { type: 'feature', properties: { cartodb_id: '6', pop: undefined } }, - { type: 'feature', properties: { cartodb_id: '7', pop: 50 } }, - { type: 'feature', properties: { cartodb_id: '8', pop: 90 } } + { + type: 'feature', + properties: { cartodb_id: '3', pop: 30 }, + geometry: { type: 'Point', coordinates: [0, 0] } + }, + { + type: 'feature', + properties: { cartodb_id: '4', pop: 40 }, + geometry: { type: 'Point', coordinates: [0, 0] } + }, + { + type: 'feature', + properties: { cartodb_id: '5', pop: null }, + geometry: { type: 'Point', coordinates: [0, 0] } + }, + { + type: 'feature', + properties: { cartodb_id: '6', pop: undefined }, + geometry: { type: 'Point', coordinates: [0, 0] } + }, + { + type: 'feature', + properties: { cartodb_id: '7', pop: 50 }, + geometry: { type: 'Point', coordinates: [0, 0] } + }, + { + type: 'feature', + properties: { cartodb_id: '8', pop: 90 }, + geometry: { type: 'Point', coordinates: [0, 0] } + } ]; spyOn(layer, 'getViewportFeatures').and.returnValue(Promise.resolve(sourceData)); From 3a0ac2f2fe75669c86a273adda825efa43a3c92f Mon Sep 17 00:00:00 2001 From: manmorjim Date: Fri, 14 Aug 2020 13:51:49 +0200 Subject: [PATCH 3/4] SQL API request for feature coordinate This fixes the postion error for popups when the feature comes from an MVT layer. --- .../utils.ts => core/utils/request-utils.ts} | 0 src/maps/Client.ts | 2 +- src/sql/SQLClient.ts | 82 +++++++++++++++++++ src/viz/layer/LayerInteractivity.ts | 5 +- src/viz/popups/Popup.ts | 19 +++-- src/viz/source/DOSource.ts | 5 ++ src/viz/source/GeoJSONSource.ts | 5 ++ src/viz/source/SQLSource.ts | 17 +++- src/viz/source/Source.ts | 2 + 9 files changed, 126 insertions(+), 11 deletions(-) rename src/{maps/utils.ts => core/utils/request-utils.ts} (100%) create mode 100644 src/sql/SQLClient.ts diff --git a/src/maps/utils.ts b/src/core/utils/request-utils.ts similarity index 100% rename from src/maps/utils.ts rename to src/core/utils/request-utils.ts diff --git a/src/maps/Client.ts b/src/maps/Client.ts index 582874cb..0eb70f5e 100644 --- a/src/maps/Client.ts +++ b/src/maps/Client.ts @@ -2,7 +2,7 @@ import { AggregationType } from '@/data/operations/aggregation'; import { uuidv4 } from '@/core/utils/uuid'; import { Credentials } from '../auth'; import errorHandlers from './errors'; -import { encodeParameter, getRequest, postRequest } from './utils'; +import { encodeParameter, getRequest, postRequest } from '../core/utils/request-utils'; const REQUEST_GET_MAX_URL_LENGTH = 2048; const VECTOR_EXTENT = 2048; diff --git a/src/sql/SQLClient.ts b/src/sql/SQLClient.ts new file mode 100644 index 00000000..ee430c6a --- /dev/null +++ b/src/sql/SQLClient.ts @@ -0,0 +1,82 @@ +import { DEFAULT_ID_PROPERTY } from '@/viz/source/SQLSource'; +import { Credentials } from '../auth'; +import { encodeParameter, getRequest, postRequest } from '../core/utils/request-utils'; +import errorHandlers from '../maps/errors'; + +const REQUEST_GET_MAX_URL_LENGTH = 2048; +export const GEOMETRY_WKT_ALIAS = 'the_geom_wkt'; + +export class SQLClient { + private _credentials: Credentials; + + constructor(credentials: Credentials) { + this._credentials = credentials; + } + + public async getRowById(id: string, origin: string) { + const sql = `SELECT *, ST_AsText(the_geom) as ${GEOMETRY_WKT_ALIAS} FROM (${origin}) as __cdb_orig WHERE ${DEFAULT_ID_PROPERTY} = '${id}'`; + return this.execute(sql); + } + + /** + * + * @param sql + */ + public async execute(sql: string) { + if (!sql) { + throw new Error('Please provide an SQL query'); + } + + let response; + + try { + response = await fetch(this.makeSQLApiRequest(sql)); + } catch (error) { + throw new Error( + `Failed to connect to Maps API with the user ('${this._credentials.username}'): ${error}` + ); + } + + const result = (await response.json()) as never; + + if (!response.ok) { + this.dealWithSQLApiErrors(response, result); + } + + return result; + } + + private makeSQLApiRequest(sql: string) { + const encodedApiKey = encodeParameter('api_key', this._credentials.apiKey); + const parameters = [encodedApiKey]; + const url = this.generateSQLApiUrl(parameters); + + const getUrl = `${url}&${encodeParameter('q', sql)}`; + + if (getUrl.length < REQUEST_GET_MAX_URL_LENGTH) { + return getRequest(getUrl); + } + + return postRequest(url, sql); + } + + private dealWithSQLApiErrors( + response: { status: number }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + result: any + ) { + const errorForCode = errorHandlers[response.status]; + + if (errorForCode) { + errorForCode(this._credentials); + return; + } + + throw new Error(`${JSON.stringify(result.errors)}`); + } + + private generateSQLApiUrl(parameters: string[] = []) { + const base = `${this._credentials.serverURL}api/v1/sql`; + return `${base}?${parameters.join('&')}`; + } +} diff --git a/src/viz/layer/LayerInteractivity.ts b/src/viz/layer/LayerInteractivity.ts index f63fb932..6f679508 100644 --- a/src/viz/layer/LayerInteractivity.ts +++ b/src/viz/layer/LayerInteractivity.ts @@ -140,7 +140,10 @@ export class LayerInteractivity { if (eventType === InteractivityEvent.CLICK) { if (!this._clickPopupHandler) { - this._clickPopupHandler = popup.createHandler(elements); + this._clickPopupHandler = popup.createHandler( + elements, + this._layer.source.getRemoteFeatureCoordinates.bind(this._layer.source) + ); } handlerFn = this._clickPopupHandler; diff --git a/src/viz/popups/Popup.ts b/src/viz/popups/Popup.ts index c1474033..5d4ec4af 100644 --- a/src/viz/popups/Popup.ts +++ b/src/viz/popups/Popup.ts @@ -153,21 +153,24 @@ export class Popup { * @param elements popup elements to generate popup * content. */ - public createHandler(elements: PopupElement[] | string[] | null = []) { - return ([features, coordinates]: [Record[], number[], HammerInput]) => { + public createHandler( + elements: PopupElement[] | string[] | null = [], + getRemoteFeatureCoordinatesFn?: (feature: Record) => Promise + ) { + return async ([features, coordinates]: [Record[], number[], HammerInput]) => { if (features.length > 0) { const popupContent: string = generatePopupContent(elements, features); this.open(); this.setContent(popupContent); - let popupCoordinates = coordinates; + this.setCoordinates(coordinates); - // to be more accurate on points we use the feature + // to be more accurate on points we use the real feature // coordinates instead of the coordinates where the user clicked - if (features[0].geometry.type === 'Point') { - popupCoordinates = features[0].geometry.coordinates; + if (features[0].geometry.type === 'Point' && getRemoteFeatureCoordinatesFn) { + getRemoteFeatureCoordinatesFn(features[0]).then(remoteCoordinates => + this.setCoordinates(remoteCoordinates) + ); } - - this.setCoordinates(popupCoordinates); } else { this.close(); } diff --git a/src/viz/source/DOSource.ts b/src/viz/source/DOSource.ts index adad685f..58a3eee4 100644 --- a/src/viz/source/DOSource.ts +++ b/src/viz/source/DOSource.ts @@ -158,6 +158,11 @@ export class DOSource extends Source { return this._credentials; } + // eslint-disable-next-line class-methods-use-this, @typescript-eslint/no-unused-vars + public getRemoteFeatureCoordinates(_feature: Record): Promise { + throw new Error('Method not implemented.'); + } + public async init(): Promise { // Get geography from metadata const variable = await this._getVariable(this._variable); diff --git a/src/viz/source/GeoJSONSource.ts b/src/viz/source/GeoJSONSource.ts index e34a4d4d..a54ed168 100644 --- a/src/viz/source/GeoJSONSource.ts +++ b/src/viz/source/GeoJSONSource.ts @@ -71,6 +71,11 @@ export class GeoJSONSource extends Source { .flat(); } + // eslint-disable-next-line class-methods-use-this + public getRemoteFeatureCoordinates(feature: Record): Promise { + return new Promise(resolve => resolve((feature.geometry as any).coordinates)); + } + public async init(): Promise { if (!this.needsInitialization) { return true; diff --git a/src/viz/source/SQLSource.ts b/src/viz/source/SQLSource.ts index f9165f0b..8a7a7ea5 100644 --- a/src/viz/source/SQLSource.ts +++ b/src/viz/source/SQLSource.ts @@ -2,6 +2,7 @@ import { Credentials, defaultCredentials } from '@/auth'; import { MapInstance, MapOptions, Client } from '@/maps/Client'; import { uuidv4 } from '@/core/utils/uuid'; import { SQLFilterApplicator } from '@/viz/filters/SQLFilterApplicator'; +import { SQLClient, GEOMETRY_WKT_ALIAS } from '@/sql/SQLClient'; import { Source, SourceProps, @@ -21,7 +22,7 @@ export interface SourceOptions { mapOptions?: MapOptions; } -const DEFAULT_ID_PROPERTY = 'cartodb_id'; +export const DEFAULT_ID_PROPERTY = 'cartodb_id'; export const defaultMapOptions: MapOptions = { vectorExtent: 2048, @@ -93,6 +94,20 @@ export class SQLSource extends Source { return `SELECT * FROM (${this._value}) as originalQuery ${whereClause}`.trim(); } + public async getRemoteFeatureCoordinates(feature: Record): Promise { + const sqlClient = new SQLClient(this._credentials); + const id = (feature.properties as any)[DEFAULT_ID_PROPERTY] as string; + const element = await sqlClient.getRowById(id, this._value); + const remoteFeature = (element as any).rows[0]; + const remoteCoordinatesWkt = remoteFeature[GEOMETRY_WKT_ALIAS] as string; + const remoteCoords = remoteCoordinatesWkt + .replace(/POINT\s*\(([^\s]+)\s([^\s]+)\)/gi, '$1,$2') + .split(',') + .map(coordStr => Number.parseFloat(coordStr)); + + return remoteCoords; + } + /** * It returns the props of the source: * - URL of the tiles provided by MAPs API diff --git a/src/viz/source/Source.ts b/src/viz/source/Source.ts index d106c276..7058086f 100644 --- a/src/viz/source/Source.ts +++ b/src/viz/source/Source.ts @@ -75,6 +75,8 @@ export abstract class Source extends WithEvents { abstract getFeatures(excludedFilters: string[]): Record[]; + abstract getRemoteFeatureCoordinates(feature: Record): Promise; + // eslint-disable-next-line @typescript-eslint/no-unused-vars, class-methods-use-this addFilter(_filterId: string, _filter: ColumnFilters) { throw new Error(`Method not implemented`); From 4ce5b5203ab2f40a97dfd0f402c64dcebaed438d Mon Sep 17 00:00:00 2001 From: manmorjim Date: Sun, 16 Aug 2020 13:17:33 +0200 Subject: [PATCH 4/4] Caching remote coordinates calculated Adding a "cache" system in LayerInteractivity in order to cache the remotes coordinates calculated for each feature. --- src/viz/layer/LayerInteractivity.ts | 42 +++++++++++++++++++++++++---- src/viz/popups/Popup.ts | 30 ++++++++++----------- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/viz/layer/LayerInteractivity.ts b/src/viz/layer/LayerInteractivity.ts index 6f679508..ea08e2c0 100644 --- a/src/viz/layer/LayerInteractivity.ts +++ b/src/viz/layer/LayerInteractivity.ts @@ -1,7 +1,8 @@ import { Deck, RGBAColor } from '@deck.gl/core'; -import { Popup, PopupElement } from '../popups/Popup'; +import { Popup, PopupElement, REMOTE_COORD_FLAG } from '../popups/Popup'; import { Style, StyleProperties } from '../style/Style'; import { StyledLayer } from '../style/layer-style'; +import { DEFAULT_ID_PROPERTY } from '../source/SQLSource'; export class LayerInteractivity { private _deckInstance?: Deck; @@ -28,6 +29,12 @@ export class LayerInteractivity { private _clickPopupHandler?: InteractionHandler; private _hoverPopupHandler?: InteractionHandler; + /** + * Remote coordinates cache in order to prevent + * requesting a feature multiple times + */ + private _remoteCoordinates: Record; + constructor(options: LayerInteractivityOptions) { this._layer = options.layer; @@ -55,6 +62,8 @@ export class LayerInteractivity { } this._layerEmitFn = options.layerEmitFn; + + this._remoteCoordinates = {}; } public onClick(info: any, event: HammerInput) { @@ -75,6 +84,12 @@ export class LayerInteractivity { if (eventType === InteractivityEvent.CLICK) { this._clickFeature = object; + + // to be more accurate on points we use the real feature + // coordinates instead of the coordinates where the user clicked + if (this._clickFeature && this._clickFeature.geometry.type === 'Point') { + this.calculateRemoteCoordinates(this._clickFeature); + } } else if (eventType === InteractivityEvent.HOVER) { this._hoverFeature = object; this._setStyleCursor(info); @@ -140,10 +155,7 @@ export class LayerInteractivity { if (eventType === InteractivityEvent.CLICK) { if (!this._clickPopupHandler) { - this._clickPopupHandler = popup.createHandler( - elements, - this._layer.source.getRemoteFeatureCoordinates.bind(this._layer.source) - ); + this._clickPopupHandler = popup.createHandler(elements); } handlerFn = this._clickPopupHandler; @@ -295,6 +307,26 @@ export class LayerInteractivity { return new Style(defaultHighlightProps); } + + private calculateRemoteCoordinates(feature: Record) { + const remoteCoords = this._remoteCoordinates[feature.properties[DEFAULT_ID_PROPERTY]]; + + if (remoteCoords) { + // eslint-disable-next-line no-param-reassign + feature.geometry.coordinates = remoteCoords; + // eslint-disable-next-line no-param-reassign + feature.properties[REMOTE_COORD_FLAG] = true; + } else { + this._layer.source.getRemoteFeatureCoordinates(feature).then(coords => { + this._remoteCoordinates[feature.properties[DEFAULT_ID_PROPERTY]] = coords; + + if (this._clickPopup) { + // prevent rendering to avoid "jumping" effect + this._clickPopup.setCoordinates(coords, false); + } + }); + } + } } /** diff --git a/src/viz/popups/Popup.ts b/src/viz/popups/Popup.ts index 5d4ec4af..4984fb0f 100644 --- a/src/viz/popups/Popup.ts +++ b/src/viz/popups/Popup.ts @@ -13,6 +13,13 @@ const defaultOptions: PopupOptions = { position: 'top-right' }; +/** + * Attribute name for the flag used to check if + * the feature coordinates were already calculated + * from the SQL API + */ +export const REMOTE_COORD_FLAG = '__cdb_remote_coords'; + /** * @class * This class wraps the popup based on the @@ -65,14 +72,14 @@ export class Popup { * * @param coordinates with long lat. */ - public setCoordinates(coordinates: number[]) { + public setCoordinates(coordinates: number[], render = true) { if (!coordinates || coordinates.length !== 2) { throw new CartoPopupError('Popup coordinates invalid', popupErrorTypes.COORDINATE_INVALID); } this._coordinates = coordinates; - if (this._deckInstance) { + if (render && this._deckInstance) { this._render(); } } @@ -153,24 +160,17 @@ export class Popup { * @param elements popup elements to generate popup * content. */ - public createHandler( - elements: PopupElement[] | string[] | null = [], - getRemoteFeatureCoordinatesFn?: (feature: Record) => Promise - ) { + public createHandler(elements: PopupElement[] | string[] | null = []) { return async ([features, coordinates]: [Record[], number[], HammerInput]) => { if (features.length > 0) { const popupContent: string = generatePopupContent(elements, features); this.open(); this.setContent(popupContent); - this.setCoordinates(coordinates); - - // to be more accurate on points we use the real feature - // coordinates instead of the coordinates where the user clicked - if (features[0].geometry.type === 'Point' && getRemoteFeatureCoordinatesFn) { - getRemoteFeatureCoordinatesFn(features[0]).then(remoteCoordinates => - this.setCoordinates(remoteCoordinates) - ); - } + + const popupCoordinates = features[0].properties[REMOTE_COORD_FLAG] + ? features[0].geometry.coordinates + : coordinates; + this.setCoordinates(popupCoordinates); } else { this.close(); }