From 3aadd86a053f9c56fa7defdb24d48176d75e8e34 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Fri, 23 Oct 2026 22:15:49 +0200 Subject: [PATCH 1/4] fix(symbology): choose compatible renderer fields --- crates/engine/src/cmd/layer.rs | 21 ++++++++++++----- crates/engine/src/tests.rs | 42 ++++++++++++++++++++++++++++++++++ crates/ui-egui/src/panes.rs | 32 +++++++++++++++++++++++++- docs/user-guide/cartography.md | 13 +++++++---- 4 files changed, 97 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index 7d98fe36a..a2e38bedd 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -2598,15 +2598,24 @@ fn symbology(s: &mut Session, p: &Value) -> CmdResult { } else { base }; - let field = || -> Result { + let field = |numeric_only: bool| -> Result { if let Some(x) = ps(p, "field") { return Ok(x.to_string()); } l.renderer .field() + .filter(|name| { + !numeric_only || l.dataset.field_index(name).and_then(|i| l.dataset.fields.get(i)).is_some_and(|f| f.ty.is_numeric()) + }) .map(str::to_string) - .or_else(|| l.dataset.fields.iter().find(|f| f.ty.is_numeric()).map(|f| f.name.clone())) - .ok_or_else(|| "field is required".into()) + .or_else(|| l.dataset.fields.iter().find(|f| !numeric_only || f.ty.is_numeric()).map(|f| f.name.clone())) + .ok_or_else(|| { + if numeric_only { + "no numeric field is available for this symbology".into() + } else { + "no field is available for this symbology".into() + } + }) }; // Classes and sizes describe the rows the definition query lets through, taken once: changing // the query later keeps them until the symbology is applied again. @@ -2619,15 +2628,15 @@ fn symbology(s: &mut Session, p: &Value) -> CmdResult { }; let mut r = match kind.as_str() { "single" | "singlesymbol" => Renderer::Single(base), - "unique" | "uniquevalues" => Renderer::unique(&l.dataset, &rows()?, &field()?, ramp.unwrap_or(Ramp::Harbour), base)?, + "unique" | "uniquevalues" => Renderer::unique(&l.dataset, &rows()?, &field(false)?, ramp.unwrap_or(Ramp::Harbour), base)?, "graduated" | "graduatedcolors" => { - Renderer::graduated(&l.dataset, &rows()?, &field()?, classes, method, ramp.unwrap_or(Ramp::Ember), base)? + Renderer::graduated(&l.dataset, &rows()?, &field(true)?, classes, method, ramp.unwrap_or(Ramp::Ember), base)? } "proportional" | "proportionalsymbols" => { if g != GeometryType::Point { return Err("Proportional Symbols needs a point layer".into()); } - Renderer::proportional(&l.dataset, &rows()?, &field()?, base)? + Renderer::proportional(&l.dataset, &rows()?, &field(true)?, base)? } other => return Err(format!("unknown symbology '{other}'")), }; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 0a44d4a44..1e995ab26 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1994,6 +1994,48 @@ fn terrain_is_set_from_a_dem_or_tiles() { assert!(s.map().view.terrain.is_none()); } +#[test] +fn graduated_symbology_skips_a_current_text_field() { + let mut s = world(); + s.run("layer.symbology", &json!({"layer": "Countries", "type": "unique", "field": "income_grp"})).unwrap(); + + s.run("layer.symbology", &json!({"layer": "Countries", "type": "graduated"})).unwrap(); + let layer = s.map().layer_by_name("Countries").unwrap(); + let rustgis_map::Renderer::Graduated { field, .. } = &layer.renderer else { panic!("Graduated Colors renderer expected") }; + assert_ne!(field, "income_grp", "the text field from the previous renderer is not reused"); + assert!(layer.dataset.field_index(field).and_then(|i| layer.dataset.fields.get(i)).is_some_and(|f| f.ty.is_numeric())); +} + +#[test] +fn symbology_chooses_fields_by_renderer_requirements() { + let mut s = Session::new(Project::default()); + s.run( + "layer.addDataText", + &json!({ + "fileName": "text-fields.geojson", + "text": r#"{"type":"FeatureCollection","features":[ + {"type":"Feature","properties":{"g_id":"61259","g_name":"Liezen"},"geometry":{"type":"Point","coordinates":[14.24,47.57]}}, + {"type":"Feature","properties":{"g_id":"61260","g_name":"Graz"},"geometry":{"type":"Point","coordinates":[15.44,47.07]}} + ]}"# + }), + ) + .unwrap(); + + s.run("layer.symbology", &json!({"layer": "text-fields", "type": "unique"})).unwrap(); + let rustgis_map::Renderer::Unique { field, classes, .. } = &s.map().layer_by_name("text-fields").unwrap().renderer else { + panic!("Unique Values renderer expected") + }; + assert_eq!(field, "g_id"); + assert_eq!(classes.len(), 2); + + let before = s.map().layer_by_name("text-fields").unwrap().renderer.clone(); + let depth = s.undo_depth(); + let err = s.run("layer.symbology", &json!({"layer": "text-fields", "type": "graduated"})).unwrap_err(); + assert_eq!(err, "no numeric field is available for this symbology"); + assert_eq!(s.map().layer_by_name("text-fields").unwrap().renderer, before, "failed mode switch leaves current styling intact"); + assert_eq!(s.undo_depth(), depth, "failed mode switch adds no undo step"); +} + #[test] fn symbology_classes_follow_the_definition_query() { let mut s = world(); diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index 59f7d0401..09863a596 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -3323,6 +3323,7 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { }; let sel = cur_kind == *label; let (r, resp) = ui.allocate_exact_size(vec2(68.0, 56.0), Sense::click()); + resp.widget_info(|| egui::WidgetInfo::selected(egui::WidgetType::Button, true, sel, tl!(*label))); ui.painter().rect_filled( r, CornerRadius::same(6), @@ -3350,7 +3351,11 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { let short = tl!(label).split(' ').next().unwrap_or(label); ui.painter().text(pos2(r.center().x, r.max.y - 11.0), Align2::CENTER_CENTER, short, regular(11.0), t.text); if resp.on_hover_text(tl!(*label)).clicked() && !sel { - let _ = app.run("layer.symbology", json!({"type": code})); + if app.run("layer.symbology", json!({"type": code})).is_err() { + ui.data_mut(|data| data.insert_temp(egui::Id::new("symbology_pending_field"), (l.id, (*code).to_string()))); + } else { + ui.data_mut(|data| data.remove::<(u32, String)>(egui::Id::new("symbology_pending_field"))); + } } } }); @@ -3363,6 +3368,31 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { } let numeric: Vec = l.dataset.fields.iter().filter(|f| f.ty.is_numeric()).map(|f| f.name.clone()).collect(); let all: Vec = l.dataset.fields.iter().map(|f| f.name.clone()).collect(); + let pending_id = egui::Id::new("symbology_pending_field"); + let pending = ui.data(|data| data.get_temp::<(u32, String)>(pending_id)).filter(|(layer, _)| *layer == l.id); + if let Some((_, kind)) = pending { + let fields = if kind == "unique" { &all } else { &numeric }; + ui.horizontal(|ui| { + ui.label(tl!("Choose a field to apply this renderer:")); + egui::ComboBox::from_id_salt("symbology_pending_field_choice").selected_text(tl!("Select a field")).show_ui(ui, |ui| { + for field in fields { + if ui.selectable_label(false, field).clicked() + && app.run("layer.symbology", json!({"type": kind, "field": field})).is_ok() + { + ui.data_mut(|data| data.remove::<(u32, String)>(pending_id)); + } + } + }); + }); + if fields.is_empty() { + if kind == "unique" { + ui.label(tl!("No field is available for this symbology.")); + } else { + ui.label(tl!("No numeric field is available for this symbology.")); + } + } + ui.add_space(6.0); + } match &l.renderer { Renderer::Single(s) => symbol_editor(app, ui, *s, g, t), Renderer::Unique { field, classes, ramp, .. } => { diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 12222f6e8..67b94eb44 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -10,14 +10,19 @@ Select a layer in Layers and open **Style › Symbology** (or the Symbology pane): - **Single Symbol**: one fill, outline and marker for every feature. -- **Unique Values**: one colour per distinct value of a field. The Layers pane and layout legends - list only the classes that drawn features use (after the definition query and display filters); +- **Unique Values**: one colour per distinct value of a field. If no field is chosen, the first + available field is used, including text fields. The Layers pane and layout legends list only the + classes that drawn features use (after the definition query and display filters); `` appears only when a drawn feature falls outside the classes and its symbol is not fully transparent. A layout legend can leave it out with **Hide other values** (see [Layouts](layouts.md)). - **Graduated Colors**: 1 to 12 classes of a numeric field by natural breaks (Jenks), equal - interval or quantile. -- **Proportional Symbols**: point size grows with a numeric field. + interval or quantile. If no field is chosen, a numeric field is selected automatically; if none + exists, the pane reports that a numeric field is required and keeps the current renderer. When + a switch fails and candidate fields exist, the pane offers a field picker without discarding the + current renderer. +- **Proportional Symbols**: point size grows with a numeric field; the field is selected from the + available numeric fields. Pick a colour scheme from the pane's swatches, listed in groups: **Perceptually Uniform** (Viridis, Plasma, Inferno, Magma, Cividis, Batlow and Aurora: even steps in lightness, so they read in From 0aed56caa8b781deaec488c1c37c94db5f2cc45c Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Fri, 23 Oct 2026 22:24:25 +0200 Subject: [PATCH 2/4] fix(ui): clear pending renderer when layer changes --- crates/ui-egui/src/panes.rs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index 09863a596..1f5888719 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -3290,6 +3290,10 @@ fn ramp_swatch(ui: &mut Ui, ramp: &Ramp, w: f32, selected: bool, t: &Tokens) -> /// Shows the active layer's renderer controls, including No Fill for polygon layers. fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { + let pending_id = egui::Id::new("symbology_pending_field"); + if ui.data(|data| data.get_temp::<(u32, String)>(pending_id)).is_some_and(|(layer, _)| Some(layer) != app.session.active_layer) { + ui.data_mut(|data| data.remove::<(u32, String)>(pending_id)); + } let Some(l) = app.session.active_layer.and_then(|id| app.session.map().layer(id)).cloned() else { ui.label(egui::RichText::new(tl!("Select a layer in the Layers pane.")).color(t.text_dim)); return; @@ -3368,7 +3372,6 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { } let numeric: Vec = l.dataset.fields.iter().filter(|f| f.ty.is_numeric()).map(|f| f.name.clone()).collect(); let all: Vec = l.dataset.fields.iter().map(|f| f.name.clone()).collect(); - let pending_id = egui::Id::new("symbology_pending_field"); let pending = ui.data(|data| data.get_temp::<(u32, String)>(pending_id)).filter(|(layer, _)| *layer == l.id); if let Some((_, kind)) = pending { let fields = if kind == "unique" { &all } else { &numeric }; @@ -4991,6 +4994,31 @@ mod tests { assert_eq!((app.ui.layer_filter.as_str(), app.ui.layer_filter_hid), ("", None)); } + /// A pending renderer choice belongs only to the layer where the switch failed. + #[test] + fn pending_symbology_choice_clears_when_active_layer_changes() { + let mut app = RustGisApp::with_sample(); + let (countries, cities) = (layer(&app, "Countries"), layer(&app, "Cities")); + app.session.active_layer = Some(countries); + app.ui.open_right(RightPane::Symbology); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1400.0, 900.0)).with_max_steps(64).build_ui_state( + |ui, app: &mut RustGisApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + app, + ); + let pending_id = egui::Id::new("symbology_pending_field"); + h.ctx.data_mut(|data| data.insert_temp(pending_id, (countries, "graduated".to_owned()))); + h.step(); + assert!(h.ctx.data(|data| data.get_temp::<(u32, String)>(pending_id).is_some())); + + h.state_mut().session.active_layer = Some(cities); + h.step(); + assert!(h.ctx.data(|data| data.get_temp::<(u32, String)>(pending_id).is_none())); + } + /// The Symbology pane's Transparency slider changes the layer while it's dragged, not only /// when it's let go, and the whole drag is one undo step back to where it started. #[test] From c275458fcb36742076d046bfd8e8a01a787e08f7 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Fri, 23 Oct 2026 23:30:06 +0200 Subject: [PATCH 3/4] fix(ui): keep symbology retries scoped and explained --- crates/ui-egui/src/lib.rs | 13 +- crates/ui-egui/src/panes.rs | 270 ++++++++++++++++++++++++++++----- docs/user-guide/cartography.md | 11 +- 3 files changed, 244 insertions(+), 50 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1fe308d02..8314062bb 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -760,9 +760,7 @@ pub struct RustGisApp { /// A screen snapshot an agent attached through the relay asked for, while it is captured. #[cfg(not(target_arch = "wasm32"))] relay_shot: Option, - /// The frame's `egui::Context`, kept so `ui.mcpRelay` can hand the relay its wake callback: - /// a queued message asks for a repaint, or it waits for the user to move the mouse. - #[cfg(not(target_arch = "wasm32"))] + /// The frame's context, for pane-local command cleanup and the native relay's wake callback. frame_ctx: Option, /// Set when `ui.mcpRelay` started the relay before the first frame, where there is no context /// to give its wake callback yet: `logic` installs the wake as soon as a context exists. @@ -873,7 +871,6 @@ impl RustGisApp { relay_shot: None, #[cfg(not(target_arch = "wasm32"))] connect: Default::default(), - #[cfg(not(target_arch = "wasm32"))] frame_ctx: None, #[cfg(not(target_arch = "wasm32"))] relay_wake_pending: false, @@ -1433,6 +1430,11 @@ impl RustGisApp { if let Err(e) = &r { log::info!("{id}: {e}"); } + if r.is_ok() + && let Some(ctx) = &self.frame_ctx + { + panes::symbology_command_finished(self, ctx, id); + } // A project replacing this one hasn't been saved in the browser yet (opening a file // names it again). if r.is_ok() && matches!(id, "project.new" | "project.sample" | "project.open" | "project.openText" | "project.fromTemplate") { @@ -3177,11 +3179,11 @@ impl RustGisApp { /// Per-frame work that doesn't draw (theme, fonts, files that arrived). pub fn logic(&mut self, ctx: &egui::Context) { + self.frame_ctx = Some(ctx.clone()); // The MCP relay: what attached agents sent runs here, on this session, before the frame // draws (Session lives on this thread; the listener only queues messages). #[cfg(not(target_arch = "wasm32"))] { - self.frame_ctx = Some(ctx.clone()); // Agents the user turned on last time: the relay starts with the app. connectui::resume(self); if let Some(relay) = &self.relay { @@ -3215,6 +3217,7 @@ impl RustGisApp { } // A project an agent or a script opened, before the frame draws the old one's fields. layoutview::follow_project(self); + panes::follow_symbology(self, ctx); // Fonts installed last frame are active now. self.fonts_pending = false; i18n::set_current(self.lang()); diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index 1f5888719..ecba78043 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -3288,12 +3288,62 @@ fn ramp_swatch(ui: &mut Ui, ramp: &Ramp, w: f32, selected: bool, t: &Tokens) -> resp } +const PENDING_SYMBOLOGY: &str = "symbology_pending_field"; + +#[derive(Clone)] +struct PendingSymbology { + project: u64, + map: usize, + layer: u32, + kind: &'static str, + error: std::sync::Arc, +} + +fn clear_pending_symbology(ctx: &egui::Context) { + ctx.data_mut(|data| data.remove::(egui::Id::new(PENDING_SYMBOLOGY))); +} + +/// Ends a pane-local retry when its project, map or active layer is no longer in view, +/// including while the Symbology pane is closed. +pub(crate) fn follow_symbology(app: &RustGisApp, ctx: &egui::Context) { + let stale = ctx.data(|data| { + data.get_temp::(egui::Id::new(PENDING_SYMBOLOGY)).is_some_and(|pending| { + pending.project != app.session.opened + || pending.map != app.session.project.active_map + || Some(pending.layer) != app.session.active_layer + }) + }); + if stale { + clear_pending_symbology(ctx); + } +} + +/// A successful style edit supersedes a failed switch, including edits from the ribbon. +pub(crate) fn symbology_command_finished(app: &RustGisApp, ctx: &egui::Context, command: &str) { + follow_symbology(app, ctx); + if matches!( + command, + "layer.symbology" + | "layer.ramp" + | "layer.classStyle" + | "layer.color" + | "layer.noFill" + | "layer.outline" + | "layer.marker" + | "layer.lineStrokes" + | "style.apply" + | "map.remove" + | "edit.undo" + | "edit.redo" + ) { + clear_pending_symbology(ctx); + } +} + /// Shows the active layer's renderer controls, including No Fill for polygon layers. fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { - let pending_id = egui::Id::new("symbology_pending_field"); - if ui.data(|data| data.get_temp::<(u32, String)>(pending_id)).is_some_and(|(layer, _)| Some(layer) != app.session.active_layer) { - ui.data_mut(|data| data.remove::<(u32, String)>(pending_id)); - } + follow_symbology(app, ui.ctx()); + let pending_id = egui::Id::new(PENDING_SYMBOLOGY); let Some(l) = app.session.active_layer.and_then(|id| app.session.map().layer(id)).cloned() else { ui.label(egui::RichText::new(tl!("Select a layer in the Layers pane.")).color(t.text_dim)); return; @@ -3355,10 +3405,18 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { let short = tl!(label).split(' ').next().unwrap_or(label); ui.painter().text(pos2(r.center().x, r.max.y - 11.0), Align2::CENTER_CENTER, short, regular(11.0), t.text); if resp.on_hover_text(tl!(*label)).clicked() && !sel { - if app.run("layer.symbology", json!({"type": code})).is_err() { - ui.data_mut(|data| data.insert_temp(egui::Id::new("symbology_pending_field"), (l.id, (*code).to_string()))); - } else { - ui.data_mut(|data| data.remove::<(u32, String)>(egui::Id::new("symbology_pending_field"))); + match app.run("layer.symbology", json!({"type": code})) { + Err(error) => { + let pending = PendingSymbology { + project: app.session.opened, + map: app.session.project.active_map, + layer: l.id, + kind: code, + error: error.into(), + }; + ui.data_mut(|data| data.insert_temp(pending_id, pending)); + } + Ok(_) => clear_pending_symbology(ui.ctx()), } } } @@ -3372,27 +3430,27 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { } let numeric: Vec = l.dataset.fields.iter().filter(|f| f.ty.is_numeric()).map(|f| f.name.clone()).collect(); let all: Vec = l.dataset.fields.iter().map(|f| f.name.clone()).collect(); - let pending = ui.data(|data| data.get_temp::<(u32, String)>(pending_id)).filter(|(layer, _)| *layer == l.id); - if let Some((_, kind)) = pending { - let fields = if kind == "unique" { &all } else { &numeric }; - ui.horizontal(|ui| { - ui.label(tl!("Choose a field to apply this renderer:")); - egui::ComboBox::from_id_salt("symbology_pending_field_choice").selected_text(tl!("Select a field")).show_ui(ui, |ui| { - for field in fields { - if ui.selectable_label(false, field).clicked() - && app.run("layer.symbology", json!({"type": kind, "field": field})).is_ok() - { - ui.data_mut(|data| data.remove::<(u32, String)>(pending_id)); + if let Some(pending) = ui.data(|data| data.get_temp::(pending_id)) { + ui.label(egui::RichText::new(pending.error.as_ref()).color(ui.visuals().error_fg_color)); + let fields = if pending.kind == "unique" { &all } else { &numeric }; + if !fields.is_empty() { + ui.horizontal(|ui| { + ui.label(tl!("Choose a field to apply this renderer:")); + egui::ComboBox::from_id_salt("symbology_pending_field_choice").selected_text(tl!("Select a field")).show_ui(ui, |ui| { + for field in fields { + if ui.selectable_label(false, field).clicked() { + match app.run("layer.symbology", json!({"type": pending.kind, "field": field})) { + Ok(_) => clear_pending_symbology(ui.ctx()), + Err(error) => { + ui.data_mut(|data| { + data.insert_temp(pending_id, PendingSymbology { error: error.into(), ..pending.clone() }) + }); + } + } + } } - } + }); }); - }); - if fields.is_empty() { - if kind == "unique" { - ui.label(tl!("No field is available for this symbology.")); - } else { - ui.label(tl!("No numeric field is available for this symbology.")); - } } ui.add_space(6.0); } @@ -4994,12 +5052,21 @@ mod tests { assert_eq!((app.ui.layer_filter.as_str(), app.ui.layer_filter_hid), ("", None)); } - /// A pending renderer choice belongs only to the layer where the switch failed. - #[test] - fn pending_symbology_choice_clears_when_active_layer_changes() { - let mut app = RustGisApp::with_sample(); - let (countries, cities) = (layer(&app, "Countries"), layer(&app, "Cities")); - app.session.active_layer = Some(countries); + fn symbology_retry_harness() -> egui_kittest::Harness<'static, RustGisApp> { + let mut app = RustGisApp::new(Session::new(rustgis_engine::Project::default())); + app.run( + "layer.addDataText", + json!({ + "fileName": "retry.geojson", + "text": r#"{"type":"FeatureCollection","features":[ + {"type":"Feature","properties":{"a_empty":null,"b_empty":null,"c_valid":10},"geometry":{"type":"Point","coordinates":[14,47]}}, + {"type":"Feature","properties":{"a_empty":null,"b_empty":null,"c_valid":20},"geometry":{"type":"Point","coordinates":[15,48]}}, + {"type":"Feature","properties":{"a_empty":100,"b_empty":200,"c_valid":30},"geometry":{"type":"Point","coordinates":[16,49]}} + ]}"# + }), + ) + .unwrap(); + app.run("layer.definitionQuery", json!({"where": "c_valid < 30"})).unwrap(); app.ui.open_right(RightPane::Symbology); let mut h = egui_kittest::Harness::builder().with_size(vec2(1400.0, 900.0)).with_max_steps(64).build_ui_state( |ui, app: &mut RustGisApp| { @@ -5009,14 +5076,137 @@ mod tests { }, app, ); - let pending_id = egui::Id::new("symbology_pending_field"); - h.ctx.data_mut(|data| data.insert_temp(pending_id, (countries, "graduated".to_owned()))); - h.step(); - assert!(h.ctx.data(|data| data.get_temp::<(u32, String)>(pending_id).is_some())); + h.run_steps(6); + h + } - h.state_mut().session.active_layer = Some(cities); - h.step(); - assert!(h.ctx.data(|data| data.get_temp::<(u32, String)>(pending_id).is_none())); + /// A different populated field fixes classification without changing the query or styling on failure. + #[test] + fn failed_symbology_switch_retries_another_numeric_field() { + use egui_kittest::kittest::Queryable; + let mut h = symbology_retry_harness(); + let id = layer(h.state(), "retry"); + let before = h.state().session.map().layer(id).unwrap().renderer.clone(); + let undo = h.state().session.undo_depth(); + h.get_by_label("Graduated Colors").click(); + h.run_steps(3); + assert_eq!(h.state().session.map().layer(id).unwrap().renderer, before); + assert_eq!(h.state().session.undo_depth(), undo); + assert!(h.query_all_by_label("'a_empty' has no numeric values").next().is_some()); + + for field in ["b_empty", "c_valid"] { + h.query_all_by_role(egui::accesskit::Role::ComboBox).find(|n| n.value().as_deref() == Some("Select a field")).unwrap().click(); + h.run_steps(3); + h.get_by_label(field).click(); + h.run_steps(3); + if field == "b_empty" { + assert_eq!(h.state().session.map().layer(id).unwrap().renderer, before); + assert_eq!(h.state().session.undo_depth(), undo); + assert!(h.query_all_by_label("'b_empty' has no numeric values").next().is_some()); + } + } + let Renderer::Graduated { field, breaks, .. } = &h.state().session.map().layer(id).unwrap().renderer else { + panic!("the populated field must apply Graduated Colors"); + }; + assert_eq!(field, "c_valid"); + assert_eq!(breaks.last(), Some(&20.0), "only the rows kept by the query are classified"); + assert_eq!(h.state().session.undo_depth(), undo + 1); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_none()); + } + + /// No numeric candidates means an explanation, not an empty field selector. + #[test] + fn failed_symbology_without_numeric_fields_has_no_empty_picker() { + use egui_kittest::kittest::Queryable; + let mut h = symbology_retry_harness(); + h.state_mut() + .run( + "layer.addDataText", + json!({ + "fileName": "text-only.geojson", + "text": r#"{"type":"Feature","properties":{"name":"Liezen"},"geometry":{"type":"Point","coordinates":[14,47]}}"# + }), + ) + .unwrap(); + h.run_steps(3); + h.get_by_label("Graduated Colors").click(); + h.run_steps(3); + assert!(h.query_all_by_label("no numeric field is available for this symbology").next().is_some()); + assert!( + h.query_all_by_role(egui::accesskit::Role::ComboBox).all(|n| n.value().as_deref() != Some("Select a field")), + "an empty picker cannot help choose a numeric field" + ); + let id = layer(h.state(), "text-only"); + assert!(matches!(h.state().session.map().layer(id).unwrap().renderer, Renderer::Single(_))); + } + + /// A closed pane must not retain a failed request through a layer or map round trip. + #[test] + fn pending_symbology_clears_across_closed_pane_context_changes() { + use egui_kittest::kittest::Queryable; + let mut h = symbology_retry_harness(); + let map = h.state().session.map().name.clone(); + let id = layer(h.state(), "retry"); + h.state_mut() + .run("layer.addDataText", json!({"fileName": "other.csv", "text": "x,y,name\n1,2,Other\n", "x": "x", "y": "y"})) + .unwrap(); + let other = layer(h.state(), "other"); + h.state_mut().run("map.new", json!({"name": "Other map"})).unwrap(); + h.state_mut() + .run("layer.addDataText", json!({"fileName": "collision.csv", "text": "x,y,name\n1,2,Other\n", "x": "x", "y": "y"})) + .unwrap(); + assert_eq!(layer(h.state(), "collision"), id, "layer ids overlap across maps"); + h.state_mut().run("map.activate", json!({"name": map})).unwrap(); + h.state_mut().run("layer.select", json!({"layer": id})).unwrap(); + h.run_steps(3); + + for map_change in [false, true] { + h.get_by_label("Graduated Colors").click(); + h.run_steps(3); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_some()); + h.state_mut().ui.right = None; + if map_change { + h.state_mut().run("map.activate", json!({"name": "Other map"})).unwrap(); + h.state_mut().run("map.activate", json!({"name": map})).unwrap(); + h.state_mut().run("layer.select", json!({"layer": id})).unwrap(); + } else { + h.state_mut().session.active_layer = Some(other); + h.run_steps(3); + h.state_mut().session.active_layer = Some(id); + } + h.state_mut().ui.open_right(RightPane::Symbology); + h.run_steps(3); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_none()); + assert!(matches!(h.state().session.map().layer(id).unwrap().renderer, Renderer::Single(_))); + } + } + + /// Both the existing field editor and commands from outside the pane supersede a failed switch. + #[test] + fn successful_symbology_edits_supersede_a_pending_switch() { + use egui_kittest::kittest::Queryable; + let mut h = symbology_retry_harness(); + let id = layer(h.state(), "retry"); + for through_field_editor in [true, false] { + h.state_mut().run("layer.symbology", json!({"type": "unique", "field": "a_empty"})).unwrap(); + h.run_steps(3); + h.get_by_label("Graduated Colors").click(); + h.run_steps(3); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_some()); + if through_field_editor { + h.query_all_by_role(egui::accesskit::Role::ComboBox).find(|n| n.value().as_deref() == Some("a_empty")).unwrap().click(); + h.run_steps(3); + h.get_by_label("c_valid").click(); + } else { + h.state_mut().run("layer.symbology", json!({"type": "unique", "field": "c_valid"})).unwrap(); + } + h.run_steps(3); + let Renderer::Unique { field, .. } = &h.state().session.map().layer(id).unwrap().renderer else { + panic!("the successful edit must keep Unique Values"); + }; + assert_eq!(field, "c_valid"); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_none()); + } } /// The Symbology pane's Transparency slider changes the layer while it's dragged, not only diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 67b94eb44..2a4e45956 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -10,17 +10,18 @@ Select a layer in Layers and open **Style › Symbology** (or the Symbology pane): - **Single Symbol**: one fill, outline and marker for every feature. -- **Unique Values**: one colour per distinct value of a field. If no field is chosen, the first - available field is used, including text fields. The Layers pane and layout legends list only the +- **Unique Values**: one colour per distinct value of a field. If no field is chosen, the current + renderer's field is kept when available; otherwise the first available field is used, including + text fields. The Layers pane and layout legends list only the classes that drawn features use (after the definition query and display filters); `` appears only when a drawn feature falls outside the classes and its symbol is not fully transparent. A layout legend can leave it out with **Hide other values** (see [Layouts](layouts.md)). - **Graduated Colors**: 1 to 12 classes of a numeric field by natural breaks (Jenks), equal interval or quantile. If no field is chosen, a numeric field is selected automatically; if none - exists, the pane reports that a numeric field is required and keeps the current renderer. When - a switch fails and candidate fields exist, the pane offers a field picker without discarding the - current renderer. + exists, the pane explains why the switch failed and keeps the current renderer. When candidate + fields exist, a field picker lets you try another field; the error remains visible if a retry + fails. Fix other reported problems, such as a definition query that keeps no features, before retrying. - **Proportional Symbols**: point size grows with a numeric field; the field is selected from the available numeric fields. From abd93e15ae5b54e4774e0cbb264e3c5a5aa26e60 Mon Sep 17 00:00:00 2001 From: Daniel Hofbauer Date: Fri, 23 Oct 2026 23:47:01 +0200 Subject: [PATCH 4/4] fix(ui): end symbology retries with the pane, layer or renderer --- crates/ui-egui/src/lib.rs | 13 ++-- crates/ui-egui/src/panes.rs | 121 ++++++++++++++++----------------- docs/user-guide/cartography.md | 13 ++-- 3 files changed, 72 insertions(+), 75 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8314062bb..1fe308d02 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -760,7 +760,9 @@ pub struct RustGisApp { /// A screen snapshot an agent attached through the relay asked for, while it is captured. #[cfg(not(target_arch = "wasm32"))] relay_shot: Option, - /// The frame's context, for pane-local command cleanup and the native relay's wake callback. + /// The frame's `egui::Context`, kept so `ui.mcpRelay` can hand the relay its wake callback: + /// a queued message asks for a repaint, or it waits for the user to move the mouse. + #[cfg(not(target_arch = "wasm32"))] frame_ctx: Option, /// Set when `ui.mcpRelay` started the relay before the first frame, where there is no context /// to give its wake callback yet: `logic` installs the wake as soon as a context exists. @@ -871,6 +873,7 @@ impl RustGisApp { relay_shot: None, #[cfg(not(target_arch = "wasm32"))] connect: Default::default(), + #[cfg(not(target_arch = "wasm32"))] frame_ctx: None, #[cfg(not(target_arch = "wasm32"))] relay_wake_pending: false, @@ -1430,11 +1433,6 @@ impl RustGisApp { if let Err(e) = &r { log::info!("{id}: {e}"); } - if r.is_ok() - && let Some(ctx) = &self.frame_ctx - { - panes::symbology_command_finished(self, ctx, id); - } // A project replacing this one hasn't been saved in the browser yet (opening a file // names it again). if r.is_ok() && matches!(id, "project.new" | "project.sample" | "project.open" | "project.openText" | "project.fromTemplate") { @@ -3179,11 +3177,11 @@ impl RustGisApp { /// Per-frame work that doesn't draw (theme, fonts, files that arrived). pub fn logic(&mut self, ctx: &egui::Context) { - self.frame_ctx = Some(ctx.clone()); // The MCP relay: what attached agents sent runs here, on this session, before the frame // draws (Session lives on this thread; the listener only queues messages). #[cfg(not(target_arch = "wasm32"))] { + self.frame_ctx = Some(ctx.clone()); // Agents the user turned on last time: the relay starts with the app. connectui::resume(self); if let Some(relay) = &self.relay { @@ -3217,7 +3215,6 @@ impl RustGisApp { } // A project an agent or a script opened, before the frame draws the old one's fields. layoutview::follow_project(self); - panes::follow_symbology(self, ctx); // Fonts installed last frame are active now. self.fonts_pending = false; i18n::set_current(self.lang()); diff --git a/crates/ui-egui/src/panes.rs b/crates/ui-egui/src/panes.rs index ecba78043..3a421d418 100644 --- a/crates/ui-egui/src/panes.rs +++ b/crates/ui-egui/src/panes.rs @@ -47,6 +47,12 @@ pub fn show(app: &mut RustGisApp, ui: &mut Ui) { if app.ui.right != Some(RightPane::TimeSeries) && app.workflow.pick.as_ref().is_some_and(|p| p.purpose == "ts_point") { app.workflow.pick = None; } + // A failed renderer switch waits for a field only while the Symbology pane shows its layer. + if app.ui.right == Some(RightPane::Symbology) { + let _ = pending_symbology(app, ui.ctx()); + } else { + clear_pending_symbology(ui.ctx()); + } // A Zarr summary or a trend running on its thread lands whatever pane is in view (both are // looked at: `|`, not `||`), and the frames go on while one runs so it's taken when it ends. #[cfg(not(target_arch = "wasm32"))] @@ -3290,6 +3296,8 @@ fn ramp_swatch(ui: &mut Ui, ramp: &Ramp, w: f32, selected: bool, t: &Tokens) -> const PENDING_SYMBOLOGY: &str = "symbology_pending_field"; +/// A renderer switch that failed, kept so the pane can offer a field to retry with. It belongs to +/// one layer of one map in one project, and to the renderer the layer had when it failed. #[derive(Clone)] struct PendingSymbology { project: u64, @@ -3297,53 +3305,30 @@ struct PendingSymbology { layer: u32, kind: &'static str, error: std::sync::Arc, + renderer: std::sync::Arc, } fn clear_pending_symbology(ctx: &egui::Context) { ctx.data_mut(|data| data.remove::(egui::Id::new(PENDING_SYMBOLOGY))); } -/// Ends a pane-local retry when its project, map or active layer is no longer in view, -/// including while the Symbology pane is closed. -pub(crate) fn follow_symbology(app: &RustGisApp, ctx: &egui::Context) { - let stale = ctx.data(|data| { - data.get_temp::(egui::Id::new(PENDING_SYMBOLOGY)).is_some_and(|pending| { - pending.project != app.session.opened - || pending.map != app.session.project.active_map - || Some(pending.layer) != app.session.active_layer - }) - }); - if stale { - clear_pending_symbology(ctx); - } -} - -/// A successful style edit supersedes a failed switch, including edits from the ribbon. -pub(crate) fn symbology_command_finished(app: &RustGisApp, ctx: &egui::Context, command: &str) { - follow_symbology(app, ctx); - if matches!( - command, - "layer.symbology" - | "layer.ramp" - | "layer.classStyle" - | "layer.color" - | "layer.noFill" - | "layer.outline" - | "layer.marker" - | "layer.lineStrokes" - | "style.apply" - | "map.remove" - | "edit.undo" - | "edit.redo" - ) { +/// The pending retry, unless another project, map or layer is in view or the layer's renderer +/// changed since the switch failed (an edit from the pane, the ribbon, an agent or undo +/// supersedes it); a stale one is dropped. +fn pending_symbology(app: &RustGisApp, ctx: &egui::Context) -> Option { + let pending = ctx.data(|data| data.get_temp::(egui::Id::new(PENDING_SYMBOLOGY)))?; + let current = pending.project == app.session.opened + && pending.map == app.session.project.active_map + && Some(pending.layer) == app.session.active_layer + && app.session.map().layer(pending.layer).is_some_and(|l| l.renderer == *pending.renderer); + if !current { clear_pending_symbology(ctx); } + current.then_some(pending) } /// Shows the active layer's renderer controls, including No Fill for polygon layers. fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { - follow_symbology(app, ui.ctx()); - let pending_id = egui::Id::new(PENDING_SYMBOLOGY); let Some(l) = app.session.active_layer.and_then(|id| app.session.map().layer(id)).cloned() else { ui.label(egui::RichText::new(tl!("Select a layer in the Layers pane.")).color(t.text_dim)); return; @@ -3413,8 +3398,9 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { layer: l.id, kind: code, error: error.into(), + renderer: std::sync::Arc::new(l.renderer.clone()), }; - ui.data_mut(|data| data.insert_temp(pending_id, pending)); + ui.data_mut(|data| data.insert_temp(egui::Id::new(PENDING_SYMBOLOGY), pending)); } Ok(_) => clear_pending_symbology(ui.ctx()), } @@ -3430,7 +3416,7 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { } let numeric: Vec = l.dataset.fields.iter().filter(|f| f.ty.is_numeric()).map(|f| f.name.clone()).collect(); let all: Vec = l.dataset.fields.iter().map(|f| f.name.clone()).collect(); - if let Some(pending) = ui.data(|data| data.get_temp::(pending_id)) { + if let Some(pending) = pending_symbology(app, ui.ctx()) { ui.label(egui::RichText::new(pending.error.as_ref()).color(ui.visuals().error_fg_color)); let fields = if pending.kind == "unique" { &all } else { &numeric }; if !fields.is_empty() { @@ -3442,9 +3428,8 @@ fn symbology(app: &mut RustGisApp, ui: &mut Ui, t: &Tokens) { match app.run("layer.symbology", json!({"type": pending.kind, "field": field})) { Ok(_) => clear_pending_symbology(ui.ctx()), Err(error) => { - ui.data_mut(|data| { - data.insert_temp(pending_id, PendingSymbology { error: error.into(), ..pending.clone() }) - }); + let pending = PendingSymbology { error: error.into(), ..pending.clone() }; + ui.data_mut(|data| data.insert_temp(egui::Id::new(PENDING_SYMBOLOGY), pending)); } } } @@ -5140,9 +5125,10 @@ mod tests { assert!(matches!(h.state().session.map().layer(id).unwrap().renderer, Renderer::Single(_))); } - /// A closed pane must not retain a failed request through a layer or map round trip. + /// A failed switch is not offered again after the pane closes, or after a layer or map round + /// trip back to the same layer id. #[test] - fn pending_symbology_clears_across_closed_pane_context_changes() { + fn pending_symbology_ends_with_the_pane_or_its_layer() { use egui_kittest::kittest::Queryable; let mut h = symbology_retry_harness(); let map = h.state().session.map().name.clone(); @@ -5160,23 +5146,32 @@ mod tests { h.state_mut().run("layer.select", json!({"layer": id})).unwrap(); h.run_steps(3); - for map_change in [false, true] { + for change in ["close the pane", "layer round trip", "map round trip"] { h.get_by_label("Graduated Colors").click(); h.run_steps(3); assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_some()); - h.state_mut().ui.right = None; - if map_change { - h.state_mut().run("map.activate", json!({"name": "Other map"})).unwrap(); - h.state_mut().run("map.activate", json!({"name": map})).unwrap(); - h.state_mut().run("layer.select", json!({"layer": id})).unwrap(); - } else { - h.state_mut().session.active_layer = Some(other); - h.run_steps(3); - h.state_mut().session.active_layer = Some(id); + match change { + "close the pane" => { + h.state_mut().ui.right = None; + h.run_steps(3); + h.state_mut().ui.open_right(RightPane::Symbology); + } + "layer round trip" => { + h.state_mut().session.active_layer = Some(other); + h.run_steps(3); + h.state_mut().session.active_layer = Some(id); + } + _ => { + // The other map's active layer has the same id: only the map tells them apart. + h.state_mut().run("map.activate", json!({"name": "Other map"})).unwrap(); + assert_eq!(h.state().session.active_layer, Some(id)); + h.run_steps(3); + h.state_mut().run("map.activate", json!({"name": map})).unwrap(); + h.state_mut().run("layer.select", json!({"layer": id})).unwrap(); + } } - h.state_mut().ui.open_right(RightPane::Symbology); h.run_steps(3); - assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_none()); + assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_none(), "{change}"); assert!(matches!(h.state().session.map().layer(id).unwrap().renderer, Renderer::Single(_))); } } @@ -5187,18 +5182,22 @@ mod tests { use egui_kittest::kittest::Queryable; let mut h = symbology_retry_harness(); let id = layer(h.state(), "retry"); - for through_field_editor in [true, false] { + for edit in ["pane field", "ribbon", "agent"] { h.state_mut().run("layer.symbology", json!({"type": "unique", "field": "a_empty"})).unwrap(); h.run_steps(3); h.get_by_label("Graduated Colors").click(); h.run_steps(3); assert!(h.query_all_by_label("Choose a field to apply this renderer:").next().is_some()); - if through_field_editor { - h.query_all_by_role(egui::accesskit::Role::ComboBox).find(|n| n.value().as_deref() == Some("a_empty")).unwrap().click(); - h.run_steps(3); - h.get_by_label("c_valid").click(); - } else { - h.state_mut().run("layer.symbology", json!({"type": "unique", "field": "c_valid"})).unwrap(); + let params = json!({"type": "unique", "field": "c_valid"}); + match edit { + "pane field" => { + h.query_all_by_role(egui::accesskit::Role::ComboBox).find(|n| n.value().as_deref() == Some("a_empty")).unwrap().click(); + h.run_steps(3); + h.get_by_label("c_valid").click(); + } + "ribbon" => drop(h.state_mut().run("layer.symbology", params).unwrap()), + // An attached agent's command runs on the session, not through the app. + _ => drop(h.state_mut().session.run("layer.symbology", ¶ms).unwrap()), } h.run_steps(3); let Renderer::Unique { field, .. } = &h.state().session.map().layer(id).unwrap().renderer else { diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 2a4e45956..0b982f432 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -18,12 +18,13 @@ Select a layer in Layers and open **Style › Symbology** (or the Symbology pane is not fully transparent. A layout legend can leave it out with **Hide other values** (see [Layouts](layouts.md)). - **Graduated Colors**: 1 to 12 classes of a numeric field by natural breaks (Jenks), equal - interval or quantile. If no field is chosen, a numeric field is selected automatically; if none - exists, the pane explains why the switch failed and keeps the current renderer. When candidate - fields exist, a field picker lets you try another field; the error remains visible if a retry - fails. Fix other reported problems, such as a definition query that keeps no features, before retrying. -- **Proportional Symbols**: point size grows with a numeric field; the field is selected from the - available numeric fields. + interval or quantile. If no field is chosen, the current renderer's field is kept when it is + numeric; otherwise the first numeric field is used. If the layer has no numeric field, or the + switch fails for another reason, the pane shows why and keeps the current renderer. When numeric + fields exist, a field picker lets you try another one; the error stays visible if a retry fails. + A problem no field can fix, such as a definition query that keeps no features, needs fixing + first. Closing the pane or selecting another layer drops the retry. +- **Proportional Symbols**: point size grows with a numeric field, chosen as for Graduated Colors. Pick a colour scheme from the pane's swatches, listed in groups: **Perceptually Uniform** (Viridis, Plasma, Inferno, Magma, Cividis, Batlow and Aurora: even steps in lightness, so they read in