Conversation
|
🚀 Deployed on https://691e7e9b468e48500a7a2680--opengeos.netlify.app |
There was a problem hiding this comment.
Pull Request Overview
This PR adds an optional info box feature to the Geoman control that displays feature properties when users interact with map features. The implementation provides configurable display modes (click or hover) and adjustable hit-testing tolerance.
Key Changes:
- Added three new parameters to
add_geoman_control():show_info_box,info_box_mode, andinfo_box_tolerance - Implemented JavaScript info box UI with automatic feature detection using geometry hit-testing algorithms
- Added runtime control via new
set_geoman_info_box_enabled()method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| anymap/maplibre.py | Added Python API with three new parameters for info box configuration and a runtime control method |
| anymap/static/maplibre_widget.js | Implemented info box UI, early fallback mechanism, geometry hit-testing algorithms, and event handling for click/hover modes |
Comments suppressed due to low confidence (1)
anymap/maplibre.py:2772
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const showEarly = (props) => { | ||
| const box = ensureEarlyBox(); | ||
| if (!box) return; | ||
| const content = box.querySelector('.gm-info-box-content'); | ||
| if (content) { | ||
| content.innerHTML = ''; | ||
| const entries = Object.entries(props || {}); | ||
| if (entries.length === 0) { | ||
| const none = document.createElement('div'); | ||
| none.textContent = 'No properties'; | ||
| content.appendChild(none); | ||
| } else { | ||
| const list = document.createElement('div'); | ||
| entries.slice(0, 16).forEach(([k, v]) => { | ||
| const row = document.createElement('div'); | ||
| const kEl = document.createElement('span'); | ||
| kEl.style.color = '#555'; | ||
| kEl.textContent = `${k}: `; | ||
| const vEl = document.createElement('span'); | ||
| vEl.style.color = '#111'; | ||
| try { vEl.textContent = typeof v === 'object' ? JSON.stringify(v) : String(v); } | ||
| catch (_e) { vEl.textContent = String(v); } | ||
| row.appendChild(kEl); row.appendChild(vEl); list.appendChild(row); | ||
| }); | ||
| if (entries.length > 16) { | ||
| const more = document.createElement('div'); | ||
| more.style.color = '#777'; | ||
| more.style.marginTop = '4px'; | ||
| more.textContent = `(+${entries.length - 16} more)`; | ||
| list.appendChild(more); | ||
| } | ||
| content.appendChild(list); | ||
| } | ||
| } | ||
| box.style.display = 'block'; | ||
| }; |
There was a problem hiding this comment.
The property rendering logic is duplicated between the early fallback (lines 4063-4098) and the main implementation (lines 4261-4301). Both blocks iterate over properties, create the same DOM elements, and apply identical truncation logic (16 properties max). This duplication makes maintenance error-prone. Consider extracting this into a reusable function.
| const earlyLookup = (lngLat) => { | ||
| try { | ||
| const data = model.get('geoman_data') || { type: 'FeatureCollection', features: [] }; | ||
| const features = Array.isArray(data.features) ? data.features : []; | ||
| if (!features.length) return null; | ||
| const pt = map.project([lngLat.lng, lngLat.lat]); | ||
| const tol = Number.isFinite(el._gmInfoTolerance) ? Math.max(1, Number(el._gmInfoTolerance)) : 8; | ||
| const threshold2 = tol * tol; | ||
| let best = null; | ||
| let bestScore = Infinity; | ||
| for (const f of features) { | ||
| if (!f || !f.geometry) continue; | ||
| const g = f.geometry; | ||
| if (g.type === 'Point') { | ||
| const p = map.project(g.coordinates); | ||
| const d2 = dist2(pt, p); | ||
| if (d2 <= threshold2 && d2 < bestScore) { best = f.properties || {}; bestScore = d2; } | ||
| } else if (g.type === 'MultiPoint') { | ||
| for (const c of g.coordinates || []) { | ||
| const p = map.project(c); | ||
| const d2 = dist2(pt, p); | ||
| if (d2 <= threshold2 && d2 < bestScore) { best = f.properties || {}; bestScore = d2; } | ||
| } | ||
| } else if (g.type === 'LineString') { | ||
| const coords = g.coordinates || []; | ||
| for (let i = 1; i < coords.length; i++) { | ||
| const a = map.project(coords[i - 1]), b = map.project(coords[i]); | ||
| const d2 = distToSegment2(pt, a, b); | ||
| if (d2 <= threshold2 && d2 < bestScore) { best = f.properties || {}; bestScore = d2; } | ||
| } | ||
| } else if (g.type === 'MultiLineString') { | ||
| for (const line of g.coordinates || []) { | ||
| for (let i = 1; i < line.length; i++) { | ||
| const a = map.project(line[i - 1]), b = map.project(line[i]); | ||
| const d2 = distToSegment2(pt, a, b); | ||
| if (d2 <= threshold2 && d2 < bestScore) { best = f.properties || {}; bestScore = d2; } | ||
| } | ||
| } | ||
| } else if (g.type === 'Polygon') { | ||
| if (pointInPolygon([lngLat.lng, lngLat.lat], g.coordinates)) return f.properties || {}; | ||
| } else if (g.type === 'MultiPolygon') { | ||
| for (const poly of g.coordinates || []) { | ||
| if (pointInPolygon([lngLat.lng, lngLat.lat], poly)) return f.properties || {}; | ||
| } | ||
| } | ||
| } | ||
| return best; | ||
| } catch (_e) { return null; } |
There was a problem hiding this comment.
The feature hit-testing logic in earlyLookup (lines 4141-4188) is nearly identical to the logic in updateInfoFromEventPoint (lines 4508-4599). Both implement the same geometry type checks (Point, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon) with identical distance calculations and thresholds. This substantial duplication (~50 lines) makes the code harder to maintain. Consider extracting this into a shared function that takes the click point, features array, and tolerance as parameters.
| const features = Array.isArray(data.features) ? data.features : []; | ||
| if (!features.length) return null; | ||
| const pt = map.project([lngLat.lng, lngLat.lat]); | ||
| const tol = Number.isFinite(el._gmInfoTolerance) ? Math.max(1, Number(el._gmInfoTolerance)) : 8; |
There was a problem hiding this comment.
Magic number tolerance values (8 for default, 6 for hover) are hardcoded here and again at line 4514. Consider defining these as named constants (e.g., DEFAULT_CLICK_TOLERANCE = 8, DEFAULT_HOVER_TOLERANCE = 6) to make the code more maintainable and ensure consistency.
|
|
||
| // Fallback: if Geoman doesn't emit gm:loaded promptly, attach info box handlers early | ||
| try { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Magic number 400ms timeout is used without explanation. Consider defining this as a named constant (e.g., GEOMAN_LOADED_FALLBACK_DELAY = 400) to make the code more self-documenting and easier to tune if needed.
| } catch (_e3) {} | ||
| } | ||
| // Auto clear after a short delay | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Magic number 700ms is used for the flash highlight duration without explanation. Consider defining this as a named constant (e.g., FLASH_HIGHLIGHT_DURATION_MS = 700) to make the code more self-documenting and easier to adjust.
| for (const f of features) { | ||
| if (!f || !f.geometry) continue; | ||
| const g = f.geometry; | ||
| if (g.type === 'Point') { | ||
| const p = map.project(g.coordinates); | ||
| const d2 = dist2(clickPoint, p); | ||
| if (d2 <= threshold2 && d2 < bestScore) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = d2; | ||
| } | ||
| } else if (g.type === 'MultiPoint') { | ||
| for (const c of g.coordinates || []) { | ||
| const p = map.project(c); | ||
| const d2 = dist2(clickPoint, p); | ||
| if (d2 <= threshold2 && d2 < bestScore) { | ||
| best = f.properties || {}; | ||
| bestGeom = { type: 'Point', coordinates: c }; | ||
| bestScore = d2; | ||
| } | ||
| } | ||
| } else if (g.type === 'LineString') { | ||
| const coords = g.coordinates || []; | ||
| for (let i = 1; i < coords.length; i++) { | ||
| const a = map.project(coords[i - 1]); | ||
| const b = map.project(coords[i]); | ||
| const d2 = distToSegment2(clickPoint, a, b); | ||
| if (d2 <= threshold2 && d2 < bestScore) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = d2; | ||
| } | ||
| } | ||
| } else if (g.type === 'MultiLineString') { | ||
| for (const line of g.coordinates || []) { | ||
| for (let i = 1; i < line.length; i++) { | ||
| const a = map.project(line[i - 1]); | ||
| const b = map.project(line[i]); | ||
| const d2 = distToSegment2(clickPoint, a, b); | ||
| if (d2 <= threshold2 && d2 < bestScore) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = d2; | ||
| } | ||
| } | ||
| } | ||
| } else if (g.type === 'Polygon') { | ||
| const pt = [ev.lngLat.lng, ev.lngLat.lat]; | ||
| if (pointInPolygon(pt, g.coordinates)) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = 0; | ||
| break; | ||
| } | ||
| } else if (g.type === 'MultiPolygon') { | ||
| const pt = [ev.lngLat.lng, ev.lngLat.lat]; | ||
| let hit = false; | ||
| for (const poly of g.coordinates || []) { | ||
| if (pointInPolygon(pt, poly)) { hit = true; break; } | ||
| } | ||
| if (hit) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = 0; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The feature hit-testing algorithm iterates through all features in the collection without early termination for most geometry types. For large feature collections, this could cause performance issues on every click/hover event. Consider adding spatial indexing or at least terminating the search after finding a hit for Points and LineStrings (similar to how Polygons use break at line 4571).
| if (pointInPolygon(pt, g.coordinates)) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = 0; |
There was a problem hiding this comment.
The value assigned to bestScore here is unused.
| if (hit) { | ||
| best = f.properties || {}; | ||
| bestGeom = g; | ||
| bestScore = 0; |
There was a problem hiding this comment.
The value assigned to bestScore here is unused.
| if (best) { | ||
| showInfoBoxWithProps(best); | ||
| // Flash highlight only on explicit clicks (force === true) | ||
| if (force && bestGeom) { |
There was a problem hiding this comment.
This use of variable 'force' always evaluates to true.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.