Repository navigation
Conversation
Three presets for which tools All tools lists: every tool, Read and comment, and Forms and signing. Data and pure lookups only; the UI that uses them follows.
…quick-tools) Preferences gets a Tools shown in All tools section: a preset (All tools, Read and comment, Forms and signing) and one checkbox per tool. A hidden tool leaves All tools, the ⌘K palette (with the commands only it lists) and Home's recommended tools. The menus, the mode tabs, shortcuts, ui.set tool and automation keep every tool, so hiding never switches a feature off. The setting saves the hidden tool ids, so tools a later version adds show up. The settings file is untrusted: unknown ids are dropped and a value that isn't a list hides nothing. The control channel sets it with ui.set tool-preset / hidden-tools and reports hidden_tools and tool_preset in ui.state. New strings are in all 22 catalogues (translated from the English meaning, not reviewed by native speakers), with their checksums.
…tomize-quick-tools) view.customize-quick-tools is partial: Preferences chooses the tools All tools, the palette and Home list. Customising the quick-action toolbar, pinning and reordering are still missing.
Contributor
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Only ATTRIBUTION.toml conflicted: the 22 catalogue checksums, recomputed from the merged catalogues. cargo fmt, clippy and tests for engine and ui-egui, xtask layers, assets and parity pass.
Only ATTRIBUTION.toml conflicted: the 22 catalogue checksums, recomputed from the merged catalogues. cargo fmt, clippy and tests for engine and ui-egui, xtask layers, assets and parity pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 72.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 9 files. (3 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: hiding tools from All tools, the command palette, and Home, while identifying the related parity objective. Description check The description follows the repository template and covers behavior, verification, tests, safety, provenance, security, parity, documentation, and reviewer focus areas. It also records platform limita… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description follows the repository template and covers behavior, verification, tests, safety, provenance, security, parity, documentation, and reviewer focus areas. It also records platform limitations and an unrelated flaky test.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 72.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 9 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ 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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
A new Preferences section, Tools shown in All tools, chooses which tools PdfCraft lists. Many people use PdfCraft for a few tools only (say reading, commenting and signing) and do the rest elsewhere; for them All tools, the ⌘K palette and Home are full of tools they never open.
A hidden tool is left out of three lists:
Nothing is switched off. The mode tabs, the menu bar, keyboard shortcuts,
ui.set tool, the CLI and MCP keep every tool, and Preferences says so in one line. Default behaviour is unchanged.Details:
hidden_tools), so a tool a later version adds shows up instead of missing. The preset is derived from that list, not stored, so there is one source of truth.TOOL_GROUPSinpdfcraft_engine::catalog(no egui below L7). Their names aretl!literals in the UI, so the catalogue-coverage tests enforce their translations.default-mode:ui.setwithtool-preset(all,read-comment,forms-sign) andhidden-tools(comma-separated ids ornone; an unknown id is an error and changes nothing).ui.statereportshidden_toolsandtool_preset(a preset id orcustom). Documented in thecontrol.rsmodule docs.crates/ui-egui/src/tool_visibility.rs; the existing files get one short call site each, to keep this easy to rebase.ATTRIBUTION.tomlchecksums updated. They were written from the meaning of the English labels, following each catalogue's existing terms (All tools, Preferences, …). No native-speaker review, so corrections are very welcome.How you verified it
cargo fmt --check-D warningson the crates I changedcargo xtask layersandcargo xtask assets(if you touched crate boundaries or any asset)Also ran the whole of
cargo xtask cilocally on stable 1.99.1 (fmt, clippy--workspace --all-targets,test --workspace, layers, wasm, assets, deny, parity: all 8 steps passed) pluscargo check -p pdfcraft-cli --no-default-features, withCRAFT_FONTS_DIRat the pinned craft-fonts commit,CRAFT_FONTS_REQUIRED=1andPDFCRAFT_ORACLES_REQUIRED=1. macOS only; no Windows or Linux run, but the change has no platform branches, paths or fonts.Tests at three scales, each seen failing with the code it pins reverted:
catalog.rs): presets name real tools; a hidden list maps to its preset whatever the order or repeats, and a near miss is no preset.tool_visibility.rs): saved lists keep known ids once in catalogue order and drop anything else; a 200,000-entry list stays bounded; the control value rejects unknown ids; the preset follows single toggles; a command hides only with every tool that lists it;persist→restoreround trip, and a malformed value hides nothing.tests/tool_visibility.rs, kittest over the control channel): a preset tidies All tools and the palette whileui.set tool compressstill opens the hidden tool; unknown tools and presets are refused without changing anything; Home recommends only shown tools and keeps Open file; All tools points to Preferences when everything is hidden; the Preferences checkboxes and preset change the list.Reverted one at a time, each of these made a test fail: the All tools filter, the View more condition, the palette's tool filter, the palette's command filter, the Home filter,
restore, and the checkbox handler.Unrelated to this change:
comments::a_released_pen_stroke_stays_drawn_until_the_page_raster_has_itfailed once on an unchangedmainhere and passed on the rerun (timing; possibly what #2812 addresses).Never-crash
unwrap/expect/panic!/unreachable!/todo!outside tests, unless it is provably infallible with a comment saying whyget, and strings only at char boundaries); checked or saturating arithmetic; allocations sized by input are capped; recursion is boundedunsafeProvenance
ATTRIBUTION.tomlentry andcargo xtask assetspasses (no new assets; only the changed catalogues' checksums)Security
--rootconfinement still holds on every path (no paths involved)ui.set)Parity and reach
parity/acrobat-features.tomlupdated:view.customize-quick-toolsis nowpartial, with tests as evidence and a note on what is still missing (customising the quick-action toolbar, pinning, reordering)ui.set/ui.state, likedefault-mode; no automation tool, since it is a UI preference onlydocs/ui-parity.md(rows and revision history), control-channel docs incontrol.rsAnything a reviewer should look at first
view.customize-quick-tools. If you would rather have a separate parity entry, or prefer pinning only, I'm happy to adapt.edit, Convertexport, E-Signfill_sign). Hiding is meant to tidy lists, not remove features, so I left them. Should they hide too?I can add screenshots of the Preferences section (light and dark, English and German) if useful.
Summary by CodeRabbit