batch of fixes

gpx-rs/src/core/algorithm/simplify.rs

@@ -44,14 +44,24 @@ where
             let last = trkseg.last_index().unwrap();
 
             let mut kept = vec![(first, None)];
-            ramer_douglas_peucker_helper(trkseg, first, last, measure, epsilon, &mut kept);
+            ramer_douglas_peucker_ranges(trkseg, first, last, measure, epsilon, &mut kept);
             kept.push((last, None));
             kept
         }
     }
 }
 
-fn ramer_douglas_peucker_helper<M>(
+/// What is left to do for the points between two ends, in the order of the points.
+enum Step {
+    /// Looks for the point to keep between these two.
+    Range(TrackSegmentIndex, TrackSegmentIndex),
+    /// Keeps a point, selected at this distance.
+    Keep(TrackSegmentIndex, f64),
+}
+
+/// Adds the points to keep between `start` and `end` to `kept`, in order. It does not recurse: a
+/// track can make it go as deep as it has points, which would overflow the stack.
+fn ramer_douglas_peucker_ranges<M>(
     trkseg: &TrackSegment,
     start: TrackSegmentIndex,
     end: TrackSegmentIndex,
@@ -61,28 +71,40 @@ fn ramer_douglas_peucker_helper<M>(
 ) where
     M: Fn(TrackSegmentIndex, TrackSegmentIndex, TrackSegmentIndex) -> f64,
 {
-    let mut max_idx = None;
-    let mut max_dist = 0.0;
+    let mut steps = vec![Step::Range(start, end)];
+    while let Some(step) = steps.pop() {
+        let (start, end) = match step {
+            Step::Keep(idx, dist) => {
+                kept.push((idx, Some(dist)));
+                continue;
+            }
+            Step::Range(start, end) => (start, end),
+        };
 
-    let mut cur = trkseg.next_index(Some(start));
-    while let Some(idx) = cur {
-        if idx == end {
-            break;
+        let mut max_idx = None;
+        let mut max_dist = 0.0;
+
+        let mut cur = trkseg.next_index(Some(start));
+        while let Some(idx) = cur {
+            if idx == end {
+                break;
+            }
+
+            let dist = measure(start, end, idx);
+            if dist > max_dist {
+                max_idx = Some(idx);
+                max_dist = dist;
+            }
+
+            cur = trkseg.next_index(cur);
         }
 
-        let dist = measure(start, end, idx);
-        if dist > max_dist {
-            max_idx = Some(idx);
-            max_dist = dist;
+        if let Some(idx) = max_idx.filter(|_| max_dist > epsilon) {
+            // the last one pushed is done first
+            steps.push(Step::Range(idx, end));
+            steps.push(Step::Keep(idx, max_dist));
+            steps.push(Step::Range(start, idx));
         }
-
-        cur = trkseg.next_index(cur);
-    }
-
-    if let Some(idx) = max_idx.filter(|_| max_dist > epsilon) {
-        ramer_douglas_peucker_helper(trkseg, start, idx, measure, epsilon, kept);
-        kept.push((idx, Some(max_dist)));
-        ramer_douglas_peucker_helper(trkseg, idx, end, measure, epsilon, kept);
     }
 }
 
@@ -195,6 +217,16 @@ mod tests {
             .collect()
     }
 
+    #[test]
+    fn test_goes_as_deep_as_the_segment_is_long_in_order() {
+        let n = 12_000;
+        let trkseg = segment(&vec![0.0; n]);
+        // the point before the end is always the farthest one, so each range only loses one point
+        let kept = ramer_douglas_peucker_by(&trkseg, &|_, _, idx| idx.flat as f64 + 1.0, 0.5);
+        let kept: Vec<usize> = kept.iter().map(|(idx, _)| idx.flat).collect();
+        assert_eq!(kept, (0..n).collect::<Vec<_>>());
+    }
+
     #[test]
     fn test_empty_and_single_point() {
         assert!(simplify(&[], 1.0).is_empty());

gpx-rs/src/core/io/parse.rs

@@ -6,7 +6,7 @@ use chrono::DateTime;
 use quick_xml::Error;
 use quick_xml::escape::resolve_predefined_entity;
 use quick_xml::events::Event;
-use quick_xml::events::attributes::Attributes;
+use quick_xml::events::attributes::{Attribute, Attributes};
 use quick_xml::reader::Reader;
 
 enum GPXElement {
@@ -260,6 +260,13 @@ fn apply_text(
     }
 }
 
+/// The text of an attribute, without its escaping (`&` is `&`), which is written back when the
+/// file is. As it is in the file if it cannot be read.
+fn attribute_text(attr: &Attribute<'_>) -> String {
+    attr.unescape_value()
+        .map_or_else(|_| attr.value.to_string(), |text| text.into_owned())
+}
+
 /// The email of the author, given as the `id` and `domain` attributes of the element.
 fn set_email(stack: &mut [GPXElement], attributes: Attributes<'_>) {
     let Some(GPXElement::Author(author)) = stack.last_mut() else {
@@ -268,8 +275,8 @@ fn set_email(stack: &mut [GPXElement], attributes: Attributes<'_>) {
     let (mut id, mut domain) = (None, None);
     for attr in attributes.flatten() {
         match attr.key.as_ref() {
-            "id" => id = Some(attr.value.to_string()),
-            "domain" => domain = Some(attr.value.to_string()),
+            "id" => id = Some(attribute_text(&attr)),
+            "domain" => domain = Some(attribute_text(&attr)),
             _ => (),
         }
     }
@@ -283,7 +290,7 @@ fn parse_link(attributes: Attributes<'_>) -> Link {
     let mut link = Link::default();
     for attr in attributes.flatten() {
         if attr.key.as_ref() == "href" {
-            link.href = attr.value.to_string();
+            link.href = attribute_text(&attr);
         }
     }
     link
@@ -626,6 +633,14 @@ mod tests {
         .unwrap()
     }
 
+    #[test]
+    fn test_link_href_is_unescaped_and_written_back_once() {
+        let data =
+            r#"<gpx><metadata><link href="https://a.example/?x=1&amp;y=2"/></metadata></gpx>"#;
+        let gpx = parse(data.as_bytes(), &mut Default::default()).unwrap();
+        assert_eq!(gpx.info.links[0].href, "https://a.example/?x=1&y=2");
+    }
+
     #[test]
     fn test_parse_entities_and_cdata_in_text() {
         let data = r#"<gpx><metadata><name>Tom &amp; Jerry &lt;3 &#233;&#x41; &quot;x&quot; &apos;y&apos;</name>

wasm/src/persistence.rs

@@ -26,6 +26,9 @@ thread_local! {
     static PERSISTER: RefCell<Option<Persister>> = const { RefCell::new(None) };
     static SAVING: Cell<bool> = const { Cell::new(false) };
     static DIRTY: Cell<bool> = const { Cell::new(false) };
+    /// Whether the last write failed, or was not done because a newer version wrote the storage:
+    /// what the engine holds is then not all stored.
+    static NOT_WRITTEN: Cell<bool> = const { Cell::new(false) };
     /// Resolve functions of the promises of `flush_storage`.
     /// Called after the files were written, and when a write was refused because they were
     /// written by someone else since: see `open_storage`.
@@ -73,20 +76,28 @@ pub(crate) fn schedule_save() {
             let result = persister.save(&*storage, snapshot).await;
             PERSISTER.with(|slot| *slot.borrow_mut() = Some(persister));
             match result {
-                Ok(()) => saved = true,
+                Ok(()) => {
+                    NOT_WRITTEN.set(false);
+                    saved = true;
+                }
                 // nothing was written, so that the files of the other tab are not damaged: this
                 // tab has to read them, which replaces what it did
                 Err(engine::CommitError::Stale { .. }) => {
+                    NOT_WRITTEN.set(false);
                     DIRTY.set(false);
                     behind = true;
                 }
                 // nothing is written, so that the newer version finds its data again
                 Err(engine::CommitError::Failed(engine::StorageError::Newer)) => {
+                    NOT_WRITTEN.set(true);
                     DIRTY.set(false);
                     newer = true;
                 }
                 // what failed is written with the next change
-                Err(error) => console_error(&error.to_string()),
+                Err(error) => {
+                    NOT_WRITTEN.set(true);
+                    console_error(&error.to_string());
+                }
             }
         }
         SAVING.set(false);
@@ -188,17 +199,18 @@ pub(crate) fn to_js_error(error: engine::StorageError) -> JsValue {
     JsValue::from_str(&error.to_string())
 }
 
-/// Resolves when what was changed has been written to the storage.
+/// Resolves when the writes of what was changed are done, to whether they went well. A write that
+/// was refused because another tab wrote since is fine: this tab then reads what that tab wrote.
 #[wasm_bindgen]
-pub async fn flush_storage() -> Result<(), JsValue> {
+pub async fn flush_storage() -> Result<bool, JsValue> {
     if !SAVING.get() {
-        return Ok(());
+        return Ok(!NOT_WRITTEN.get());
     }
     let promise = js_sys::Promise::new(&mut |resolve, _| {
         WAITERS.with(|waiters| waiters.borrow_mut().push(resolve));
     });
     wasm_bindgen_futures::JsFuture::from(promise).await?;
-    Ok(())
+    Ok(!NOT_WRITTEN.get())
 }
 
 /// If another tab wrote the files since this one last did, reads them again and overwrites the

website/src/lib/engine/backend/tauri.ts

@@ -245,7 +245,7 @@ export async function createTauriBackend(): Promise<Backend> {
 
         importsLegacyData: false,
         open_storage: () => invoke<StorageOpened>('open_storage'),
-        flush_storage: () => invoke('flush_storage'),
+        flush_storage: () => invoke('flush_storage').then(() => true),
         sync_storage: () => invoke('sync_storage'),
         set_setting: (key, value) => invoke('set_setting', { key, value }),
         delete_setting: (key) => invoke('delete_setting', { key }),

website/src/lib/engine/backend/types.ts

@@ -340,8 +340,8 @@ export interface Backend {
         onBehind: () => void,
         onNewer: () => void
     ): Promise<StorageOpened>;
-    /** Resolves when what was changed has been written to the storage. */
-    flush_storage(): Promise<void>;
+    /** Resolves when the writes of what was changed are done, to whether they went well. */
+    flush_storage(): Promise<boolean>;
     /** Reads the files again if another window wrote them since, and tells what changed. */
     sync_storage(): Promise<Outcome>;
     set_setting(key: string, json: string): Promise<void>;

website/src/lib/engine/storage.test.ts

@@ -0,0 +1,120 @@
+import assert from 'node:assert/strict';
+import { describe, it } from 'node:test';
+import { LEGACY_IMPORTED_KEY, type LegacyData } from '$lib/storage/legacy-import';
+import { openStoredFiles, type StorageHost } from '$lib/engine/storage';
+
+// a channel would keep the tests running: there are no other tabs here
+Object.defineProperty(globalThis, 'BroadcastChannel', { value: undefined, configurable: true });
+
+const legacy: LegacyData = {
+    files: [{ gpx: '<gpx/>', name: 'a' }],
+    settings: { distanceUnits: '"imperial"', routing: 'false' },
+};
+
+type Options = {
+    stored?: Record<string, string>;
+    storedFiles?: number;
+    loads?: boolean;
+    writes?: boolean;
+};
+
+/** A host and a backend that note what is done, in order. */
+function setup({ stored = {}, storedFiles = 0, loads = true, writes = true }: Options = {}) {
+    const calls: string[] = [];
+    const settings: Record<string, string> = { ...stored };
+    let files = storedFiles;
+    const host = {
+        backend: {
+            importsLegacyData: true,
+            open_storage: async () => ({ settings: { ...stored }, outcome: {} }),
+            flush_storage: async () => {
+                calls.push('flush');
+                return writes;
+            },
+            set_setting: async (key: string, value: string) => {
+                calls.push(key);
+                settings[key] = value;
+            },
+        },
+        visibility: new Map(),
+        // like the engine, what was stored is only known once this is done
+        sync: async () => {
+            await Promise.resolve();
+            calls.push('sync');
+        },
+        outdated() {},
+        hasFiles: () => calls.includes('sync') && files > 0,
+        loadFiles: async () => {
+            calls.push('load');
+            if (loads) {
+                files += 1;
+            }
+            return loads;
+        },
+        deselect: async () => true,
+    } as unknown as StorageHost;
+    return { host, calls, settings };
+}
+
+describe('importing the former database', () => {
+    it('counts as done once the files are written', async () => {
+        const { host, calls, settings } = setup();
+        const opened = await openStoredFiles(host, async () => legacy);
+        assert.deepEqual(calls, [
+            'sync',
+            'distanceUnits',
+            'routing',
+            'load',
+            'flush',
+            LEGACY_IMPORTED_KEY,
+        ]);
+        assert.equal(settings[LEGACY_IMPORTED_KEY], 'true');
+        assert.equal(opened.distanceUnits, '"imperial"');
+    });
+
+    it('is tried again when the files cannot be loaded', async () => {
+        const { host, settings } = setup({ loads: false });
+        const opened = await openStoredFiles(host, async () => legacy);
+        assert.equal(settings[LEGACY_IMPORTED_KEY], undefined);
+        assert.equal(opened.distanceUnits, '"imperial"');
+    });
+
+    it('is tried again when the files cannot be written', async () => {
+        const { host, settings } = setup({ writes: false });
+        await openStoredFiles(host, async () => legacy);
+        assert.equal(settings[LEGACY_IMPORTED_KEY], undefined);
+    });
+
+    it('is tried again when the database cannot be read', async () => {
+        const { host, settings } = setup();
+        await openStoredFiles(host, async () => {
+            throw new Error('cannot read');
+        });
+        assert.equal(settings[LEGACY_IMPORTED_KEY], undefined);
+    });
+
+    it('keeps the settings that were changed since a previous try', async () => {
+        const { host, settings } = setup({ stored: { routing: 'true' } });
+        const opened = await openStoredFiles(host, async () => legacy);
+        assert.equal(settings.routing, 'true');
+        assert.equal(opened.routing, 'true');
+        assert.equal(settings.distanceUnits, '"imperial"');
+    });
+
+    it('does not load the files twice: the stored ones mean it was done', async () => {
+        const { host, calls, settings } = setup({ storedFiles: 3 });
+        await openStoredFiles(host, async () => legacy);
+        assert.ok(!calls.includes('load'));
+        assert.equal(settings[LEGACY_IMPORTED_KEY], 'true');
+    });
+
+    it('is done when there is nothing to import, and only once', async () => {
+        const nothing = setup();
+        await openStoredFiles(nothing.host, async () => undefined);
+        assert.equal(nothing.settings[LEGACY_IMPORTED_KEY], 'true');
+
+        const done = setup({ stored: { [LEGACY_IMPORTED_KEY]: 'true' } });
+        await openStoredFiles(done.host, async () => legacy);
+        assert.ok(!done.calls.includes('load'));
+    });
+});

website/src/lib/engine/storage.ts

@@ -1,4 +1,4 @@
-import { LEGACY_IMPORTED_KEY, readLegacyData } from '$lib/storage/legacy-import';
+import { LEGACY_IMPORTED_KEY, readLegacyData, type LegacyData } from '$lib/storage/legacy-import';
 import { parseVisibility, VISIBILITY_KEY, type Visibility } from '$lib/engine/file-visibility';
 import type { Outcome, StorageOpened } from 'gpx-wasm';
 import type { Backend } from './backend';
@@ -15,7 +15,7 @@ export type StorageHost = {
     /** The explicit visibility per file, to fill with the one that was saved. */
     visibility: Map<string, Visibility>;
     /** Applies what a call changed to what the facade keeps of the files. */
-    sync(update: Outcome): void;
+    sync(update: Outcome): Promise<void>;
     /** Tells that what is stored comes from a newer version of the app: nothing can be saved. */
     outdated(): void;
     hasFiles(): boolean;
@@ -66,7 +66,10 @@ function followOtherTabs(host: StorageHost): {
  *
  * The first time, what the previous versions of the app kept in the browser is imported.
  */
-export async function openStoredFiles(host: StorageHost): Promise<Record<string, string>> {
+export async function openStoredFiles(
+    host: StorageHost,
+    readLegacy: () => Promise<LegacyData | undefined> = readLegacyData
+): Promise<Record<string, string>> {
     const { backend } = host;
     let opened: StorageOpened;
     const tabs = followOtherTabs(host);
@@ -81,47 +84,68 @@ export async function openStoredFiles(host: StorageHost): Promise<Record<string,
     parseVisibility(settings[VISIBILITY_KEY]).forEach((visibility, fileId) =>
         host.visibility.set(fileId, visibility)
     );
-    host.sync(opened.outcome);
+    await host.sync(opened.outcome);
 
-    if (
-        backend.importsLegacyData &&
-        settings[LEGACY_IMPORTED_KEY] === undefined &&
-        !host.hasFiles()
-    ) {
-        settings = { ...settings, ...(await importLegacyData(host)) };
-        await backend.flush_storage();
+    if (backend.importsLegacyData && settings[LEGACY_IMPORTED_KEY] === undefined) {
+        settings = { ...settings, ...(await importLegacyData(host, settings, readLegacy)) };
     }
     return settings;
 }
 
-/** Puts the files and the settings of the former database in the engine. */
-async function importLegacyData(host: StorageHost): Promise<Record<string, string>> {
+/**
+ * Puts the files and the settings of the former database in the engine. It only counts as done once
+ * the files are written: until then, it is tried again the next time. The files that are stored
+ * mean that it was done, by a visit that did not get to say so.
+ */
+async function importLegacyData(
+    host: StorageHost,
+    stored: Record<string, string>,
+    readLegacy: () => Promise<LegacyData | undefined>
+): Promise<Record<string, string>> {
     const { backend } = host;
     const imported: Record<string, string> = {};
-    try {
-        const legacy = await readLegacyData();
-        if (legacy) {
-            const encoder = new TextEncoder();
-            if (legacy.files.length > 0) {
-                await host.loadFiles(
-                    legacy.files.map((file) => ({
-                        data: encoder.encode(file.gpx),
-                        name: file.name,
-                    }))
-                );
-                await host.deselect();
-            }
-            for (const [key, value] of Object.entries(legacy.settings)) {
-                await backend.set_setting(key, value);
-                imported[key] = value;
+    if (!host.hasFiles()) {
+        try {
+            const legacy = await readLegacy();
+            if (legacy) {
+                // what was changed since a previous try is more recent
+                for (const [key, value] of Object.entries(legacy.settings)) {
+                    if (stored[key] === undefined) {
+                        await backend.set_setting(key, value);
+                        imported[key] = value;
+                    }
+                }
+                if (legacy.files.length > 0 && !(await importFiles(host, legacy))) {
+                    return imported;
+                }
             }
+        } catch (error) {
+            console.error('The files of the previous version could not be imported', error);
+            return imported;
         }
-    } catch (error) {
-        // it is tried again next time
-        console.error('The files of the previous version could not be imported', error);
-        return imported;
     }
     await backend.set_setting(LEGACY_IMPORTED_KEY, 'true');
     imported[LEGACY_IMPORTED_KEY] = 'true';
     return imported;
 }
+
+/** Loads the files of the former database, and tells whether they are written. */
+async function importFiles(host: StorageHost, legacy: LegacyData): Promise<boolean> {
+    const encoder = new TextEncoder();
+    const loaded = await host.loadFiles(
+        legacy.files.map((file) => ({
+            data: encoder.encode(file.gpx),
+            name: file.name,
+        }))
+    );
+    if (!loaded) {
+        console.error('The files of the previous version could not be loaded');
+        return false;
+    }
+    await host.deselect();
+    if (!(await host.backend.flush_storage())) {
+        console.error('The files of the previous version could not be written');
+        return false;
+    }
+    return true;
+}

website/src/lib/storage/legacy-import.test.ts

@@ -95,6 +95,21 @@ describe('legacyFileToGPX', () => {
         assert.ok(gpx.includes('href="https://example.com/?a="1"&b=2"'));
     });
 
+    it('writes all the links of an element', () => {
+        const links = [
+            { attributes: { href: 'https://a.example/' }, text: 'first' },
+            { attributes: { href: 'https://b.example/' } },
+        ];
+        const gpx = legacyFileToGPX({
+            metadata: { name: 'x', link: links },
+            trk: [{ link: links, trkseg: [] }],
+        });
+        const written =
+            '<link href="https://a.example/"><text>first</text></link><link href="https://b.example/"></link>';
+        assert.ok(gpx.includes(`<name>x</name>${written}</metadata>`));
+        assert.ok(gpx.includes(`<trk>${written}</trk>`));
+    });
+
     it('leaves out what is missing or empty', () => {
         const gpx = legacyFileToGPX({
             metadata: { name: '', desc: null, time: 'not a date' },

website/src/lib/storage/legacy-import.ts

@@ -84,7 +84,11 @@ function timeElement(value: unknown): string {
     return isNaN(date.getTime()) ? '' : element('time', date.toISOString());
 }
 
+/** The link of an element, or the links: the former parser gave a list when there were several. */
 function link(value: Json): string {
+    if (Array.isArray(value)) {
+        return value.map(link).join('');
+    }
     const href = value?.attributes?.href;
     return href
         ? `<link href="${escape(href).replaceAll('"', '&quot;')}">${element('text', value.text)}</link>`