diff --git a/gpx-rs/src/core/algorithm/simplify.rs b/gpx-rs/src/core/algorithm/simplify.rs index 5dfe90de2..8f5c98856 100644 --- a/gpx-rs/src/core/algorithm/simplify.rs +++ b/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( +/// 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( trkseg: &TrackSegment, start: TrackSegmentIndex, end: TrackSegmentIndex, @@ -61,28 +71,40 @@ fn ramer_douglas_peucker_helper( ) 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 = kept.iter().map(|(idx, _)| idx.flat).collect(); + assert_eq!(kept, (0..n).collect::>()); + } + #[test] fn test_empty_and_single_point() { assert!(simplify(&[], 1.0).is_empty()); diff --git a/gpx-rs/src/core/io/parse.rs b/gpx-rs/src/core/io/parse.rs index f221f40eb..0e9e2ff7a 100644 --- a/gpx-rs/src/core/io/parse.rs +++ b/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#""#; + 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#"Tom & Jerry <3 éA "x" 'y' diff --git a/wasm/src/persistence.rs b/wasm/src/persistence.rs index 96e7e36fc..4f5aab017 100644 --- a/wasm/src/persistence.rs +++ b/wasm/src/persistence.rs @@ -26,6 +26,9 @@ thread_local! { static PERSISTER: RefCell> = const { RefCell::new(None) }; static SAVING: Cell = const { Cell::new(false) }; static DIRTY: Cell = 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 = 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 { 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 diff --git a/website/src/lib/engine/backend/tauri.ts b/website/src/lib/engine/backend/tauri.ts index c2bcd7dd6..53c1af4e4 100644 --- a/website/src/lib/engine/backend/tauri.ts +++ b/website/src/lib/engine/backend/tauri.ts @@ -245,7 +245,7 @@ export async function createTauriBackend(): Promise { importsLegacyData: false, open_storage: () => invoke('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 }), diff --git a/website/src/lib/engine/backend/types.ts b/website/src/lib/engine/backend/types.ts index 6791a68fc..450b3a98d 100644 --- a/website/src/lib/engine/backend/types.ts +++ b/website/src/lib/engine/backend/types.ts @@ -340,8 +340,8 @@ export interface Backend { onBehind: () => void, onNewer: () => void ): Promise; - /** Resolves when what was changed has been written to the storage. */ - flush_storage(): Promise; + /** Resolves when the writes of what was changed are done, to whether they went well. */ + flush_storage(): Promise; /** Reads the files again if another window wrote them since, and tells what changed. */ sync_storage(): Promise; set_setting(key: string, json: string): Promise; diff --git a/website/src/lib/engine/storage.test.ts b/website/src/lib/engine/storage.test.ts new file mode 100644 index 000000000..8b851a2d9 --- /dev/null +++ b/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: '', name: 'a' }], + settings: { distanceUnits: '"imperial"', routing: 'false' }, +}; + +type Options = { + stored?: Record; + 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 = { ...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')); + }); +}); diff --git a/website/src/lib/engine/storage.ts b/website/src/lib/engine/storage.ts index 4b4abee4b..c726c959b 100644 --- a/website/src/lib/engine/storage.ts +++ b/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; /** Applies what a call changed to what the facade keeps of the files. */ - sync(update: Outcome): void; + sync(update: Outcome): Promise; /** 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> { +export async function openStoredFiles( + host: StorageHost, + readLegacy: () => Promise = readLegacyData +): Promise> { const { backend } = host; let opened: StorageOpened; const tabs = followOtherTabs(host); @@ -81,47 +84,68 @@ export async function openStoredFiles(host: StorageHost): Promise 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> { +/** + * 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, + readLegacy: () => Promise +): Promise> { const { backend } = host; const imported: Record = {}; - 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 { + 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; +} diff --git a/website/src/lib/storage/legacy-import.test.ts b/website/src/lib/storage/legacy-import.test.ts index efe354498..0ae4ddb8d 100644 --- a/website/src/lib/storage/legacy-import.test.ts +++ b/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 = + 'first'; + assert.ok(gpx.includes(`x${written}`)); + assert.ok(gpx.includes(`${written}`)); + }); + it('leaves out what is missing or empty', () => { const gpx = legacyFileToGPX({ metadata: { name: '', desc: null, time: 'not a date' }, diff --git a/website/src/lib/storage/legacy-import.ts b/website/src/lib/storage/legacy-import.ts index 93ef522f2..63a93ad51 100644 --- a/website/src/lib/storage/legacy-import.ts +++ b/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 ? `${element('text', value.text)}`