Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/claude-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ jobs:
| select(.starter.nodes[0] | trusted)
| select(.comments.nodes[-1]
| (.author.__typename == "Bot" and (.body | startswith("**Claude autofix ("))) | not)
# A thread a maintainer left for the release quality pass (its last trusted reply
# ends with the quality-sweep marker) waits for that pass, not for autofix. A later
# reply from a non-writer doesn't bring it back.
| select(([.comments.nodes[] | select(trusted)] | last | (.body // ""))
| test("<!-- quality-sweep -->\\s*$") | not)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment thread
giswqs marked this conversation as resolved.
Resolved
| { id, path, line: (.line // .originalLine), outdated: .isOutdated,
comments: (.comments.totalCount as $n | .starter.nodes[0] as $s
| [ $s, (.comments.nodes[] | select(.id != $s.id)) ]
Expand Down
5 changes: 4 additions & 1 deletion apps/rustgis-python/python/rustgis/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,10 @@ def add_tiles(self, url: str, name: Optional[str] = None, **params: Any) -> Any:
**params: More ``map.addTiles`` parameters (``kind``, ``attribution``, ...).

Returns:
The tile layer's position, kind and name.
The tile layer's position, kind and name. An archive (PMTiles or MBTiles)
added without an ``id`` also reports ``zoomed``, whether its bounds framed
the map by the time the call returned (``False`` while its metadata is
still loading, which can frame the map later).
"""
if name is not None:
params["name"] = name
Expand Down
4 changes: 2 additions & 2 deletions crates/analysis/src/control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ pub fn install_progress(control: Option<Arc<Control>>) -> rustgis_data::progress
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Cancelled;

/// What [`crate::run_with`] says of a run that was cancelled (it names the tool and how far it
/// came).
/// The error text the `From<Cancelled>` conversions carry. [`crate::run_with`] replaces it with
/// its own "… was cancelled" message, which names the tool and how far it came.
pub(crate) const CANCELLED: &str = "cancelled";

impl From<Cancelled> for String {
Expand Down
76 changes: 69 additions & 7 deletions crates/engine/src/cmd/workflows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub fn commands() -> Vec<C> {
.params(r#"{"name"?: model (default: the last), "graph"?: run this graph instead (as model.new)}"#),
C::new("model.validate", "Validate Model", "Analyze › Geoprocessing", |s, p| {
let (_, g, _) = model_graph(s, p)?;
note_passed_graph(s, p);
let problems = crate::model::validate(s, &g);
Ok(json!({"ok": problems.is_empty(), "problems": problems}))
}).roles(&[crate::ParamRole { key: "graph", role: crate::Role::ModelGraph }]).effects(crate::policy::Effects::NONE)
Expand Down Expand Up @@ -443,9 +444,60 @@ pub fn gallery_list(kind: &str) -> CmdResult {
Ok(Value::Array(out))
}

/// Tells the user what a graph passed to a command lost or changed when it was read: one notice
/// per kind, each with a count.
fn notify_graph_dropped(s: &mut Session, dropped: crate::model::GraphDropped) {
if dropped.nodes > 0 {
s.notify(format!("model graph: {} node(s) left out: unreadable, without an id, or with the id of another", dropped.nodes), false);
}
if dropped.edges > 0 {
s.notify(
format!(
"model graph: {} connection(s) left out: unreadable, to a missing node or itself, without an input port, or repeating another",
dropped.edges
),
false,
);
}
if dropped.ids > 0 {
s.notify(
format!(
"model graph: {} node id(s) changed: trimmed of spaces or cut to {} characters (connections naming the old id follow it)",
dropped.ids,
crate::model::MAX_NODE_ID
),
false,
);
}
if dropped.positions > 0 {
s.notify(
format!("model graph: {} node position(s) changed: not a number (moved to 0, 0) or out of range (clamped)", dropped.positions),
false,
);
}
if dropped.params > 0 {
s.notify(format!("model graph: {} node(s) had parameters that were not an object; they were left out", dropped.params), false);
}
}

/// Reads the `graph` a command was passed, if any, with what reading it left out or changed. The
/// command reports that once it succeeds, so a command that fails says nothing about it.
fn read_graph(p: &Value) -> Result<(Option<crate::project::ModelGraph>, crate::model::GraphDropped), String> {
let Some(g) = p.get("graph").filter(|g| !g.is_null()) else { return Ok((None, Default::default())) };
let (graph, dropped) = crate::model::graph_from_json_counted(g)?;
Ok((Some(graph), dropped))
}

/// Says what the graph a command was passed (rather than a saved model's) lost when read.
fn note_passed_graph(s: &mut Session, p: &Value) {
if let Some(Ok((_, dropped))) = p.get("graph").filter(|g| !g.is_null()).map(crate::model::graph_from_json_counted) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (performance / clarity): every command that takes a graph now parses it twice. read_graph (or model_graph) reads it, and note_passed_graph runs graph_from_json_counted again just to get the counts, including the layout pass. It is harmless at the 2000-node cap, but model_graph could return the GraphDropped it already computes, which would remove the second parse in model.run, model.validate and model.export.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left for the release quality pass: the second parse is bounded by the node and connection caps, and returning the counts from model_graph is a refactor.

notify_graph_dropped(s, dropped);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

fn new_model(s: &mut Session, p: &Value) -> CmdResult {
let base = ps(p, "name").map(str::trim).filter(|n| !n.is_empty()).unwrap_or("Model").chars().take(200).collect::<String>();
let graph = p.get("graph").filter(|g| !g.is_null()).map(crate::model::graph_from_json).transpose()?;
let (graph, dropped) = read_graph(p)?;
let steps: Vec<ModelStep> = match p.get("steps") {
_ if graph.is_some() => graph.as_ref().map(crate::model::graph_to_steps).unwrap_or_default(),
Some(v) => serde_json::from_value(v.clone()).map_err(|e| format!("steps: {e}"))?,
Expand Down Expand Up @@ -506,6 +558,7 @@ fn new_model(s: &mut Session, p: &Value) -> CmdResult {
}
let n = steps.len();
s.project.models.push(Model { name: name.clone(), steps, graph });
notify_graph_dropped(s, dropped);
Ok(json!({"model": name, "steps": n}))
}

Expand Down Expand Up @@ -637,11 +690,14 @@ fn run_model_here(s: &mut Session, p: &Value) -> CmdResult {
}
let (name, _, trusted) = model_graph(s, p)?;
let op = s.authorize_model(p)?;
if trusted || p.get("graph").is_some_and(|g| !g.is_null()) {
crate::model::run(s, op)
let out = if trusted || p.get("graph").is_some_and(|g| !g.is_null()) {
crate::model::run(s, op)?
} else {
s.narrowed(crate::policy::Narrow::Item { kind: "model", name }, |s| crate::model::run(s, op))?
}
s.narrowed(crate::policy::Narrow::Item { kind: "model", name }, |s| crate::model::run(s, op))??
};
// Said once the run went ahead, not for one that was refused.
note_passed_graph(s, p);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Ok(out)
}

/// The stored source and destination names used by graph saving and its provenance check.
Expand All @@ -654,7 +710,8 @@ pub(crate) fn model_save_names(p: &Value) -> Result<(String, String), String> {
/// Saves a graph as a model, replacing the one of that name (or adding it).
fn save_model(s: &mut Session, p: &Value) -> CmdResult {
let (name, new_name) = model_save_names(p)?;
let g = crate::model::graph_from_json(p.get("graph").ok_or("graph is required")?)?;
let (g, dropped) = read_graph(p)?;
let g = g.ok_or("graph is required")?;
Comment thread
giswqs marked this conversation as resolved.
Resolved
let i = s.project.models.iter().position(|m| m.name.eq_ignore_ascii_case(&name));
if !new_name.eq_ignore_ascii_case(&name) && s.project.models.iter().any(|m| m.name.eq_ignore_ascii_case(&new_name)) {
return Err(format!("a model named '{new_name}' already exists"));
Expand All @@ -667,6 +724,7 @@ fn save_model(s: &mut Session, p: &Value) -> CmdResult {
None => s.project.models.push(model),
}
s.trusted_models.remove(&name.to_ascii_lowercase());
notify_graph_dropped(s, dropped);
Ok(json!({"model": new_name}))
}

Expand All @@ -680,9 +738,13 @@ fn export_model(s: &mut Session, p: &Value) -> CmdResult {
match ps(p, "path") {
Some(path) => {
rustgis_data::io::write_atomic(std::path::Path::new(path), text.as_bytes()).map_err(|e| format!("{path}: {e}"))?;
note_passed_graph(s, p);
Ok(json!({"model": name, "path": path}))
}
None => Ok(json!({"model": name, "fileName": format!("{file}.model.json"), "text": text})),
None => {
note_passed_graph(s, p);
Ok(json!({"model": name, "fileName": format!("{file}.model.json"), "text": text}))
}
}
}

Expand Down
64 changes: 57 additions & 7 deletions crates/engine/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::project::{ModelEdge, ModelGraph, ModelNode, ModelNodeKind, ModelStep}
pub const MAX_NODES: usize = 2000;
pub const MAX_EDGES: usize = 4000;
/// Identifier length and canvas coordinate limits used by graph commands and file checks.
const MAX_NODE_ID: usize = 100;
pub(crate) const MAX_NODE_ID: usize = 100;
const MAX_NODE_POSITION: f32 = 1e6;
/// The output port of input and tool nodes.
pub const OUT: &str = "out";
Expand Down Expand Up @@ -477,6 +477,28 @@ pub fn graph_to_steps(g: &ModelGraph) -> Vec<ModelStep> {
/// edges to missing nodes or to themselves. Bounded in size.
/// Repeated connections into many-valued input ports retain their multiplicity.
pub fn graph_from_json(v: &Value) -> Result<ModelGraph, String> {
graph_from_json_counted(v).map(|(g, _)| g)
}

/// What [`graph_from_json_counted`] left out of a graph.
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub struct GraphDropped {
/// Nodes that could not be read, had no id, or repeated another's id.
pub nodes: usize,
/// Connections that could not be read, joined a missing node or itself, had no input port,
/// or repeated another.
pub edges: usize,
/// Kept nodes whose id was trimmed of spaces or cut to its longest length.
pub ids: usize,
/// Kept nodes whose position was not a number (moved to 0, 0) or out of range (clamped).
pub positions: usize,
/// Kept nodes whose parameters were not an object, so they were left out.
pub params: usize,
}

/// Reads a graph as [`graph_from_json`] does, and counts the nodes and connections it left out
/// and the nodes it changed, so a command can say so. Errors when `nodes` is missing or either list is too long.
pub fn graph_from_json_counted(v: &Value) -> Result<(ModelGraph, GraphDropped), String> {
let nodes = v.get("nodes").and_then(Value::as_array).ok_or("a model graph needs \"nodes\"")?;
if nodes.len() > MAX_NODES {
return Err(format!("a model can have at most {MAX_NODES} nodes"));
Expand All @@ -486,37 +508,65 @@ pub fn graph_from_json(v: &Value) -> Result<ModelGraph, String> {
return Err(format!("a model can have at most {MAX_EDGES} connections"));
}
let mut g = ModelGraph::default();
let mut dropped = GraphDropped::default();
Comment thread
giswqs marked this conversation as resolved.
Resolved
// Ids trimmed or cut, from what the file gave to what is kept: connections naming a node by
// the id it was given still reach it.
let mut renamed: Vec<(String, String)> = Vec::new();
for n in nodes {
let Ok(mut node) = serde_json::from_value::<ModelNode>(n.clone()) else { continue };
node.id = node.id.trim().chars().take(MAX_NODE_ID).collect();
if node.id.is_empty() || g.nodes.iter().any(|m| m.id == node.id) {
let Ok(mut node) = serde_json::from_value::<ModelNode>(n.clone()) else {
dropped.nodes += 1;
continue;
};
let id: String = node.id.trim().chars().take(MAX_NODE_ID).collect();
if id.is_empty() || g.nodes.iter().any(|m| m.id == id) {
Comment thread
giswqs marked this conversation as resolved.
Resolved
dropped.nodes += 1;
continue;
}
if id != node.id {
dropped.ids += 1;
renamed.push((std::mem::take(&mut node.id), id.clone()));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
node.id = id;
}
Comment thread
giswqs marked this conversation as resolved.
Resolved
let (x, y) = (node.x, node.y);
if !node.x.is_finite() || !node.y.is_finite() {
node.x = 0.0;
node.y = 0.0;
}
node.x = node.x.clamp(-MAX_NODE_POSITION, MAX_NODE_POSITION);
node.y = node.y.clamp(-MAX_NODE_POSITION, MAX_NODE_POSITION);
if !node.params.is_object() {
if node.x.to_bits() != x.to_bits() || node.y.to_bits() != y.to_bits() {
dropped.positions += 1;
}
Comment thread
giswqs marked this conversation as resolved.
Resolved
if !node.params.is_object() && !node.params.is_null() {
dropped.params += 1;
node.params = Value::Null;
}
g.nodes.push(node);
}
for e in edges {
let Ok(e) = serde_json::from_value::<ModelEdge>(e.clone()) else { continue };
let Ok(mut e) = serde_json::from_value::<ModelEdge>(e.clone()) else {
dropped.edges += 1;
continue;
};
for end in [&mut e.from, &mut e.to] {
if let Some((_, id)) = renamed.iter().find(|(given, _)| given == end) {
*end = id.clone();
}
}
let has = |id: &str| g.nodes.iter().any(|n| n.id == id);
let repeated_input =
g.nodes.iter().find(|n| n.id == e.to).is_some_and(|n| input_ports(n).iter().any(|p| p.id == e.to_port && p.many));
if e.from != e.to && has(&e.from) && has(&e.to) && !e.to_port.is_empty() && (repeated_input || !g.edges.contains(&e)) {
g.edges.push(e);
} else {
dropped.edges += 1;
}
}
// A graph with no positions (written by hand or by an agent) is laid out.
if g.nodes.iter().all(|n| n.x == 0.0 && n.y == 0.0) {
layout(&mut g);
}
Ok(g)
Ok((g, dropped))
}

/// How one node fared in a run.
Expand Down
62 changes: 62 additions & 0 deletions crates/engine/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25721,6 +25721,68 @@ fn dem_tiles_mosaic_clip_by_polygon_share_a_stretch_and_report_overviews() {
assert!(e.to_string().contains("overview"), "{e}");
}

/// A graph passed to a model command says what reading it left out (one notice per kind, with a
/// count); a clean graph says nothing.
#[test]
fn model_graph_drops_are_noticed() {
let mut s = world();
let clean = json!({
"nodes": [
{"id": "i1", "kind": "input", "x": 0, "y": 0, "layer": "Cities"},
{"id": "o1", "kind": "output", "x": 240, "y": 0, "name": "Kept"}
],
"edges": [{"from": "i1", "fromPort": "out", "to": "o1", "toPort": "in"}]
});
let said = |s: &Session, mark: u64, what: &str| s.notices_since(mark).iter().filter(|n| n.text.contains(what)).count();
let mark = s.notice_mark();
s.run("model.new", &json!({"name": "Clean", "graph": clean})).unwrap();
s.run("model.save", &json!({"name": "Clean", "graph": clean})).unwrap();
s.run("model.validate", &json!({"graph": clean})).unwrap();
assert_eq!(said(&s, mark, "model graph"), 0, "{:?}", s.notices_since(mark));

let lossy = json!({
"nodes": [
{"id": "i1", "kind": "input", "x": 0, "y": 0, "layer": "Cities"},
{"id": "i1", "kind": "input", "x": 10, "y": 0, "layer": "Countries"},
{"id": " ", "kind": "input", "x": 20, "y": 0, "layer": "Cities"},
{"kind": 7},
{"id": "o1", "kind": "output", "x": 240, "y": 0, "name": "Kept"},
{"id": " i2 ", "kind": "input", "x": 5e7, "y": 0, "layer": "Cities", "params": "not an object"}
],
"edges": [
{"from": "i1", "fromPort": "out", "to": "o1", "toPort": "in"},
{"from": "i1", "fromPort": "out", "to": "missing", "toPort": "in"},
{"from": "o1", "fromPort": "out", "to": "o1", "toPort": "in"},
{"from": "i1", "fromPort": "out", "to": "o1", "toPort": ""},
// Names the node by the id it was given: it follows the trimmed id, so it is kept.
{"from": " i2 ", "fromPort": "out", "to": "o1", "toPort": "in"},
"not an edge"
]
});
for (cmd, params) in [
("model.new", json!({"name": "Lossy", "graph": lossy})),
("model.save", json!({"name": "Lossy", "graph": lossy})),
("model.validate", json!({"graph": lossy})),
("model.export", json!({"graph": lossy})),
] {
let mark = s.notice_mark();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
s.run(cmd, &params).unwrap();
assert_eq!(said(&s, mark, "3 node(s) left out"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "4 connection(s) left out"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node id(s) changed"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node position(s) changed"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node(s) had parameters"), 1, "{cmd}: {:?}", s.notices_since(mark));
}
// A command that fails says nothing about the graph it was passed.
let mark = s.notice_mark();
assert!(s.run("model.save", &json!({"name": "Lossy", "rename": "Clean", "graph": lossy})).is_err());
let unwritable = std::env::temp_dir().join("rustgis-no-such-folder").join("nested").join("lossy.model.json");
assert!(s.run("model.export", &json!({"graph": lossy, "path": unwritable.to_string_lossy()})).is_err());
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
assert_eq!(said(&s, mark, "model graph"), 0, "{:?}", s.notices_since(mark));
let saved = s.project.models.iter().find(|m| m.name == "Lossy").and_then(|m| m.graph.as_ref()).map(|g| g.edges.len());
assert_eq!(saved, Some(2), "the connection naming \" i2 \" follows its trimmed id");
}

/// ModelBuilder graphs: validated, run in flow order, intermediate results dropped, outputs named.
fn model_graphs(s: &mut Session) {
let graph = json!({
Expand Down
11 changes: 10 additions & 1 deletion crates/geom/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,13 @@ pub fn format_area(m2: f64) -> String {
if !m2.is_finite() {
return "—".to_string();
}
if m2.abs().round() < 1.0e6 { format!("{m2:.0} m²") } else { format!("{:.1} km²", m2 / 1.0e6) }
if m2.abs().round() < 1.0e6 {
// A tiny negative area rounds to `-0`; it reads `0 m²`.
let whole = format!("{m2:.0}");
format!("{} m²", if whole == "-0" { "0" } else { &whole })
} else {
format!("{:.1} km²", m2 / 1.0e6)
}
}

/// Formats a lon/lat pair in decimal degrees with hemispheres: `12.345° E 45.678° N`.
Expand Down Expand Up @@ -1234,6 +1240,9 @@ mod tests {
assert_eq!(format_distance(-12_500.0), "-12.5 km");
assert_eq!(format_distance(-99_949.0), "-99.9 km");
assert_eq!(format_distance(-99_960.0), "-100 km");
assert_eq!(format_area(-0.3), "0 m²");
assert_eq!(format_area(-0.0), "0 m²");
assert_eq!(format_area(-0.6), "-1 m²");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
assert_eq!(format_area(-999_999.4), "-999999 m²");
assert_eq!(format_area(-999_999.6), "-1.0 km²");
assert_eq!(format_area(-1.0e6), "-1.0 km²");
Expand Down
2 changes: 1 addition & 1 deletion crates/tiles/src/style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ fn parse_sprites(entries: &[Json]) -> (Vec<(String, String)>, Vec<String>) {
}
if past > 0 {
left_out.push(format!(
"{past} of {total} sprites left out, so their icons and patterns aren't drawn: a style's first {MAX_SPRITES} are read"
"{past} of {total} sprites left out, so their icons and patterns aren't drawn: only the first {MAX_SPRITES} with a usable id of their own and an address are used"
));
}
(sprites, left_out)
Expand Down
3 changes: 2 additions & 1 deletion docs/i18n.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ file-state Open Offen
```

Use `tl!("Open", context = "file-state")` at the call site. `tl_raw!` accepts the same context
for fragments, and `i18n::fmt_context("file-state", "{n} open", &[("n", count)])` formats a
for fragments, and
`i18n::fmt_context("file-state", "{n} open", &[("n", &count.to_string())])` formats a
contextual message. Contexts describe the sense of a word, not a translated label. When a
contextual entry is missing, lookup falls back to the ordinary translation, then to English.
Plugin catalogs keep their existing format and provide the ordinary fallback.
Expand Down
Loading
Loading