Skip to content

fix(print): the printer list grows with the window before it scrolls (#4399) - #4435

Open
Natkiio wants to merge 1 commit into
storytold:mainfrom
Natkiio:fix/printer-list-height
Open

Natkiio wants to merge 1 commit into
storytold:mainfrom
Natkiio:fix/printer-list-height

Conversation

@Natkiio

@Natkiio Natkiio commented Oct 24, 2026 •

Copy link
Copy Markdown

Fixes #4399

What this changes

The printer list in the Print dialog only showed about 6 printers before you had to scroll. It can now grow to three fifths of the window, so in a normal-sized window it shows about 17. A short list still fits its rows, and a tiny window keeps the old height.

How I tested it

  • New unit test, which fails without the fix
  • All 27 Print dialog tests pass
  • cargo fmt --check and clippy with -D warnings are clean
  • Checked with 20 printers in a 1400 × 900 window: 6 visible before, 17 after

Summary by CodeRabbit

  • Improvements
    • The printer dropdown adjusts its height to the window size, while maintaining a minimum height for usability.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

The printer dropdown now uses 60% of the window content height, with a minimum equal to the combo box minimum height. Tests cover a 900-point window, a tiny window, and an unknown (NaN) window height.

Changes: Printer selector sizing

Layer / File(s) Summary
Calculate and apply printer list height
crates/ui-egui/src/workflows/print_ui/body.rs
A helper calculates the printer list height as 60% of the window height or the combo box minimum, whichever is greater. The printer dropdown uses this height. Tests cover normal, tiny, and NaN window heights.

Priority: Low

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

Change: Feature

Suggested reviewers: aiongg, echelon

Merge Risk: Minimal · up to a5f27

The printer selector sizing change is ready to merge with no established actionable risk.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly identifies the main change: the printer list grows with the window before scrolling.
Description check Passed The description explains the behavior change, references the fixed issue, and reports focused tests, formatting, clippy, and manual verification. The testing heading differs from the template, but the…
Linked Issues check Passed Issue #4399 asks for a bigger printer selector so that many printers are easier to select. The reviewed diff (merge base to head) changes only crates/ui-egui/src/workflows/print_ui/body.rs. The new …
Out of Scope Changes check Passed The whole-PR diff changes one production file, crates/ui-egui/src/workflows/print_ui/body.rs. The changes are: the printer_list_height helper with a doc comment that cites #4399, the call site tha…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Description check

Explanation

The description explains the behavior change, references the fixed issue, and reports focused tests, formatting, clippy, and manual verification. The testing heading differs from the template, but the required information is present and the omitted sections appear non-applicable.

Full details: Linked Issues check

Explanation

Issue #4399 asks for a bigger printer selector so that many printers are easier to select. The reviewed diff (merge base to head) changes only crates/ui-egui/src/workflows/print_ui/body.rs. The new helper printer_list_height(window, min) returns max(window * 0.6, min), where window is ui.ctx().content_rect().height() and min is ui.spacing().combo_height. The craft_ui::ComboBox for from_id_salt("printer") now calls .height(list). In a normal window, the list grows to 60% of the window height. In a small window, it keeps the egui minimum. The unit test the_printer_list_grows_with_the_window covers a 900-point window, a 100-point window, and a NaN height. This satisfies the coding requirement of #4399. The PR description reports 6 visible printers before the change and 17 after, with 20 printers in a 1400 × 900 window. I did not run the tests. The PR objectives summary describes a different approach (PR #4435, about 12 printers, a 400-point cap, 25 tests). That text does not match the reviewed code, so I did not use it as evidence. The earlier assessment cited print_ui.rs. The current code is in body.rs. I assessed the current code. Egui may cap popup height at 400 points. I did not verify that cap. It does not change the outcome, because the list still grows above the egui default in a normal window.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff changes one production file, crates/ui-egui/src/workflows/print_ui/body.rs. The changes are: the printer_list_height helper with a doc comment that cites #4399, the call site that passes the computed height to the printer ComboBox, and a #[cfg(test)] module that tests the helper. All changes relate to printer-list sizing. No unrelated production code, configuration, or other test changes are present in the reviewed diff.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. 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 0 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

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

@Natkiio
Natkiio force-pushed the fix/printer-list-height branch from 3a1978b to a5f27a5 Compare October 29, 2026 22:13
@Natkiio Natkiio changed the title fix(print): the printer list shows twelve printers before it scrolls, not six (#4399) fix(print): the printer list grows with the window before it scrolls (#4399) Oct 29, 2026

@FredApps FredApps left a comment

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.

ReviewerA — source-read COMMENT (not an approval)

I read the diff: the printer list grows with the window before it scrolls (#4399). A new function printer_list_height(window, min) returns (window * 0.6).max(min) ("three fifths of the window, and never less than min, egui's default, about six rows") and the combo box takes .height(list) computed from ui.ctx().content_rect().height() and ui.spacing().combo_height. A tall window shows more printers without scrolling, a small one keeps egui's default, and a pure function that is easy to test is the right structure. Checks: .max(min) on a NaN window height returns min (f32::max ignores NaN), so a zero-size or not yet laid out window is safe; the function should have a unit test with a small and a large window, which I did not see in the hunks I fetched. I have not run it, and no exact-head CI result exists for this head. Nothing was run.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.

Status: waiting_author.

This branch has not been deployed

No deployments
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.

Make printer selector bigger - its easier to select if you have a lot of printers

2 participants