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..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"))] @@ -3288,6 +3294,39 @@ fn ramp_swatch(ui: &mut Ui, ramp: &Ramp, w: f32, selected: bool, t: &Tokens) -> resp } +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, + map: usize, + 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))); +} + +/// 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) { let Some(l) = app.session.active_layer.and_then(|id| app.session.map().layer(id)).cloned() else { @@ -3323,6 +3362,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 +3390,20 @@ 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})); + 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(), + renderer: std::sync::Arc::new(l.renderer.clone()), + }; + ui.data_mut(|data| data.insert_temp(egui::Id::new(PENDING_SYMBOLOGY), pending)); + } + Ok(_) => clear_pending_symbology(ui.ctx()), + } } } }); @@ -3363,6 +3416,29 @@ 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) = 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() { + 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) => { + let pending = PendingSymbology { error: error.into(), ..pending.clone() }; + ui.data_mut(|data| data.insert_temp(egui::Id::new(PENDING_SYMBOLOGY), pending)); + } + } + } + } + }); + }); + } + ui.add_space(6.0); + } match &l.renderer { Renderer::Single(s) => symbol_editor(app, ui, *s, g, t), Renderer::Unique { field, classes, ramp, .. } => { @@ -4961,6 +5037,177 @@ mod tests { assert_eq!((app.ui.layer_filter.as_str(), app.ui.layer_filter_hid), ("", None)); } + 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| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + app, + ); + h.run_steps(6); + h + } + + /// 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 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_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(); + 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 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()); + 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.run_steps(3); + 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(_))); + } + } + + /// 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 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()); + 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 { + 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 /// when it's let go, and the whole drag is one undo step back to where it started. #[test] diff --git a/docs/user-guide/cartography.md b/docs/user-guide/cartography.md index 12222f6e8..0b982f432 100644 --- a/docs/user-guide/cartography.md +++ b/docs/user-guide/cartography.md @@ -10,14 +10,21 @@ 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 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. -- **Proportional Symbols**: point size grows with a numeric field. + 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