Skip to content

FIX: POI: load the saved list once and skip duplicate entries - #3799

Open
Autonomost wants to merge 6 commits into
ArduPilot:masterfrom
Autonomost:feature/poi-load-once
Open

Autonomost wants to merge 6 commits into
ArduPilot:masterfrom
Autonomost:feature/poi-load-once

Conversation

@Autonomost

@Autonomost Autonomost commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Every page that subscribes to POI.POIModified (FlightData and FlightPlanner) triggered a load of poi.txt from the event's add accessor, so each start doubled the in-memory list, and the next save wrote the doubled list back. After a few sessions the map shows stacks of identical markers; one affected poi.txt had 1280 lines for 16 distinct points.

The saved file is now read on the first subscription only, and a page that subscribes later draws the list already held. LoadFile skips an entry whose position and name are already held, so loading a file twice, or a file that already contains duplicates, cannot pile markers up. When the saved file held duplicates it is rewritten clean at startup, which repairs existing installs on their next start. Malformed lines are ignored instead of throwing out of the loader.

The second commit addresses review:

  • A failed read (the file held open by a virus scanner, a sync client or another instance) is no longer treated as loaded. It is retried on the next subscription, and the list is not written to poi.txt until the file has been read, so a failed read can no longer end with the next add, edit or delete saving a list without the saved POIs over the file.
  • poi.txt is opened read only and shared. File.Open(path, FileMode.Open) asks for read/write access with no sharing, which fails whenever another program has the file open.
  • The duplicate repair only rewrites poi.txt at startup. Importing a file with Load file writes neither poi.txt nor the imported file.
  • Duplicates are matched on the position as the file stores it (15 significant digits), so a point placed on the map matches the same point read back from a saved file.
  • With the file loaded once, the page that subscribed second (normally Plan, which subscribes when first opened) was not drawn until a POI changed. It now draws when it subscribes.

The third commit logs a failed startup read and the two failure paths after it, uses items.Length, and gives SaveFile and the duplicate key one shared CoordText helper so the key is always exactly the text the file holds.

The fourth commit (after the merge of master) stops relying on File.Exists to decide that there is no saved file: File.Exists is also false for a file the process cannot read, which would have marked an inaccessible poi.txt as loaded and let the next edit write over it. Only "not found" now counts as no file; any other read failure is logged and retried on the next subscription, with the file left unwritten until then.

The first commit was verified on a real 1280-line poi.txt: the first start rewrote it to 16 lines and the markers are no longer stacked. The review changes are covered by unit tests in our fork (locked file read again on the next subscription, failed read not overwritten, file open in another program, unreadable file not overwritten, import leaves both files alone, re-import after placing a point, later page drawn). They need InternalsVisibleTo on the main assembly, so they are not included here.

Every page that subscribes to POI.POIModified (FlightData and
FlightPlanner) triggered a load of poi.txt from the event's add
accessor, so each start doubled the in-memory list, and the next save
wrote the doubled list back. After a few sessions the map shows stacks
of identical markers; one affected poi.txt had 1280 lines for 16
distinct points.

Load the file on the first subscription only. LoadFile now skips an
entry whose position and name are already held, so loading a file twice
or a file that already contains duplicates cannot pile markers up, and
when duplicates were skipped the saved file is rewritten clean, which
repairs existing installs on their next start. Malformed lines are
ignored instead of throwing out of the loader.

Verified on a real 1280-line poi.txt: the first start rewrote it to 16
lines and the markers are no longer stacked.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Address import-file rewriting and POI loading/subscriber initialization issues.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Updates POI loading to prevent duplicate markers and clean malformed or repeated entries.

Changes:

  • Loads the saved POI file once.
  • Deduplicates entries and ignores malformed records.
  • Rewrites cleaned POI data.
File Summary
Utilities/​POI.cs Updates POI loading, deduplication, and persistence behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Utilities/POI.cs Outdated
Comment thread Utilities/POI.cs Outdated
…ry page

Follow-up to loading the saved list once:

- The saved file was marked loaded before it was read, so a failed read
  (the file held open by a virus scanner, a sync client or another
  instance) was never retried, and the next add, edit or delete saved the
  list without its POIs over the file. It is now marked loaded only after
  a successful read or when there is no file, a failed read is retried on
  the next subscription, and the list is not written to the file until
  the file has been read.
- The file is opened read only and shared. File.Open(path, FileMode.Open)
  asks for read/write access with no sharing, which fails whenever another
  program has the file open.
- The duplicate repair ran inside LoadFile, so importing a file with Load
  file saved the merged list to poi.txt whenever the import contained a
  duplicate. The repair now only runs for the saved file.
- Duplicates are matched on the position as the file stores it (15
  significant digits), so a point placed on the map matches the same
  point read back from a saved file.
- Pages draw POIs only when POIModified fires, and with the file loaded
  once only the first page to subscribe was redrawn; the Plan page showed
  no POIs until one changed. A page subscribing later now draws the list.
@meee1
meee1 requested a lite review from Copilot September 30, 2026 21:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 High severity · 3 Medium severity

Open (4)
Resolved since last review (2)

Comment thread Utilities/POI.cs
Comment thread Utilities/POI.cs Outdated
Comment thread Utilities/POI.cs Outdated
Comment thread Utilities/POI.cs Outdated
…d the duplicate key

Review follow-ups:

- A saved file that cannot be read at startup is now logged (with the exception) instead of
  failing silently, and so is a page handler or the duplicate repair throwing during the first
  load. The retry on the next subscription is unchanged.
- SaveFile and the duplicate key both call CoordText for the latitude and longitude, so the
  key is exactly the text the file holds on whichever runtime formats the doubles, and the
  two cannot drift apart. The comment no longer claims a fixed 15 digits; that is only what
  .NET Framework's default format gives.
- items.Length instead of items.Count() on the split line.
@Autonomost

Copy link
Copy Markdown
Contributor Author

@meee1 feel free to give it another review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread Utilities/POI.cs
File.Exists returns false both for a missing file and for one the process
cannot read, so an inaccessible poi.txt was marked loaded and the next add,
edit or delete would have written the list over it. LoadSaved now opens the
file and treats only FileNotFoundException and DirectoryNotFoundException as
"no saved file"; any other failure is logged and retried on the next
subscription, with the file left unwritten until then, as for a locked file.
Picks up a489d27, which fixes the Mavlink.cs syntax errors that have
failed every CI run of this PR since 2026-09-30, and the commits since.
No conflicts; the PR's own changes are untouched.
@Autonomost

Copy link
Copy Markdown
Contributor Author

Merged upstream master (694628a) into the branch as 4580a2e. It brings a489d27, which fixes the Mavlink.cs syntax errors behind every red CI run here since 2026-09-30 (the last run on a2c8df1 had 890 of them, all in Mavlink.cs, none in POI.cs), plus the commits since. No conflicts; the PR's own change is untouched, and the branch builds on Windows (MissionPlanner.csproj, Release).

@Autonomost

Copy link
Copy Markdown
Contributor Author

@meee1 this should be good to go.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Ensure redraw still occurs when duplicate-file repair fails.

No open issues 🎉

1 resolved since last review

Review effort: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants