Grid Survey Pattern Generator, Quickly See Heading and RTH Icons along with new Keyboard Navigation (conflict resolution for #2593) - #2698
Open
sensei-hacker wants to merge 17 commits into
Conversation
Add a lawnmower/grid survey pattern generator that allows users to define a polygon area on the map and automatically generate survey waypoints within it. Features: - Grid survey button in Action Menu with polygon drawing interaction - Sidebar settings card with line spacing, altitude, speed, sweep angle, overshoot, and End with RTH checkbox - Live auto-preview: polygon outline, dashed survey path, and numbered waypoint dots update as parameters change - Lawnmower pattern algorithm with configurable sweep angle and overshoot - RTH waypoint automatically appended when End with RTH is checked - Waypoint count display with remaining capacity - Ctrl+G keyboard shortcut to activate grid draw mode - Full i18n support for all UI strings
- Arrow keys (Left/Right) navigate between waypoints with card transition - Delete key removes selected waypoint with confirmation dialog - Auto-select previous waypoint after deletion - Auto-select WP1 after grid survey generation - Show 'Add WP' tooltip with crosshair when hovering flight path lines - Green RTH marker at last waypoint position for all missions - Ctrl+G keyboard shortcut for grid polygon draw
- SET_HEAD waypoints show black circle with white arrow pointing in heading direction - Heading degree label displayed below the marker - Heading marker and RTH marker render above flight path lines (zIndex: 99)
- SET_HEAD shows black circle with white directional dot (pure geometry, no text rotation) - RTH and heading markers aligned with WP pin center via MARKER_ICON_OFFSET_X/Y - Both marker circles same size (radius 10) - Offset constants adjustable: MARKER_ICON_OFFSET_Y=12, MARKER_ICON_OFFSET_X=-2
…issionGridPreview locale key
…ht#2593 # Conflicts: # locale/en/messages.json # tabs/mission_control.js
Contributor
PR Summary by QodoMission Control: grid survey generator, keyboard waypoint nav, heading/RTH markers
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
15 rules 1. Delete confirm not awaited
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+43
to
+44
| title="Search for address (Ctrl+A)" | ||
| aria-label="Search for address (Ctrl+A)"><span class="sr-only">Search for address</span></a> |
Contributor
There was a problem hiding this comment.
1. sr-only text lacks data-i18n 📘 Rule violation ✧ Quality
New accessibility labels in tabs/mission_control.html add hardcoded English text without data-i18n, so they won’t be translated by i18next. This can cause untranslated UI for screen readers and violates the project’s i18n HTML convention.
Agent Prompt
## Issue description
New screen-reader-only labels (and related accessible text) were added as hardcoded English strings without `data-i18n`, so they won’t be translated by i18next.
## Issue Context
In `tabs/mission_control.html`, the new `<span class="sr-only">…</span>` content for the search and center buttons is missing `data-i18n`.
## Fix Focus Areas
- tabs/mission_control.html[39-53]
- locale/en/messages.json[5379-5384]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4557
to
+4560
| e.preventDefault(); | ||
| if (dialog.confirm(i18n.getMessage('confirm_delete_selected_point'))) { | ||
| $('#removePoint').trigger('click'); | ||
| } |
Contributor
There was a problem hiding this comment.
2. Delete confirm not awaited 🐞 Bug ≡ Correctness
handleMissionControlDeleteShortcut() treats dialog.confirm() as a synchronous boolean, but dialog.confirm() returns a Promise, so the condition is always truthy and the selected waypoint can be deleted even if the user cancels.
Agent Prompt
### Issue description
The Delete-key shortcut uses `if (dialog.confirm(...))` even though `dialog.confirm()` returns a Promise, so cancellation is ignored and the waypoint deletion is triggered.
### Issue Context
`dialog.confirm()` is a thin wrapper over `window.electronAPI.confirmDialog(...)` and is used with `await` elsewhere in this same file.
### Fix
Resolve the Promise before acting.
- Option A (keep keydown handler synchronous):
- Replace the `if (dialog.confirm(...))` block with `dialog.confirm(...).then(ok => { if (ok) ... })`.
- Option B: make the keydown path async end-to-end (more invasive).
### Fix Focus Areas
- tabs/mission_control.js[4552-4563]
- js/dialog.js[8-13]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4279
to
+4284
| gridDrawInteraction.on('drawend', function (evt) { | ||
| const polygon = evt.feature.getGeometry(); | ||
| map.removeInteraction(gridDrawInteraction); | ||
| gridDrawInteraction = null; | ||
| hideGridBanner(); | ||
| showGridSettingsDialog(polygon); |
Contributor
There was a problem hiding this comment.
3. Escape handler leaks after draw 🐞 Bug ☼ Reliability
startGridPolygonDraw() registers a keydown.gridDraw Escape handler, but the drawend callback never unregisters it; pressing Escape after finishing the polygon draw can unexpectedly call cancelGridDraw() and remove the preview layer while the settings card stays open.
Agent Prompt
### Issue description
The Escape key handler (`keydown.gridDraw`) is installed when drawing starts, but is only removed by `cancelGridDraw()`. After a successful polygon draw (`drawend`), the handler remains active, so later Escape presses can remove the preview layer and leave the UI in an inconsistent state.
### Issue Context
`drawend` currently removes the Draw interaction and opens the settings card, but does not call `$(document).off('keydown.gridDraw')`.
### Fix
In the `drawend` handler, explicitly unregister `keydown.gridDraw` (or refactor so the handler lifecycle is always cleaned up on both cancel and success).
### Fix Focus Areas
- tabs/mission_control.js[4257-4295]
- tabs/mission_control.js[4297-4308]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4446
to
+4448
| // Clear existing waypoints before generating grid | ||
| removeAllWaypoints(); | ||
|
|
Contributor
There was a problem hiding this comment.
4. Grid bypasses edit lock 🐞 Bug ≡ Correctness
The grid generator can clear and rewrite missions via removeAllWaypoints()/mission.put() even when disableMarkerEdit is true (multi-mission/all-missions view where editing is disabled), violating the tab’s read-only behavior and allowing destructive edits.
Agent Prompt
### Issue description
Grid generation mutates mission state (clears all waypoints and inserts new ones) without checking `disableMarkerEdit`, which is the mechanism used throughout the tab to prevent editing in read-only modes.
### Issue Context
`setMultimissionEditControl(true)` sets `disableMarkerEdit` and disables edit UI; other interactions early-return when `disableMarkerEdit` is true. The grid flow currently does not.
### Fix
Add a guard for read-only mode:
- In `startGridPolygonDraw()` and the Ctrl+G shortcut path: return early if `disableMarkerEdit`.
- In the `#gridGenerate` handler: also guard (so generation cannot happen via any path even if drawing started earlier).
Optionally show a dialog/tooltip explaining that editing is disabled.
### Fix Focus Areas
- tabs/mission_control.js[1659-1669]
- tabs/mission_control.js[4251-4263]
- tabs/mission_control.js[4427-4448]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+152
to
+155
| toGeo(point) { | ||
| return [ | ||
| centroid[0] + point[0] / metersPerDegLon, | ||
| centroid[1] + point[1] / metersPerDegLat, |
Contributor
There was a problem hiding this comment.
5. Extreme-latitude grid instability 🐞 Bug ☼ Reliability
createGridProjection() divides by metersPerDegLon (computed from cos(latitude)); at very high latitudes this becomes extremely small, causing numerically unstable longitude conversions and potentially wildly incorrect generated waypoints.
Agent Prompt
### Issue description
Grid generation uses a simple degrees→meters approximation where `metersPerDegLon = 111320 * cos(lat)` and later divides by it. As latitude approaches ±90°, `cos(lat)` approaches 0, making conversions unstable.
### Issue Context
This affects both preview and generated mission waypoints because `generateGridWaypoints()` relies on `createGridProjection().toGeo()`.
### Fix
Add an explicit safeguard:
- Clamp `metersPerDegLon` to a reasonable minimum epsilon, OR
- Reject/disable grid generation when `|cosLat|` is below a threshold and inform the user.
### Fix Focus Areas
- tabs/mission_control.js[136-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Configurator test build ready — commit Download build artifacts for PR #2698 Available platforms (scroll to the Artifacts section at the bottom of the run page):
|
|
@sensei-hacker — the resolved review-follow-up changes are now in PR #2703 which is the one I can make changed in going forward. If you want screenshots and all like before let me know |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #2593 (GenCodeInc's
feature/grid-pattern— Grid Survey Pattern Generator, heading/RTH icons, keyboard navigation) was retargeted frommaintenance-9.xtomaintenance-10.xand picked up a merge conflict against the new base. Attempting to push the resolution directly to the author's fork via maintainer-edit failed (403 — the available credential doesn't cover write access to that third-party fork), so this PR carries the resolved merge from my own fork instead.This branch is
feature/grid-pattern+ a merge commit resolving conflicts againstmaintenance-10.x. No functional changes beyond what #2593 already contained.Conflict resolution
Two files conflicted:
locale/en/messages.json:maintenance-10.xhad independently reformatted/expanded this file (770+ line diff: many new keys for settings search, migration preview, backup/restore, unrelated to this PR). The PR's actual change here was a clean 42-line addition (two new translation keys:confirm_delete_selected_point, and themissionGridPattern*block). Resolved by takingmaintenance-10.x's version of the file and inserting only those two hunks at the same relative positions, matchingmaintenance-10.x's indentation.tabs/mission_control.js: the PR restructured the#removePointclick handler (early-return guard, extractedprevLayerNum/attachedWaypoints, newremoveAttachedWaypoints()/finalizeWaypointRemoval()helpers). Independently,maintenance-10.xhad migrateddialog.confirm()calls file-wide to beawaited (part of a broader async-dialog migration — confirmed by checking other call sites in the file). Resolved by keeping the PR's restructured control flow and applyingmaintenance-10.x'sasync/awaitpattern to thedialog.confirm()call in that function, consistent with the rest of the file.Testing
node --check tabs/mission_control.js— syntax OK.locale/en/messages.jsonvalidated as well-formed JSON after resolution.maintenance-10.x:mission_control.jsshows exactly 751 insertions / 92 deletions — byte-for-byte identical to PR Grid Survey Pattern Generator, Quickly See Heading and RTH Icons along with new Keyboard Navigation #2593's own diff against the merge-base, confirming no base-branch content was dropped and no PR content was lost.images/icons/cf_icon_MP_grid_grey.svg,src/css/tabs/mission_planer.css,tabs/mission_control.html) againstmaintenance-10.x: also byte-for-byte identical to the PR's own diff.Closes/supersedes #2593.
https://claude.ai/code/session_073a8e72-596e-49b0-8684-e90575ae33fe