Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/client/src/translations/en/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -2104,7 +2104,8 @@
"raster": "Raster",
"vector_light": "Vector (Light)",
"vector_dark": "Vector (Dark)",
"show-scale": "Show scale"
"show-scale": "Show scale",
"show-labels": "Show marker names"
},
"table_context_menu": {
"delete_row": "Delete row"
Expand Down
54 changes: 41 additions & 13 deletions apps/client/src/widgets/collections/geomap/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { ViewModeProps } from "../interface";
import { createNewNote, moveMarker } from "./api";
import openContextMenu, { openMapContextMenu } from "./context_menu";
import Map from "./map";
import { DEFAULT_MAP_LAYER_NAME } from "./map_layer";
import { DEFAULT_MAP_LAYER_NAME, MAP_LAYERS, MapLayer } from "./map_layer";
import Marker, { GpxTrack } from "./marker";

const DEFAULT_COORDINATES: [number, number] = [3.878638227135724, 446.6630455551659];
Expand All @@ -45,10 +45,11 @@ export default function GeoView({ note, noteIds, viewConfig, saveConfig }: ViewM
const [ state, setState ] = useState(State.Normal);
const [ coordinates, setCoordinates ] = useState(viewConfig?.view?.center);
const [ zoom, setZoom ] = useState(viewConfig?.view?.zoom);
const [ layerName ] = useNoteLabel(note, "map:style");
const [ hasScale ] = useNoteLabelBoolean(note, "map:scale");
const [ hideLabels ] = useNoteLabelBoolean(note, "map:hideLabels");
const [ isReadOnly ] = useNoteLabelBoolean(note, "readOnly");
const [ notes, setNotes ] = useState<FNote[]>([]);
const layerData = useLayerData(note);
const spacedUpdate = useSpacedUpdate(() => {
if (viewConfig) {
saveConfig(viewConfig);
Expand Down Expand Up @@ -152,7 +153,7 @@ export default function GeoView({ note, noteIds, viewConfig, saveConfig }: ViewM
apiRef={apiRef} containerRef={containerRef}
coordinates={coordinates}
zoom={zoom}
layerName={layerName ?? DEFAULT_MAP_LAYER_NAME}
layerData={layerData}
viewportChanged={(coordinates, zoom) => {
if (!viewConfig) viewConfig = {};
viewConfig.view = { center: coordinates, zoom };
Expand All @@ -162,13 +163,35 @@ export default function GeoView({ note, noteIds, viewConfig, saveConfig }: ViewM
onContextMenu={onContextMenu}
scale={hasScale}
>
{notes.map(note => <NoteWrapper note={note} isReadOnly={isReadOnly} />)}
{notes.map(note => <NoteWrapper note={note} isReadOnly={isReadOnly} hideLabels={hideLabels} />)}
</Map>}
<GeoMapTouchBar state={state} map={apiRef.current} />
</div>
);
}

function useLayerData(note: FNote) {
const [ layerName ] = useNoteLabel(note, "map:style");
// Memo is needed because it would generate unnecessary reloads due to layer change.
const layerData = useMemo(() => {
// Custom layers.
if (layerName?.startsWith("http")) {
return {
name: "Custom",
type: "raster",
url: layerName,
attribution: ""
} satisfies MapLayer;
}

// Built-in layers.
const layerData = MAP_LAYERS[layerName ?? ""] ?? MAP_LAYERS[DEFAULT_MAP_LAYER_NAME];
return layerData;
}, [ layerName ]);

return layerData;
}

function ToggleReadOnlyButton({ note }: { note: FNote }) {
const [ isReadOnly, setReadOnly ] = useNoteLabelBoolean(note, "readOnly");

Expand All @@ -179,31 +202,36 @@ function ToggleReadOnlyButton({ note }: { note: FNote }) {
/>;
}

function NoteWrapper({ note, isReadOnly }: { note: FNote, isReadOnly: boolean }) {
function NoteWrapper({ note, isReadOnly, hideLabels }: {
note: FNote,
isReadOnly: boolean,
hideLabels: boolean
}) {
const mime = useNoteProperty(note, "mime");
const [ location ] = useNoteLabel(note, LOCATION_ATTRIBUTE);

if (mime === "application/gpx+xml") {
return <NoteGpxTrack note={note} />;
return <NoteGpxTrack note={note} hideLabels={hideLabels} />;
}

if (location) {
const latLng = location?.split(",", 2).map((el) => parseFloat(el)) as [ number, number ] | undefined;
if (!latLng) return;
return <NoteMarker note={note} editable={!isReadOnly} latLng={latLng} />;
return <NoteMarker note={note} editable={!isReadOnly} latLng={latLng} hideLabels={hideLabels} />;
}
}

function NoteMarker({ note, editable, latLng }: { note: FNote, editable: boolean, latLng: [number, number] }) {
function NoteMarker({ note, editable, latLng, hideLabels }: { note: FNote, editable: boolean, latLng: [number, number], hideLabels: boolean }) {
// React to changes
const [ color ] = useNoteLabel(note, "color");
const [ iconClass ] = useNoteLabel(note, "iconClass");
const [ archived ] = useNoteLabelBoolean(note, "archived");

const title = useNoteProperty(note, "title");
const icon = useMemo(() => {
return buildIcon(note.getIcon(), note.getColorClass() ?? undefined, title, note.noteId, archived);
}, [ iconClass, color, title, note.noteId, archived]);
const titleOrNone = hideLabels ? undefined : title;
return buildIcon(note.getIcon(), note.getColorClass() ?? undefined, titleOrNone, note.noteId, archived);
}, [ iconClass, color, title, note.noteId, archived, hideLabels ]);

const onClick = useCallback(() => {
appContext.triggerCommand("openInPopup", { noteIdOrPath: note.noteId });
Expand Down Expand Up @@ -235,7 +263,7 @@ function NoteMarker({ note, editable, latLng }: { note: FNote, editable: boolean
/>;
}

function NoteGpxTrack({ note }: { note: FNote }) {
function NoteGpxTrack({ note, hideLabels }: { note: FNote, hideLabels?: boolean }) {
const [ xmlString, setXmlString ] = useState<string>();
const blob = useNoteBlob(note);

Expand All @@ -256,7 +284,7 @@ function NoteGpxTrack({ note }: { note: FNote }) {

const options = useMemo<GPXOptions>(() => ({
markers: {
startIcon: buildIcon(note.getIcon(), note.getColorClass(), note.title),
startIcon: buildIcon(note.getIcon(), note.getColorClass(), hideLabels ? undefined : note.title),
endIcon: buildIcon("bxs-flag-checkered"),
wptIcons: {
"": buildIcon("bx bx-pin")
Expand All @@ -265,7 +293,7 @@ function NoteGpxTrack({ note }: { note: FNote }) {
polyline_options: {
color: note.getLabelValue("color") ?? "blue"
}
}), [ color, iconClass ]);
}), [ color, iconClass, hideLabels ]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The dependency array for useMemo is not correctly capturing all dependencies, which can lead to stale data for the track options.

  1. color and iconClass (from lines 282-283) are arrays returned by useNoteLabel ([value, setter]). Their references are stable across re-renders, so useMemo will not re-evaluate when the label values change. You should destructure the value from them (e.g., const [colorValue] = useNoteLabel(...)) and use colorValue in the dependency array.
  2. note.title is used on line 287 to build the startIcon, but it's not listed as a dependency. To make it reactive to changes, you should use const title = useNoteProperty(note, "title"); and include title in the dependency array.
  3. note.getLabelValue("color") is used for polyline_options on line 294. The color value from useNoteLabel should be used here as well to ensure consistency and reactivity.

A full fix would involve changes outside of this line, but the dependency array should be updated to include all reactive values it depends on.

return xmlString && <GpxTrack gpxXmlString={xmlString} options={options} />;
}

Expand Down
12 changes: 5 additions & 7 deletions apps/client/src/widgets/collections/geomap/map.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useEffect, useImperativeHandle, useRef, useState } from "preact/hooks";
import L, { control, LatLng, Layer, LeafletMouseEvent } from "leaflet";
import "leaflet/dist/leaflet.css";
import { MAP_LAYERS } from "./map_layer";
import { MAP_LAYERS, type MapLayer } from "./map_layer";
import { ComponentChildren, createContext, RefObject } from "preact";
import { useElementSize, useSyncedRef } from "../../react/hooks";

Expand All @@ -12,7 +12,7 @@ interface MapProps {
containerRef?: RefObject<HTMLDivElement>;
coordinates: LatLng | [number, number];
zoom: number;
layerName: string;
layerData: MapLayer;
viewportChanged: (coordinates: LatLng, zoom: number) => void;
children: ComponentChildren;
onClick?: (e: LeafletMouseEvent) => void;
Expand All @@ -21,7 +21,7 @@ interface MapProps {
scale: boolean;
}

export default function Map({ coordinates, zoom, layerName, viewportChanged, children, onClick, onContextMenu, scale, apiRef, containerRef: _containerRef, onZoom }: MapProps) {
export default function Map({ coordinates, zoom, layerData, viewportChanged, children, onClick, onContextMenu, scale, apiRef, containerRef: _containerRef, onZoom }: MapProps) {
const mapRef = useRef<L.Map>(null);
const containerRef = useSyncedRef<HTMLDivElement>(_containerRef);

Expand Down Expand Up @@ -49,8 +49,6 @@ export default function Map({ coordinates, zoom, layerName, viewportChanged, chi
const [ layer, setLayer ] = useState<Layer>();
useEffect(() => {
async function load() {
const layerData = MAP_LAYERS[layerName];

if (layerData.type === "vector") {
const style = (typeof layerData.style === "string" ? layerData.style : await layerData.style());
await import("@maplibre/maplibre-gl-leaflet");
Expand All @@ -68,7 +66,7 @@ export default function Map({ coordinates, zoom, layerName, viewportChanged, chi
}

load();
}, [ layerName ]);
}, [ layerData ]);

// Attach layer to the map.
useEffect(() => {
Expand Down Expand Up @@ -139,7 +137,7 @@ export default function Map({ coordinates, zoom, layerName, viewportChanged, chi
return (
<div
ref={containerRef}
className={`geo-map-container ${MAP_LAYERS[layerName].isDarkTheme ? "dark" : ""}`}
className={`geo-map-container ${layerData.isDarkTheme ? "dark" : ""}`}
>
<ParentMap.Provider value={mapRef.current}>
{children}
Expand Down
19 changes: 8 additions & 11 deletions apps/client/src/widgets/collections/geomap/map_layer.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
export interface MapLayer {
name: string;
isDarkTheme?: boolean;
}

interface VectorLayer extends MapLayer {
export type MapLayer = ({
type: "vector";
style: string | (() => Promise<{}>)
}

interface RasterLayer extends MapLayer {
} | {
type: "raster";
url: string;
attribution: string;
}
}) & {
// Common properties
name: string;
isDarkTheme?: boolean;
};

export const MAP_LAYERS: Record<string, VectorLayer | RasterLayer> = {
export const MAP_LAYERS: Record<string, MapLayer> = {
"openstreetmap": {
name: "OpenStreetMap",
type: "raster",
Expand Down
4 changes: 2 additions & 2 deletions apps/client/src/widgets/note_bars/CollectionProperties.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,8 @@ function CheckBoxPropertyView({ note, property }: { note: FNote, property: Check
<FormListToggleableItem
icon={property.icon}
title={property.label}
currentValue={value}
onChange={setValue}
currentValue={ property.reverseValue ? !value : value }
onChange={newValue => setValue(property.reverseValue ? !newValue : newValue)}
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export interface CheckBoxProperty {
label: string;
bindToLabel: FilterLabelsByType<boolean>;
icon?: string;
/** When true, the checkbox will be checked when the label value is false. Useful when the label represents a "hide" action, without exposing double negatives to the user. */
reverseValue?: boolean;
}

export interface ButtonProperty {
Expand Down Expand Up @@ -156,6 +158,13 @@ export const bookPropertiesConfig: Record<ViewTypeOptions, BookConfig> = {
icon: "bx bx-ruler",
type: "checkbox",
bindToLabel: "map:scale"
},
{
label: t("book_properties_config.show-labels"),
icon: "bx bx-label",
type: "checkbox",
bindToLabel: "map:hideLabels",
reverseValue: true
}
]
},
Expand Down
1 change: 1 addition & 0 deletions packages/commons/src/lib/attribute_names.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Labels = {
"calendar:initialDate": string;
"map:style": string;
"map:scale": boolean;
"map:hideLabels": boolean;
"board:groupBy": string;
maxNestingDepth: number;
includeArchived: boolean;
Expand Down