Live location sharing - smart location marker (#8232)

* extract location markers into generic Marker

Signed-off-by: Kerry Archibald <kerrya@element.io>

* wrap marker in smartmarker

Signed-off-by: Kerry Archibald <kerrya@element.io>

* test smartmarker

Signed-off-by: Kerry Archibald <kerrya@element.io>

* remove skinned-sdk

Signed-off-by: Kerry Archibald <kerrya@element.io>

* lint

Signed-off-by: Kerry Archibald <kerrya@element.io>

* better types for LocationBodyContent

Signed-off-by: Kerry Archibald <kerrya@element.io>

__mocks__/maplibre-gl.js

@@ -16,10 +16,11 @@ const MockGeolocateInstance = new MockGeolocateControl();
 const MockMarker = {}
 MockMarker.setLngLat = jest.fn().mockReturnValue(MockMarker);
 MockMarker.addTo = jest.fn().mockReturnValue(MockMarker);
+MockMarker.remove = jest.fn().mockReturnValue(MockMarker);
 module.exports = {
     Map: jest.fn().mockReturnValue(MockMapInstance),
     GeolocateControl: jest.fn().mockReturnValue(MockGeolocateInstance),
     Marker: jest.fn().mockReturnValue(MockMarker),
     LngLat,
-    NavigationControl
+    NavigationControl,
 };

src/components/views/location/LocationViewDialog.tsx

@@ -22,7 +22,7 @@ import BaseDialog from "../dialogs/BaseDialog";
 import { IDialogProps } from "../dialogs/IDialogProps";
 import { LocationBodyContent } from '../messages/MLocationBody';
 import { tileServerFromWellKnown } from '../../../utils/WellKnownUtils';
-import { parseGeoUri, locationEventGeoUri, createMap } from '../../../utils/location';
+import { parseGeoUri, locationEventGeoUri, createMapWithCoords } from '../../../utils/location';
 
 interface IProps extends IDialogProps {
     matrixClient: MatrixClient;
@@ -54,7 +54,7 @@ export default class LocationViewDialog extends React.Component<IProps, IState>
 
         this.props.matrixClient.on(ClientEvent.ClientWellKnown, this.updateStyleUrl);
 
-        this.map = createMap(
+        this.map = createMapWithCoords(
             this.coords,
             true,
             this.getBodyId(),

src/components/views/location/Marker.tsx

@@ -33,9 +33,10 @@ interface Props {
 /**
  * Generic location marker
  */
-const Marker: React.FC<Props> = ({ id, roomMember, useMemberColor }) => {
+const Marker = React.forwardRef<HTMLDivElement, Props>(({ id, roomMember, useMemberColor }, ref) => {
     const memberColorClass = useMemberColor && roomMember ? getUserNameColorClass(roomMember.userId) : '';
     return <div
+        ref={ref}
         id={id}
         className={classNames("mx_Marker", memberColorClass, {
             "mx_Marker_defaultColor": !memberColorClass,
@@ -53,6 +54,6 @@ const Marker: React.FC<Props> = ({ id, roomMember, useMemberColor }) => {
             }
         </div>
     </div>;
-};
+});
 
 export default Marker;

src/components/views/location/SmartMarker.tsx

@@ -0,0 +1,83 @@
+/*
+Copyright 2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import React, { useCallback, useEffect, useState } from 'react';
+import maplibregl from 'maplibre-gl';
+import { RoomMember } from 'matrix-js-sdk/src/matrix';
+
+import { createMarker, parseGeoUri } from '../../../utils/location';
+import Marker from './Marker';
+
+const useMapMarker = (
+    map: maplibregl.Map,
+    geoUri: string,
+): { marker?: maplibregl.Marker, onElementRef: (el: HTMLDivElement) => void } => {
+    const [marker, setMarker] = useState<maplibregl.Marker>();
+
+    const onElementRef = useCallback((element: HTMLDivElement) => {
+        if (marker || !element) {
+            return;
+        }
+        const coords = parseGeoUri(geoUri);
+        const newMarker = createMarker(coords, element);
+        newMarker.addTo(map);
+        setMarker(newMarker);
+    }, [marker, geoUri, map]);
+
+    useEffect(() => {
+        if (marker) {
+            const coords = parseGeoUri(geoUri);
+            marker.setLngLat({ lon: coords.longitude, lat: coords.latitude });
+        }
+    }, [marker, geoUri]);
+
+    useEffect(() => () => {
+        if (marker) {
+            marker.remove();
+        }
+    }, [marker]);
+
+    return {
+        marker,
+        onElementRef,
+    };
+};
+
+interface SmartMarkerProps {
+    map: maplibregl.Map;
+    geoUri: string;
+    id?: string;
+    // renders MemberAvatar when provided
+    roomMember?: RoomMember;
+    // use member text color as background
+    useMemberColor?: boolean;
+}
+
+/**
+ * Generic location marker
+ */
+const SmartMarker: React.FC<SmartMarkerProps> = ({ id, map, geoUri, roomMember, useMemberColor }) => {
+    const { onElementRef } = useMapMarker(map, geoUri);
+
+    return <Marker
+        ref={onElementRef}
+        id={id}
+        roomMember={roomMember}
+        useMemberColor={useMemberColor}
+    />;
+};
+
+export default SmartMarker;

src/components/views/messages/MLocationBody.tsx

@@ -30,7 +30,7 @@ import Modal from '../../../Modal';
 import {
     parseGeoUri,
     locationEventGeoUri,
-    createMap,
+    createMapWithCoords,
     getLocationShareErrorMessage,
     LocationShareError,
 } from '../../../utils/location';
@@ -75,7 +75,7 @@ export default class MLocationBody extends React.Component<IBodyProps, IState> {
 
         this.context.on(ClientEvent.ClientWellKnown, this.updateStyleUrl);
 
-        this.map = createMap(
+        this.map = createMapWithCoords(
             this.coords,
             false,
             this.bodyId,
@@ -138,18 +138,6 @@ export function isSelfLocation(locationContent: ILocationContent): boolean {
     return assetType == LocationAssetType.Self;
 }
 
-interface ILocationBodyContentProps {
-    mxEvent: MatrixEvent;
-    bodyId: string;
-    markerId: string;
-    error: Error;
-    tooltip?: string;
-    onClick?: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void;
-    zoomButtons?: boolean;
-    onZoomIn?: () => void;
-    onZoomOut?: () => void;
-}
-
 export const LocationBodyFallbackContent: React.FC<{ event: MatrixEvent, error: Error }> = ({ error, event }) => {
     const errorType = error?.message as LocationShareError;
     const message = `${_t('Unable to load map')}: ${getLocationShareErrorMessage(errorType)}`;
@@ -167,8 +155,18 @@ export const LocationBodyFallbackContent: React.FC<{ event: MatrixEvent, error:
     </div>;
 };
 
-export function LocationBodyContent(props: ILocationBodyContentProps):
+interface LocationBodyContentProps {
-        React.ReactElement<HTMLDivElement> {
+    mxEvent: MatrixEvent;
+    bodyId: string;
+    markerId: string;
+    error: Error;
+    tooltip?: string;
+    onClick?: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void;
+    zoomButtons?: boolean;
+    onZoomIn?: () => void;
+    onZoomOut?: () => void;
+}
+export const LocationBodyContent: React.FC<LocationBodyContentProps> = (props) => {
     const mapDiv = <div
         id={props.bodyId}
         onClick={props.onClick}
@@ -200,7 +198,7 @@ export function LocationBodyContent(props: ILocationBodyContentProps):
                 : null
         }
     </div>;
-}
+};
 
 interface IZoomButtonsProps {
     onZoomIn: () => void;

src/utils/location/map.ts

@@ -24,6 +24,46 @@ import { findMapStyleUrl } from "./findMapStyleUrl";
 import { LocationShareError } from "./LocationShareErrors";
 
 export const createMap = (
+    interactive: boolean,
+    bodyId: string,
+    onError: (error: Error) => void,
+): maplibregl.Map => {
+    try {
+        const styleUrl = findMapStyleUrl();
+
+        const map = new maplibregl.Map({
+            container: bodyId,
+            style: styleUrl,
+            zoom: 15,
+            interactive,
+        });
+
+        map.on('error', (e) => {
+            logger.error(
+                "Failed to load map: check map_style_url in config.json has a "
+                + "valid URL and API key",
+                e.error,
+            );
+            onError(new Error(LocationShareError.MapStyleUrlNotReachable));
+        });
+
+        return map;
+    } catch (e) {
+        logger.error("Failed to render map", e);
+        throw e;
+    }
+};
+
+export const createMarker = (coords: GeolocationCoordinates, element: HTMLElement): maplibregl.Marker => {
+    const marker = new maplibregl.Marker({
+        element,
+        anchor: 'bottom',
+        offset: [0, -1],
+    }).setLngLat({ lon: coords.longitude, lat: coords.latitude });
+    return marker;
+};
+
+export const createMapWithCoords = (
     coords: GeolocationCoordinates,
     interactive: boolean,
     bodyId: string,
@@ -31,24 +71,14 @@ export const createMap = (
     onError: (error: Error) => void,
 ): maplibregl.Map => {
     try {
-        const styleUrl = findMapStyleUrl();
+        const map = createMap(interactive, bodyId, onError);
+
         const coordinates = new maplibregl.LngLat(coords.longitude, coords.latitude);
+        // center on coordinates
+        map.setCenter(coordinates);
 
-        const map = new maplibregl.Map({
+        const marker = createMarker(coords, document.getElementById(markerId));
-            container: bodyId,
+        marker.addTo(map);
-            style: styleUrl,
-            center: coordinates,
-            zoom: 15,
-            interactive,
-        });
-
-        new maplibregl.Marker({
-            element: document.getElementById(markerId),
-            anchor: 'bottom',
-            offset: [0, -1],
-        })
-            .setLngLat(coordinates)
-            .addTo(map);
 
         map.on('error', (e) => {
             logger.error(

test/components/views/location/SmartMarker-test.tsx

@@ -0,0 +1,80 @@
+/*
+Copyright 2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import React from 'react';
+import { mount } from 'enzyme';
+import { mocked } from 'jest-mock';
+import maplibregl from 'maplibre-gl';
+
+import SmartMarker from '../../../../src/components/views/location/SmartMarker';
+
+jest.mock('../../../../src/utils/location/findMapStyleUrl', () => ({
+    findMapStyleUrl: jest.fn().mockReturnValue('tileserver.com'),
+}));
+
+describe('<SmartMarker />', () => {
+    const mockMap = new maplibregl.Map();
+    const mockMarker = new maplibregl.Marker();
+
+    const defaultProps = {
+        map: mockMap,
+        geoUri: 'geo:43.2,54.6',
+    };
+    const getComponent = (props = {}) =>
+        mount(<SmartMarker {...defaultProps} {...props} />);
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+    });
+
+    it('creates a marker on mount', () => {
+        const component = getComponent();
+        expect(component).toMatchSnapshot();
+
+        component.setProps({});
+
+        // marker added only once
+        expect(maplibregl.Marker).toHaveBeenCalledTimes(1);
+        // set to correct position
+        expect(mockMarker.setLngLat).toHaveBeenCalledWith({ lon: 54.6, lat: 43.2 });
+        // added to map
+        expect(mockMarker.addTo).toHaveBeenCalledWith(mockMap);
+    });
+
+    it('updates marker position on change', () => {
+        const component = getComponent({ geoUri: 'geo:40,50' });
+
+        component.setProps({ geoUri: 'geo:41,51' });
+        component.setProps({ geoUri: 'geo:42,52' });
+
+        // marker added only once
+        expect(maplibregl.Marker).toHaveBeenCalledTimes(1);
+        // set positions
+        expect(mocked(mockMarker.setLngLat)).toHaveBeenCalledWith({ lat: 40, lon: 50 });
+        expect(mocked(mockMarker.setLngLat)).toHaveBeenCalledWith({ lat: 41, lon: 51 });
+        expect(mocked(mockMarker.setLngLat)).toHaveBeenCalledWith({ lat: 42, lon: 52 });
+    });
+
+    it('removes marker on unmount', () => {
+        const component = getComponent();
+        expect(component).toMatchSnapshot();
+
+        component.setProps({});
+
+        component.unmount();
+        expect(mockMarker.remove).toHaveBeenCalled();
+    });
+});

test/components/views/location/__snapshots__/Marker-test.tsx.snap

@@ -1,7 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`<Marker /> renders with location icon when no room member 1`] = `
-<Marker
+<ForwardRef
   id="abc123"
 >
   <div
@@ -16,5 +16,5 @@ exports[`<Marker /> renders with location icon when no room member 1`] = `
       />
     </div>
   </div>
-</Marker>
+</ForwardRef>
 `;

test/components/views/location/__snapshots__/SmartMarker-test.tsx.snap

@@ -0,0 +1,61 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`<SmartMarker /> creates a marker on mount 1`] = `
+<SmartMarker
+  geoUri="geo:43.2,54.6"
+  map={
+    MockMap {
+      "_events": Object {},
+      "_eventsCount": 0,
+      "_maxListeners": undefined,
+      "addControl": [MockFunction],
+      "removeControl": [MockFunction],
+      Symbol(kCapture): false,
+    }
+  }
+>
+  <ForwardRef>
+    <div
+      className="mx_Marker mx_Marker_defaultColor"
+    >
+      <div
+        className="mx_Marker_border"
+      >
+        <div
+          className="mx_Marker_icon"
+        />
+      </div>
+    </div>
+  </ForwardRef>
+</SmartMarker>
+`;
+
+exports[`<SmartMarker /> removes marker on unmount 1`] = `
+<SmartMarker
+  geoUri="geo:43.2,54.6"
+  map={
+    MockMap {
+      "_events": Object {},
+      "_eventsCount": 0,
+      "_maxListeners": undefined,
+      "addControl": [MockFunction],
+      "removeControl": [MockFunction],
+      Symbol(kCapture): false,
+    }
+  }
+>
+  <ForwardRef>
+    <div
+      className="mx_Marker mx_Marker_defaultColor"
+    >
+      <div
+        className="mx_Marker_border"
+      >
+        <div
+          className="mx_Marker_icon"
+        />
+      </div>
+    </div>
+  </ForwardRef>
+</SmartMarker>
+`;