Skip to content

feat(labels): the ribbon's label groups edit and choose the Labeling pane's class - #863

Merged
giswqs merged 2 commits into
opengeos:mainfrom
jgregoriorincon:feat/ribbon-label-class
Oct 25, 2026
Merged

giswqs merged 2 commits into
opengeos:mainfrom
jgregoriorincon:feat/ribbon-label-class

Conversation

@jgregoriorincon

@jgregoriorincon jgregoriorincon commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Step 1 of #858. The ribbon's label groups (Label Range, Label Placement, Label Class, Text Symbol) edited Default while the Labeling pane edited the class chosen in it, so with a class chosen the two showed and changed different classes. Now there is one chosen class:

  • Style › Label Class gets a Class chooser above Field: the same chooser as the pane's (labelpane::class_combo), on the same per-layer choice, so a class chosen in either is chosen in both. Managing classes (New, Rename, Move, Remove) stays in the pane.
  • Every ribbon label control reads and writes the chosen class: scale and distance limits, Clear Limits, placement, field, size, colour, bold and halo. Their commands name it in class; on Default they send none, so recorded steps stay as before.
  • The ribbon's size drag keeps the class it began on, as the colour drag already did: one left pending on another layer or class is finished first, and a class removed mid-drag is left alone.

No engine change: every labels.* command already takes class. The user guide's label classes section says the ribbon edits the chosen class.

Testing

  • cargo xtask ci passes.
  • New tests in ribbon.rs: the ribbon and the pane choose one class (the_ribbon_and_the_pane_choose_one_class); the ribbon edits the chosen class, naming it, and Default's steps name none (the_ribbon_edits_the_chosen_class); a size drag on a class is one undo step on it (ribbon_label_size_drag_on_a_class_is_one_step_on_it). Falling back to Default when the chosen class goes is labelpane::chosen's, already tested in the pane.
  • Manually: with a class added in the pane, chose it in the ribbon and in the pane; dragged the size, picked a colour, changed placement, bold, halo and range from the ribbon on it, with Default left as it was; removed it and the ribbon showed Default.

Related issue

Part of #858.

Summary by CodeRabbit

  • New Features

    • The ribbon’s label controls now apply changes to the selected label class, including scale, distance, placement, field, style, and limits.
    • Choose a class from the ribbon or the Labeling pane; both selections stay synchronized.
    • Label-size previews and commits are associated with the selected class and layer.
  • Documentation

    • Updated the cartography guide to explain that ribbon label controls edit the class selected in the Labeling pane.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: d2a51c8b-8cad-4d25-867e-55defb7a858c
Commits

Reviewing files that changed from the base of the PR and between 9a88d22 and cf2764b.

Files selected for processing: 3
  • crates/ui-egui/src/labelpane.rs
  • crates/ui-egui/src/ribbon.rs
  • docs/user-guide/cartography.md

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

Walkthrough

The Labeling pane and ribbon now share the selected label class. Ribbon label controls read and update that class, including during size drags. The user guide describes the shared selection.

Changes: Shared label class editing

Layer / File(s) Summary
Resolve and share class selection
crates/ui-egui/src/labelpane.rs
The pane resolves its selected class and uses a shared chooser. If the saved class name is absent or no longer matches, the selection falls back to Default.
Apply ribbon controls to selected class
crates/ui-egui/src/ribbon.rs, docs/user-guide/cartography.md
Ribbon label controls use the selected class for settings and size drags. Tests cover synchronized selection, class-specific edits, and drag undo behavior. The guide describes the shared chooser and selection.

Priority: Normal

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

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  actor Operator
  participant ClassChooser
  participant LabelingPane
  participant RibbonLabelGroups
  participant LabelClass
  Operator->>ClassChooser: Select a named class or Default
  ClassChooser->>LabelingPane: Update shared class selection
  RibbonLabelGroups->>LabelClass: Read selected class parameters
  RibbonLabelGroups->>LabelClass: Write class-specific parameters
Loading

Possibly related issues

  • opengeos/RustGIS#858 — The ribbon controls edit the class selected in the Labeling pane, with Default as the fallback.
  • opengeos/RustGIS#722 — The ribbon now follows the per-layer class selection and falls back to Default when the selected class is removed.
  • opengeos/RustGIS#651 — The changes test size-drag preview, commit, and undo behavior for the selected class.

Possibly related PRs

  • opengeos/RustGIS#789 — That PR adds the Labeling pane’s class chooser, which this change exposes for reuse by the ribbon.

Suggested reviewers: giswqs

Merge Risk: Minimal · up to cf276

The ribbon’s inspected limit-clear action edits the chosen class. No issue requiring a change before merge was established; normal checks still apply.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 79.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 2 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: ribbon label groups now edit and choose the Labeling pane's class.
Description check Passed The description is complete. It explains the change, testing, manual verification, and related issue. It also documents class-specific behavior, fallback behavior, and drag handling.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 79.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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 picks a class with care,
The ribbon follows everywhere.
A size drag moves, then settles right,
One class stays changed through undo’s flight.
The Default waits, untouched and fair.

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

@github-actions

Copy link
Copy Markdown
Contributor

Code review

I found nothing that needs changing. I read the whole diff (labelpane.rs, ribbon.rs, the docs change) and the surrounding code. I could not run the tests or the build here, so none of this was executed.

What I checked

  • Bugs
    • chosen() now returns a Class that borrows the layer. The ribbon and the pane both resolve the class from the same ui.data temp key, so they stay in sync. A class that was removed or renamed falls back to Default.
    • The size-drag preview and the commit are keyed on layer and class. apply_label_size_drag bails out if the layer or class has gone, and it restores the original size before labels.style runs, so Undo captures the right value. A drag that began on another class is committed before a new one starts.
    • push_id(("label_size", layer.id, c.name)) and the matching colour id give each class its own widget. An in-flight drag cannot jump to another class.
    • Default's steps still carry no class param, because named only inserts one for Some. Recorded macros are unchanged, and the new test asserts this.
    • Both ComboBox::from_id_salt("label_pane_class") calls are created inside different parent Uis (the ribbon group and the pane grid), so the egui ids don't clash.
  • Security and performance: no new input handling or dependencies. The per-frame work is a small lookup over the layer's classes, and the layer clone was already there.
  • AGENTS.md
    • There is no unwrap or expect in non-test code. The new ones are all inside #[cfg(test)].
    • New interface text goes through tl!.
    • The change stays in the UI layer and adds no dependencies.
    • The user guide is updated.

Minor, non-blocking (low confidence)

  • Quality: the ribbon's "Label Class" group lost 2 px of top padding (8.0 → 6.0) to fit the new Class row. I couldn't check the ribbon height or alignment against its neighbours. A headless screenshot would confirm it, as AGENTS.md recommends for UI work.
  • Quality: label_size_drag and apply_label_size_drag call crate::labelpane::class_mut through a long path. A use crate::labelpane::{self, Class} at the top of the ribbon code would shorten it. This is cosmetic.

Change size

Compared: the whole PR against main (merge base 9a88d22 to PR head cf2764b)

Metric Value
Files changed 3 (added 0, modified 3, removed 0, renamed 0)
Size before 288.3 KB
Size after 293.9 KB
Total size change +5.6 KB
File Before After Change
crates/ui-egui/src/ribbon.rs 200.6 KB 205.8 KB +5.2 KB
crates/ui-egui/src/labelpane.rs 63.2 KB 63.4 KB +251 B
docs/user-guide/cartography.md 24.5 KB 24.6 KB +106 B

@jgregoriorincon
jgregoriorincon marked this pull request as ready for review October 25, 2026 01:32
@github-actions

Copy link
Copy Markdown
Contributor

Code review

I read the full diff and the surrounding labelpane.rs and ribbon.rs code. I found no bugs or guideline violations worth blocking on, so I posted no inline comments. I did not build the code or run the tests; this review is from reading only.

Bugs

  • No defects found. The shared labelpane::chosen, class_combo, class_mut and named helpers keep the pane and the ribbon on one class.
  • The size-drag preview now stores the class name, and apply_label_size_drag restores the old size on that class before the labels.style command runs. Undo therefore records the right "before" size. The drag key also includes the class, so switching class mid-gesture flushes the previous drag. (high confidence)
  • Default still sends no class parameter, so recorded steps for Default are unchanged. (high confidence)
  • Low confidence: if the dragged class is renamed or removed mid-gesture, apply_label_size_drag returns early and the drag is dropped silently. The code comment says this is intended, and it is a rare edge case.

Security

  • Nothing applicable. The change is UI wiring only, and the class name only reaches commands as a parameter.

Performance

  • No concerns. chosen does one temp-data lookup and a linear scan over a layer's few classes per frame.

Quality

  • Making Class, named, class_mut, chosen and class_combo pub(crate) and reusing them from the ribbon is cleaner than duplicating the chooser.
  • Minor nit: the ribbon's label_groups calls crate::labelpane::... in many places. A use crate::labelpane::{self, Class} would shorten it.
  • Tests: the new ones cover the shared chooser, class-scoped edits including the recorded class parameters, and a class-scoped size drag with undo. Testing the stale-class case (a class removed mid-drag) would be a nice addition.

AGENTS.md

  • New user-visible strings ("Class" and the hover text) use tl!, and the PR doesn't need to touch the catalogs.
  • There are no unwraps in non-test code. The unwraps are all inside #[cfg(test)].
  • The docs in cartography.md are updated.
  • The layering is respected: all changes are in L6.

Change size

Compared: the whole PR against main (merge base 9a88d22 to PR head cf2764b)

Metric Value
Files changed 3 (added 0, modified 3, removed 0, renamed 0)
Size before 288.3 KB
Size after 293.9 KB
Total size change +5.6 KB
File Before After Change
crates/ui-egui/src/ribbon.rs 200.6 KB 205.8 KB +5.2 KB
crates/ui-egui/src/labelpane.rs 63.2 KB 63.4 KB +251 B
docs/user-guide/cartography.md 24.5 KB 24.6 KB +106 B

@github-actions

Copy link
Copy Markdown
Contributor

📖 Docs preview

Item Value
Preview https://preview.rustgis.app/pr-863/
Web app https://preview.rustgis.app/pr-863/app/
Commit cf2764b

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).

@giswqs
giswqs merged commit c627efe into opengeos:main Oct 25, 2026
19 checks passed
@jgregoriorincon
jgregoriorincon deleted the feat/ribbon-label-class branch October 25, 2026 03:37
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.

2 participants