From 96f69047fe6f500edbc485675076f5c18cba2a75 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 13:34:22 +0100 Subject: [PATCH 01/14] feat: delete layer from addService dialog --- crates/ui-egui/src/dialogs.rs | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 0b786211c..87aee5562 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -796,6 +796,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } // The service: connecting, an error, or its layers. let mut add: Option = None; + let mut remove: Option<(&str, Value)> = None; let mut import = None; // A directory's folder or service to open, or Back to the directory before. let mut open: Option = None; @@ -907,17 +908,35 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { .max_width(ui_width) .min_width(ui_width) .show(ui, |ui| { - egui::ScrollArea::vertical().show(ui, |ui| { + ui.set_width(ui_width); + egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]).show(ui, |ui| { for l in matching.into_iter().take(SHOWN_LAYERS) { ui.horizontal(|ui| { + ui.set_min_width(ui_width); + ui.add_space(2.0); let kind = l.get("offer").and_then(|o| o.get("type")).and_then(Value::as_str); - if kind == Some("service") { if ui.small_button(tl!("Open")).clicked() { open = l.get("offer").and_then(|o| o.get("url")).and_then(Value::as_str).map(str::to_string); } - } else if !(grouped && kind == Some("features")) && ui.small_button(tl!("Add")).clicked() { - add = Some(l.clone()); + } else if !(grouped && kind == Some("features")) { + let name = l.get("name").and_then(Value::as_str).unwrap_or(""); + let title = l.get("title").and_then(Value::as_str).unwrap_or(name); + // Map layers and tile layers (a web service's) are kept apart. + let map = app.session.map(); + let already_added = if let Some(layer) = map.layers.iter().find(|layer| layer.name == title) { + Some(("layer.remove", json!({"layer": layer.id}))) + } else { + map.tiles.iter().find(|t| t.name == title).map(|t| ("map.removeTiles", json!({"id": t.id}))) + }; + let button_label = if already_added.is_some() { tl!("Remove") } else { tl!("Add") }; + + if ui.small_button(button_label).clicked() { + match already_added { + Some(command) => remove = Some(command), + None => add = Some(l.clone()), + } + } } let name = l.get("name").and_then(Value::as_str).unwrap_or(""); @@ -977,6 +996,11 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { ui.label(tl!("Downloading…")); }); } + if let Some((command, params)) = remove + && let Err(e) = app.run(command, params) + { + app.dialogs.error = Some(e); + } if let (Some(l), Some(conn)) = (add, &app.ui.service) { let src = crate::serviceui::ServiceSource { url: conn.url.clone(), From b6105fa709a60daa1382681b1190bee0a37f3129 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 13:40:53 +0100 Subject: [PATCH 02/14] test: delete layer from addService dialog --- crates/ui-egui/tests/smoke.rs | 40 +++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index 9f44d5e21..0faf9eba9 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -1612,6 +1612,46 @@ fn a_service_is_listed_when_its_capabilities_land() { h.step(); } +#[test] +fn a_service_layer_added_here_shows_remove_and_removes_it() { + use std::io::{Read, Write}; + const CAPABILITIES: &str = r#"Localimage/pngEPSG:3857aA"#; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + std::thread::spawn(move || { + for stream in listener.incoming().take(1) { + let Ok(mut s) = stream else { continue }; + let mut buf = [0u8; 2048]; + let _ = s.read(&mut buf); + let _ = write!(s, "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", CAPABILITIES.len()); + let _ = s.write_all(CAPABILITIES.as_bytes()); + } + }); + let mut h = harness(); + h.state_mut().run("ui.addService", json!(null)).unwrap(); + h.step(); + h.state_mut().run("ui.addService", json!({"url": format!("http://{addr}/wms"), "type": "wms"})).unwrap(); + for _ in 0..100 { + h.step(); + if h.state().ui.service.as_ref().is_some_and(|c| c.listing.is_some() || c.error.is_some() || c.failure.is_some()) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + h.step(); + let tiles_before = h.state().session.map().tiles.len(); + assert!(h.query_by_label("Remove").is_none(), "Remove shows before the layer is added"); + + h.get_by_label("Add").click(); + h.run(); + assert_eq!(h.state().session.map().tiles.iter().filter(|t| t.name == "A").count(), 1, "Add adds the layer once"); + h.get_by_label("Remove").click(); + h.run(); + assert!(h.state().session.map().tiles.iter().all(|t| t.name != "A"), "Remove takes the layer off the map"); + assert_eq!(h.state().session.map().tiles.len(), tiles_before); + assert!(h.query_by_label("Remove").is_none(), "Remove goes back to Add once the layer is removed"); +} + #[test] fn connections_section_and_dialog() { use std::io::{Read, Write}; From b1e041243f20902972cd600fb2a5d14c1946be56 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 14:15:28 +0100 Subject: [PATCH 03/14] test: address review comments --- crates/ui-egui/src/dialogs.rs | 66 +++++++++++++++++++++++++++++------ crates/ui-egui/tests/smoke.rs | 35 ++++++++++++++----- 2 files changed, 82 insertions(+), 19 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 87aee5562..24e6f8b65 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -717,6 +717,42 @@ fn go_to(app: &mut RustGisApp, url: &str, kind: &str, way: Vec<(String, &'static /// How many of a service's layers Add From Service lists. const SHOWN_LAYERS: usize = 500; +/// The command that removes a listed service layer already on `map`, if it is there. A layer is +/// matched by the address the listing offers it at (what `service.add` stores), never by its name +/// alone, so a file's layer or another service's that shares the title is left alone. +fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'static str, Value)> { + let offer = l.get("offer")?; + let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; + if offer.get("type").and_then(Value::as_str) == Some("features") { + // Downloaded features keep the address they came from (an area only adds to it). + return map + .layers + .iter() + .find(|layer| layer.dataset.source.as_deref().is_some_and(|s| s.starts_with(url))) + .map(|layer| ("layer.remove", json!({"layer": layer.id}))); + } + // By position: a service's tile layers have no catalog id to tell them apart. + map.tiles.iter().position(|t| template_matches(url, &t.url)).map(|i| ("map.removeTiles", json!({"index": i}))) +} + +/// Whether `url` is `template` with its `{Time}` placeholders (a time-enabled layer's) filled in. +fn template_matches(template: &str, url: &str) -> bool { + let mut parts = template.split("{Time}"); + let Some(first) = parts.next() else { return false }; + let Some(mut rest) = url.strip_prefix(first) else { return false }; + let mut parts = parts.peekable(); + while let Some(part) = parts.next() { + if parts.peek().is_none() { + return rest.ends_with(part); + } + match rest.find(part) { + Some(at) => rest = rest.get(at + part.len()..).unwrap_or(""), + None => return false, + } + } + rest.is_empty() +} + /// Add From Service: a WMS, WMTS, WFS, WCS, OGC API Features or ArcGIS REST service by its address /// (or a catalogue searched for them, `cswui`). /// Connect fetches its capabilities in the background; its layers are then listed, each with an @@ -898,14 +934,13 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { egui::RichText::new(crate::i18n::fmt("The first {n} are listed.", &[("n", &SHOWN_LAYERS.to_string())])).small().weak(), ); } - let ui_width = ui.available_width(); + let ui_width: f32 = 560.0; egui::Resize::default() .id_salt("add_service_layers") .default_height(280.0) .min_height(100.0) .max_height(350.0) .resizable(true) - .max_width(ui_width) .min_width(ui_width) .show(ui, |ui| { ui.set_width(ui_width); @@ -920,15 +955,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { open = l.get("offer").and_then(|o| o.get("url")).and_then(Value::as_str).map(str::to_string); } } else if !(grouped && kind == Some("features")) { - let name = l.get("name").and_then(Value::as_str).unwrap_or(""); - let title = l.get("title").and_then(Value::as_str).unwrap_or(name); - // Map layers and tile layers (a web service's) are kept apart. - let map = app.session.map(); - let already_added = if let Some(layer) = map.layers.iter().find(|layer| layer.name == title) { - Some(("layer.remove", json!({"layer": layer.id}))) - } else { - map.tiles.iter().find(|t| t.name == title).map(|t| ("map.removeTiles", json!({"id": t.id}))) - }; + let already_added = added_from_listing(app.session.map(), l); let button_label = if already_added.is_some() { tl!("Remove") } else { tl!("Add") }; if ui.small_button(button_label).clicked() { @@ -1945,3 +1972,20 @@ mod tests { assert_eq!(format_degrees(104.25), "104.25°"); } } + +#[cfg(test)] +mod service_listing_tests { + use super::template_matches; + + #[test] + fn a_template_matches_only_its_own_address() { + let wms = "http://h/wms?layers=a&bbox={bbox-epsg-3857}"; + assert!(template_matches(wms, wms)); + assert!(!template_matches(wms, "http://h/wms?layers=b&bbox={bbox-epsg-3857}")); + assert!(!template_matches("", "http://h/x")); + let timed = "http://h/{Time}/{z}/{x}/{y}.png"; + assert!(template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.png")); + assert!(!template_matches(timed, "http://other/2024-06-01/{z}/{x}/{y}.png")); + assert!(!template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.jpg")); + } +} diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index 0faf9eba9..d587a91bf 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -1614,6 +1614,8 @@ fn a_service_is_listed_when_its_capabilities_land() { #[test] fn a_service_layer_added_here_shows_remove_and_removes_it() { + use egui::accesskit::Role; + use egui_kittest::kittest::NodeT; use std::io::{Read, Write}; const CAPABILITIES: &str = r#"Localimage/pngEPSG:3857aA"#; let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); @@ -1639,17 +1641,34 @@ fn a_service_layer_added_here_shows_remove_and_removes_it() { std::thread::sleep(std::time::Duration::from_millis(20)); } h.step(); + // Layers named like the listed one that this service didn't add: a tile layer and a feature layer. + h.state_mut().run("map.addTiles", json!({"url": "http://elsewhere/{z}/{x}/{y}.png", "name": "A"})).unwrap(); + let first = h.state().session.map().layers.first().map(|l| l.id).unwrap(); + h.state_mut().run("layer.rename", json!({"layer": first, "name": "A"})).unwrap(); + h.run(); + // Remove buttons elsewhere in the window (the Layers pane's) are told apart by their nodes. + let removes = |h: &egui_kittest::Harness<'_, RustGisApp>| -> Vec<_> { + h.query_all_by_role_and_label(Role::Button, "Remove").map(|n| n.accesskit_node().id()).collect() + }; + let elsewhere = |h: &egui_kittest::Harness<'_, RustGisApp>| { + h.state().session.map().tiles.iter().filter(|t| t.url.starts_with("http://elsewhere/")).count() + }; let tiles_before = h.state().session.map().tiles.len(); - assert!(h.query_by_label("Remove").is_none(), "Remove shows before the layer is added"); - - h.get_by_label("Add").click(); + let layers_before = h.state().session.map().layers.len(); + let base = removes(&h); + h.get_by_role_and_label(Role::Button, "Add").click(); h.run(); - assert_eq!(h.state().session.map().tiles.iter().filter(|t| t.name == "A").count(), 1, "Add adds the layer once"); - h.get_by_label("Remove").click(); + assert_eq!(h.state().session.map().tiles.len(), tiles_before + 1, "the row offered Add despite the layers named A"); + let after = removes(&h); + assert_eq!(after.len(), base.len() + 1, "the added layer's row offers Remove"); + // The service's tile layer goes; the other tile layer and the feature layer named A stay. + let row = after.into_iter().find(|id| !base.contains(id)).unwrap(); + h.query_all_by_role_and_label(Role::Button, "Remove").find(|n| n.accesskit_node().id() == row).unwrap().click(); h.run(); - assert!(h.state().session.map().tiles.iter().all(|t| t.name != "A"), "Remove takes the layer off the map"); - assert_eq!(h.state().session.map().tiles.len(), tiles_before); - assert!(h.query_by_label("Remove").is_none(), "Remove goes back to Add once the layer is removed"); + assert_eq!(h.state().session.map().tiles.len(), tiles_before, "Remove takes the service's layer off the map"); + assert_eq!(elsewhere(&h), 1, "the other tile layer named A stays"); + assert_eq!(h.state().session.map().layers.len(), layers_before, "the feature layer named A stays"); + assert_eq!(removes(&h), base, "Remove goes back to Add once the layer is removed"); } #[test] From 840c278252c16e353f514dde9fbfcce32450a83d Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 18:00:50 +0100 Subject: [PATCH 04/14] review: ensure only items from the list can be removed --- crates/ui-egui/src/dialogs.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 24e6f8b65..e85dc0c08 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -728,7 +728,13 @@ fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'st return map .layers .iter() - .find(|layer| layer.dataset.source.as_deref().is_some_and(|s| s.starts_with(url))) + .find(|layer| { + layer + .dataset + .source + .as_deref() + .is_some_and(|s| s == url || s.strip_prefix(url).is_some_and(|rest| rest.starts_with(['?', '&']))) + }) .map(|layer| ("layer.remove", json!({"layer": layer.id}))); } // By position: a service's tile layers have no catalog id to tell them apart. From 813e3645e299ce7300e15a0f6e37d95c2d732c32 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 18:03:58 +0100 Subject: [PATCH 05/14] feat: delete layer from addService, add dialog info of the removal --- crates/ui-egui/src/dialogs.rs | 11 ++++++----- crates/ui-egui/tests/smoke.rs | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index e85dc0c08..741f003a1 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -838,7 +838,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } // The service: connecting, an error, or its layers. let mut add: Option = None; - let mut remove: Option<(&str, Value)> = None; + let mut remove: Option<((&str, Value), String)> = None; let mut import = None; // A directory's folder or service to open, or Back to the directory before. let mut open: Option = None; @@ -1029,10 +1029,11 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { ui.label(tl!("Downloading…")); }); } - if let Some((command, params)) = remove - && let Err(e) = app.run(command, params) - { - app.dialogs.error = Some(e); + if let Some(((command, params), title)) = remove { + match app.run(command, params) { + Ok(_) => app.dialogs.info = Some(format!("{} {title}", tl!("Removed"))), + Err(e) => app.dialogs.error = Some(e), + } } if let (Some(l), Some(conn)) = (add, &app.ui.service) { let src = crate::serviceui::ServiceSource { diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index d587a91bf..a0216002f 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -1666,6 +1666,7 @@ fn a_service_layer_added_here_shows_remove_and_removes_it() { h.query_all_by_role_and_label(Role::Button, "Remove").find(|n| n.accesskit_node().id() == row).unwrap().click(); h.run(); assert_eq!(h.state().session.map().tiles.len(), tiles_before, "Remove takes the service's layer off the map"); + assert_eq!(h.state().dialogs.info.as_deref(), Some("Removed A"), "the dialog says what was removed"); assert_eq!(elsewhere(&h), 1, "the other tile layer named A stays"); assert_eq!(h.state().session.map().layers.len(), layers_before, "the feature layer named A stays"); assert_eq!(removes(&h), base, "Remove goes back to Add once the layer is removed"); From 5cf632f7c446e009d91f95cd35cbe5aea9591b63 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 18:09:03 +0100 Subject: [PATCH 06/14] fix: addService visual issues and edge cases Adds the option to configure dialogs with autosized setting, and use of for addService Split long services into multiple lines --- crates/ui-egui/src/dialogs.rs | 44 +++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 741f003a1..c6a862d17 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -221,7 +221,10 @@ pub fn show(app: &mut RustGisApp, ctx: &egui::Context) { // list of dialogs which should allow collapse feature let collapsible_windows = ["addService"]; + let autosized_windows = ["addService"]; + let should_be_collapsible = collapsible_windows.contains(&which); + let should_autosize = autosized_windows.contains(&which); let t = Tokens::get(ctx); let mut open = true; @@ -237,14 +240,17 @@ pub fn show(app: &mut RustGisApp, ctx: &egui::Context) { } // Opens centred and can be moved; each dialog remembers where it was put. - egui::Window::new(tl!(title)) + let window = egui::Window::new(tl!(title)) .id(window_id) .open(&mut open) .collapsible(should_be_collapsible) .resizable(false) .pivot(Align2::CENTER_CENTER) - .default_pos(ctx.content_rect().center() + vec2(0.0, -40.0)) - .show(ctx, |ui| { + .default_pos(ctx.content_rect().center() + vec2(0.0, -40.0)); + // Add Service follows its resizable layer list: an auto-sized window also shrinks, where + // egui otherwise keeps the title bar at the widest width the window has had. + let window = if should_autosize { window.auto_sized() } else { window }; + window.show(ctx, |ui| { ui.set_width(match which { "coordinateSystem" | "zarr" => 660.0, "addTiles" | "addUrl" | "addService" | "s3" => 560.0, @@ -940,20 +946,20 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { egui::RichText::new(crate::i18n::fmt("The first {n} are listed.", &[("n", &SHOWN_LAYERS.to_string())])).small().weak(), ); } - let ui_width: f32 = 560.0; egui::Resize::default() .id_salt("add_service_layers") .default_height(280.0) .min_height(100.0) .max_height(350.0) .resizable(true) - .min_width(ui_width) + .min_width(560.0) + .max_width(660.0) .show(ui, |ui| { - ui.set_width(ui_width); - egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]).show(ui, |ui| { + let scroll = egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]); + scroll.show(ui, |ui| { for l in matching.into_iter().take(SHOWN_LAYERS) { + // Long titles wrap onto more lines; the button stays at the top. ui.horizontal(|ui| { - ui.set_min_width(ui_width); ui.add_space(2.0); let kind = l.get("offer").and_then(|o| o.get("type")).and_then(Value::as_str); if kind == Some("service") { @@ -966,15 +972,17 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { if ui.small_button(button_label).clicked() { match already_added { - Some(command) => remove = Some(command), + Some(command) => { + let name = l.get("name").and_then(Value::as_str).unwrap_or(""); + let title = l.get("title").and_then(Value::as_str).unwrap_or(name); + remove = Some((command, title.to_string())); + } None => add = Some(l.clone()), } } } let name = l.get("name").and_then(Value::as_str).unwrap_or(""); - ui.label(l.get("title").and_then(Value::as_str).unwrap_or(name)).on_hover_text(name); - let what = match kind { Some("features") => tl!("features"), Some("style") => tl!("style"), @@ -985,7 +993,19 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { _ => tl!("map"), }; - ui.label(egui::RichText::new(what).small().weak()); + // The title and its kind in one label, so both wrap together. + let mut job = egui::text::LayoutJob::default(); + let style = ui.style().clone(); + let fallback = egui::FontSelection::Default; + let title = l.get("title").and_then(Value::as_str).unwrap_or(name); + egui::RichText::new(format!("{title} ")).append_to( + &mut job, + &style, + fallback.clone(), + egui::Align::Center, + ); + egui::RichText::new(what).small().weak().append_to(&mut job, &style, fallback, egui::Align::Center); + ui.add(egui::Label::new(job).wrap()).on_hover_text(name); }); } }); From cdd72d51ded7ecd0edd4c092a348f7407d4e7bd4 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 19:30:53 +0100 Subject: [PATCH 07/14] review: Remove for feature, coverage and tileset layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Feature and WCS layers added from a service keep its address as their source (not the download's file name), so Add From Service offers Remove for them. - Coverages are told apart by their id; OGC API tile layers are matched under their tileset list. - An address matches only itself or what follows it at a boundary (`…/items` no longer matches `…/items2`). - "Removed {title}" goes through i18n::fmt. - Smoke tests look up the listing rows by their merged title and kind. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/layer.rs | 4 ++ crates/engine/src/cmd/service.rs | 28 ++++++++++- crates/engine/src/tests.rs | 9 ++++ crates/ui-egui/src/dialogs.rs | 80 +++++++++++++++++++++++++------- crates/ui-egui/tests/smoke.rs | 46 ++++++++++++++++-- 5 files changed, 146 insertions(+), 21 deletions(-) diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index 53554ce17..847b27896 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -2598,6 +2598,10 @@ fn add_download(s: &mut Session, p: &Value) -> CmdResult { url_file_name(&url, &bytes).map_err(|e| format!("{url}: {e}"))? }; let mut r = super::raster::add_named_bytes(s, p, name, bytes)?; + // A service's features or coverage: known by the address it came from (Add From Service). + if super::b(p, "service").unwrap_or(false) || super::b(p, "coverage").unwrap_or(false) { + super::service::mark_service_source(s, &r, &url); + } // `"zoom": true` zooms to everything added, whatever the map held (a link's data); `zoomed` // says whether there was anything to zoom to (a tile archive adds no layer). if b(p, "zoom") == Some(true) { diff --git a/crates/engine/src/cmd/service.rs b/crates/engine/src/cmd/service.rs index d0907d9a9..12610113f 100644 --- a/crates/engine/src/cmd/service.rs +++ b/crates/engine/src/cmd/service.rs @@ -226,6 +226,26 @@ pub fn features_answer(s: &mut Session, name: &str, bytes: Vec) -> Vec { rustgis_data::ogc::fix_axes(&bytes).unwrap_or(bytes) } +/// The layers an add's result `r` names (`layer`, `layers`) marked as read from the service +/// address `url`, not the file name their download was read under: Add From Service then knows +/// them as added, to remove. +pub(crate) fn mark_service_source(s: &mut Session, r: &Value, url: &str) { + let ids: Vec = r + .get("layers") + .and_then(Value::as_array) + .into_iter() + .flatten() + .chain(r.get("layer")) + .filter_map(Value::as_u64) + .filter_map(|i| u32::try_from(i).ok()) + .collect(); + for id in ids { + if let Some(l) = s.map_mut().layer_mut(id) { + l.dataset.source = Some(url.to_string()); + } + } +} + /// A service's features at `url`, with the pages that follow (its `next` links, or an ArcGIS /// layer's pages past its record limit, see [`read_pages`](rustgis_data::ogc::read_pages)): at /// most `max` bytes and [`BLOCKING_TIMEOUT`](crate::render::rustgis_tiles::cache::BLOCKING_TIMEOUT) @@ -426,7 +446,9 @@ pub(crate) fn add(s: &mut Session, p: &Value) -> CmdResult { { let bytes = crate::render::rustgis_tiles::cache::download_blocking(&get).map_err(|e| format!("{name}: {e}"))?; coverage_check(&name, &bytes)?; - super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.tif"), bytes) + let r = super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.tif"), bytes)?; + mark_service_source(s, &r, &get); + Ok(r) } } Offer::Features { url: items } => { @@ -447,9 +469,11 @@ pub(crate) fn add(s: &mut Session, p: &Value) -> CmdResult { s.notify(format!("{name}: {why}, so the whole layer is asked for"), false); } let max = crate::render::rustgis_tiles::cache::MAX_DOWNLOAD; - let bytes = download_features(s, &name, asked.as_deref().unwrap_or(items), max).map_err(|e| slow_whole_layer(e, given))?; + let from = asked.as_deref().unwrap_or(items); + let bytes = download_features(s, &name, from, max).map_err(|e| slow_whole_layer(e, given))?; let bytes = features_answer(s, &name, bytes); let mut r = super::raster::add_named_bytes(s, &json!({"name": name}), format!("{name}.geojson"), bytes)?; + mark_service_source(s, &r, from); // Over an area, the features crossing it: the view stays, or goes to a box given // (the whole layer's bounds would show a country to fill one state). match (asked.is_some(), area, p.get("bbox")) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index de902fa25..6f130e4bd 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -9267,6 +9267,9 @@ fn service_features_asked_for_over_an_area() { let r = add(&mut s, "1.1.0", Some(piaui.clone())); assert_eq!(r["features"], 1); assert_eq!(r["area"], piaui); + // The layer keeps the service address it came from, not its download's file name. + let source = s.map().layer_by_name("M0").and_then(|l| l.dataset.source.clone()); + assert!(source.as_deref().is_some_and(|src| src.starts_with(&format!("{url}?"))), "{source:?}"); let r = add(&mut s, "2.0.0", Some(json!("view"))); assert!(r["area"].is_array(), "{r}"); let r = add(&mut s, "2.0.0", None); @@ -9339,6 +9342,12 @@ fn wcs_coverages_and_catalogue_records() { let r = s.run("service.add", &p).unwrap(); let id = r["layer"].as_u64().unwrap() as u32; assert!(s.map().layer(id).is_some_and(|l| l.is_raster() && l.name == "Soil carbon"), "{r}"); + // The raster keeps its GetCoverage request, so Add From Service knows it as added. + let source = s.map().layer(id).and_then(|l| l.dataset.source.clone()); + assert!(source.as_deref().is_some_and(|src| src.contains("REQUEST=GetCoverage") && src.contains("=soc")), "{source:?}"); + // The raster keeps its GetCoverage request, so Add From Service knows it as added. + let source = s.map().layer(id).and_then(|l| l.dataset.source.clone()); + assert!(source.as_deref().is_some_and(|src| src.contains("REQUEST=GetCoverage") && src.contains("=soc")), "{source:?}"); assert!( s.run("service.coverage", &json!({"url": url, "layer": "soc", "text": caps, "describe": describe, "bbox": [10, 10, 11, 11]})) .is_err() diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index c6a862d17..b4940001a 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -729,22 +729,47 @@ const SHOWN_LAYERS: usize = 500; fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'static str, Value)> { let offer = l.get("offer")?; let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; - if offer.get("type").and_then(Value::as_str) == Some("features") { - // Downloaded features keep the address they came from (an area only adds to it). - return map - .layers + let layer_from = |matches: &dyn Fn(&str) -> bool| { + map.layers .iter() - .find(|layer| { - layer - .dataset - .source - .as_deref() - .is_some_and(|s| s == url || s.strip_prefix(url).is_some_and(|rest| rest.starts_with(['?', '&']))) - }) - .map(|layer| ("layer.remove", json!({"layer": layer.id}))); + .find(|layer| layer.dataset.source.as_deref().is_some_and(matches)) + .map(|layer| ("layer.remove", json!({"layer": layer.id}))) + }; + match offer.get("type").and_then(Value::as_str) { + // Downloaded features keep the address they came from (`service.mark_service_source`; + // an area only adds a query to it). + Some("features") => layer_from(&|s| from_address(url, s)), + // A coverage keeps its GetCoverage request: the service's address, which its other + // coverages share, with this coverage's id. + Some("coverage") => { + let id = offer.get("describe").and_then(Value::as_str).and_then(coverage_id)?; + layer_from(&|s| from_address(url, s) && coverage_id(s) == Some(id)) + } + // By position: a service's tile layers have no catalog id to tell them apart. An OGC API + // layer's tiles use the template its tileset list advertises, found under that list. + _ => { + let list = l.get("tilesets").and_then(Value::as_str).map(|t| t.split(['?', '#']).next().unwrap_or(t)); + map.tiles + .iter() + .position(|t| template_matches(url, &t.url) || list.is_some_and(|list| from_address(list, &t.url))) + .map(|i| ("map.removeTiles", json!({"index": i}))) + } } - // By position: a service's tile layers have no catalog id to tell them apart. - map.tiles.iter().position(|t| template_matches(url, &t.url)).map(|i| ("map.removeTiles", json!({"index": i}))) +} + +/// Whether `source` is the address `url`, or `url` with more after it (an area's query, a page): +/// never another address that only starts with the same letters (`…/items2` for `…/items`). +fn from_address(url: &str, source: &str) -> bool { + source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&', '/']) || rest.starts_with(['?', '&', '/', '#'])) +} + +/// A WCS request's coverage (`COVERAGEID`, or a WCS 1.0's `COVERAGE`), as its address gives it. +fn coverage_id(url: &str) -> Option<&str> { + let query = url.split('#').next()?.split_once('?')?.1; + query.split('&').find_map(|kv| { + let (k, v) = kv.split_once('=')?; + (k.eq_ignore_ascii_case("coverageid") || k.eq_ignore_ascii_case("coverage")).then_some(v).filter(|v| !v.is_empty()) + }) } /// Whether `url` is `template` with its `{Time}` placeholders (a time-enabled layer's) filled in. @@ -1051,7 +1076,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } if let Some(((command, params), title)) = remove { match app.run(command, params) { - Ok(_) => app.dialogs.info = Some(format!("{} {title}", tl!("Removed"))), + Ok(_) => app.dialogs.info = Some(crate::i18n::fmt("Removed {title}", &[("title", &title)])), Err(e) => app.dialogs.error = Some(e), } } @@ -2002,7 +2027,7 @@ mod tests { #[cfg(test)] mod service_listing_tests { - use super::template_matches; + use super::{coverage_id, from_address, template_matches}; #[test] fn a_template_matches_only_its_own_address() { @@ -2015,4 +2040,27 @@ mod service_listing_tests { assert!(!template_matches(timed, "http://other/2024-06-01/{z}/{x}/{y}.png")); assert!(!template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.jpg")); } + + #[test] + fn an_address_matches_only_itself_and_what_follows_it() { + let items = "https://h/items"; + assert!(from_address(items, items)); + assert!(from_address(items, "https://h/items?bbox=1,2,3,4")); + assert!(from_address(items, "https://h/items/2")); + assert!(from_address(items, "https://h/items#top")); + assert!(!from_address(items, "https://h/items2"), "another service's address sharing the prefix"); + assert!(!from_address(items, "https://h/item")); + assert!(!from_address(items, "wfs.geojson")); + // An address ending in its query's start: what follows is that query. + assert!(from_address("https://h/wcs?", "https://h/wcs?SERVICE=WCS")); + } + + #[test] + fn a_coverage_is_told_by_its_id() { + let v2 = "https://h/wcs?SERVICE=WCS&VERSION=2.0.1&REQUEST=GetCoverage&COVERAGEID=dem&FORMAT=image%2Ftiff"; + assert_eq!(coverage_id(v2), Some("dem")); + assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&VERSION=1.0.0&REQUEST=DescribeCoverage&COVERAGE=soc"), Some("soc")); + assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&COVERAGEID="), None); + assert_eq!(coverage_id("https://h/wcs"), None); + } } diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index a0216002f..242d24767 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -2547,7 +2547,7 @@ fn ogc_landing_connect_and_add_use_fixture_documents() { assert_eq!(conn.documents.len(), 1); assert_eq!(conn.listing.as_ref().unwrap()["count"], 1); h.step(); - h.get_by_label("Places"); + h.get_by_label("Places features"); h.get_by_label("Import All").click(); wait_for_ogc(&mut h, |app| app.session.map().layer_by_name("Places").is_some()); let dataset = &h.state().session.map().layer_by_name("Places").unwrap().dataset; @@ -2559,6 +2559,46 @@ fn ogc_landing_connect_and_add_use_fixture_documents() { server.assert_fixture_requests(&[ROOT, CATALOG, ITEMS]); } +/// A service's feature layer, downloaded in the background, is known as added once it lands: its +/// row offers Remove, which takes it off the map and offers Add again. +#[test] +fn a_service_feature_layer_added_here_shows_remove_and_removes_it() { + use egui::accesskit::Role; + use egui_kittest::kittest::NodeT; + use ogc_server::{Route, Server, fixture_features}; + // The capabilities and the features at different paths: the server answers by path. + let server = Server::new(vec![Route::new("/wfs", fixture_features())]); + let caps = format!( + r#"placesPlacesurn:ogc:def:crs:EPSG::4326"#, + server.base + ); + server.set("/caps", &caps); + let mut h = harness(); + h.state_mut().run("ui.language", json!({"code":"en"})).unwrap(); + h.state_mut().run("project.new", json!(null)).unwrap(); + h.state_mut().run("ui.addService", json!(null)).unwrap(); + h.state_mut().run("ui.addService", json!({"url": format!("{}/caps", server.base), "type": "wfs"})).unwrap(); + wait_for_ogc(&mut h, |app| app.ui.service.as_ref().is_some_and(|c| c.pending.is_none())); + h.run(); + let removes = |h: &egui_kittest::Harness<'_, RustGisApp>| -> Vec<_> { + h.query_all_by_role_and_label(Role::Button, "Remove").map(|n| n.accesskit_node().id()).collect() + }; + let base = removes(&h); + h.get_by_role_and_label(Role::Button, "Add").click(); + wait_for_ogc(&mut h, |app| app.session.map().layer_by_name("Places").is_some()); + h.run(); + let source = h.state().session.map().layer_by_name("Places").and_then(|l| l.dataset.source.clone()); + assert!(source.as_deref().is_some_and(|s| s.contains("/wfs?")), "the layer keeps the address it came from: {source:?}"); + let after = removes(&h); + assert_eq!(after.len(), base.len() + 1, "the downloaded layer's row offers Remove"); + let row = after.into_iter().find(|id| !base.contains(id)).unwrap(); + h.query_all_by_role_and_label(Role::Button, "Remove").find(|n| n.accesskit_node().id() == row).unwrap().click(); + h.run(); + assert!(h.state().session.map().layer_by_name("Places").is_none(), "Remove takes the layer off the map"); + assert_eq!(h.state().dialogs.info.as_deref(), Some("Removed Places")); + assert_eq!(removes(&h), base, "Remove goes back to Add once the layer is removed"); +} + #[test] fn ogc_landing_lists_both_offerings_and_keeps_maps_after_failure() { use ogc_server::{CATALOG, ROOT, Route, Server, fixture_collections, fixture_landing}; @@ -2582,9 +2622,9 @@ fn ogc_landing_lists_both_offerings_and_keeps_maps_after_failure() { assert_eq!(conn.listing.as_ref().unwrap()["count"], count); assert_eq!(conn.failure.as_ref().and_then(|f| f.status()), fail.then_some(503)); h.step(); - h.get_by_label("Example GIS (map tiles)"); + h.get_by_label("Example GIS (map tiles) map"); if !fail { - h.get_by_label("Places"); + h.get_by_label("Places features"); } // The root map remains addable with its own document even after another fetch failed. h.query_all_by_label("Add").next().unwrap().click(); From a82480c9b9d312a7cfcac3bf88b0692fa31ce535 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 19:54:21 +0100 Subject: [PATCH 08/14] review: cache Add/Remove per revision, cover re-adds and tilesets - Which listed layers are on the map is worked out when the session or the listing changes, not for every row on every frame. - Tests: Remove for an OGC API tileset; adding again after Remove; two copies from one service removed one at a time. - Drop a duplicated WCS assertion; style and font hoisted out of the row loop; `which == "addService"` for the one auto-sized dialog. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/tests.rs | 3 --- crates/ui-egui/src/dialogs.rs | 35 +++++++++++++++++++++++++---------- crates/ui-egui/src/lib.rs | 5 +++++ crates/ui-egui/tests/smoke.rs | 26 ++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6f130e4bd..f1179c116 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -9345,9 +9345,6 @@ fn wcs_coverages_and_catalogue_records() { // The raster keeps its GetCoverage request, so Add From Service knows it as added. let source = s.map().layer(id).and_then(|l| l.dataset.source.clone()); assert!(source.as_deref().is_some_and(|src| src.contains("REQUEST=GetCoverage") && src.contains("=soc")), "{source:?}"); - // The raster keeps its GetCoverage request, so Add From Service knows it as added. - let source = s.map().layer(id).and_then(|l| l.dataset.source.clone()); - assert!(source.as_deref().is_some_and(|src| src.contains("REQUEST=GetCoverage") && src.contains("=soc")), "{source:?}"); assert!( s.run("service.coverage", &json!({"url": url, "layer": "soc", "text": caps, "describe": describe, "bbox": [10, 10, 11, 11]})) .is_err() diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index b4940001a..995588ae0 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -221,10 +221,9 @@ pub fn show(app: &mut RustGisApp, ctx: &egui::Context) { // list of dialogs which should allow collapse feature let collapsible_windows = ["addService"]; - let autosized_windows = ["addService"]; let should_be_collapsible = collapsible_windows.contains(&which); - let should_autosize = autosized_windows.contains(&which); + let should_autosize = which == "addService"; let t = Tokens::get(ctx); let mut open = true; @@ -723,6 +722,19 @@ fn go_to(app: &mut RustGisApp, url: &str, kind: &str, way: Vec<(String, &'static /// How many of a service's layers Add From Service lists. const SHOWN_LAYERS: usize = 500; +/// Which listed layers are on the map ([`added_from_listing`], in the listing's order), worked out +/// again only when the session or the listing changed, not on every frame. +fn refresh_added(app: &mut RustGisApp) { + let revision = app.session.revision; + let Some(conn) = app.ui.service.as_mut() else { return }; + if conn.added.as_ref().is_some_and(|(r, _)| *r == revision) { + return; + } + let layers = conn.listing.as_ref().and_then(|v| v.get("layers")).and_then(Value::as_array); + let added = layers.into_iter().flatten().map(|l| added_from_listing(app.session.map(), l)).collect(); + conn.added = Some((revision, added)); +} + /// The command that removes a listed service layer already on `map`, if it is there. A layer is /// matched by the address the listing offers it at (what `service.add` stores), never by its name /// alone, so a file's layer or another service's that shares the title is left alone. @@ -748,7 +760,7 @@ fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'st // By position: a service's tile layers have no catalog id to tell them apart. An OGC API // layer's tiles use the template its tileset list advertises, found under that list. _ => { - let list = l.get("tilesets").and_then(Value::as_str).map(|t| t.split(['?', '#']).next().unwrap_or(t)); + let list = l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()); map.tiles .iter() .position(|t| template_matches(url, &t.url) || list.is_some_and(|list| from_address(list, &t.url))) @@ -879,6 +891,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { crate::connectionui::picked(&list, app.dialogs.connection.as_deref(), url).is_none().then(|| (url.to_string(), kind)) }); let mut keep_root = false; + refresh_added(app); if let Some(conn) = &app.ui.service { ui.separator(); if let Some(e) = &conn.error { @@ -962,9 +975,10 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } let words: Vec = app.ui.service_browse.filter.split_whitespace().map(str::to_lowercase).collect(); let text = |l: &Value, k: &str| l.get(k).and_then(Value::as_str).unwrap_or("").to_lowercase(); - let matching: Vec<&Value> = layers + let matching: Vec<(usize, &Value)> = layers .iter() - .filter(|l| words.iter().all(|w| text(l, "title").contains(w.as_str()) || text(l, "name").contains(w.as_str()))) + .enumerate() + .filter(|(_, l)| words.iter().all(|w| text(l, "title").contains(w.as_str()) || text(l, "name").contains(w.as_str()))) .collect(); if matching.len() > SHOWN_LAYERS { ui.label( @@ -982,7 +996,10 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { .show(ui, |ui| { let scroll = egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]); scroll.show(ui, |ui| { - for l in matching.into_iter().take(SHOWN_LAYERS) { + // The same for every row. + let style = ui.style().clone(); + let fallback = egui::FontSelection::Default; + for (i, l) in matching.into_iter().take(SHOWN_LAYERS) { // Long titles wrap onto more lines; the button stays at the top. ui.horizontal(|ui| { ui.add_space(2.0); @@ -992,7 +1009,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { open = l.get("offer").and_then(|o| o.get("url")).and_then(Value::as_str).map(str::to_string); } } else if !(grouped && kind == Some("features")) { - let already_added = added_from_listing(app.session.map(), l); + let already_added = conn.added.as_ref().and_then(|(_, added)| added.get(i)).cloned().flatten(); let button_label = if already_added.is_some() { tl!("Remove") } else { tl!("Add") }; if ui.small_button(button_label).clicked() { @@ -1020,8 +1037,6 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { // The title and its kind in one label, so both wrap together. let mut job = egui::text::LayoutJob::default(); - let style = ui.style().clone(); - let fallback = egui::FontSelection::Default; let title = l.get("title").and_then(Value::as_str).unwrap_or(name); egui::RichText::new(format!("{title} ")).append_to( &mut job, @@ -1029,7 +1044,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { fallback.clone(), egui::Align::Center, ); - egui::RichText::new(what).small().weak().append_to(&mut job, &style, fallback, egui::Align::Center); + egui::RichText::new(what).small().weak().append_to(&mut job, &style, fallback.clone(), egui::Align::Center); ui.add(egui::Label::new(job).wrap()).on_hover_text(name); }); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e7a348de2..6e7ec3dda 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -324,6 +324,9 @@ pub struct ServiceConn { pub pending: Option, /// What `service.layers` made of it. pub listing: Option, + /// For each listed layer, the command that removes it when it is on the map, as of a session + /// revision (Add From Service's Remove; see `dialogs::refresh_added`). + pub added: Option<(u64, Vec>)>, pub error: Option, /// A capabilities request that failed, with what the server said (#509). pub failure: Option, @@ -2399,6 +2402,7 @@ impl RustGisApp { documents: Vec::new(), pending: Some(caps), listing: None, + added: None, error: None, failure: None, }); @@ -3443,6 +3447,7 @@ impl RustGisApp { conn.slot = fetch_detailed_into(next); } conn.listing = Some(v); + conn.added = None; } Err(e) => conn.error = Some(e), } diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index 242d24767..bc4444d52 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -2597,6 +2597,27 @@ fn a_service_feature_layer_added_here_shows_remove_and_removes_it() { assert!(h.state().session.map().layer_by_name("Places").is_none(), "Remove takes the layer off the map"); assert_eq!(h.state().dialogs.info.as_deref(), Some("Removed Places")); assert_eq!(removes(&h), base, "Remove goes back to Add once the layer is removed"); + // Added again after Remove, then a second copy from the same service: Remove takes one + // copy at a time, and the row offers Remove while a copy is left. + h.get_by_role_and_label(Role::Button, "Add").click(); + wait_for_ogc(&mut h, |app| app.session.map().layer_by_name("Places").is_some()); + let address = h.state().session.map().layer_by_name("Places").and_then(|l| l.dataset.source.clone()).unwrap(); + h.state_mut().run("ui.addUrl", json!({"url": address, "name": "Places", "service": true})).unwrap(); + let copies = + |app: &RustGisApp| app.session.map().layers.iter().filter(|l| l.dataset.source.as_deref() == Some(address.as_str())).count(); + wait_for_ogc(&mut h, |app| copies(app) == 2); + h.run(); + let remove = |h: &mut egui_kittest::Harness<'_, RustGisApp>| { + let row = removes(h).into_iter().find(|id| !base.contains(id)).unwrap(); + h.query_all_by_role_and_label(Role::Button, "Remove").find(|n| n.accesskit_node().id() == row).unwrap().click(); + h.run(); + }; + remove(&mut h); + assert_eq!(copies(h.state()), 1, "Remove takes one copy"); + assert_eq!(removes(&h).len(), base.len() + 1, "the row offers Remove while a copy is left"); + remove(&mut h); + assert_eq!(copies(h.state()), 0); + assert_eq!(removes(&h), base); } #[test] @@ -2627,9 +2648,14 @@ fn ogc_landing_lists_both_offerings_and_keeps_maps_after_failure() { h.get_by_label("Places features"); } // The root map remains addable with its own document even after another fetch failed. + let removes = h.query_all_by_label("Remove").count(); h.query_all_by_label("Add").next().unwrap().click(); wait_for_ogc(&mut h, |app| !app.session.map().tiles.is_empty()); assert!(h.state().session.map().tiles[0].url.contains("/map/tiles/")); + // Its tiles use the template its tileset list advertises, known as added: Remove. + h.step(); + h.step(); + assert_eq!(h.query_all_by_label("Remove").count(), removes + 1, "the added tileset's row offers Remove"); } } From cb552958aca4a9596c1f43de2812a975ed4d7eb8 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 20:14:42 +0100 Subject: [PATCH 09/14] review: redact Copy Path, tighten address match - layer.copyPath gives the address without its keys, tokens or password (sources::redact), now that service layers keep their service address as their source. - An address ending in `/` no longer matches every address under it. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/clipboard.rs | 4 +++- crates/engine/src/tests.rs | 15 +++++++++++++++ crates/ui-egui/src/dialogs.rs | 5 ++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/clipboard.rs b/crates/engine/src/cmd/clipboard.rs index 892a8650d..e627681f7 100644 --- a/crates/engine/src/cmd/clipboard.rs +++ b/crates/engine/src/cmd/clipboard.rs @@ -174,5 +174,7 @@ fn copy_path(s: &mut Session, p: &Value) -> CmdResult { let l = s.map().layer(id).ok_or("no such layer")?; let path = l.raster.as_ref().and_then(|r| r.path.clone()).or_else(|| l.dataset.source.clone()); let path = path.ok_or_else(|| format!("{} isn't stored in a file", l.name))?; - Ok(json!({"text": path})) + // A web or database address without its keys, tokens or password (a service layer's + // address is its source), as a saved project keeps it. + Ok(json!({"text": crate::sources::redact(&path)})) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f1179c116..d0a271918 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -8553,6 +8553,21 @@ fn a_failed_command_takes_its_request_back() { assert_eq!(left[0]["url"], "https://x.org/kept.geojson"); } +/// A service's features keep the address they came from, and Copy Path gives it without its key. +#[test] +fn a_service_layers_address_is_copied_without_its_key() { + let mut s = Session::new(Project::default()); + let id = s.download_started(); + assert!(s.download_landed(id, br#"{"type": "Point", "coordinates": [1, 2]}"#.to_vec())); + let url = "https://x.org/collections/a/items?f=json&api_key=SECRET"; + let r = s.run("layer.addDownload", &json!({"id": id, "url": url, "name": "Api", "service": true})).unwrap(); + let layer = r["layer"].as_u64().or_else(|| r["layers"][0].as_u64()).unwrap() as u32; + assert_eq!(s.map().layer(layer).and_then(|l| l.dataset.source.as_deref()), Some(url)); + let copied = s.run("layer.copyPath", &json!({"layer": layer})).unwrap(); + let text = copied["text"].as_str().unwrap(); + assert!(text.starts_with("https://x.org/collections/a/items?") && !text.contains("SECRET"), "{text}"); +} + /// The app's downloads land in the project they were started for, as bytes (no base64); one /// whose project was closed meanwhile is dropped. #[test] diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 995588ae0..0efe5ba8c 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -772,7 +772,7 @@ fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'st /// Whether `source` is the address `url`, or `url` with more after it (an area's query, a page): /// never another address that only starts with the same letters (`…/items2` for `…/items`). fn from_address(url: &str, source: &str) -> bool { - source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&', '/']) || rest.starts_with(['?', '&', '/', '#'])) + source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&']) || rest.starts_with(['?', '&', '/', '#'])) } /// A WCS request's coverage (`COVERAGEID`, or a WCS 1.0's `COVERAGE`), as its address gives it. @@ -2068,6 +2068,9 @@ mod service_listing_tests { assert!(!from_address(items, "wfs.geojson")); // An address ending in its query's start: what follows is that query. assert!(from_address("https://h/wcs?", "https://h/wcs?SERVICE=WCS")); + // One ending in `/` is a path, not a prefix of every address under it. + assert!(!from_address("https://h/", "https://h/other")); + assert!(from_address("https://h/", "https://h/?bbox=1,2,3,4")); } #[test] From 801726c5d38f75c6fa45d70f367f7591168a1243 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 20:47:01 +0100 Subject: [PATCH 10/14] review: exact tile match first, keyless matching, redacted Source - A tile row matches its own template first; the tileset list address is only a fallback, so a sibling tileset isn't removed. - Layers are matched by their address without its keys too, so a keyed service layer is still known after a save and reopen. - The Properties pane's Source row hides keys, tokens and passwords. - Remove state is worked out for the rows shown (the first 500 matching the filter), when the session, listing or filter changes. - The list is never wider than the dialog; style nits. - The WMS smoke test fails plainly when the listing never arrives. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 94 +++++++++++++++++++++++------------ crates/ui-egui/src/lib.rs | 4 +- crates/ui-egui/src/panes.rs | 3 +- crates/ui-egui/tests/smoke.rs | 1 + 4 files changed, 66 insertions(+), 36 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 0efe5ba8c..dcddf94d6 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -221,7 +221,6 @@ pub fn show(app: &mut RustGisApp, ctx: &egui::Context) { // list of dialogs which should allow collapse feature let collapsible_windows = ["addService"]; - let should_be_collapsible = collapsible_windows.contains(&which); let should_autosize = which == "addService"; @@ -722,53 +721,84 @@ fn go_to(app: &mut RustGisApp, url: &str, kind: &str, way: Vec<(String, &'static /// How many of a service's layers Add From Service lists. const SHOWN_LAYERS: usize = 500; -/// Which listed layers are on the map ([`added_from_listing`], in the listing's order), worked out -/// again only when the session or the listing changed, not on every frame. +/// The listed layers matching `filter` (words their title or name contains), with their place in +/// the listing. +fn matching_rows<'a>(layers: &'a [Value], filter: &str) -> Vec<(usize, &'a Value)> { + let words: Vec = filter.split_whitespace().map(str::to_lowercase).collect(); + let text = |l: &Value, k: &str| l.get(k).and_then(Value::as_str).unwrap_or("").to_lowercase(); + layers + .iter() + .enumerate() + .filter(|(_, l)| words.iter().all(|w| text(l, "title").contains(w.as_str()) || text(l, "name").contains(w.as_str()))) + .collect() +} + +/// Which of the rows shown (the first [`SHOWN_LAYERS`] matching the filter) are on the map +/// ([`added_from_listing`], by their place in the listing), worked out again only when the +/// session, the listing or the filter changed, not on every frame. fn refresh_added(app: &mut RustGisApp) { let revision = app.session.revision; let Some(conn) = app.ui.service.as_mut() else { return }; - if conn.added.as_ref().is_some_and(|(r, _)| *r == revision) { + let filter = &app.ui.service_browse.filter; + if conn.added.as_ref().is_some_and(|(r, f, _)| *r == revision && f == filter) { return; } - let layers = conn.listing.as_ref().and_then(|v| v.get("layers")).and_then(Value::as_array); - let added = layers.into_iter().flatten().map(|l| added_from_listing(app.session.map(), l)).collect(); - conn.added = Some((revision, added)); + let map = app.session.map(); + // Each layer's source as it is and without its keys, once: a project saved and opened again + // keeps its addresses without them (the listing's still carry them). + let sources: Vec> = + map.layers.iter().filter_map(|l| l.dataset.source.as_deref().map(|s| (l.id, s, rustgis_engine::sources::redact(s)))).collect(); + let none = Vec::new(); + let layers = conn.listing.as_ref().and_then(|v| v.get("layers")).and_then(Value::as_array).unwrap_or(&none); + let mut added = vec![None; layers.len()]; + for (i, l) in matching_rows(layers, filter).into_iter().take(SHOWN_LAYERS) { + if let Some(slot) = added.get_mut(i) { + *slot = added_from_listing(map, &sources, l); + } + } + conn.added = Some((revision, filter.clone(), added)); } /// The command that removes a listed service layer already on `map`, if it is there. A layer is /// matched by the address the listing offers it at (what `service.add` stores), never by its name /// alone, so a file's layer or another service's that shares the title is left alone. -fn added_from_listing(map: &rustgis_engine::map::Map, l: &Value) -> Option<(&'static str, Value)> { +fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[Source<'_>], l: &Value) -> Option<(&'static str, Value)> { let offer = l.get("offer")?; let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; - let layer_from = |matches: &dyn Fn(&str) -> bool| { - map.layers + let keyless = rustgis_engine::sources::redact(url); + let layer_from = |matches: &dyn Fn(&str, &str) -> bool| { + sources .iter() - .find(|layer| layer.dataset.source.as_deref().is_some_and(matches)) - .map(|layer| ("layer.remove", json!({"layer": layer.id}))) + .find(|(_, s, redacted)| matches(url, s) || matches(&keyless, redacted)) + .map(|(id, ..)| ("layer.remove", json!({"layer": id}))) }; match offer.get("type").and_then(Value::as_str) { // Downloaded features keep the address they came from (`service.mark_service_source`; // an area only adds a query to it). - Some("features") => layer_from(&|s| from_address(url, s)), + Some("features") => layer_from(&|url, s| from_address(url, s)), // A coverage keeps its GetCoverage request: the service's address, which its other // coverages share, with this coverage's id. Some("coverage") => { let id = offer.get("describe").and_then(Value::as_str).and_then(coverage_id)?; - layer_from(&|s| from_address(url, s) && coverage_id(s) == Some(id)) + layer_from(&|url, s| from_address(url, s) && coverage_id(s) == Some(id)) } - // By position: a service's tile layers have no catalog id to tell them apart. An OGC API - // layer's tiles use the template its tileset list advertises, found under that list. + // By position: a service's tile layers have no catalog id to tell them apart. The listed + // template first; else, for an OGC API layer, the template its tileset list advertises, + // found under that list. _ => { let list = l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()); map.tiles .iter() - .position(|t| template_matches(url, &t.url) || list.is_some_and(|list| from_address(list, &t.url))) + .position(|t| template_matches(url, &t.url)) + .or_else(|| map.tiles.iter().position(|t| list.is_some_and(|list| from_address(list, &t.url)))) .map(|i| ("map.removeTiles", json!({"index": i}))) } } } +/// A map layer's id and source, as it is and without its keys. +type Source<'a> = (u32, &'a str, String); + /// Whether `source` is the address `url`, or `url` with more after it (an area's query, a page): /// never another address that only starts with the same letters (`…/items2` for `…/items`). fn from_address(url: &str, source: &str) -> bool { @@ -881,7 +911,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } // The service: connecting, an error, or its layers. let mut add: Option = None; - let mut remove: Option<((&str, Value), String)> = None; + let mut remove: Option<((&'static str, Value), String)> = None; let mut import = None; // A directory's folder or service to open, or Back to the directory before. let mut open: Option = None; @@ -973,13 +1003,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { } }); } - let words: Vec = app.ui.service_browse.filter.split_whitespace().map(str::to_lowercase).collect(); - let text = |l: &Value, k: &str| l.get(k).and_then(Value::as_str).unwrap_or("").to_lowercase(); - let matching: Vec<(usize, &Value)> = layers - .iter() - .enumerate() - .filter(|(_, l)| words.iter().all(|w| text(l, "title").contains(w.as_str()) || text(l, "name").contains(w.as_str()))) - .collect(); + let matching = matching_rows(layers, &app.ui.service_browse.filter); if matching.len() > SHOWN_LAYERS { ui.label( egui::RichText::new(crate::i18n::fmt("The first {n} are listed.", &[("n", &SHOWN_LAYERS.to_string())])).small().weak(), @@ -991,14 +1015,13 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { .min_height(100.0) .max_height(350.0) .resizable(true) - .min_width(560.0) + // Never wider than the dialog to begin with, as wide as its siblings. + .min_width(ui.available_width().min(560.0)) .max_width(660.0) .show(ui, |ui| { - let scroll = egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]); - scroll.show(ui, |ui| { + egui::ScrollArea::vertical().id_salt("add_service_layers_scroll").auto_shrink([false, false]).show(ui, |ui| { // The same for every row. let style = ui.style().clone(); - let fallback = egui::FontSelection::Default; for (i, l) in matching.into_iter().take(SHOWN_LAYERS) { // Long titles wrap onto more lines; the button stays at the top. ui.horizontal(|ui| { @@ -1009,7 +1032,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { open = l.get("offer").and_then(|o| o.get("url")).and_then(Value::as_str).map(str::to_string); } } else if !(grouped && kind == Some("features")) { - let already_added = conn.added.as_ref().and_then(|(_, added)| added.get(i)).cloned().flatten(); + let already_added = conn.added.as_ref().and_then(|(_, _, added)| added.get(i)).and_then(Option::as_ref); let button_label = if already_added.is_some() { tl!("Remove") } else { tl!("Add") }; if ui.small_button(button_label).clicked() { @@ -1017,7 +1040,7 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { Some(command) => { let name = l.get("name").and_then(Value::as_str).unwrap_or(""); let title = l.get("title").and_then(Value::as_str).unwrap_or(name); - remove = Some((command, title.to_string())); + remove = Some((command.clone(), title.to_string())); } None => add = Some(l.clone()), } @@ -1041,10 +1064,15 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { egui::RichText::new(format!("{title} ")).append_to( &mut job, &style, - fallback.clone(), + egui::FontSelection::Default, + egui::Align::Center, + ); + egui::RichText::new(what).small().weak().append_to( + &mut job, + &style, + egui::FontSelection::Default, egui::Align::Center, ); - egui::RichText::new(what).small().weak().append_to(&mut job, &style, fallback.clone(), egui::Align::Center); ui.add(egui::Label::new(job).wrap()).on_hover_text(name); }); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 6e7ec3dda..a3184ddf7 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -325,8 +325,8 @@ pub struct ServiceConn { /// What `service.layers` made of it. pub listing: Option, /// For each listed layer, the command that removes it when it is on the map, as of a session - /// revision (Add From Service's Remove; see `dialogs::refresh_added`). - pub added: Option<(u64, Vec>)>, + /// revision and list filter (Add From Service's Remove; see `dialogs::refresh_added`). + pub added: Option<(u64, String, Vec>)>, pub error: Option, /// A capabilities request that failed, with what the server said (#509). pub failure: Option, diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index c9227e65f..c836c54d8 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -5205,7 +5205,8 @@ fn details(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { ("Fields", l.dataset.fields.len().to_string()), ("Selected", l.selection.len().to_string()), ("Symbology", tl!(l.renderer.kind_label()).to_string()), - ("Source", l.dataset.source.clone().unwrap_or_else(|| tl!("In project").into())), + // A web or database address without its keys, tokens or password. + ("Source", l.dataset.source.as_deref().map_or_else(|| tl!("In project").into(), rustgis_engine::sources::redact)), ("Source coordinate system", l.dataset.crs.map(|c| c.label()).unwrap_or_else(|| "EPSG:4326".into())), ("West", format!("{:.4}°", e.min_x)), ("East", format!("{:.4}°", e.max_x)), diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index bc4444d52..18590dec4 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -1640,6 +1640,7 @@ fn a_service_layer_added_here_shows_remove_and_removes_it() { } std::thread::sleep(std::time::Duration::from_millis(20)); } + assert!(h.state().ui.service.as_ref().is_some_and(|c| c.listing.is_some()), "the service's layers were listed"); h.step(); // Layers named like the listed one that this service didn't add: a tile layer and a feature layer. h.state_mut().run("map.addTiles", json!({"url": "http://elsewhere/{z}/{x}/{y}.png", "name": "A"})).unwrap(); From 352005d327e2630b41bafe578fca6ef37d77aeb8 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 21:10:25 +0100 Subject: [PATCH 11/14] review: unambiguous tileset fallback, dated {Time}, naming - A tile row matches its own template first; an OGC API row then takes a tile layer under its tileset list only when no other shown row's list covers it and no row matched it by template. - A trailing {Time} placeholder takes a date, not any address under the prefix. - The list width comment says what the code does. - remove_command, ListedSource, &from_address; smoke tests find rows by the title in their label. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 67 +++++++++++++++++++++++++---------- crates/ui-egui/tests/smoke.rs | 6 ++-- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index dcddf94d6..f11afe865 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -746,27 +746,50 @@ fn refresh_added(app: &mut RustGisApp) { let map = app.session.map(); // Each layer's source as it is and without its keys, once: a project saved and opened again // keeps its addresses without them (the listing's still carry them). - let sources: Vec> = + let sources: Vec> = map.layers.iter().filter_map(|l| l.dataset.source.as_deref().map(|s| (l.id, s, rustgis_engine::sources::redact(s)))).collect(); let none = Vec::new(); let layers = conn.listing.as_ref().and_then(|v| v.get("layers")).and_then(Value::as_array).unwrap_or(&none); + let shown: Vec<(usize, &Value)> = matching_rows(layers, filter).into_iter().take(SHOWN_LAYERS).collect(); let mut added = vec![None; layers.len()]; - for (i, l) in matching_rows(layers, filter).into_iter().take(SHOWN_LAYERS) { + for &(i, l) in &shown { if let Some(slot) = added.get_mut(i) { *slot = added_from_listing(map, &sources, l); } } + // An OGC API layer's tiles use the template its tileset list advertises, which the listing + // doesn't give: a tile layer under one row's list alone, that no row matched by its own + // template, is that row's. One under several rows' lists is nobody's, so Remove never takes + // another row's tiles. + let mut claimed: Vec = added + .iter() + .flatten() + .filter(|(command, _)| *command == "map.removeTiles") + .filter_map(|(_, p)| p.get("index").and_then(Value::as_u64)) + .collect(); + let lists: Vec<(usize, &str)> = shown.iter().filter_map(|&(i, l)| tileset_list(l).map(|list| (i, list))).collect(); + for &(i, list) in &lists { + if added.get(i).is_none_or(Option::is_some) { + continue; + } + let under = |url: &str| lists.iter().filter(|(_, other)| from_address(other, url)).count(); + let found = map.tiles.iter().zip(0u64..).find(|(t, n)| !claimed.contains(n) && from_address(list, &t.url) && under(&t.url) == 1); + if let (Some((_, n)), Some(slot)) = (found, added.get_mut(i)) { + *slot = Some(("map.removeTiles", json!({"index": n}))); + claimed.push(n); + } + } conn.added = Some((revision, filter.clone(), added)); } /// The command that removes a listed service layer already on `map`, if it is there. A layer is /// matched by the address the listing offers it at (what `service.add` stores), never by its name /// alone, so a file's layer or another service's that shares the title is left alone. -fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[Source<'_>], l: &Value) -> Option<(&'static str, Value)> { +fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[ListedSource<'_>], l: &Value) -> Option<(&'static str, Value)> { let offer = l.get("offer")?; let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; let keyless = rustgis_engine::sources::redact(url); - let layer_from = |matches: &dyn Fn(&str, &str) -> bool| { + let remove_command = |matches: &dyn Fn(&str, &str) -> bool| { sources .iter() .find(|(_, s, redacted)| matches(url, s) || matches(&keyless, redacted)) @@ -775,29 +798,26 @@ fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[Source<'_>], l: match offer.get("type").and_then(Value::as_str) { // Downloaded features keep the address they came from (`service.mark_service_source`; // an area only adds a query to it). - Some("features") => layer_from(&|url, s| from_address(url, s)), + Some("features") => remove_command(&from_address), // A coverage keeps its GetCoverage request: the service's address, which its other // coverages share, with this coverage's id. Some("coverage") => { let id = offer.get("describe").and_then(Value::as_str).and_then(coverage_id)?; - layer_from(&|url, s| from_address(url, s) && coverage_id(s) == Some(id)) - } - // By position: a service's tile layers have no catalog id to tell them apart. The listed - // template first; else, for an OGC API layer, the template its tileset list advertises, - // found under that list. - _ => { - let list = l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()); - map.tiles - .iter() - .position(|t| template_matches(url, &t.url)) - .or_else(|| map.tiles.iter().position(|t| list.is_some_and(|list| from_address(list, &t.url)))) - .map(|i| ("map.removeTiles", json!({"index": i}))) + remove_command(&|url, s| from_address(url, s) && coverage_id(s) == Some(id)) } + // By position: a service's tile layers have no catalog id to tell them apart. Matched by + // the listed template here; an OGC API layer's advertised one, by `refresh_added`. + _ => map.tiles.iter().position(|t| template_matches(url, &t.url)).map(|i| ("map.removeTiles", json!({"index": i}))), } } /// A map layer's id and source, as it is and without its keys. -type Source<'a> = (u32, &'a str, String); +type ListedSource<'a> = (u32, &'a str, String); + +/// An OGC API layer's tileset list address, without its query. +fn tileset_list(l: &Value) -> Option<&str> { + l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()).filter(|t| !t.is_empty()) +} /// Whether `source` is the address `url`, or `url` with more after it (an area's query, a page): /// never another address that only starts with the same letters (`…/items2` for `…/items`). @@ -822,6 +842,10 @@ fn template_matches(template: &str, url: &str) -> bool { let mut parts = parts.peekable(); while let Some(part) = parts.next() { if parts.peek().is_none() { + // A trailing placeholder takes a date, not the rest of any address. + if part.is_empty() { + return !rest.is_empty() && !rest.contains(['/', '?', '#']); + } return rest.ends_with(part); } match rest.find(part) { @@ -1015,7 +1039,8 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { .min_height(100.0) .max_height(350.0) .resizable(true) - // Never wider than the dialog to begin with, as wide as its siblings. + // As wide as the dialog to begin with (never wider, for a narrow window), and + // widened up to 660 px by dragging, the window growing with it. .min_width(ui.available_width().min(560.0)) .max_width(660.0) .show(ui, |ui| { @@ -2082,6 +2107,10 @@ mod service_listing_tests { assert!(template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.png")); assert!(!template_matches(timed, "http://other/2024-06-01/{z}/{x}/{y}.png")); assert!(!template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.jpg")); + // A trailing placeholder: a date, not every address under the prefix. + assert!(template_matches("http://h/tiles?time={Time}", "http://h/tiles?time=2024-06-01")); + assert!(!template_matches("http://h/{Time}", "http://h/other/{z}/{x}/{y}.png")); + assert!(!template_matches("http://h/{Time}", "http://h/")); } #[test] diff --git a/crates/ui-egui/tests/smoke.rs b/crates/ui-egui/tests/smoke.rs index 18590dec4..87963668e 100644 --- a/crates/ui-egui/tests/smoke.rs +++ b/crates/ui-egui/tests/smoke.rs @@ -2548,7 +2548,7 @@ fn ogc_landing_connect_and_add_use_fixture_documents() { assert_eq!(conn.documents.len(), 1); assert_eq!(conn.listing.as_ref().unwrap()["count"], 1); h.step(); - h.get_by_label("Places features"); + h.get_by_label_contains("Places"); h.get_by_label("Import All").click(); wait_for_ogc(&mut h, |app| app.session.map().layer_by_name("Places").is_some()); let dataset = &h.state().session.map().layer_by_name("Places").unwrap().dataset; @@ -2644,9 +2644,9 @@ fn ogc_landing_lists_both_offerings_and_keeps_maps_after_failure() { assert_eq!(conn.listing.as_ref().unwrap()["count"], count); assert_eq!(conn.failure.as_ref().and_then(|f| f.status()), fail.then_some(503)); h.step(); - h.get_by_label("Example GIS (map tiles) map"); + h.get_by_label_contains("Example GIS (map tiles)"); if !fail { - h.get_by_label("Places features"); + h.get_by_label_contains("Places"); } // The root map remains addable with its own document even after another fetch failed. let removes = h.query_all_by_label("Remove").count(); From 73224ea0694224bb66f897ba0f09107f2c2d4d9e Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 21:33:49 +0100 Subject: [PATCH 12/14] review: {Time} takes one segment, tile coverage counted once - Every {Time} placeholder, not only a trailing one, takes one non-empty piece of the address (no '/', '?' or '#'). - How many shown rows' tileset lists cover each tile layer is counted once per tile, not for every (row, tile) pair. - A row's title is read once. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 41 +++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index f11afe865..fc64a7a30 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -768,13 +768,20 @@ fn refresh_added(app: &mut RustGisApp) { .filter_map(|(_, p)| p.get("index").and_then(Value::as_u64)) .collect(); let lists: Vec<(usize, &str)> = shown.iter().filter_map(|&(i, l)| tileset_list(l).map(|list| (i, list))).collect(); + // How many rows' lists each tile layer is under, once. + let under: Vec = map.tiles.iter().map(|t| lists.iter().filter(|(_, list)| from_address(list, &t.url)).count()).collect(); for &(i, list) in &lists { if added.get(i).is_none_or(Option::is_some) { continue; } - let under = |url: &str| lists.iter().filter(|(_, other)| from_address(other, url)).count(); - let found = map.tiles.iter().zip(0u64..).find(|(t, n)| !claimed.contains(n) && from_address(list, &t.url) && under(&t.url) == 1); - if let (Some((_, n)), Some(slot)) = (found, added.get_mut(i)) { + let found = map + .tiles + .iter() + .zip(&under) + .zip(0u64..) + .find(|((t, under), n)| **under == 1 && !claimed.contains(n) && from_address(list, &t.url)) + .map(|(_, n)| n); + if let (Some(n), Some(slot)) = (found, added.get_mut(i)) { *slot = Some(("map.removeTiles", json!({"index": n}))); claimed.push(n); } @@ -839,19 +846,15 @@ fn template_matches(template: &str, url: &str) -> bool { let mut parts = template.split("{Time}"); let Some(first) = parts.next() else { return false }; let Some(mut rest) = url.strip_prefix(first) else { return false }; + // Each placeholder takes a date: one piece of the address, never a path or query. + let date = |time: &str| !time.is_empty() && !time.contains(['/', '?', '#']); let mut parts = parts.peekable(); while let Some(part) = parts.next() { if parts.peek().is_none() { - // A trailing placeholder takes a date, not the rest of any address. - if part.is_empty() { - return !rest.is_empty() && !rest.contains(['/', '?', '#']); - } - return rest.ends_with(part); - } - match rest.find(part) { - Some(at) => rest = rest.get(at + part.len()..).unwrap_or(""), - None => return false, + return rest.strip_suffix(part).is_some_and(date); } + let Some(at) = rest.find(part).filter(|&at| part.is_empty() || rest.get(..at).is_some_and(date)) else { return false }; + rest = rest.get(at + part.len()..).unwrap_or(""); } rest.is_empty() } @@ -1052,6 +1055,8 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { ui.horizontal(|ui| { ui.add_space(2.0); let kind = l.get("offer").and_then(|o| o.get("type")).and_then(Value::as_str); + let name = l.get("name").and_then(Value::as_str).unwrap_or(""); + let title = l.get("title").and_then(Value::as_str).unwrap_or(name); if kind == Some("service") { if ui.small_button(tl!("Open")).clicked() { open = l.get("offer").and_then(|o| o.get("url")).and_then(Value::as_str).map(str::to_string); @@ -1062,17 +1067,12 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { if ui.small_button(button_label).clicked() { match already_added { - Some(command) => { - let name = l.get("name").and_then(Value::as_str).unwrap_or(""); - let title = l.get("title").and_then(Value::as_str).unwrap_or(name); - remove = Some((command.clone(), title.to_string())); - } + Some(command) => remove = Some((command.clone(), title.to_string())), None => add = Some(l.clone()), } } } - let name = l.get("name").and_then(Value::as_str).unwrap_or(""); let what = match kind { Some("features") => tl!("features"), Some("style") => tl!("style"), @@ -1085,7 +1085,6 @@ fn add_service(app: &mut RustGisApp, ui: &mut egui::Ui) -> bool { // The title and its kind in one label, so both wrap together. let mut job = egui::text::LayoutJob::default(); - let title = l.get("title").and_then(Value::as_str).unwrap_or(name); egui::RichText::new(format!("{title} ")).append_to( &mut job, &style, @@ -2111,6 +2110,10 @@ mod service_listing_tests { assert!(template_matches("http://h/tiles?time={Time}", "http://h/tiles?time=2024-06-01")); assert!(!template_matches("http://h/{Time}", "http://h/other/{z}/{x}/{y}.png")); assert!(!template_matches("http://h/{Time}", "http://h/")); + // A middle one: one piece of the path, not several. + assert!(!template_matches(timed, "http://h/a/b/{z}/{x}/{y}.png")); + assert!(!template_matches("http://h/{Time}/x/{Time}.png", "http://h/a/b/x/c.png")); + assert!(template_matches("http://h/{Time}/x/{Time}.png", "http://h/2024/x/2025.png")); } #[test] From 5b2d74246cd01a62e8474dcaea6eb4800a000409 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 22:09:11 +0100 Subject: [PATCH 13/14] review: keyless tile match, narrower feature match, docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tile rows also match the template without its keys, as a saved and reopened project keeps it, so they still offer Remove. - A feature row matches its address with a query or fragment after it, not one further down its path (`…/items/2`); tileset lists keep their own path rule. - The tileset claim loop is its own function (claim_tilesets). - Docs: when Add From Service shows Remove; Copy Path gives addresses without their keys. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/dialogs.rs | 51 +++++++++++++++++++++++++++------ docs/user-guide/data-formats.md | 4 +++ docs/user-guide/editing.md | 3 +- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index fc64a7a30..f35b70da1 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -757,6 +757,13 @@ fn refresh_added(app: &mut RustGisApp) { *slot = added_from_listing(map, &sources, l); } } + claim_tilesets(map, &shown, &mut added); + conn.added = Some((revision, filter.clone(), added)); +} + +/// Gives the OGC API rows shown (`shown`) that `added` doesn't yet match the tile layer their +/// tileset list advertised: its template isn't in the listing, so it is found under that list. +fn claim_tilesets(map: &rustgis_engine::map::Map, shown: &[(usize, &Value)], added: &mut [Option<(&'static str, Value)>]) { // An OGC API layer's tiles use the template its tileset list advertises, which the listing // doesn't give: a tile layer under one row's list alone, that no row matched by its own // template, is that row's. One under several rows' lists is nobody's, so Remove never takes @@ -769,7 +776,7 @@ fn refresh_added(app: &mut RustGisApp) { .collect(); let lists: Vec<(usize, &str)> = shown.iter().filter_map(|&(i, l)| tileset_list(l).map(|list| (i, list))).collect(); // How many rows' lists each tile layer is under, once. - let under: Vec = map.tiles.iter().map(|t| lists.iter().filter(|(_, list)| from_address(list, &t.url)).count()).collect(); + let under: Vec = map.tiles.iter().map(|t| lists.iter().filter(|(_, list)| under_list(list, &t.url)).count()).collect(); for &(i, list) in &lists { if added.get(i).is_none_or(Option::is_some) { continue; @@ -779,14 +786,13 @@ fn refresh_added(app: &mut RustGisApp) { .iter() .zip(&under) .zip(0u64..) - .find(|((t, under), n)| **under == 1 && !claimed.contains(n) && from_address(list, &t.url)) + .find(|((t, under), n)| **under == 1 && !claimed.contains(n) && under_list(list, &t.url)) .map(|(_, n)| n); if let (Some(n), Some(slot)) = (found, added.get_mut(i)) { *slot = Some(("map.removeTiles", json!({"index": n}))); claimed.push(n); } } - conn.added = Some((revision, filter.clone(), added)); } /// The command that removes a listed service layer already on `map`, if it is there. A layer is @@ -814,22 +820,33 @@ fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[ListedSource<'_ } // By position: a service's tile layers have no catalog id to tell them apart. Matched by // the listed template here; an OGC API layer's advertised one, by `refresh_added`. - _ => map.tiles.iter().position(|t| template_matches(url, &t.url)).map(|i| ("map.removeTiles", json!({"index": i}))), + // A saved project keeps its tile templates without their keys, too. + _ => map + .tiles + .iter() + .position(|t| template_matches(url, &t.url) || template_matches(&keyless, &t.url)) + .map(|i| ("map.removeTiles", json!({"index": i}))), } } /// A map layer's id and source, as it is and without its keys. type ListedSource<'a> = (u32, &'a str, String); +/// Whether the tile template `url` is under the tileset list address `list` (its tilesets' paths). +fn under_list(list: &str, url: &str) -> bool { + url.strip_prefix(list).is_some_and(|rest| rest.starts_with('/')) +} + /// An OGC API layer's tileset list address, without its query. fn tileset_list(l: &Value) -> Option<&str> { l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()).filter(|t| !t.is_empty()) } -/// Whether `source` is the address `url`, or `url` with more after it (an area's query, a page): -/// never another address that only starts with the same letters (`…/items2` for `…/items`). +/// Whether `source` is the address `url`, or `url` with a query or fragment after it (an area's +/// bounding box): never another address that only starts with the same letters (`…/items2` for +/// `…/items`), nor one further down its path (`…/items/2`, one feature added by hand). fn from_address(url: &str, source: &str) -> bool { - source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&']) || rest.starts_with(['?', '&', '/', '#'])) + source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&']) || rest.starts_with(['?', '&', '#'])) } /// A WCS request's coverage (`COVERAGEID`, or a WCS 1.0's `COVERAGE`), as its address gives it. @@ -2094,7 +2111,8 @@ mod tests { #[cfg(test)] mod service_listing_tests { - use super::{coverage_id, from_address, template_matches}; + use super::{Session, coverage_id, from_address, template_matches}; + use serde_json::json; #[test] fn a_template_matches_only_its_own_address() { @@ -2121,7 +2139,7 @@ mod service_listing_tests { let items = "https://h/items"; assert!(from_address(items, items)); assert!(from_address(items, "https://h/items?bbox=1,2,3,4")); - assert!(from_address(items, "https://h/items/2")); + assert!(!from_address(items, "https://h/items/2"), "one feature further down the path"); assert!(from_address(items, "https://h/items#top")); assert!(!from_address(items, "https://h/items2"), "another service's address sharing the prefix"); assert!(!from_address(items, "https://h/item")); @@ -2133,6 +2151,21 @@ mod service_listing_tests { assert!(from_address("https://h/", "https://h/?bbox=1,2,3,4")); } + #[test] + fn a_keyed_tile_layer_is_known_after_a_reopen() { + use rustgis_engine::map::tiles::{TileKind, TileLayer}; + let offer = "https://h/wms?layers=a&api_key=K&bbox={bbox-epsg-3857}"; + let mut s = Session::new(rustgis_engine::Project::default()); + // A saved project keeps the template without its key. + let saved = rustgis_engine::sources::redact(offer); + assert!(!saved.contains("api_key"), "{saved}"); + s.map_mut().tiles.push(TileLayer::new("A", TileKind::Raster, saved)); + let index = s.map().tiles.len() - 1; + let listed = json!({"title": "A", "offer": {"type": "tiles", "url": offer}}); + let found = super::added_from_listing(s.map(), &[], &listed); + assert_eq!(found, Some(("map.removeTiles", json!({"index": index})))); + } + #[test] fn a_coverage_is_told_by_its_id() { let v2 = "https://h/wcs?SERVICE=WCS&VERSION=2.0.1&REQUEST=GetCoverage&COVERAGEID=dem&FORMAT=image%2Ftiff"; diff --git a/docs/user-guide/data-formats.md b/docs/user-guide/data-formats.md index 62a3311ce..993772c2b 100644 --- a/docs/user-guide/data-formats.md +++ b/docs/user-guide/data-formats.md @@ -185,6 +185,10 @@ its type). A WebMercatorQuad tileset is found by its tile matrix set, whatever the server calls it. A service that asks for a token says so. In the web app the server must allow requests from web pages (CORS). +A listed layer already on the map shows **Remove** instead of **Add**: it is known by the address +it came from (a coverage by its id too, an OGC API layer's tiles by its tileset list), never by +its name, so a layer of your own with the same name is left alone. Remove takes one copy at a time. + A coverage is read once, for the view when it was added (its DescribeCoverage first, for its axes, format and bounds; `service.coverage` gives the GetCoverage address alone). Moving the map doesn't ask for it again: add it again for another area. The server reprojects it to EPSG:4326 (WCS 2.0 diff --git a/docs/user-guide/editing.md b/docs/user-guide/editing.md index ff0b8fbdf..9bc83464a 100644 --- a/docs/user-guide/editing.md +++ b/docs/user-guide/editing.md @@ -131,7 +131,8 @@ adds them to the layer selected in Layers, matching fields by name and convertin destination field types; unmatched fields stay empty. Geometry types must match. **Paste Special** adds them as a new layer instead. Copied features also go on the system clipboard as GeoJSON, so they can be pasted into another RustGIS window or a text editor, and GeoJSON copied elsewhere -pastes into RustGIS. **Copy Path** copies the selected layer's data source path. +pastes into RustGIS. **Copy Path** copies the selected layer's data source path; a web or +database address comes without its keys, tokens or password (as a saved project keeps it). Whole-layer copying is separate: use **Copy Layer** or **Copy Group** and **Paste Layer** in the Layers pane's context menus to preserve symbology and other layer settings, including when From a7f355c981ebbf8d9eb94f595390e03455e42056 Mon Sep 17 00:00:00 2001 From: Gaux Date: Tue, 27 Oct 2026 22:28:25 +0100 Subject: [PATCH 14/14] review: listing matcher in the engine, all rows' tilesets, exact first - Which listed service layers are on the map (and the command that removes each) moves from the egui dialog to engine::cmd::service_added, with its tests; the dialog only caches it. - A tile layer is claimed under a tileset list counting every listed layer, so a row a filter hides still keeps its tiles. - A layer's exact address is matched before its keyless one, so two copies of a service under different keys each find their own. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/mod.rs | 1 + crates/engine/src/cmd/service_added.rs | 242 +++++++++++++++++++++++++ crates/ui-egui/src/dialogs.rs | 203 +-------------------- 3 files changed, 248 insertions(+), 198 deletions(-) create mode 100644 crates/engine/src/cmd/service_added.rs diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 9c2cda25d..b006caf9e 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -40,6 +40,7 @@ pub mod s3; pub mod search; pub mod select; pub mod service; +pub mod service_added; pub mod share; pub mod sharing; pub mod sources; diff --git a/crates/engine/src/cmd/service_added.rs b/crates/engine/src/cmd/service_added.rs new file mode 100644 index 000000000..ef7047b7a --- /dev/null +++ b/crates/engine/src/cmd/service_added.rs @@ -0,0 +1,242 @@ +//! Which layers of a service's listing (`service.layers`) are on the map already, and the command +//! that removes each (Add From Service's Remove). A listed layer is known by the address it came +//! from (`service::mark_service_source`, a tile layer's template), never by its name alone, so a +//! file's layer or another service's that shares the title is left alone. + +use rustgis_map::Map; +use serde_json::{Value, json}; + +/// A command and its parameters (`layer.remove {"layer"}`, `map.removeTiles {"index"}`). +pub type Removal = (&'static str, Value); + +/// For each layer of `listed` (a `service.layers` listing's `layers`) whose place is in `rows`, +/// the command that removes it from `map` when it is there; `None` for the others, and for every +/// row not asked about. Each map layer is given to one row at most. +pub fn added(map: &Map, listed: &[Value], rows: &[usize]) -> Vec> { + // Each layer's source as it is and without its keys, once: a project saved and opened again + // keeps its addresses without them (the listing's still carry them). + let sources: Vec> = + map.layers.iter().filter_map(|l| l.dataset.source.as_deref().map(|s| (l.id, s, crate::sources::redact(s)))).collect(); + let mut added = vec![None; listed.len()]; + for &i in rows { + if let (Some(l), Some(slot)) = (listed.get(i), added.get_mut(i)) { + *slot = removal(map, &sources, l); + } + } + claim_tilesets(map, listed, rows, &mut added); + added +} + +/// A map layer's id and source, as it is and without its keys. +type Source<'a> = (u32, &'a str, String); + +/// The command that removes the listed layer `l` from `map`, when it is there: matched by its +/// address as it is first, then without its keys (as a saved project keeps it), so a copy of the +/// same service added under another key is only taken when nothing matches exactly. +fn removal(map: &Map, sources: &[Source<'_>], l: &Value) -> Option { + let offer = l.get("offer")?; + let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; + let keyless = crate::sources::redact(url); + let remove_layer = |matches: &dyn Fn(&str, &str) -> bool| { + let exact = sources.iter().find(|(_, s, _)| matches(url, s)); + exact + .or_else(|| sources.iter().find(|(_, _, redacted)| matches(&keyless, redacted))) + .map(|(id, ..)| ("layer.remove", json!({"layer": id}))) + }; + match offer.get("type").and_then(Value::as_str) { + // Downloaded features keep the address they came from (an area only adds a query to it). + Some("features") => remove_layer(&from_address), + // A coverage keeps its GetCoverage request: the service's address, which its other + // coverages share, with this coverage's id. + Some("coverage") => { + let id = offer.get("describe").and_then(Value::as_str).and_then(coverage_id)?; + remove_layer(&|url, s| from_address(url, s) && coverage_id(s) == Some(id)) + } + // By position: a service's tile layers have no catalog id to tell them apart. Matched by + // the listed template here (a saved project keeps it without its keys); an OGC API + // layer's advertised one, by `claim_tilesets`. + _ => map + .tiles + .iter() + .position(|t| template_matches(url, &t.url)) + .or_else(|| map.tiles.iter().position(|t| template_matches(&keyless, &t.url))) + .map(|i| ("map.removeTiles", json!({"index": i}))), + } +} + +/// Gives the OGC API rows asked about (`rows`) that `added` doesn't match yet the tile layer their +/// tileset list advertised. Its template isn't in the listing, so it is found under that list: a +/// tile layer under one listed layer's list alone (counting every layer listed, shown or not), +/// that no row matched by its own template, is that row's. One under several lists is nobody's, +/// so Remove never takes another row's tiles. +fn claim_tilesets(map: &Map, listed: &[Value], rows: &[usize], added: &mut [Option]) { + let mut claimed: Vec = added + .iter() + .flatten() + .filter(|(command, _)| *command == "map.removeTiles") + .filter_map(|(_, p)| p.get("index").and_then(Value::as_u64)) + .collect(); + let lists: Vec<&str> = listed.iter().filter_map(tileset_list).collect(); + // How many listed layers' lists each tile layer is under, once. + let under: Vec = map.tiles.iter().map(|t| lists.iter().filter(|list| under_list(list, &t.url)).count()).collect(); + for &i in rows { + if added.get(i).is_none_or(Option::is_some) { + continue; + } + let Some(list) = listed.get(i).and_then(tileset_list) else { continue }; + let found = map + .tiles + .iter() + .zip(&under) + .zip(0u64..) + .find(|((t, under), n)| **under == 1 && !claimed.contains(n) && under_list(list, &t.url)) + .map(|(_, n)| n); + if let (Some(n), Some(slot)) = (found, added.get_mut(i)) { + *slot = Some(("map.removeTiles", json!({"index": n}))); + claimed.push(n); + } + } +} + +/// An OGC API layer's tileset list address, without its query. +fn tileset_list(l: &Value) -> Option<&str> { + l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()).filter(|t| !t.is_empty()) +} + +/// Whether the tile template `url` is under the tileset list address `list` (its tilesets' paths). +fn under_list(list: &str, url: &str) -> bool { + url.strip_prefix(list).is_some_and(|rest| rest.starts_with('/')) +} + +/// Whether `source` is the address `url`, or `url` with a query or fragment after it (an area's +/// bounding box): never another address that only starts with the same letters (`…/items2` for +/// `…/items`), nor one further down its path (`…/items/2`, one feature added by hand). +fn from_address(url: &str, source: &str) -> bool { + source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&']) || rest.starts_with(['?', '&', '#'])) +} + +/// A WCS request's coverage (`COVERAGEID`, or a WCS 1.0's `COVERAGE`), as its address gives it. +fn coverage_id(url: &str) -> Option<&str> { + let query = url.split('#').next()?.split_once('?')?.1; + query.split('&').find_map(|kv| { + let (k, v) = kv.split_once('=')?; + (k.eq_ignore_ascii_case("coverageid") || k.eq_ignore_ascii_case("coverage")).then_some(v).filter(|v| !v.is_empty()) + }) +} + +/// Whether `url` is `template` with its `{Time}` placeholders (a time-enabled layer's) filled in. +fn template_matches(template: &str, url: &str) -> bool { + let mut parts = template.split("{Time}"); + let Some(first) = parts.next() else { return false }; + let Some(mut rest) = url.strip_prefix(first) else { return false }; + // Each placeholder takes a date: one piece of the address, never a path or query. + let date = |time: &str| !time.is_empty() && !time.contains(['/', '?', '#']); + let mut parts = parts.peekable(); + while let Some(part) = parts.next() { + if parts.peek().is_none() { + return rest.strip_suffix(part).is_some_and(date); + } + let Some(at) = rest.find(part).filter(|&at| part.is_empty() || rest.get(..at).is_some_and(date)) else { return false }; + rest = rest.get(at + part.len()..).unwrap_or(""); + } + rest.is_empty() +} + +#[cfg(test)] +mod tests { + use super::*; + use rustgis_map::{TileKind, TileLayer}; + + fn tiles(urls: &[&str]) -> Map { + let mut map = Map::new("Map"); + map.tiles = urls.iter().map(|u| TileLayer::new("T", TileKind::Raster, *u)).collect(); + map + } + + #[test] + fn a_template_matches_only_its_own_address() { + let wms = "http://h/wms?layers=a&bbox={bbox-epsg-3857}"; + assert!(template_matches(wms, wms)); + assert!(!template_matches(wms, "http://h/wms?layers=b&bbox={bbox-epsg-3857}")); + assert!(!template_matches("", "http://h/x")); + let timed = "http://h/{Time}/{z}/{x}/{y}.png"; + assert!(template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.png")); + assert!(!template_matches(timed, "http://other/2024-06-01/{z}/{x}/{y}.png")); + assert!(!template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.jpg")); + // A trailing placeholder: a date, not every address under the prefix. + assert!(template_matches("http://h/tiles?time={Time}", "http://h/tiles?time=2024-06-01")); + assert!(!template_matches("http://h/{Time}", "http://h/other/{z}/{x}/{y}.png")); + assert!(!template_matches("http://h/{Time}", "http://h/")); + // A middle one: one piece of the path, not several. + assert!(!template_matches(timed, "http://h/a/b/{z}/{x}/{y}.png")); + assert!(!template_matches("http://h/{Time}/x/{Time}.png", "http://h/a/b/x/c.png")); + assert!(template_matches("http://h/{Time}/x/{Time}.png", "http://h/2024/x/2025.png")); + } + + #[test] + fn an_address_matches_only_itself_and_what_follows_it() { + let items = "https://h/items"; + assert!(from_address(items, items)); + assert!(from_address(items, "https://h/items?bbox=1,2,3,4")); + assert!(!from_address(items, "https://h/items/2"), "one feature further down the path"); + assert!(from_address(items, "https://h/items#top")); + assert!(!from_address(items, "https://h/items2"), "another service's address sharing the prefix"); + assert!(!from_address(items, "https://h/item")); + assert!(!from_address(items, "wfs.geojson")); + // An address ending in its query's start: what follows is that query. + assert!(from_address("https://h/wcs?", "https://h/wcs?SERVICE=WCS")); + // One ending in `/` is a path, not a prefix of every address under it. + assert!(!from_address("https://h/", "https://h/other")); + assert!(from_address("https://h/", "https://h/?bbox=1,2,3,4")); + } + + #[test] + fn a_coverage_is_told_by_its_id() { + let v2 = "https://h/wcs?SERVICE=WCS&VERSION=2.0.1&REQUEST=GetCoverage&COVERAGEID=dem&FORMAT=image%2Ftiff"; + assert_eq!(coverage_id(v2), Some("dem")); + assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&VERSION=1.0.0&REQUEST=DescribeCoverage&COVERAGE=soc"), Some("soc")); + assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&COVERAGEID="), None); + assert_eq!(coverage_id("https://h/wcs"), None); + } + + /// A saved project keeps a tile template without its key: the row still finds it. + #[test] + fn a_keyed_tile_layer_is_known_after_a_reopen() { + let offer = "https://h/wms?layers=a&api_key=K&bbox={bbox-epsg-3857}"; + let saved = crate::sources::redact(offer); + assert!(!saved.contains("api_key"), "{saved}"); + let map = tiles(&["https://elsewhere/{z}/{x}/{y}.png", &saved]); + let listed = [json!({"title": "A", "offer": {"type": "tiles", "url": offer}})]; + assert_eq!(added(&map, &listed, &[0]), [Some(("map.removeTiles", json!({"index": 1})))]); + } + + /// Two copies of one service under different keys: each row takes its own copy first. + #[test] + fn the_exact_address_wins_over_the_keyless_one() { + let mut map = Map::new("Map"); + for (id, key) in [(7, "A"), (8, "B")] { + let mut ds = rustgis_data::Dataset::new("Items", rustgis_data::GeometryType::Point, Vec::new()); + ds.source = Some(format!("https://h/items?f=json&api_key={key}")); + map.layers.push(rustgis_map::Layer::new(id, ds)); + } + let row = |key: &str| json!({"offer": {"type": "features", "url": format!("https://h/items?f=json&api_key={key}")}}); + let listed = [row("B"), row("A"), row("C")]; + let found = added(&map, &listed, &[0, 1, 2]); + assert_eq!(found[0], Some(("layer.remove", json!({"layer": 8})))); + assert_eq!(found[1], Some(("layer.remove", json!({"layer": 7})))); + // A key no copy has: the first copy without its key, as after a reopen. + assert_eq!(found[2], Some(("layer.remove", json!({"layer": 7})))); + } + + /// An OGC API row's advertised tiles: claimed under its tileset list alone, counting the rows + /// a filter hides too, so a row never takes another's tiles. + #[test] + fn a_tileset_is_claimed_only_under_one_list() { + let map = tiles(&["https://h/tiles/B/{z}/{x}/{y}.png", "https://h/c/tiles/WebMercatorQuad/{z}/{y}/{x}"]); + let row = |id: &str, list: &str| json!({"offer": {"type": "tiles", "url": format!("https://h/offer/{id}")}, "tilesets": list}); + let listed = [row("a", "https://h/tiles"), row("b", "https://h/tiles"), row("c", "https://h/c/tiles?f=json")]; + // Rows a and b share a list: neither takes its tiles, even with b filtered out. + assert_eq!(added(&map, &listed, &[0]), [None, None, None]); + assert_eq!(added(&map, &listed, &[0, 1, 2]), [None, None, Some(("map.removeTiles", json!({"index": 1})))]); + } +} diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index f35b70da1..7458bbd07 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -733,9 +733,9 @@ fn matching_rows<'a>(layers: &'a [Value], filter: &str) -> Vec<(usize, &'a Value .collect() } -/// Which of the rows shown (the first [`SHOWN_LAYERS`] matching the filter) are on the map -/// ([`added_from_listing`], by their place in the listing), worked out again only when the -/// session, the listing or the filter changed, not on every frame. +/// Which of the rows shown (the first [`SHOWN_LAYERS`] matching the filter) are on the map, by +/// their place in the listing (`service_added::added`), worked out again only when the session, +/// the listing or the filter changed, not on every frame. fn refresh_added(app: &mut RustGisApp) { let revision = app.session.revision; let Some(conn) = app.ui.service.as_mut() else { return }; @@ -743,139 +743,13 @@ fn refresh_added(app: &mut RustGisApp) { if conn.added.as_ref().is_some_and(|(r, f, _)| *r == revision && f == filter) { return; } - let map = app.session.map(); - // Each layer's source as it is and without its keys, once: a project saved and opened again - // keeps its addresses without them (the listing's still carry them). - let sources: Vec> = - map.layers.iter().filter_map(|l| l.dataset.source.as_deref().map(|s| (l.id, s, rustgis_engine::sources::redact(s)))).collect(); let none = Vec::new(); let layers = conn.listing.as_ref().and_then(|v| v.get("layers")).and_then(Value::as_array).unwrap_or(&none); - let shown: Vec<(usize, &Value)> = matching_rows(layers, filter).into_iter().take(SHOWN_LAYERS).collect(); - let mut added = vec![None; layers.len()]; - for &(i, l) in &shown { - if let Some(slot) = added.get_mut(i) { - *slot = added_from_listing(map, &sources, l); - } - } - claim_tilesets(map, &shown, &mut added); + let shown: Vec = matching_rows(layers, filter).into_iter().take(SHOWN_LAYERS).map(|(i, _)| i).collect(); + let added = rustgis_engine::cmd::service_added::added(app.session.map(), layers, &shown); conn.added = Some((revision, filter.clone(), added)); } -/// Gives the OGC API rows shown (`shown`) that `added` doesn't yet match the tile layer their -/// tileset list advertised: its template isn't in the listing, so it is found under that list. -fn claim_tilesets(map: &rustgis_engine::map::Map, shown: &[(usize, &Value)], added: &mut [Option<(&'static str, Value)>]) { - // An OGC API layer's tiles use the template its tileset list advertises, which the listing - // doesn't give: a tile layer under one row's list alone, that no row matched by its own - // template, is that row's. One under several rows' lists is nobody's, so Remove never takes - // another row's tiles. - let mut claimed: Vec = added - .iter() - .flatten() - .filter(|(command, _)| *command == "map.removeTiles") - .filter_map(|(_, p)| p.get("index").and_then(Value::as_u64)) - .collect(); - let lists: Vec<(usize, &str)> = shown.iter().filter_map(|&(i, l)| tileset_list(l).map(|list| (i, list))).collect(); - // How many rows' lists each tile layer is under, once. - let under: Vec = map.tiles.iter().map(|t| lists.iter().filter(|(_, list)| under_list(list, &t.url)).count()).collect(); - for &(i, list) in &lists { - if added.get(i).is_none_or(Option::is_some) { - continue; - } - let found = map - .tiles - .iter() - .zip(&under) - .zip(0u64..) - .find(|((t, under), n)| **under == 1 && !claimed.contains(n) && under_list(list, &t.url)) - .map(|(_, n)| n); - if let (Some(n), Some(slot)) = (found, added.get_mut(i)) { - *slot = Some(("map.removeTiles", json!({"index": n}))); - claimed.push(n); - } - } -} - -/// The command that removes a listed service layer already on `map`, if it is there. A layer is -/// matched by the address the listing offers it at (what `service.add` stores), never by its name -/// alone, so a file's layer or another service's that shares the title is left alone. -fn added_from_listing(map: &rustgis_engine::map::Map, sources: &[ListedSource<'_>], l: &Value) -> Option<(&'static str, Value)> { - let offer = l.get("offer")?; - let url = offer.get("url").and_then(Value::as_str).filter(|u| !u.is_empty())?; - let keyless = rustgis_engine::sources::redact(url); - let remove_command = |matches: &dyn Fn(&str, &str) -> bool| { - sources - .iter() - .find(|(_, s, redacted)| matches(url, s) || matches(&keyless, redacted)) - .map(|(id, ..)| ("layer.remove", json!({"layer": id}))) - }; - match offer.get("type").and_then(Value::as_str) { - // Downloaded features keep the address they came from (`service.mark_service_source`; - // an area only adds a query to it). - Some("features") => remove_command(&from_address), - // A coverage keeps its GetCoverage request: the service's address, which its other - // coverages share, with this coverage's id. - Some("coverage") => { - let id = offer.get("describe").and_then(Value::as_str).and_then(coverage_id)?; - remove_command(&|url, s| from_address(url, s) && coverage_id(s) == Some(id)) - } - // By position: a service's tile layers have no catalog id to tell them apart. Matched by - // the listed template here; an OGC API layer's advertised one, by `refresh_added`. - // A saved project keeps its tile templates without their keys, too. - _ => map - .tiles - .iter() - .position(|t| template_matches(url, &t.url) || template_matches(&keyless, &t.url)) - .map(|i| ("map.removeTiles", json!({"index": i}))), - } -} - -/// A map layer's id and source, as it is and without its keys. -type ListedSource<'a> = (u32, &'a str, String); - -/// Whether the tile template `url` is under the tileset list address `list` (its tilesets' paths). -fn under_list(list: &str, url: &str) -> bool { - url.strip_prefix(list).is_some_and(|rest| rest.starts_with('/')) -} - -/// An OGC API layer's tileset list address, without its query. -fn tileset_list(l: &Value) -> Option<&str> { - l.get("tilesets").and_then(Value::as_str).and_then(|t| t.split(['?', '#']).next()).filter(|t| !t.is_empty()) -} - -/// Whether `source` is the address `url`, or `url` with a query or fragment after it (an area's -/// bounding box): never another address that only starts with the same letters (`…/items2` for -/// `…/items`), nor one further down its path (`…/items/2`, one feature added by hand). -fn from_address(url: &str, source: &str) -> bool { - source.strip_prefix(url).is_some_and(|rest| rest.is_empty() || url.ends_with(['?', '&']) || rest.starts_with(['?', '&', '#'])) -} - -/// A WCS request's coverage (`COVERAGEID`, or a WCS 1.0's `COVERAGE`), as its address gives it. -fn coverage_id(url: &str) -> Option<&str> { - let query = url.split('#').next()?.split_once('?')?.1; - query.split('&').find_map(|kv| { - let (k, v) = kv.split_once('=')?; - (k.eq_ignore_ascii_case("coverageid") || k.eq_ignore_ascii_case("coverage")).then_some(v).filter(|v| !v.is_empty()) - }) -} - -/// Whether `url` is `template` with its `{Time}` placeholders (a time-enabled layer's) filled in. -fn template_matches(template: &str, url: &str) -> bool { - let mut parts = template.split("{Time}"); - let Some(first) = parts.next() else { return false }; - let Some(mut rest) = url.strip_prefix(first) else { return false }; - // Each placeholder takes a date: one piece of the address, never a path or query. - let date = |time: &str| !time.is_empty() && !time.contains(['/', '?', '#']); - let mut parts = parts.peekable(); - while let Some(part) = parts.next() { - if parts.peek().is_none() { - return rest.strip_suffix(part).is_some_and(date); - } - let Some(at) = rest.find(part).filter(|&at| part.is_empty() || rest.get(..at).is_some_and(date)) else { return false }; - rest = rest.get(at + part.len()..).unwrap_or(""); - } - rest.is_empty() -} - /// Add From Service: a WMS, WMTS, WFS, WCS, OGC API Features or ArcGIS REST service by its address /// (or a catalogue searched for them, `cswui`). /// Connect fetches its capabilities in the background; its layers are then listed, each with an @@ -2108,70 +1982,3 @@ mod tests { assert_eq!(format_degrees(104.25), "104.25°"); } } - -#[cfg(test)] -mod service_listing_tests { - use super::{Session, coverage_id, from_address, template_matches}; - use serde_json::json; - - #[test] - fn a_template_matches_only_its_own_address() { - let wms = "http://h/wms?layers=a&bbox={bbox-epsg-3857}"; - assert!(template_matches(wms, wms)); - assert!(!template_matches(wms, "http://h/wms?layers=b&bbox={bbox-epsg-3857}")); - assert!(!template_matches("", "http://h/x")); - let timed = "http://h/{Time}/{z}/{x}/{y}.png"; - assert!(template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.png")); - assert!(!template_matches(timed, "http://other/2024-06-01/{z}/{x}/{y}.png")); - assert!(!template_matches(timed, "http://h/2024-06-01/{z}/{x}/{y}.jpg")); - // A trailing placeholder: a date, not every address under the prefix. - assert!(template_matches("http://h/tiles?time={Time}", "http://h/tiles?time=2024-06-01")); - assert!(!template_matches("http://h/{Time}", "http://h/other/{z}/{x}/{y}.png")); - assert!(!template_matches("http://h/{Time}", "http://h/")); - // A middle one: one piece of the path, not several. - assert!(!template_matches(timed, "http://h/a/b/{z}/{x}/{y}.png")); - assert!(!template_matches("http://h/{Time}/x/{Time}.png", "http://h/a/b/x/c.png")); - assert!(template_matches("http://h/{Time}/x/{Time}.png", "http://h/2024/x/2025.png")); - } - - #[test] - fn an_address_matches_only_itself_and_what_follows_it() { - let items = "https://h/items"; - assert!(from_address(items, items)); - assert!(from_address(items, "https://h/items?bbox=1,2,3,4")); - assert!(!from_address(items, "https://h/items/2"), "one feature further down the path"); - assert!(from_address(items, "https://h/items#top")); - assert!(!from_address(items, "https://h/items2"), "another service's address sharing the prefix"); - assert!(!from_address(items, "https://h/item")); - assert!(!from_address(items, "wfs.geojson")); - // An address ending in its query's start: what follows is that query. - assert!(from_address("https://h/wcs?", "https://h/wcs?SERVICE=WCS")); - // One ending in `/` is a path, not a prefix of every address under it. - assert!(!from_address("https://h/", "https://h/other")); - assert!(from_address("https://h/", "https://h/?bbox=1,2,3,4")); - } - - #[test] - fn a_keyed_tile_layer_is_known_after_a_reopen() { - use rustgis_engine::map::tiles::{TileKind, TileLayer}; - let offer = "https://h/wms?layers=a&api_key=K&bbox={bbox-epsg-3857}"; - let mut s = Session::new(rustgis_engine::Project::default()); - // A saved project keeps the template without its key. - let saved = rustgis_engine::sources::redact(offer); - assert!(!saved.contains("api_key"), "{saved}"); - s.map_mut().tiles.push(TileLayer::new("A", TileKind::Raster, saved)); - let index = s.map().tiles.len() - 1; - let listed = json!({"title": "A", "offer": {"type": "tiles", "url": offer}}); - let found = super::added_from_listing(s.map(), &[], &listed); - assert_eq!(found, Some(("map.removeTiles", json!({"index": index})))); - } - - #[test] - fn a_coverage_is_told_by_its_id() { - let v2 = "https://h/wcs?SERVICE=WCS&VERSION=2.0.1&REQUEST=GetCoverage&COVERAGEID=dem&FORMAT=image%2Ftiff"; - assert_eq!(coverage_id(v2), Some("dem")); - assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&VERSION=1.0.0&REQUEST=DescribeCoverage&COVERAGE=soc"), Some("soc")); - assert_eq!(coverage_id("https://h/wcs?SERVICE=WCS&COVERAGEID="), None); - assert_eq!(coverage_id("https://h/wcs"), None); - } -}