[jgregoriorincon]

Summary

Follow-up to #967, from CodeRabbit's review there. The note under SQL Query in the Labeling pane's Content tab, "Each class labels the features its own clause matches, so a feature can get a label from more than one.", showed whenever the layer had any class besides Default, even a hidden one. In that case only one class labels the layer, so overlaps cannot happen.

The note now shows only when more than one class is visible. It counts them with labels.in_order(), the classes the map draws (the extra ones up to MAX_LABEL_CLASSES, then Default). The order of the notes is unchanged: labels off first, then a hidden class, then this rule.

It counts visibility only, not the scale range, so the note doesn't come and go with the zoom. A class on a missing field already has its own notes in the Field box and the SQL box.

Testing

• cargo xtask ci.
• the_sql_tab_says_what_else_decides_the_labels adds a step: with Default chosen and visible, hiding the extra class "Big" removes the rule. That step fails with the old condition.
• Manual: one class (no note), a visible extra class (rule), a hidden extra class with Default chosen (no note), making it visible again (rule back), and everything hidden (only the hidden-class note).

Related issue

Closes #993.

Summary by CodeRabbit

• Bug Fixes
  • The SQL pane now shows the multiple-class note only when more than one class is visible. Hiding the only additional class suppresses the note.

[coderabbitai]

No actionable comments were generated in the recent review.

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: 50aa44d1-f55d-4316-b981-fba950189790

Commits

Reviewing files that changed from the base of the PR and between 249c61a and ea44918.

Files selected for processing: 1

• crates/ui-egui/src/labelpane.rs

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

Walkthrough

The SQL pane now displays the multiple-class note only when more than one class in labels.in_order() is visible. The test hides the extra class and checks that the note is absent.

Changes: Label visibility note

Layer / File(s)	Summary
Update and test the SQL note condition crates/ui-egui/src/labelpane.rs	The note condition now checks for multiple visible classes. Comments clarify the visible-class wording and note order. The test verifies that hiding the extra class suppresses the note.

Priority: Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

• opengeos/RustGIS#967: This PR updates the condition for the SQL note introduced in #967.
• opengeos/RustGIS#789: This PR uses class visibility state added by #789.
• opengeos/RustGIS#737: This PR changes when the multi-class note introduced in #737 appears.

Suggested reviewers: giswqs

Merge Risk: Minimal · up to ea449

No actionable merge-blocking risk remains for this focused visibility-note change.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title names the change: the class rule note in the Labeling pane's SQL Query section now appears only when more than one class is visible. It uses the conventional fix(scope): prefix and states th…
Description check		The description follows the template. The Summary section explains what changed and why, including the prior behavior and the new visibility rule. The Testing section lists the CI command, the updated…
Linked Issues check		The change satisfies #993. In crates/ui-egui/src/labelpane.rs, the SQL note now appears only when labels.in_order() contains more than one visible class. This includes Default and matches the map-…
Out of Scope Changes check		The pull request changes only the SQL note condition, related comments, and the focused automated test in crates/ui-egui/src/labelpane.rs. These changes directly support #993 and do not show unrelat…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…

Full details: Title check

Explanation

The title names the change: the class rule note in the Labeling pane's SQL Query section now appears only when more than one class is visible. It uses the conventional fix(scope): prefix and states the main behavior change. It is specific enough for a teammate scanning history to understand the primary change.

Full details: Description check

Explanation

The description follows the template. The Summary section explains what changed and why, including the prior behavior and the new visibility rule. The Testing section lists the CI command, the updated test step, and the manual checks. The Related issue section links issue #993 with a closing keyword. Each required section is filled in with specific detail.

Full details: Linked Issues check

Explanation

The change satisfies #993. In crates/ui-egui/src/labelpane.rs, the SQL note now appears only when labels.in_order() contains more than one visible class. This includes Default and matches the map-drawn class set. The existing note order remains unchanged. The test the_sql_tab_says_what_else_decides_the_labels hides the extra Big class and verifies that the note is absent when only Default remains visible.

Full details: Out of Scope Changes check

Explanation

The pull request changes only the SQL note condition, related comments, and the focused automated test in crates/ui-egui/src/labelpane.rs. These changes directly support #993 and do not show unrelated behavior changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.

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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each class in view,
The hidden one no note is due.
When two are shown, the words appear,
When one remains, the pane is clear.
The rabbit hops, its review complete.}

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Code review

No blocking findings. I checked the change against Labeling::in_order() (crates/map/src/symbology.rs:2004), the Class wrapper and branch order in crates/ui-egui/src/labelpane.rs, the class parameter of labels.class.update (crates/engine/src/cmd/labels.rs), and the new test step, which fails on the old !labels.classes.is_empty() condition.

Bugs

• None found. The branches still run in the order the note's comment describes: labels off, then a hidden class, then the rule note. With Default hidden and one extra visible class, the rule note is correctly absent. Confidence: high.

Security

• None found.

Performance

• None. The count walks at most 65 entries per frame.

Quality

• Low-confidence nit: the rule note counts visible classes whatever their field or scale range. A visible class that labels nothing, because its field is missing or the zoom is outside its range, can still trigger "a feature can get a label from more than one" when no overlap is possible. The PR states the scale-range choice and points to the Field box for missing fields, so this looks deliberate. No change requested. Confidence: medium that it is intended.
• Optional nit: .filter(|k| k.visible).count() > 1 could stop early with .nth(1).is_some(). Not worth changing. Confidence: high.

AGENTS.md

• Compliant. No unwrap, expect or panic is added to non-test code. The note string is unchanged and still wrapped in tl!, so no catalog entry is needed. No dependencies changed. The added test code uses unwrap, as the surrounding tests already do.

No inline comments: nothing here warrants one.

Change size

Compared: the whole PR against main (merge base 249c61a to PR head ea44918).

Measure	Value
Files changed	1 (0 added, 1 modified, 0 removed, 0 renamed)
Size before	80.0 KB
Size after	80.4 KB
Total size change	+379 B

Path	Before	After	Change
crates/ui-egui/src/labelpane.rs	80.0 KB	80.4 KB	+379 B

[github-actions]

📖 Docs preview

Item	Value
Preview	https://preview.rustgis.app/pr-994/
Web app	https://preview.rustgis.app/pr-994/app/
Commit	ea44918

GitHub Pages takes a minute or two to publish a new commit. Newer commits from the fork aren't published until a maintainer adds the preview label (once per commit).

[github-actions]

Code review

Reviewed the one-file change to crates/ui-egui/src/labelpane.rs (+8/−4). The new condition counts visible classes with labels.in_order(), which matches what the map draws (capped at MAX_LABEL_CLASSES, Default last). The logic is correct against the PR's stated intent, and I found no defects. I checked it by reading the code, not by running tests.

Bugs: None found. Checked the hidden-class and labels-off branches (unchanged, and they take precedence), the case of Default hidden with one extra visible class (no note, covered by the existing test), and the case of Default plus one visible extra class (note shown). in_order() also applies the same MAX_LABEL_CLASSES cap as the renderer, so the count can't exceed what is drawn. Confidence: high.

Security: None found. The change only alters a UI condition and a note's visibility; no input handling is touched.

Performance: None found. The count runs once per UI frame over at most 65 classes.

Quality:

• The note ignores each class's visibility range (scale). At some zoom levels it can say "more than one label" when only one class is drawn. The PR body states this is intentional, to avoid the note flickering with zoom, so this is informational only. Confidence: high that it is intended; no change requested.
• The new test step covers the regression case: with Default visible and an extra class "Big" hidden, the rule note must go away.

AGENTS.md: Compliant. No new user-visible literals (the string is unchanged, so no catalog work is needed), no unwrap/panic in non-test code, no layering or dependency changes. The .unwrap() calls are in the test module, which is allowed. The existing docs (docs/user-guide/cartography.md, around line 351) don't contradict the new behaviour.

No inline comments were posted, because there is nothing that needs a line-level fix.

Change size

Compared: the whole PR against main (merge base 249c61a to PR head ea44918).

Measure	Value
Files changed	1 (added 0, modified 1, removed 0, renamed 0)
Size before	80.0 KB
Size after	80.4 KB
Total size change	+379 B

Path	Before	After	Change
crates/ui-egui/src/labelpane.rs	80.0 KB	80.4 KB	+379 B