Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions crates/assistant/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,9 @@ fn gate_rules() {
assert!(g("run_command", json!({"command": "edit.undo"})).is_none());
assert!(g("run_command", json!({"command": "layer.addData", "params": {"path": "/etc/passwd"}})).is_some());
assert!(g("run_command", json!({"command": "project.new"})).is_some());
// A QGIS project given as text names its files and addresses inside: asked, as with a path.
let qgis = g("run_command", json!({"command": "project.importQgis", "params": {"text": "<qgis/>"}}));
assert!(qgis.as_deref().is_some_and(|w| w.contains("QGIS project")), "{qgis:?}");
assert!(g("run_command", json!({"command": "script.run", "params": {"script": "map.zoomFull {}"}})).is_some());
assert!(g("run_command", json!({"command": "plugin.install", "params": {"path": "x.rgplugin"}})).is_some());
assert!(g("run_command", json!({"command": "map.addTiles", "params": {"url": "https://tile.example/{z}/{x}/{y}.png"}})).is_some());
Expand Down
3 changes: 3 additions & 0 deletions crates/assistant/src/tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ pub fn gate(s: &Session, call: &ToolCall) -> Option<String> {
"project.new" | "project.open" | "project.openText" | "project.sample" | "project.fromTemplate" => {
"it replaces the open project".into()
}
// Whether given a path, text or data: the project names the files and web addresses
// its layers are added from.
"project.importQgis" => "it adds the files and web addresses a QGIS project names".into(),
_ => "it is reserved to you (plugins, scripts, tasks, models and the GeoLibre runner)".into(),
});
}
Expand Down
2 changes: 2 additions & 0 deletions crates/engine/src/cmd/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub mod plugins;
pub mod portals;
pub mod presentation;
pub mod project;
pub mod qgis;
pub mod raster;
pub mod report;
pub mod s3;
Expand All @@ -55,6 +56,7 @@ use crate::{CommandSpec, Session};
pub fn all() -> Vec<CommandSpec> {
let mut v = Vec::new();
v.extend(project::commands());
v.extend(qgis::commands());
v.extend(navigate::commands());
v.extend(geocode::commands());
v.extend(directions::commands());
Expand Down
3 changes: 3 additions & 0 deletions crates/engine/src/cmd/plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,9 @@ const NEVER: &[&str] = &[
"project.openText",
"project.sample",
"project.fromTemplate",
// A QGIS project names the files and web addresses its layers come from, and the import
// adds them without each one passing a plugin's permissions.
"project.importQgis",
"edit.undo",
"edit.redo",
// Credentials: a plugin never sets where the user's are sent.
Expand Down
334 changes: 334 additions & 0 deletions crates/engine/src/cmd/qgis.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,334 @@
//! QGIS projects and layer styles: `project.importQgis` opens a `.qgs` (or `.qgz`) as a new map,
//! with its layers, their order, visibility and styles, its coordinate system and its view;
//! `style.importQgis` gives a layer a `.qml` style. Both say what they couldn't carry over
//! (`rustgis_map::qgis::Report`).

use rustgis_geom::{Coord, Extent};
use rustgis_map::qgis::{self, NoteKind, Report, Source};
use serde_json::{Value, json};

use super::{need_feature_layer, ps};
use crate::{CmdResult, CommandSpec as C, Session};

pub fn commands() -> Vec<C> {
vec![
C::new("project.importQgis", "Import QGIS Project", "Project › Import", import_project).params(
r#"{"path"?: "*.qgs|*.qgz", "text"?: "a .qgs file's text", "data"?: "a .qgs or .qgz file, base64", "dir"?: "the folder its relative paths start from (default: the file's)"} → the layers added and notes on what wasn't carried over"#,
),
C::new("style.importQgis", "Import QGIS Style", "Style › Styles", import_style).when(need_feature_layer).params(
r#"{"layer"?, "path"?: "*.qml", "text"?: "a .qml file's text", "data"?: "a .qml file, base64"} → notes on what wasn't carried over"#,
),
]
}

/// A QGIS file's text: a `.qgs` or `.qml` as it is, a `.qgz` (a zip archive) its `.qgs`.
fn qgis_text(p: &Value) -> Result<String, String> {

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.

Project files are hostile input, but the ogr/gdal and delimitedtext paths, and any value containing ://, go straight to layer.addData (source.rs::absolute keeps absolute paths and URLs as they are). Opening a downloaded .qgs can therefore read arbitrary local files or fetch arbitrary URLs, including GDAL /vsicurl/-style paths, without the user seeing them first. Consider these changes:

  • Restrict automatic loading to paths under dir or to the user's own files.
  • List the external URLs and absolute paths in the report.
  • Confirm the same file and network gating that the assistant and plugin paths apply to layer.addData.

project.importQgis is also missing from the NEVER list in plugins.rs, and I couldn't find where the assistant gates it. Confidence: low to medium; I couldn't confirm the gating.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Partly valid, fixed in 8f8eed6. project.importQgis is now in plugins::NEVER, so plugins can't run it and the AI assistant asks every time, as for project.open. Before, a plugin could pass a project as text and have the import add files and web addresses through layer.addData / map.addTiles, which run inside the import without each passing the plugin's permissions. The test asserts user_only("project.importQgis").

I left out restricting paths to the project's folder. A project the user opens themselves loading the absolute paths and addresses it names is what QGIS does, and what a RustGIS .rgis does on project.open. A downloaded project's layers are named in the import's result (layers), and every skipped source is noted with its address. style.importQgis reads one named file and adds no data, and its path parameter already gates it as a file reader.

let bytes = match (ps(p, "data"), ps(p, "text"), ps(p, "path")) {
(Some(d), ..) => {
// Base64 is 4 characters for 3 bytes: a payload past the file limit isn't decoded.
if d.len() as u64 > MAX_FILE / 3 * 4 + 4 {
return Err("data is too large for a QGIS file".into());
}
use base64::Engine;
base64::engine::general_purpose::STANDARD.decode(d.trim()).map_err(|_| "data must be base64".to_string())?
}
(None, Some(t), _) if t.len() > MAX_XML => return Err("text is too large for a QGIS project or style".into()),
(None, Some(t), _) => return Ok(t.to_string()),
(None, None, Some(path)) => read_file(path)?,
_ => return Err("path, text or data is required".into()),
};
if bytes.starts_with(b"PK") {
let entries = rustgis_data::zip::read_matching_within(&bytes, |n| n.to_ascii_lowercase().ends_with(".qgs"), MAX_XML)?;
let (_, qgs) = entries.into_iter().next().ok_or("the .qgz holds no .qgs project")?;
return String::from_utf8(qgs).map_err(|_| "the .qgz's project isn't text".to_string());
}
if bytes.len() > MAX_XML {
return Err("too large for a QGIS project or style".into());
}
String::from_utf8(bytes).map_err(|_| "not a QGIS file (it isn't text)".to_string())
}

/// The most bytes of a QGIS file read (a `.qgz` can carry data files beside its project).
const MAX_FILE: u64 = 256 << 20;
/// The most bytes of a project's or style's XML parsed: its tree is several times larger than
/// the text, and real ones are a few megabytes.
const MAX_XML: usize = 64 << 20;

/// Reads at most [`MAX_FILE`] bytes of `path` (natively: the web app passes `text` or `data`).
fn read_file(path: &str) -> Result<Vec<u8>, String> {
use std::io::Read;
let mut bytes = Vec::new();
std::fs::File::open(path).and_then(|f| f.take(MAX_FILE + 1).read_to_end(&mut bytes)).map_err(|e| format!("{path}: {e}"))?;
if bytes.len() as u64 > MAX_FILE {
return Err(format!("{path} is too large for a QGIS project"));
}
Ok(bytes)
}

fn import_project(s: &mut Session, p: &Value) -> CmdResult {
let text = qgis_text(p)?;
// The folder relative paths start from: `dir` (a blank one is none, so they don't start
// from the working directory), else the file's own (`.` for a file named without one).
let dir = ps(p, "dir").filter(|d| !d.trim().is_empty()).map(str::to_string).or_else(|| {
ps(p, "path").and_then(|path| std::path::Path::new(path).parent()).map(|d| match d.to_string_lossy() {
d if d.is_empty() => ".".to_string(),
d => d.into_owned(),
})
});
let mut report = Report::default();
let project = qgis::read_project(&text, dir.as_deref().unwrap_or_default(), &mut report)?;
// A map of its own, named as the project.
let name = if project.title.is_empty() { "QGIS project" } else { project.title.as_str() };
s.run_part("map.new", &json!({"name": name}))?;
// QGIS draws a project on its canvas alone: no basemap, no graticule.
let m = s.map_mut();
m.basemap = rustgis_map::Basemap::Blank;
m.graticule = false;
if let Some(crs) = &project.crs {
let code = match crs.as_str() {
"EPSG:3857" | "EPSG:900913" => "mercator",
other => other,
};
if let Err(e) = s.run_part("map.projection", &json!({"type": code})) {
report.approximated("", format!("the project's coordinate system {crs} ({e}), drawn in Web Mercator"));
}
}
// Bottom first, so each is added on top of the ones below it.
let mut added = Vec::new();
for layer in project.layers.iter().rev() {
match add_layer(s, layer, dir.is_some(), &mut report) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Ok(Some(name)) => added.push(name),
Ok(None) => {}
Err(e) => report.skipped(&layer.name, format!("the layer: {e}")),
}
}
go_to_view(s, &project);
let notes: Vec<Value> = report.notes.iter().map(note_json).collect();
if !report.notes.is_empty() {
let left = report.notes.iter().filter(|n| n.kind == NoteKind::Skipped).count();
s.notify(
format!(
"Imported {} of {} QGIS layers; {} things drawn differently, {left} left out (see the import's notes)",
added.len(),
project.layers.len(),
report.notes.len() - left

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.

Concern (medium-low confidence): a text/data project carries layer paths and URLs inside the payload, and the import adds them via layer.addData, map.addTiles and service.add with no per-layer approval. The plugin sandbox is closed with NEVER. Check that the AI Assistant's approval gate (crates/assistant/src/tools.rs) also covers project.importQgis when it is called with text or data and no path, since the file and network access is hidden in the payload. When text is given without dir, relative paths also resolve against the process cwd (unwrap_or_default() gives ""). Also, text and data are not size-capped, unlike read_file's MAX_FILE.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid in part, fixed in 112181f. This follows up on the earlier thread (4367517494); the reasoning there for not restricting paths to the project's folder still holds.

  • AI assistant gate: this was already covered. gate checks user_only before anything else, whatever the parameters, so project.importQgis is asked about every time, with text or data as much as with path, and is never allowed for good. It now gives its own reason ("it adds the files and web addresses a QGIS project names") instead of the generic one, and gate_rules checks it for a text call.
  • Relative paths with text and no dir: valid. Such a layer is now noted ("relative to the project's folder, which the import wasn't given (pass dir)") and not looked for in the working directory. The engine test checks it.
  • Size caps: valid. text past MAX_FILE (256 MB), and data whose base64 would decode past it, are refused before decoding. A .qgz now inflates within MAX_FILE (read_matching_within) instead of the zip reader's general limit.

),
false,
);
}
Ok(json!({"map": name, "layers": added, "notes": notes}))
}

/// Adds one QGIS layer to the active map with its style: `Ok(Some(name))` when added, `Ok(None)`
/// when its source is one RustGIS doesn't read (noted). Without the project's folder (`has_dir`:

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.

MAX_FILE is 256 MiB for text that goes to an XML parser. A .qgs or .qml is normally well under a few MB. The parsed DOM is several times larger than the source, so a hostile 256 MiB file (or a .qgz that inflates to that size) can use gigabytes on the desktop. In the browser it can run out of wasm memory. Consider a limit around 32 MiB. This is a robustness concern, not a panic. Confidence: medium-low.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 1981d74. A new MAX_XML (64 MiB) caps what a project or style parses: text, a file's text and a .qgz's inflated .qgs. The XML tree is several times the text, and real projects are a few megabytes. MAX_FILE (256 MiB) still bounds the raw bytes read, because a .qgz can carry data files beside its project. 64 rather than 32 MiB leaves room for big projects with many embedded styles. The engine test checks that text one byte over is refused before parsing.

/// a project given as text or data with no `dir`), a relative path has nothing to start from and
/// is noted rather than looked for in the working directory.
fn add_layer(s: &mut Session, layer: &qgis::ProjectLayer, has_dir: bool, report: &mut Report) -> Result<Option<String>, String> {
let name = layer.name.as_str();
if let Source::File { path, .. } | Source::Csv { path, .. } = &layer.source {
if !has_dir && qgis::is_relative(path) {
report.skipped(name, format!("the file {path}, relative to the project's folder, which the import wasn't given (pass dir)"));
return Ok(None);
}
// `C:roads.gpkg`: relative to the working directory on drive C, which no folder fixes.
if qgis::is_drive_relative(path) {
report.skipped(name, format!("the file {path}, relative to a drive's working directory (give a full path in QGIS)"));
return Ok(None);
}
}
match &layer.source {
Source::File { path, layer: table, .. } => {
let mut q = json!({"path": path, "name": name});
if let Some(t) = table {
q["layer"] = json!(t);
}
s.run_part("layer.addData", &q)?;
}
Source::Csv { path, x, y, wkt, crs } => {
let mut q = json!({"path": path, "name": name});
for (k, v) in [("x", x), ("y", y), ("wkt", wkt), ("crs", crs)] {
if let Some(v) = v {
q[k] = json!(v);
}
}
s.run_part("layer.addData", &q)?;
}
Source::Xyz { url, zmin, zmax, vector } => {
// An overlay, as a WMS layer is, so web layers keep the tree's order between them.
let mut q = json!({"url": url, "name": name, "kind": if *vector { "vector" } else { "raster" }, "overlay": true});
if let Some(z) = zmin {
q["minZoom"] = json!(z);
}
if let Some(z) = zmax {
q["maxZoom"] = json!(z);
}
let before = s.active_layer;
let added = s.run_part("map.addTiles", &q)?;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
web_settings(s, &added, before, layer, report);
return Ok(Some(name.to_string()));
}
Source::Wms { url, layer: wms, .. } => {
let before = s.active_layer;
let added = s.run_part("service.add", &json!({"url": url, "type": "wms", "layer": wms, "name": name}))?;
web_settings(s, &added, before, layer, report);
return Ok(Some(name.to_string()));

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.

The WMS (and XYZ) branches return before the visibility, opacity and scale range are applied. A hidden or semi-transparent WMS layer is added fully visible, and no note says so. The XYZ branch notes the hidden case, but the WMS branch doesn't. The PR promises "nothing is left out without a note". Either apply layer.visible, transparency and the scale range to the layer these commands add, or add a note here as the XYZ branch does. Confidence: medium.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 8f8eed6 (CodeRabbit's 4367543712 is the same finding). The WMS and XYZ branches now hand what service.add / map.addTiles answered to tile_settings. It takes the tile layer at the returned index and gives it the QGIS layer's visibility and opacity (transparency 0–100 → opacity 1–0). Tile layers have no scale range, so a range set in QGIS is noted ("its scale range: a web layer draws at every scale"). If the index can't be found, a hidden or transparent layer is noted rather than silently added shown. a_qgis_project_opens_as_a_map_and_a_qgis_style_styles_a_layer now imports an unchecked XYZ layer with opacity 0.25 and a scale range and checks all three.

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Source::Wfs { url, layer: typename } => {
report.skipped(name, format!("the WFS layer {typename} at {url} (add it with Add Data › Web Data)"));
return Ok(None);
}
Source::Postgres { connection, table } => {
report.skipped(name, format!("the PostGIS table {table} ({connection}): connect to the database to add it"));
return Ok(None);
}
Source::Other { provider } => {
report.skipped(name, format!("the layer: RustGIS doesn't read QGIS's {provider} sources"));
return Ok(None);
}
}
let id = s.active_layer.ok_or("the layer wasn't added")?;
let Some(l) = s.map_mut().layer_mut(id) else { return Err("the layer wasn't added".into()) };
let added_name = l.name.clone();
l.visible = layer.visible;
if l.is_raster() {
// A raster takes its opacity, visibility and scale range; its colouring is noted.
let st = &layer.style;
let t = st.transparency;
let (min, max) = (st.min_scale, st.max_scale);
if let Some(t) = t {
l.transparency = t;
}
if let Some(m) = min {
l.min_scale = m;
}
if let Some(m) = max {
l.max_scale = m;
}
return Ok(Some(added_name));
}
layer.style.apply(l);
// What reads only against the layer's fields: its definition query, and what the style names.
if let Some(q) = &layer.definition_query {
match rustgis_data::query::parse(q, &l.dataset) {
Ok(_) => l.definition_query = Some(q.clone()),
Err(e) => report.skipped(name, format!("its filter `{q}` ({e})")),
}
}
check_fields(l, name, report);
Ok(Some(added_name))
}

/// Checks what a style names against the layer's fields: its label classes' clauses and fields
/// (a class that can't label is switched off) and the field a Unique Values or Graduated Colors
/// renderer classes by.
fn check_fields(l: &mut rustgis_map::Layer, name: &str, report: &mut Report) {
for c in std::iter::once(&mut l.labels.default).chain(l.labels.classes.iter_mut()) {
if let Some(w) = c.where_.clone()
&& let Err(e) = rustgis_data::query::parse(&w, &l.dataset)
{
report.skipped(name, format!("the label class \"{}\", whose clause `{w}` doesn't fit the layer ({e})", c.name));
c.visible = false;
}
// (A class with no field set labels nothing, and isn't noted.)
if c.visible && !c.field.is_empty() && l.dataset.field_index(&c.field).is_none() {
report.skipped(name, format!("labels from the field {}, which the layer hasn't", c.field));
c.visible = false;
}
}
let field = match &l.renderer {
rustgis_map::Renderer::Unique { field, .. } | rustgis_map::Renderer::Graduated { field, .. } => Some(field.clone()),
_ => None,
};

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.

service.add doesn't always add a tile layer. Depending on the capabilities, it can add a coverage raster, a feature layer or a style (see service.rs Offer::Coverage and Offer::Features). In those cases the answer has no tile index, so tile_settings takes the None branch. It then reports "a hidden web layer, added shown" even though no tile layer exists. The layer's visibility and opacity are also never applied, and the function still returns Some(name).

Consider applying the visibility, opacity and scale range to the layer actually added (s.active_layer) when there is no index. Or note that the web layer was added as a different kind. It would also help to add a test for a WMS layer, because only the XYZ path is tested.

Confidence: medium.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 4443fb5. web_settings (was tile_settings) now handles an answer with no tile index, as when the service offers the layer as features or a coverage. If the active layer changed, the map layer that was added takes the QGIS layer's visibility, transparency, blending and scale range, which a map layer has (unlike a tile layer). The "added shown" notes now apply only when neither a tile layer nor a map layer can be found. The engine test reads a project with a hidden, half-transparent, multiplied WMS layer with a scale range and applies it to a session's new map layer, so the path is tested without a live service.

if let Some(f) = field.filter(|f| l.dataset.field_index(f).is_none()) {
report.approximated(name, format!("classes by the field {f}, which the layer hasn't: every feature draws as the rest"));
}
}

/// Gives what a QGIS web layer was added as the layer's visibility, opacity and blending mode.
/// `added` is what `map.addTiles` or `service.add` answered: a tile layer's `index`, or no index
/// when the service offered the layer as features or a coverage, which come in as a map layer
/// (the active one, if it isn't `before` any more), which takes the scale range too. A tile layer
/// has no scale range, so one is noted.
pub(crate) fn web_settings(s: &mut Session, added: &Value, before: Option<u32>, layer: &qgis::ProjectLayer, report: &mut Report) {
let name = layer.name.as_str();
let st = &layer.style;
let transparency = st.transparency.filter(|t| t.is_finite() && *t > 0.0);
let index = added.get("index").and_then(Value::as_u64).and_then(|i| usize::try_from(i).ok());
if index.is_none()
&& let Some(id) = s.active_layer.filter(|id| Some(*id) != before)
&& let Some(l) = s.map_mut().layer_mut(id)
{
l.visible = layer.visible;
if let Some(tr) = transparency {
l.transparency = tr.clamp(0.0, 100.0);
}
if let Some(blend) = st.blend {
l.blend = blend;
}
if let Some(m) = st.min_scale {
l.min_scale = m;
}
if let Some(m) = st.max_scale {
l.max_scale = m;
}
return;
}
match index.and_then(|i| s.map_mut().tiles.get_mut(i)) {
Some(t) => {
t.visible = layer.visible;
if let Some(blend) = st.blend {
t.blend = blend;
}
if let Some(tr) = transparency {
t.opacity = (1.0 - tr / 100.0).clamp(0.0, 1.0);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
None => {
if !layer.visible {
report.approximated(name, "a hidden web layer, added shown");
}
if transparency.is_some() {
report.approximated(name, "its transparency: added opaque");
}
if st.blend.is_some_and(|b| b != rustgis_map::effects::Blend::default()) {
report.approximated(name, "its blending mode: added as normal");
}
}
}
if st.min_scale.flatten().is_some() || st.max_scale.flatten().is_some() {
report.approximated(name, "its scale range: a web layer draws at every scale");
}
}

/// Goes to the project's saved view, or to its layers when that view shows none of them.
fn go_to_view(s: &mut Session, project: &qgis::Project) {
let data = s.map().full_extent();
let saved = project.extent.and_then(|[x0, y0, x1, y1]| {
let m = s.map();
let corners = [(x0, y0), (x1, y0), (x0, y1), (x1, y1)].map(|(x, y)| m.to_lonlat(Coord::new(x, y)));
corners.iter().flatten().fold(None, |e: Option<Extent>, c| Some(e.unwrap_or(Extent::EMPTY).union(&Extent::new(c.x, c.y, c.x, c.y))))
});
let target = match saved {
Some(e) if !e.is_empty() && (data.is_empty() || e.intersects(&data)) => e,
_ => data,
};
if !target.is_empty() {
s.zoom_to(&target);
}
}

fn note_json(n: &qgis::Note) -> Value {
json!({"layer": n.layer, "kind": n.kind, "text": n.text, "count": n.count})
}

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: check_fields reports "labels from the field …, which the layer hasn't" for any visible class whose field is missing. If the class's clause already failed above, c.visible is false by this point, so that case is skipped. A default class with an empty field would still produce a misleading note if it is visible. Guard on !c.field.is_empty(), or check how Style::apply leaves the default class. (Low confidence.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 4443fb5 (one line): check_fields skips a class with no field set, so a visible default class that labels nothing isn't noted as labelling from a missing field.


fn import_style(s: &mut Session, p: &Value) -> CmdResult {
let text = qgis_text(p)?;
let id = s.layer_id(p)?;
let name = s.map().layer(id).map(|l| l.name.clone()).ok_or("no such layer")?;
let mut report = Report::default();
let style = qgis::read_style(&text, &name, &mut report)?;
let l = s.map_mut().layer_mut(id).ok_or("no such layer")?;
style.apply(l);
check_fields(l, &name, &mut report);
Ok(json!({"notes": report.notes.iter().map(note_json).collect::<Vec<_>>()}))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
12 changes: 12 additions & 0 deletions crates/engine/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,18 @@ impl Session {
cmd::tiles::follow_style_view(self, web_time::Duration::ZERO)
}

/// Runs command `id` as a part of the one running now (a command made of others): its
/// changes are in that command's one undo step, not a step of their own. A part that fails
/// (or panics) is undone by its own `run`, which restores what it started from; the parts
/// before it stay until the command made of them fails too, whose `run` restores the project
/// as the whole command found it. The stacks are put back on every return.
pub(crate) fn run_part(&mut self, id: &str, params: &Value) -> CmdResult {
let (undo, redo) = (std::mem::take(&mut self.undo), std::mem::take(&mut self.redo));

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.

Low-confidence concern: run_part takes the undo and redo stacks, calls self.run, and then restores them. This drops the inner command's undo snapshot, so a nested command that fails restores only its own snapshot. The enclosing command's snapshot is what undoes the earlier parts, so this relies on the outer run pushing exactly one step. If a nested command ever panics inside self.run, the stacks are still swapped back, but check that run doesn't early-return before the restore. The code looks fine as written. A short doc note on what happens when a part fails would help.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As you say, it's correct as written. run catches a panic in guarded, restores the project and returns an error, and run_part has no early return, so the stacks always go back. I added the doc note you asked for in 112181f. A part that fails is undone by its own run. The parts before it stay until the command made of them fails too, and that command's run then restores the project as it found it. The stacks are put back on every return.

let res = self.run(id, params);
(self.undo, self.redo) = (undo, redo);
res
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
/// Runs a command by id. Mutating commands snapshot the project for undo; an error or a
/// panic restores it.
pub fn run(&mut self, id: &str, params: &Value) -> CmdResult {
Expand Down
Loading
Loading