Skip to content

feat(layout): a legend shows a layer under a label instead of its name (#1654) - #1739

Merged
giswqs merged 7 commits into
opengeos:mainfrom
elpidioassinaturas:feat/1654-legend-labels
Nov 11, 2026
Merged

giswqs merged 7 commits into
opengeos:mainfrom
elpidioassinaturas:feat/1654-legend-labels

Conversation

@elpidioassinaturas

@elpidioassinaturas elpidioassinaturas commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1654

A layout legend shows each layer under its name in the project. Plugins name their layers so they can find them again: BrasilData's are BrasilData: Setores censitários 2022 (MG), BrasilData: Indicadores por município, and every map it lays out shows that prefix in its legend. Renaming the layer breaks what finds it by name, and a copy under another name duplicates its data. With this PR a legend can show a layer under a label of its own, while the layer keeps its name everywhere else.

Left: a publication figure (layout.publication, figure-1.5col) of the sample World Atlas with its layers renamed as a plugin names them; the legend shows the names, and the longest runs into the next column. Right: the same figure after layout.update {"element": 4, "labels": {"BrasilData: ne_110m_populated_places": "Cidades (população)", "BrasilData: ne_110m_admin_0_countries": "População estimada (hab.)", …}}. Built from this branch with rustgis-cli run.

What changes

  1. labels on the legend element. ElementKind::Legend gets labels: {"<layer name>": "<label>"}, matched as exclude is: by the layer's name, any case. The label goes in a plain layer's row, over a classed layer's classes, and over a WMS or WMTS layer's picture.
  2. layout.update takes "labels": {"<layer>": "<label>"|null}. Each entry replaces that layer's label and leaves the others alone. null or a blank label shows the layer's name again; {} shows every layer's name again. layout.addElement takes the same object when it adds a legend.
  3. Refused like exclude: a layer the frame's map doesn't have (no layer 'Atlantis' in map 'World'), a value that isn't an object of strings, a label over 200 characters (a legend's label is at most 200 characters ('Rivers' has 250)), the same layer named twice in one call ({"Rivers": "a", "rivers": "b"}), and labels of any value given to another kind of element (only legends label layers; a grid's own labels is true or false, and anything else there is refused too). An error leaves the element as it was.
  4. The legend measures the label it draws. legend_sizes_mm reads the labels, so sizing a legend to its content and the warnings about its box (say_legend_fit, columns) follow the label, not the name. A legend given labels, added or changed, says when they don't fit its box, in one column too (the legend lists more than fits its box: its last rows or the ends of long labels are left out); before, only a legend in columns was checked.
  5. Saved with the layout, in the .rgis and the .rgislayout, with skip_serializing_if = "BTreeMap::is_empty": files without labels don't change.
  6. Labels follow the layer, as exclude does (legends_follow_layer): a renamed layer keeps its label under its new name, and a removed layer's label is dropped, so a later layer of that name isn't labelled unasked.

Choices

  • A BTreeMap, not a HashMap. The issue proposed HashMap::is_empty; a BTreeMap writes its keys in the same order on every save, so saving a project twice gives the same file and diffs stay small.
  • Keys are the names as the map spells them. The command resolves "rivers" to Rivers (and a WMS or WMTS layer by its name) before it stores the label, as exclude does. One layer has one key, so {"RIVERS": …} replaces the label set by {"rivers": …}.
  • Two spellings of one layer in one call are refused rather than one winning: which one is last depends on how the JSON object is read.
  • Renamed onto a name another layer has, the renamed layer's label is dropped, with a notice ('Hidrografia', renamed 'Cities', lost its label in 1 legend(s): another layer is called 'Cities', and a legend labels layers by name), and the layer is shown under the label of the other one (or its name). Labels are matched by name, so two layers with the same name can't have different labels. This is what exclude does in the same case.
  • labels is also the grid's boolean in layout.addElement. Only an object is read as a legend's labels, so {"kind": "grid", "labels": false} works as before (tested). The description lists the key once, with both meanings, because param_keys_reads_descriptions requires each key to appear only once.
  • The selector state for review (element_inputs, update_inputs, element_selectors) now holds the labelled layers along with the excluded ones, so the layers a call labels count among its inputs, the same way the layers it excludes do.
  • Limits: at most 1,000 labels per legend, each at most 200 characters (as a legend's title). A command refuses a longer label rather than cut it, so a label is never shown other than as given. A file is read anyway: Layout::sanitize cuts an opened or imported layout's labels to these limits, drops blank ones, and keeps one of two spellings of a layer's name (the first in order) as a command keeps one; the notice it gives now names them (… layout element(s) with a name, text, polygon points, legend exclusions or legend labels cut to their limits while importing the layout file).

Tests

  • a_legend_shows_a_layers_label (render): a plain layer's row and a classed layer's heading show their labels, matched in any case; a blank label, or one for a layer the map doesn't have, changes nothing; the legend's measured width is narrower with a short label and wider with a long one, at the same height; the drawing ends within the width measured for the label.
  • legends_label_layers (engine): labels set on layout.addElement and layout.update, entry by entry; null, blank and {}; the refusals above, with nothing changed; labels refused on another kind of element, as an object, a string, a list or a bool, with nothing added; a grid's "labels": "no" refused; a grid's "labels": false still works; a 201-character label refused and a 200-character one kept whole; a layout file with a 250-character label imported with it cut to 200 and the notice naming legend labels, a layout file with Rivers and rivers both labelled imported with one label for the layer and the same notice; no such notice for a clean file; labels kept through project.saveAs / project.open and layout.save / layout.import; not written when empty; following a rename, dropped with a removed layer, and dropped, with the notice, when a layer is renamed onto another layer's name; a WMS layer labelled by name and followed through map.setTiles rename.
  • a_legends_labels_say_when_they_dont_fit (engine): a one-column legend added with a short label says nothing; one added with a label wider than its box, or changed to one, says its end is left out; going back to the name says nothing.
  • The existing legend tests pass unchanged, apart from the extra field and argument.

cargo fmt --check and cargo clippy --workspace --all-targets -- -D warnings pass locally (Windows). The tests of rustgis-engine, rustgis-map and rustgis-render pass except gdal_formats_open_when_gdal_is_installed, which needs GDAL 3.8 and this PC has an older one. The three archive_render_revision tests failed once in the full parallel run and passed when run alone.

Limits

  • No field for this in the legend's properties pane yet. Labels are set by command or script: this is what BrasilData's Layout button needs. A text box per layer in the legend's Layers section would be a follow-up.
  • Only the legend. The Layers pane, the identify popup and exports keep the layer's name. That was the issue's choice 5: a display name on the layer itself would touch every place a name is shown or matched.
  • The box keeps its size. A label shorter than the name leaves the legend's box wider than its rows (as on the right above), as any legend does when what it lists gets shorter; layout.publication sizes the legend it lays out, but a label set afterwards doesn't resize it.
  • No format version change. An older RustGIS opening a project or .rgislayout with labels shows the layers' names, and drops the labels if it saves the file again; no map data is lost. docs/maintaining.md has a format's version go up when a file uses something an older reader would lose, and by its letter this is such a case. The legend's exclude (Layout: a legend can leave layers out though the map draws them (#517, point 1) #518) and columns (feat(layout): a legend in up to three columns, and publication layouts use them for long legends (#1379) #1494) and the text font family (Layouts: a font family for text, sans or serif, from the installed fonts (#517) #675) were added the same way, as optional fields with no version change, and .rgislayout files have no version to raise. Raising the project format for one optional field would make RustGIS 0.6.0 refuse every project this writes, labels or not; a version written only when a legend has labels would avoid that, but would be the first of its kind. If a maintainer wants a version change here, I'll make it.
  • Matching ignores case for ASCII letters only, as exclude and layer_by_name do: Á and á are different keys.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Customize how individual layers are named in layout legends. Labels can be cleared, are matched without regard to capitalization, and persist through layer renames and saved layouts.
    • Custom labels appear across supported legend types and are included in legend sizing and fit warnings.
  • Bug Fixes
    • Legend labels are updated when layers are renamed or removed. Notices now report labels lost during renames and labels shortened to fit layout limits.
  • Documentation
    • Updated the layout guide with instructions and limitations for custom legend labels.

opengeos#1654)

A legend element keeps `labels: {"<layer name>": "<label>"}`, matched by
name in any case as `exclude` is. `layout.update` (and `layout.addElement`
for a legend) take `"labels": {"<layer>": "<label>"|null}`: each entry
replaces that layer's label, null or blank shows its name, {} every name;
a layer the map hasn't is refused. The legend draws and measures the
label (rows, headings, a WMS/WMTS layer's heading), so its size and fit
warnings follow it. Saved with the layout (BTreeMap, skipped when empty);
labels follow a renamed layer and go with a removed one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Layout legends now support per-layer display labels. Layout commands validate and persist label overrides, rendering and sizing use the displayed labels, and layer renames update matching overrides.

Changes: Per-layer legend labels

Layer / File(s) Summary
Store and repair legend labels
crates/map/src/layout.rs, crates/engine/src/project.rs
Legend elements store label overrides with limits on the number and length of labels. Layout repair removes blank and duplicate entries, trims excess entries, and truncates long labels. Repair notices now include cut legend labels.
Accept and update label overrides
crates/engine/src/cmd/layout.rs, crates/engine/src/tests.rs, docs/user-guide/layouts.md
Layout commands accept and validate legend labels against map layers and tile layers. They support clearing labels, updating existing overrides, and checking fit using configured labels. Tests and the guide cover validation, persistence, grid-label interaction, and fit notices.
Render and size legends with labels
crates/render/src/layout.rs, crates/render/src/stream_tests.rs, crates/render/src/svg.rs
Legend rows, class headings, and tile-layer headings use configured display labels. Legend sizing measures the displayed text; rendering tests pass the label map.
Update labels when layers are renamed
crates/engine/src/cmd/layer.rs
A label follows a renamed layer when the destination name is not shared. When the name is shared, the label is not transferred and a notification reports the affected legends.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant LayoutCommand
  participant parse_legend_labels
  participant LegendElement
  participant LegendOptions
  participant legend_rows
  LayoutCommand->>parse_legend_labels: validate layer names and label values
  parse_legend_labels->>LegendElement: store label overrides
  LegendElement->>LegendOptions: pass labels to rendering
  LegendOptions->>legend_rows: resolve display labels
Loading

Possibly related PRs

  • opengeos/RustGIS#1168: This PR adds label overrides to the WMS/WMTS legend headings and uses them for sizing.
  • opengeos/RustGIS#1009: This PR applies label overrides to tile-layer headings that can display service-provided legends.
  • opengeos/RustGIS#1718: This PR extends the layer-rename path to transfer or remove custom legend labels.

Suggested reviewers: giswqs

Merge Risk: Minimal · up to a0932

Legend labels are added with validation, persistence, rendering and rename handling. No actionable merge-blocking risk remains after review.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title clearly identifies the main change: per-layer labels in layout legends.
Description check Passed The description covers the change, testing, visible UI evidence, and related issue. It also explains validation, persistence, and key limitations.
Linked Issues check Passed Issue #1654 requests per-legend labels matched by layer name, without changing the layer name elsewhere; command support; label-based sizing; and persistence in project and layout files. The PR summar…
Out of Scope Changes check Passed The reported changes stay within issue #1654. Command validation, import sanitization, rename and removal handling, fit warnings, tests, and documentation support label behavior, persistence, or sizin…
Docstring Coverage Passed Docstring coverage is 81.48% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Full details: Linked Issues check

Explanation

Issue #1654 requests per-legend labels matched by layer name, without changing the layer name elsewhere; command support; label-based sizing; and persistence in project and layout files. The PR summary reports labels support in layout.addElement and layout.update, case-insensitive matching, clearing labels with null, blank labels, or {}, validation against layers absent from the frame map, label-based rendering and sizing, and persistence with empty maps omitted. It also reports tests for rendering, validation, persistence, and label lifecycle across layer renames and removal. These changes address the issue’s coding requirements.

Full details: Out of Scope Changes check

Explanation

The reported changes stay within issue #1654. Command validation, import sanitization, rename and removal handling, fit warnings, tests, and documentation support label behavior, persistence, or sizing. The PR does not change layer names outside the legend or add a display-name field to layers.

Full details: Docstring Coverage

Explanation

Docstring coverage is 81.48% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 27 functions across 7 files. (2 skipped: 1 unsupported, 1 too large.)

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

A rabbit draws a legend line,
With labels neat and names aligned.
Through map and page, the words stay clear,
A changed layer brings its title near.
Then hops away, content with the design.

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: b5490b04-dbb0-4721-8b09-30a0c011a4c9
Commits

Reviewing files that changed from the base of the PR and between bc603c0 and a09323d.

Files selected for processing: 9

  • crates/engine/src/cmd/layer.rs
  • crates/engine/src/cmd/layout.rs
  • crates/engine/src/project.rs
  • crates/engine/src/tests.rs
  • crates/map/src/layout.rs
  • crates/render/src/layout.rs
  • crates/render/src/stream_tests.rs
  • crates/render/src/svg.rs
  • docs/user-guide/layouts.md

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Comment thread crates/engine/src/cmd/layout.rs Outdated
let label = match label {
Value::Null => None,
Value::String(t) if t.trim().is_empty() => None,
Value::String(t) => Some(t.chars().take(MAX_LEGEND_LABEL_CHARS).collect()),

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.

Silent truncation (AGENTS.md "Never silent"). A label over 200 characters is cut here without any notice, so a script that sends a long label gets it shortened and is never told. The docs say "up to 200 characters", but AGENTS.md requires a command that truncates to say what, how much and where. The test legends_label_layers asserts the cut length, not a notice.

The smallest fix is to refuse over-long labels, as other bad inputs in this function are refused:

Suggested change
Value::String(t) => Some(t.chars().take(MAX_LEGEND_LABEL_CHARS).collect()),
Value::String(t) if t.chars().count() > MAX_LEGEND_LABEL_CHARS => {
return Err(format!("a legend's label is at most {MAX_LEGEND_LABEL_CHARS} characters"));
}
Value::String(t) => Some(t.clone()),

If cutting is kept instead, it needs a notice, and the test should check it. Confidence: high (about 80%).

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.

Fixed in 8795cb7. layout.addElement and layout.update now refuse a label over 200 characters, as you suggest, with a legend's label is at most 200 characters ('Rivers' has 250), and the element is left as it was. A command never cuts a label now; only a file opened or imported is cut, and that one says so (see the thread on sanitize). legends_label_layers checks that a 201-character label is refused with nothing changed and that a 200-character one is kept whole. The command help and docs/user-guide/layouts.md say a longer label is refused.

🤖 Addressed by Claude Code

Comment thread crates/map/src/layout.rs
/// case, as the map spells it): its row, or the name over its classes (#1654). The layer
/// keeps its name everywhere else. A layer not in it shows its name.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
labels: BTreeMap<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.

Question for the maintainers: does this need a format bump? docs/maintaining.md says a format's version goes up "only when a file uses something an older reader would lose". An older RustGIS reads this field as unknown, drops it, and saves the layout again without labels. That is silent loss of the kind the rule is for. .rgislayout files carry no version, and the project's FORMAT in crates/engine/src/project.rs stays at 1.

The earlier legend columns and exclude fields were added the same way without a bump, so this may be a deliberate precedent, but it is not stated anywhere. Please decide it and note the decision in the PR. Confidence: medium (about 50%).

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.

No change here: this is for a maintainer, as you say, and the PR text now says it under Limits ("No format version change"). By the letter of docs/maintaining.md it is a loss: an older RustGIS shows the layers' names, and drops the labels if it saves the file again. No map data is lost. The legend's exclude (#518) and columns (#1494) and the text font family (#675) were added the same way, as optional fields with no version change, and the same question on #1494 was left for a maintainer. .rgislayout files have no version to raise. Raising the project's FORMAT would make 0.6.0 refuse every project this writes, labels or not, since write_format always writes the current one; a version written only when a legend has labels would avoid that, but would be the first of its kind. If a maintainer wants a version change here, I'll make it. Leaving this open for that view.

🤖 Addressed by Claude Code

Comment thread crates/map/src/layout.rs
cut |= labels.len() < labelled;
for label in labels.values_mut() {
let end = label.char_indices().nth(MAX_LEGEND_LABEL_CHARS).map_or(label.len(), |(i, _)| i);
cut |= end < label.len();

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 import notice is now inaccurate for labels. This cut feeds elements_cut, which crates/engine/src/project.rs (layout_notices, not changed by this PR) reports as "a name, text, polygon points or legend exclusions cut to their limits". A label cut on import is therefore described as an exclusion cut. That notice should mention legend labels too. The PR doesn't touch project.rs, so I can't anchor the fix there. Also, no test checks the import notice for a cut label, which AGENTS.md's "test both ways" rule asks for. Confidence: high (about 80%).

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.

Fixed in 8795cb7. The notice in layout_notices now reads … layout element(s) with a name, text, polygon points, legend exclusions or legend labels cut to their limits …, and the doc comment on LayoutRepairs::elements_cut says the same. legends_label_layers now checks both ways: importing the saved .rgislayout as it is gives no such notice, and importing it with a 250-character label gives one notice naming legend labels, with the label cut to 200 characters.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs

  • No confirmed correctness bugs. I traced label-following through rename, delete, and WMS/WMTS rename, the labels update path, and the element-lock check, and found no logic errors. The legend_inputs state shape and the legend_labels call sites look correct. (Medium-high confidence.)

Security

  • None found. SVG output escapes &, <, >, ", and ', so label text is safe.

Performance

  • Minor: LegendOptions::shown falls back to a linear case-insensitive scan of labels for every unlabelled layer, on each legend draw. With the 1,000-label cap this is a few hundred thousand comparisons worst case. Low confidence that it matters.

Quality

  • legend_sizes_mm takes (exclude, labels) as a tuple argument. A named struct or a separate argument would read better. Nit.
  • legend_inputs stores labels as a list of pairs in state, but element_selectors stores them as an object. Shapes differ for the same data. Nit.
  • element_inputs calls legend_exclude and legend_labels twice. Nit.
  • Left for the release quality pass (per AGENTS.md): these nits.

AGENTS.md

  • Never silent, command path (high, about 80%): legend_labels cuts a label over 200 characters without a notice. Inline comment posted at crates/engine/src/cmd/layout.rs:2972.
  • Never silent, import notice (high, about 80–85%): the layout_notices text in crates/engine/src/project.rs:1741 names only "name, text, polygon points or legend exclusions". Label cuts are reported as exclusion cuts. That file is outside this PR, so I posted the comment at crates/map/src/layout.rs:1391 where the cut is produced.
  • Test both ways (high, about 80%): no test checks the import notice for a cut label, or that clean input gives none.
  • Format version (medium, about 50%): an older RustGIS drops labels silently on resave, and docs/maintaining.md says the format version goes up when an older reader would lose data. FORMAT stays at 1, and .rgislayout has no version field. The earlier columns and exclude fields were added the same way, so this may be a deliberate precedent. Inline comment posted at crates/map/src/layout.rs:942.
  • Silent drops, lower confidence (about 40%): a label is dropped without a notice when its layer is removed or renamed onto a name another layer already has. exclude behaves the same way, so this may be the accepted pattern.

Prior reviews: none on this PR, so I left no earlier concerns alone.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head 1d22259).

Measure Value
Files changed 8 (0 added, 8 modified, 0 removed, 0 renamed)
Size before 3.08 MB
Size after 3.10 MB
Total size change +19.5 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.21 MB +6.3 KB
crates/render/src/layout.rs 214.1 KB 219.4 KB +5.3 KB
crates/engine/src/cmd/layout.rs 219.9 KB 225.0 KB +4.9 KB
crates/map/src/layout.rs 78.9 KB 80.2 KB +1.3 KB
docs/user-guide/layouts.md 31.5 KB 32.7 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267.8 KB 268.5 KB +690 B
crates/render/src/svg.rs 23.8 KB 24.1 KB +306 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

📖 Docs preview

Item Value
Preview https://preview.rustgis.app/pr-1739/
Web app https://preview.rustgis.app/pr-1739/app/
Commit a09323d

GitHub Pages takes a minute or two to publish a new commit.

…ls in the import notice (opengeos#1654)

- layout.addElement and layout.update refuse a label longer than a
  legend keeps instead of cutting it (AGENTS.md "Never silent").
- The notice for layout elements cut on reading now names legend
  labels, which Layout::sanitize also cuts.
- Tests: a 201-character label refused, a 200-character one kept
  whole; an imported layout file with a long label cut, with the
  notice; no notice for a clean file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread crates/map/src/layout.rs
*columns = (*columns).clamp(1, MAX_LEGEND_COLUMNS);
// The same for the names labelled; a label is cut to its length, and a blank
// one shows the name, as it would without.
labels.retain(|_, label| !label.trim().is_empty());

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.

Two spellings of one layer are kept when a layout is read from a file (Never silent / consistency). The command path refuses {"Rivers": …, "rivers": …} because "which one counts is not the caller's to guess", and with_labels keeps one key per layer. This sanitize step only drops blank labels and cuts long ones, so a hand-edited or older .rgislayout with "Rivers" and "rivers" keeps both. shown() then picks the exact-case match or the first case-insensitive one in key order, and legends_follow_layer uses the last. Nothing is reported, and the legend's label depends on key order.

Suggested direction: when two keys match case-insensitively, keep one (the first in key order, say) and count it in cut so layout_notices reports it. A test would import a layout with both spellings and check the notice plus the single kept label. Confidence: medium (about 50%). The file is hand-edited input, so impact is limited, but the rule the PR states for commands should hold for files too.

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.

Fixed in d8f94a7. sanitize now keeps one label per layer: of two spellings of a name (ASCII case), the first in key order stays, here Rivers over rivers, and the other is counted in cut, so layout_notices reports … legend exclusions or legend labels cut to their limits …. legends_label_layers imports a .rgislayout with {"Rivers": "Rios", "rivers": "rios", "Lakes": "Lagos"}, and checks that one notice is given and that the legend keeps Lakes: Lagos and Rivers: Rios. The test fails with the dedupe taken out.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs

  • No new correctness bugs found. I checked the rename and removal paths (legends_follow_layer, drop_layers, the tile renames), the lock check in update, and the new element_inputs/update_inputs state shape. The label logic holds up. (Confidence: high.)

Security

  • None found. The label input is bounded (1,000 layers, 200 characters each), and the new code has no unwrap, expect or unsafe. (Confidence: high.)

Performance

  • None worth raising. legend_labels and legend_exclude run twice in element_inputs, which is negligible for a few-element call. (Confidence: high.)

Quality

  • Inline: Layout::sanitize accepts case-variant duplicate label keys from a file, although the command path refuses them. Silent and order-dependent. (Confidence: medium, about 50%.)
  • Nits, left for the release quality pass: "a legend labels at most … layers" reads ungrammatically (legend_labels, with_labels); the stray comment "(The imported layout is the active one now…)" sits in the middle of the test; docs/user-guide/layouts.md has one overlong wrapped line around the 200-character sentence.

AGENTS.md

  • Compliant on the points I checked: no panicking calls in non-test code, the limits are bounded, both the cut and the clean paths have tests, the PR includes screenshots for the visible change.
  • Still open from earlier, unanswered: whether the new labels field needs a format bump (docs/maintaining.md rule). Not re-raised.

Prior threads: 3 earlier threads on this PR. One remains open and I left it alone: the format-bump question (crates/map/src/layout.rs, line 942). Two are addressed in this head: the import notice now names legend labels, with a test, and over-long labels are refused rather than silently cut, with a test.

Change size

Compared with main (merge base 7cc9bb6 to PR head 8795cb7).

Measure Value
Files changed 9 (added 0, modified 9, removed 0, renamed 0)
Size before 3.18 MB
Size after 3.20 MB
Total size change +21.8 KB
Path Before After Change
crates/engine/src/tests.rs 2.20 MB 2.21 MB +7.7 KB
crates/render/src/layout.rs 214.1 KB 219.4 KB +5.3 KB
crates/engine/src/cmd/layout.rs 219.9 KB 225.1 KB +5.2 KB
crates/map/src/layout.rs 78.9 KB 80.2 KB +1.3 KB
docs/user-guide/layouts.md 31.5 KB 32.7 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267.8 KB 268.5 KB +690 B
crates/render/src/svg.rs 23.8 KB 24.1 KB +306 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

…ngeos#1654)

Layout::sanitize now keeps one of two spellings of a layer's name in a
legend's labels (the first in order) and counts the other as cut, so
the import notice reports it, as a command refuses both spellings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread crates/engine/src/cmd/layout.rs Outdated
return Err(ONLY_LEGEND_COLUMNS.into());
}
// (A grid's "labels" is a bool: only an object is a legend's.)
if kind != "legend" && p.get("labels").is_some_and(Value::is_object) {

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.

labels is refused on other kinds only when it is an object, so other values are silently ignored. exclude and columns refuse any non-null value on a non-legend, and the PR body says labels is "refused like exclude". So layout.addElement {"kind": "title", "labels": "Rivers"} (or ["Rivers"]) is accepted and dropped with no error. The same gap is in layout.update (the _ if p.get("labels").is_some_and(Value::is_object) arm, about line 3255): a string or array labels on a title is ignored.

Suggested fix for this line: refuse anything non-null except a grid's boolean. The update arm should get the same treatment, minus the grid exception, since grid options are nested under "grid" there.

Confidence: about 65%. I did not run this; it is from reading the code and the exclude/columns checks above it.

Suggested change
if kind != "legend" && p.get("labels").is_some_and(Value::is_object) {
if kind != "legend" && p.get("labels").is_some_and(|v| !v.is_null() && !(kind == "grid" && v.is_boolean())) {

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.

Fixed in f0a74f8. On an element that isn't a legend, layout.addElement now refuses labels of any non-null value with only legends label layers, as it does exclude and columns. On a grid, which has its own labels, anything other than true or false is refused with a grid's labels is true or false, instead of the legend's message. layout.update refuses any non-null labels on an element that isn't a legend, since a map frame's grid takes its options under "grid". On a legend, a value that isn't an object was already refused (labels is an object of layer names and labels). legends_label_layers now checks a string, a list and a bool on a title (update) and a scale bar (add), and "labels": "no" on a grid, with nothing added.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs

  • layout.addElement and layout.update accept a non-object labels (a string or array) on non-legend elements and silently ignore it. exclude and columns refuse any non-null value there, so the PR body's "refused like exclude" does not hold. Posted inline at crates/engine/src/cmd/layout.rs:2358, with a suggestion. Confidence: about 65%.
  • A hand-edited layout file with a padded key such as " Rivers" is kept on import but never matches at draw time, because shown() does not trim while the command path does. The label is silently not shown and no notice is given. This mirrors how exclude already behaves for padded file keys. Confidence: about 30%.

Security

  • None found. Labels go through the existing text drawing path, and there are no new unwraps, indexing, or unsafe code in the non-test diff.

Performance

  • None worth raising. Label lookups are bounded by the 1,000-label and 200-character limits, and the linear fallback match runs only on a miss.

Quality

  • The labels state in legend_inputs is a list of pairs, while element_selectors stores the same data as an object. Both are harmless, but the shapes differ.
  • The import notice says "legend labels cut to their limits". Duplicate-spelling and count-cap drops remove labels rather than cut them, so the wording is slightly loose.
  • legend_map binds let s = &context only to rename it.
  • A doc line in docs/user-guide/layouts.md (about line 1130) is much longer than the surrounding wrap.
  • legends_label_layers removes its temp directory only on success.

These are minor and can wait for the release quality pass.

AGENTS.md

  • The "Never crash", "Never silent" (file import notice, 200-character refusal), and "Test both ways" rules are met for the cases the PR tests. The gap in the first Bugs item is the one exception.
  • Layering and wasm constraints look fine: BTreeMap and pop_last are available on all targets, and every constructor and pattern of ElementKind::Legend across the workspace is updated.

Earlier review threads: 4 threads were already on the PR. I left one alone because it was already raised and answered and is still open for a maintainer: the format-version question. The other three are fixed in the current code and have tests, so I did not raise them again: the import notice wording, the two-spellings case on import, and the silent 200-character cut.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head d8f94a7).

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.20 MB
Total size change +22.3 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.21 MB +8.5 KB
crates/render/src/layout.rs 214 KB 219 KB +5.3 KB
crates/engine/src/cmd/layout.rs 220 KB 225 KB +5.2 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
docs/user-guide/layouts.md 31.5 KB 32.7 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267.8 KB 268.5 KB +690 B
crates/render/src/svg.rs 23.8 KB 24.1 KB +306 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

…ds (opengeos#1654)

layout.addElement and layout.update refused "labels" on another kind
of element only when it was an object; a string, list or bool was
left unread. Now any non-null value is refused there, as "exclude"
and "columns" are, and a grid's "labels" that isn't true or false is
refused with its own message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread crates/engine/src/cmd/layer.rs Outdated
// bears that name, whose label (or none) the renamed layer then shares.
let keys: Vec<String> = labels.keys().filter(|n| n.eq_ignore_ascii_case(old)).cloned().collect();
let label = keys.iter().filter_map(|k| labels.remove(k)).last();
if let (Some(n), Some(l), false) = (new, label, shared) {

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.

Possible silent loss of a user's label (AGENTS.md "Never silent"), confidence about 35%. When a layer is renamed onto a name another layer already bears (shared), the label is taken off the old name and not put on the new one, so the text the user typed is discarded with no notice. The docs and doc comment state this as deliberate, and exclude behaves the same way, so this may be an accepted design choice. But Session::notify (one notice per kind, with a count) is the rule for a command that drops data, and a label is user-entered text. Either a notice when a non-empty label is dropped here (and on layer.remove at the None path), or a note in the PR that a label is treated as presentation state and not data. I did not find a test that covers the dropped-label case with a notice.

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.

Fixed in 3f959e5. Renaming onto a name another layer has is allowed (layer.rename doesn't refuse it), and the label was dropped without a word. It is now said once per rename, with the count: 'Hidrografia', renamed 'Cities', lost its label in 4 legend(s): another layer is called 'Cities', and a legend labels layers by name. legends_label_layers checks the notice: four legends there, the original and three imported layouts. It also checks that a plain rename and a removal give no such notice. On layer.remove the label goes with the layer's row, as its exclusion does, so nothing typed is lost while the layer stays; I left that path as it is.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed the full PR diff (9 files), the staged repository guidelines, and the surrounding code for the changed paths. I could not build or run the tests in this environment, so the findings below come from reading the code.

Bugs

  • No confirmed bugs. Checked: legend_sizes_mm has no remaining callers on the old signature, and ElementKind::Legend constructions in ui-egui use ... Project open also runs Layout::clamp (project.rs:911), so labels are sanitized on open as well as on import. The label truncation in clamp cuts on char boundaries.
  • Possible silent label loss, crates/engine/src/cmd/layer.rs:4821 (inline comment posted). When a layer is renamed onto a name another layer already bears, its label is dropped with no notice. Confidence about 35%. The behaviour is documented and mirrors exclude, so it may be intended.

Security

  • None found. The new input is bounded (1000 labels, 200 characters each), refused before any mutation, and has no unwrap or indexing in non-test code.

Performance

  • LegendOptions::shown falls back to a linear case-insensitive scan for every row whose name isn't an exact key. With up to 1000 labels and hundreds of rows, that runs on every draw and every sizing pass. Impact is small in practice. Confidence about 25%.

Quality

  • The error text a legend labels at most {N} layers reads badly. Suggest "a legend can label at most N layers". Confidence high.
  • docs/user-guide/layouts.md around line 1143 has a line well past the wrap width of the surrounding bullet. Nit.
  • Layout::clamp counts the case-insensitive dedupe as "cut to their limits", which is slightly imprecise wording for a merge. Nit.

These are nits. Per AGENTS.md they can wait for the release quality pass, and I did not post them inline.

AGENTS.md

  • No violations found. Refusals replace silent truncation, the import notice names legend labels, the file-based tests cover both clean and lossy input, and hostile_params_never_panic covers the new parameters. No new background command was added, so the cancellation list needs no change.

Prior review threads

  • 5 earlier concerns left alone: the format-version question (answered, left for a maintainer), the import notice, two-spellings handling on import, silent truncation on commands, and labels ignored on other element kinds. All five are addressed in the current code or explicitly deferred to a maintainer.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head f0a74f8).

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.20 MB
Total size change +23.8 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.21 MB +9.1 KB
crates/engine/src/cmd/layout.rs 219.9 KB 225.4 KB +5.5 KB
crates/render/src/layout.rs 214.1 KB 219.4 KB +5.3 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
docs/user-guide/layouts.md 31.5 KB 32.7 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267.8 KB 268.5 KB +690 B
crates/render/src/svg.rs 23.8 KB 24.1 KB +306 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

…s#1654)

A layer renamed onto a name another layer has is listed under that
one's label (or name), so its own label is dropped. That is now said:
"'Hidrografia', renamed 'Cities', lost its label in 4 legend(s):
another layer is called 'Cities', and a legend labels layers by name".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
all_classes: bool,
hide_other: bool,
exclude: &[String],
(exclude, labels): (&[String], &BTreeMap<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.

Nit (quality, confidence about 70%). legend_sizes_mm now takes (exclude, labels) as one tuple parameter. That isn't needed: the function would have six parameters, under clippy's default too_many_arguments limit of seven. Two plain parameters read more clearly at the call sites, which pass (&[], &NO_LABELS) and (&exclude, &labels). Left for the release quality pass if the maintainers prefer.

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.

Left as it is for now. The tuple keeps the two things a legend lists by layer, what it leaves out and what it renames, together at the call sites, and it is under clippy's limit either way. If a maintainer prefers two parameters I'll split it, or it can go in the release quality pass, as you say.

🤖 Addressed by Claude Code

None => None,
},
// (A map frame's grid takes its labels under "grid".)
_ if p.get("labels").is_some_and(|v| !v.is_null()) => return Err(ONLY_LEGEND_LABELS.into()),

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.

Unclear error for grid labels (quality, confidence about 50%). layout.addElement takes a grid's labels (true or false) at the top level, but layout.update on a map frame refuses any top-level labels with only legends label layers. The grid's other options go under "grid", and the message doesn't say so. A script that sends {"element": <grid>, "labels": false} is told the element isn't a legend, when the fix is {"grid": {"labels": false}}. Suggest naming the place in the message for a map frame, e.g. a map frame's grid labels go under "grid", and keeping the refusal itself, which is correct under "Never silent".

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.

Fixed in 2b98ad1. On a map frame, layout.update with a top-level labels now refuses with a map frame's grid labels go under "grid": {"grid": {"labels": false}}. Every other element that isn't a legend still gets only legends label layers. legends_label_layers checks the message, and checks that {"grid": {"labels": true}} on the same frame sets them.

🤖 Addressed by Claude Code

Comment thread docs/user-guide/layouts.md Outdated
it, and a layer the map hasn't is refused. Each entry replaces that layer's label and leaves
the others; `null` or a blank label shows the layer's name again, and `{}` every layer's. The
label goes in the layer's row, or over its classes; a WMS or WMTS layer's goes over its
picture. It is up to 200 characters (a longer one is refused), and the legend measures it, not the name, so sizing a

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 (docs). This line is about 120 columns, while the rest of the list wraps near 100. Rewrapping it would keep the diff readable.

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.

Fixed in 2b98ad1: the bullet is rewrapped near 100 columns. It also now says that a renamed layer loses its label when another layer has its new name, which the notice added in 3f959e5 reports.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

I found no bugs, crash paths, or silent data loss in the changed code. The three prior-review fixes hold in the code. The findings below are quality nits.

Bugs

None found. I checked the legends_follow_layer rename and remove paths, the element_inputs else-branch (reached only for legends), sanitize cut counting, the lock check in update, and the legend_sizes_mm tuple coercions.

Security

None found. Label text reaches SVG output through the existing esc path, and new input is bounded (1000 labels, 200 characters each, refused rather than cut).

Performance

None worth raising. The case-insensitive fallback in LegendOptions::shown is a linear scan over at most 1000 labels per row, which is negligible.

Quality

  • Tuple parameter in legend_sizes_mm (crates/render/src/layout.rs:1973, confidence about 70%). The (exclude, labels) tuple is unnecessary: six parameters is under clippy's default limit of seven. Left for the release quality pass.
  • Unclear error for grid labels (crates/engine/src/cmd/layout.rs:3259, confidence about 50%). layout.update on a map frame refuses a top-level labels with "only legends label layers". A grid's labels go under "grid" in update, and the message doesn't say so.
  • Docs wrap (docs/user-guide/layouts.md:211). The line is about 120 columns against the list's usual 100. Nit.

AGENTS.md

  • The labels refusal, the rename notice, the bounded inputs, and the tests for both the clean and lossy paths follow the "Never silent" and "Never crash" rules.
  • The rename notice uses format! rather than i18n::fmt. Engine notices already do this (see project.rs layout_notices), so I did not raise it.
  • Screenshot and PR-description requirements are met.

Prior review threads

I left 6 earlier concerns alone. The format-version question is answered by the author and left for maintainers. The other five were fixed and verified in the code: the import notice, two-spelling dedupe, silent truncation, the update arm, and the rename notice. I did not re-post any of them.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head 3f959e5). Sizes are in KB = 1024 bytes.

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.21 MB
Total size change +24.9 KB
File Before After Change
crates/engine/src/tests.rs 2258.1 KB 2267.7 KB +9.6 KB
crates/engine/src/cmd/layout.rs 219.9 KB 225.4 KB +5.5 KB
crates/render/src/layout.rs 214.1 KB 219.4 KB +5.3 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
crates/engine/src/cmd/layer.rs 267.8 KB 269.0 KB +1.2 KB
docs/user-guide/layouts.md 31.5 KB 32.7 KB +1.2 KB
crates/render/src/svg.rs 23.8 KB 24.1 KB +0.3 KB
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

…abels docs (opengeos#1654)

- layout.update with a top-level "labels" on a map frame now says
  where they go ({"grid": {"labels": false}}) instead of "only legends
  label layers".
- docs: the labels bullet wrapped near 100 columns, and it says a
  renamed layer loses its label when another layer has its new name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
let hide_other = b(p, "hideOther").unwrap_or(false);
let exclude = legend_exclude(s, l, frame, p)?.unwrap_or_default();
let columns = legend_columns_param(p)?.unwrap_or(1);
let labels = match legend_labels(s, l, frame, p)? {

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.

A legend added with labels isn't checked against its box (Never silent, confidence about 80%).

layout.update runs say_legend_fit when a legend's labels change, but add_element runs it only when columns is given (crates/engine/src/cmd/layout.rs:2598: if p.get("columns").is_some_and(|v| !v.is_null())). So layout.addElement {"kind": "legend", "labels": {"Rivers": "<a label wider than the 60 mm box>"}} adds a single-column legend that draws the label past its box, with no warning. legend_fit_warnings exists to say exactly that: "its last rows or the ends of long labels are left out".

Suggested fix, at line 2598 (outside this diff's hunks, so not a committable suggestion here):

if p.get("columns").is_some_and(|v| !v.is_null()) || p.get("labels").is_some_and(|v| !v.is_null()) {

A test for it would add a legend with a label wider than the box and check that a fit notice is given, and that a short label gives none. legends_label_layers checks the fit warning for update nowhere, so the add path has no coverage either way. Whether a legend added with no columns or labels should be checked too is a broader, pre-existing question, not part of this change.

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.

Fixed in a09323d. It was wider than the add path: layout.update also checked the fit only for a legend in columns (in_columns && refitted) or when columns was given, so changing labels on a one-column legend wasn't checked either. Both commands now run say_legend_fit whenever a legend is given labels, in one column too, so a label wider than its box gives … its last rows or the ends of long labels are left out. The new test a_legends_labels_say_when_they_dont_fit checks four cases: a legend added with a short label (no notice), one added with a label wider than its box (the notice), the first changed to that label (the notice), and the label taken off (no notice). It fails without the fix. I left out a legend added or changed with neither columns nor labels, which is the pre-existing question you mention.

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Bugs

  • layout.addElement with labels skips the box-fit check, so a label wider than the box is cut off with no warning. add_element calls say_legend_fit only when columns is given (crates/engine/src/cmd/layout.rs:2598). layout.update does check labels. Posted inline (crates/engine/src/cmd/layout.rs:2408). Confidence: about 80%.

Security

  • None found. Label text is escaped in SVG output (crates/render/src/svg.rs:307), and the new parameters go through bounded, error-returning parsing with no unwrap or panic in non-test code.

Performance

  • None worth raising. The per-row label lookup is linear over at most 1,000 labels, which is fine.

Quality (left for the release quality pass; not posted)

  • Inputs.state stores labels as [[name, label|null]] pairs (legend_inputs), while element_selectors stores them as an object. Both are internal, so this is a readability nit only. Confidence: about 40%.

AGENTS.md

  • I checked the Never crash, Never silent, Never hold the window, wasm and translation rules against the changed code and found no other violations.
  • Existing UI callers are unaffected: the grid's Labels checkbox sends {"grid": {...}}, and the legend write-back sends only exclude.
  • The ElementKind::Legend call sites outside the diff all use .., so they still compile.

Prior threads

  • I left 8 earlier concerns alone, because the author fixed them in the diff or answered them:
    • the project format bump (answered; open for a maintainer)
    • the import notice
    • the two-spellings dedupe
    • the tuple-parameter nit (left for the quality pass)
    • the grid-labels message
    • the silent truncation
    • the non-legend labels refusal
    • the docs wrap
  • The rename-drops-label concern is fixed. The removal-drops-label path is answered by the author with a reason, so I did not re-raise it.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head 2b98ad1).

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.21 MB
Total size change +25.1 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.22 MB +10.1 KB
crates/engine/src/cmd/layout.rs 220 KB 226 KB +5.7 KB
crates/render/src/layout.rs 214 KB 219 KB +5.3 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
docs/user-guide/layouts.md 31.5 KB 32.8 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267 KB 269 KB +1.2 KB
crates/render/src/svg.rs 23.8 KB 24.1 KB +300 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +118 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

…pengeos#1654)

layout.addElement checked a legend's fit only when "columns" was
given, and layout.update only for a legend in columns. A legend given
labels is now checked in one column too, added or changed, so a label
wider than its box says that its end is left out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|| p.get("exclude").is_some_and(|v| !v.is_null())
|| p.get("labels").is_some_and(|v| !v.is_null());
// Labels given are told in one column too: a label is text chosen for the box.
let labelled = matches!(&e.kind, ElementKind::Legend { .. }) && p.get("labels").is_some_and(|v| !v.is_null());

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.

Performance (confidence about 45%, plausible). Giving a legend labels now runs say_legend_fit, which calls legend_sizes_mm with all_classes false. That goes through complete_shown_legends (crates/render/src/layout.rs), which does a full visit_complete pass over every chunked source (GeoPackage, GeoParquet, PostGIS and similar) on the calling thread. Before this PR only a columns change reached that path. Now a labels-only layout.update on a legend over a large chunked layer scans the whole source synchronously, with no progress or Cancel.

Suggested direction: either skip the fit check for label-only changes, or say in the doc comment why this command stays on the UI thread and how long a large source may take. The same applies to the add path at line 2600.

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.

Measured with this branch's rustgis-cli (a09323d, optimized dev build, Windows), median of three runs:

Project Base run One labels-only layout.update One columns update (path before this PR)
Sample World Atlas, in-memory layers 0.53 s 20 updates in 0.50–0.63 s: within noise, under 5 ms each —
A streamed shapefile, 300,000 polygons (41 MB .shp, over STREAM_ABOVE) 0.48 s 0.97 s: about +0.47 s each (5 updates: 2.8 s) 0.97 s: the same +0.47 s

So a labels change costs what a columns change already costs: one pass over the chunked source. That is about half a second for 300,000 polygons, and nothing measurable for in-memory layers. This is the price of the fit warning, which the "Never silent" thread above asked for. Skipping it for labels would bring back the silent cut. Whether say_legend_fit should move off the frame with progress and Cancel applies to it as a whole, through columns and in-column w/h/title changes too, not to labels alone. I'd leave that to a maintainer as a separate change, so no change here.

🤖 Addressed by Claude Code

Comment thread crates/map/src/layout.rs
*columns = (*columns).clamp(1, MAX_LEGEND_COLUMNS);
// The same for the names labelled; a label is cut to its length, and a blank
// one shows the name, as it would without.
labels.retain(|_, label| !label.trim().is_empty());

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.

Never silent (minor, confidence about 30%). A blank label read from a file is dropped here before labelled is counted, so it never reaches cut and no notice is given. Blank labels show the layer's name either way, so little is lost, but this is stored data dropped without a count, and the tests cover the cut and dedupe notices but not this one. Either count blank drops into cut, or say in the doc comment that they are not data.

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.

No change here, and that's on purpose. A blank label is no label: a command treats "" as null (it shows the layer's name again), and the legend draws a blank label as the name (LegendOptions::shown filters blank labels out). So dropping one on read changes nothing drawn, saved or matched. The comment above the retain says so: "a blank one shows the name, as it would without". Counting it in cut would make the import notice report "legend labels cut to their limits" for a file whose legend comes out the same, which would be noise. The PR text lists it under Limits ("Blank labels are dropped").

🤖 Addressed by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Code review

I found no bugs, crash paths, or security issues in the changed lines. The rename and label-following logic, the command validation order, the import sanitize, and the render path all check out. I did not run the test suite; this review is from reading the diff and the surrounding source.

Bugs

  • None found.

Security

  • None found.

Performance

  • Full source scan on label changes (confidence about 45%, plausible). Giving a legend labels now runs say_legend_fit, which calls legend_sizes_mm with all_classes false. That goes through complete_shown_legends, which reads every chunked source (GeoPackage, GeoParquet, PostGIS and similar) in full on the calling thread. Before this PR only a columns change reached that path. Posted inline at crates/engine/src/cmd/layout.rs:3430.

Quality

  • Shape of labels in the review state (confidence about 40%, nit). legend_inputs writes labels as a list of [name, label|null] pairs, but element_selectors, and the stored value in update_inputs, write it as an object. The same setting reads differently in the requested and stored states. Not posted; it is a nit, so it can wait for the release quality pass.

AGENTS.md

  • Never silent: blank labels dropped on import (confidence about 30%, minor). Layout::sanitize drops blank labels before counting cuts, so no notice is given, and no test covers it. Posted inline at crates/map/src/layout.rs:1383.

Earlier concerns

  • I left 10 earlier review threads alone. Each one is either answered by the author with a fix, or still open for a maintainer. The open ones are the format-version question, the silent drop of a label when its layer is removed, and the tuple-parameter nit in legend_sizes_mm. None has new evidence in this diff, so I did not re-raise them.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head a09323d).

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.21 MB
Total size change +26.9 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.22 MB +11.2 KB
crates/engine/src/cmd/layout.rs 220 KB 226 KB +6.1 KB
crates/render/src/layout.rs 214 KB 219 KB +5.3 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
docs/user-guide/layouts.md 31.5 KB 32.8 KB +1.2 KB
crates/engine/src/cmd/layer.rs 268 KB 269 KB +1.2 KB
crates/render/src/svg.rs 23.8 KB 24.1 KB +306 B
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +121 B
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

@elpidioassinaturas
elpidioassinaturas marked this pull request as ready for review November 11, 2026 00:35
@github-actions

Copy link
Copy Markdown
Contributor

Code review

I found no new bugs, security issues, or AGENTS.md violations in the changed lines, so I posted no inline comments. This was a static read: I could not build or run the tests here. Checked: the sanitize cut (char-boundary truncation, case-insensitive dedupe, and that the cut count feeds the notice), the rename and remove label-following with its notice, that the lock check at layout.rs:3100 covers label-only updates, that the element_inputs state branch only runs for legends, and that every ElementKind::Legend struct literal in the repo now sets labels.

Bugs: none found. Confidence: high for the paths read.

Security: none. Labels are display text, bounded by count and length, and never used as paths or commands.

Performance: no new finding. A label-only layout.update or layout.addElement now runs the chunked-source fit scan on the UI thread. This was raised at layout.rs:3430 and left alone (see below).

Quality (nits for the release quality pass, confidence about 40%):

  • The error text a grid's labels is true or false reads awkwardly; are would be correct.
  • The test comment (The imported layout is the active one now; the rest goes to the first.) is unclear.

AGENTS.md:

  • Saved-file rule (older RustGIS drops labels on save, no format bump). docs/maintaining.md allows optional fields whose loss "costs a setting rather than data", which supports the PR's choice. Already raised and answered, so left alone. Confidence the choice is acceptable: about 60%.
  • "Never hold the window": update and add_element have no doc comment saying why the labelled fit check runs on the UI thread. The author measured about 0.5 s for 300,000 polygons and deferred the question, so the rule is still open. Already raised at layout.rs:3430 and not re-posted. Confidence: about 60%.

Earlier concerns: I left alone 12 prior review threads, all of which have replies from the PR author. Two of them still need a maintainer decision: the format-version question and the UI-thread scan above.

Change size

Compared: the whole PR against main (merge base 7cc9bb6 to PR head a09323d).

Measure Value
Files changed 9 (0 added, 9 modified, 0 removed, 0 renamed)
Size before 3.18 MB
Size after 3.21 MB
Total size change +27.5 KB
Path Before After Change
crates/engine/src/tests.rs 2.21 MB 2.22 MB +11.5 KB
crates/engine/src/cmd/layout.rs 219.9 KB 226.1 KB +6.1 KB
crates/render/src/layout.rs 214.1 KB 219.4 KB +5.3 KB
crates/map/src/layout.rs 78.9 KB 80.6 KB +1.7 KB
docs/user-guide/layouts.md 31.5 KB 32.8 KB +1.2 KB
crates/engine/src/cmd/layer.rs 267.8 KB 269.0 KB +1.2 KB
crates/render/src/svg.rs 23.8 KB 24.1 KB +0.3 KB
crates/render/src/stream_tests.rs 62.3 KB 62.4 KB +0.1 KB
crates/engine/src/project.rs 100.8 KB 100.8 KB +15 B

@giswqs
giswqs merged commit b546388 into opengeos:main Nov 11, 2026
21 checks passed
giswqs added a commit that referenced this pull request Nov 11, 2026
#1725 added a call to legend_sizes_mm with an empty slice for its last
argument while #1739 changed that argument to a pair of the excluded
layers and the labels, so main no longer builds. The call measures the
legend at another scale, so it passes the same empty pair as the
measurement at the asked scale above it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Layout legend: a label per layer, instead of the layer's name

3 participants