Skip to content

Commit a510655

Browse files
committed
Address review feedback
- Compare freehand points with Point.equals() instead of !==. Each mouse event carries a freshly built Point, so the reference check never skipped the duplicate vertex it was meant to guard against. - Make a running selection gesture exclusive: a shared ref suppresses the Identify and geotagged-photo click handlers, so a polygon vertex click no longer also opens a photo popup, and Identify's teardown no longer resets the crosshair the gesture just set. - Only offer the map gestures on a layer that actually renders. The polygon shapes matched against the full feature set while a click matched only rendered features, so a hidden layer behaved differently per shape; the menu items are now disabled and begin() bails, folding group visibility in.
1 parent 29ff788 commit a510655

2 files changed

Lines changed: 37 additions & 9 deletions

File tree

apps/geolibre-desktop/src/components/panels/LayerPanel.tsx

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3036,8 +3036,8 @@ export function LayerPanel({
30363036
// immediate parent, so a hidden grandparent gets the cue too. Given
30373037
// the memoized `groupById` rather than the array, so folding every
30383038
// row does not rebuild that map once per layer.
3039-
const groupHidden =
3040-
layer.visible && !effectiveLayerRenderState(layer, groupById).visible;
3039+
const layerRendered = effectiveLayerRenderState(layer, groupById).visible;
3040+
const groupHidden = layer.visible && !layerRendered;
30413041
const visibilityToggleLabel = groupHidden
30423042
? `${t("layers.hiddenByGroup")}${t("layers.hideLayer")}`
30433043
: layer.visible
@@ -3764,6 +3764,11 @@ export function LayerPanel({
37643764
).map(([shape, Icon, label]) => (
37653765
<DropdownMenuItem
37663766
key={shape}
3767+
// Drawing on the map only makes sense
3768+
// against features the user can see, and
3769+
// a click gesture on a hidden layer would
3770+
// match nothing at all.
3771+
disabled={!layerRendered}
37673772
onSelect={() => {
37683773
if (identifyActive) setIdentifyLayer(null);
37693774
startFeatureSelection({

packages/map/src/MapCanvas.tsx

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {
22
applySelectionMode,
33
applyGroupEffects,
44
createPointerElevationResolver,
5+
effectiveLayerRenderState,
56
getActiveEllipsoid,
67
isDuckDBQueryLayer,
78
NETCDF_IMAGE_SOURCE_KIND,
@@ -1116,6 +1117,10 @@ export const MapCanvas = memo(function MapCanvas({
11161117
const previousDuckDBSelectionLayerId = useRef<string | null>(null);
11171118
const identifyPopup = useRef<maplibregl.Popup | null>(null);
11181119
const photoPopup = useRef<maplibregl.Popup | null>(null);
1120+
// Set for the duration of a map selection gesture. The other click handlers
1121+
// bound to the same map (Identify, geotagged-photo popups) read it and bail,
1122+
// so a rectangle drag or a polygon vertex click never also opens a popup.
1123+
const featureSelectionActive = useRef(false);
11191124

11201125
useEffect(() => {
11211126
if (!containerRef.current || controller.current) return;
@@ -1329,8 +1334,15 @@ export const MapCanvas = memo(function MapCanvas({
13291334
let cancelActive: (() => void) | null = null;
13301335
const begin = (request: FeatureSelectionRequest) => {
13311336
cancelActive?.();
1332-
const layer = useAppStore.getState().layers.find((item) => item.id === request.layerId);
1337+
const state = useAppStore.getState();
1338+
const layer = state.layers.find((item) => item.id === request.layerId);
13331339
if (!layer?.geojson?.features) return;
1340+
// A gesture on a hidden layer would match features the user cannot see —
1341+
// and the `single` shape, which queries rendered features, would match
1342+
// none at all. Folded through the group chain, so a layer hidden only by
1343+
// its group counts as hidden. LayerPanel disables the menu items; this is
1344+
// the guard for a layer hidden between opening the menu and drawing.
1345+
if (!effectiveLayerRenderState(layer, state.layerGroups).visible) return;
13341346

13351347
const canvas = map.getCanvas();
13361348
const container = map.getContainer();
@@ -1361,6 +1373,7 @@ export const MapCanvas = memo(function MapCanvas({
13611373
map.doubleClickZoom.disable();
13621374
}
13631375
canvas.style.cursor = "crosshair";
1376+
featureSelectionActive.current = true;
13641377

13651378
let points: maplibregl.Point[] = [];
13661379
let dragging = false;
@@ -1464,7 +1477,10 @@ export const MapCanvas = memo(function MapCanvas({
14641477
const onMouseUp = (event: maplibregl.MapMouseEvent) => {
14651478
if (!dragging) return;
14661479
dragging = false;
1467-
if (request.shape === "freehand" && points.at(-1) !== event.point) points.push(event.point);
1480+
// `.equals()`, not `!==`: every mouse event carries a freshly built
1481+
// Point, so a reference check would never skip the duplicate.
1482+
if (request.shape === "freehand" && !points.at(-1)?.equals(event.point))
1483+
points.push(event.point);
14681484
else if (request.shape !== "freehand") points = [points[0], event.point];
14691485
finish(event.originalEvent);
14701486
};
@@ -1506,6 +1522,7 @@ export const MapCanvas = memo(function MapCanvas({
15061522
if (boxZoomEnabled) map.boxZoom.enable();
15071523
if (doubleClickZoomEnabled) map.doubleClickZoom.enable();
15081524
canvas.style.cursor = "";
1525+
featureSelectionActive.current = false;
15091526
cancelActive = null;
15101527
};
15111528
};
@@ -1592,6 +1609,8 @@ export const MapCanvas = memo(function MapCanvas({
15921609
let pixelIdentifyAbortController: AbortController | null = null;
15931610

15941611
const handleIdentifyClick = (event: maplibregl.MapMouseEvent) => {
1612+
// A selection gesture owns the map clicks while it runs.
1613+
if (featureSelectionActive.current) return;
15951614
const clearIdentifyResult = () => {
15961615
wmsIdentifyAbortController?.abort();
15971616
wmsIdentifyAbortController = null;
@@ -1757,7 +1776,10 @@ export const MapCanvas = memo(function MapCanvas({
17571776
map.off("click", handleIdentifyClick);
17581777
identifyPopup.current?.remove();
17591778
identifyPopup.current = null;
1760-
map.getCanvas().style.cursor = "";
1779+
// Starting a selection gesture turns Identify off, so this cleanup runs
1780+
// after the gesture has already claimed the crosshair — leave its cursor
1781+
// alone rather than resetting it out from under the drawing.
1782+
if (!featureSelectionActive.current) map.getCanvas().style.cursor = "";
17611783
};
17621784
}, [identifyLayerId, layers, selectFeature]);
17631785

@@ -1777,8 +1799,9 @@ export const MapCanvas = memo(function MapCanvas({
17771799

17781800
const handleClick = (event: maplibregl.MapLayerMouseEvent) => {
17791801
// The Identify tool already renders the photo in its own popup; skip ours
1780-
// so one click never opens two popups.
1781-
if (useAppStore.getState().identifyLayerId) return;
1802+
// so one click never opens two popups. Likewise while a selection gesture
1803+
// is drawing, where a click is a vertex rather than a pick.
1804+
if (useAppStore.getState().identifyLayerId || featureSelectionActive.current) return;
17821805
const feature = event.features?.[0];
17831806
if (!feature) return;
17841807
// Anchor to the feature's own coordinate rather than the click point, so
@@ -1799,11 +1822,11 @@ export const MapCanvas = memo(function MapCanvas({
17991822
.addTo(map);
18001823
};
18011824
const handleEnter = () => {
1802-
if (useAppStore.getState().identifyLayerId) return;
1825+
if (useAppStore.getState().identifyLayerId || featureSelectionActive.current) return;
18031826
map.getCanvas().style.cursor = "pointer";
18041827
};
18051828
const handleLeave = () => {
1806-
if (useAppStore.getState().identifyLayerId) return;
1829+
if (useAppStore.getState().identifyLayerId || featureSelectionActive.current) return;
18071830
map.getCanvas().style.cursor = "";
18081831
};
18091832

0 commit comments

Comments
 (0)