Repository navigation
Conversation
Format text was a row of separate combos, a Load font button, an Installed fonts combo, a swatch grid and labelled drag values. It now follows Acrobat's arrangement: the font across the panel (the page's fonts, the standard families, Load font… and Installed fonts in one list, showing the paragraph's own font by name); size and a colour swatch, then lists and an alignment flyout; bold, italic, underline, superscript and subscript; line and paragraph spacing; horizontal scale and character spacing. One panel serves an edited paragraph and added text. What PdfCraft can't apply yet (lists, superscript and subscript, paragraph spacing, spacing for added text) is shown disabled and says it is planned. Icons stay Lucide's: superscript, subscript, pilcrow and move-horizontal are added from lucide-static 1.49.0 with attribution. Eight new strings are translated in every catalogue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Catalogues keep main's rows plus this branch's Format text labels; main already has "Font size", so its row stays. The new Vietnamese catalogue gets the five labels it lacked. ATTRIBUTION.toml takes main's entries (vi.tsv, probe.png) and keeps this branch's four Lucide icons, with the catalogue hashes refreshed and ATTRIBUTION.md regenerated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Vietnamese catalogue came from main without the Format text panel's Superscript and Subscript labels; vietnamese_covers_ui_literals lists them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… hashes Main's new lt and vi rows sit beside this branch's Format text labels; main's Vietnamese "Font size" replaces the one added here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ashes Main added snapshot-notice rows at the end of es.tsv and vi.tsv where this branch also appended rows; keep both and recompute the catalogue hashes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
FredApps
left a comment
There was a problem hiding this comment.
Reviewed 0062fe3. Changes requested for a user-visible loss of existing formatting controls.
[P2] Preserve editable character-spacing and horizontal-scale values when replacing the controls. The old extras_panel let users enter character spacing throughout -5..50 pt and horizontal scale throughout 10..400%. The new format_text_ui.rs replaces both with non-editable ComboBoxes containing only CHAR_SPACINGS [-1, -0.5, 0, 0.5, 1, 2, 3, 5] and SCALES [50, 75, 90, 100, 110, 125, 150, 200]. Values such as 0.25 pt spacing, -2 pt spacing or 80%/250% scale can no longer be selected in the panel, although the engine still supports them. Displaying a pre-existing custom value does not give the user a way to enter it or restore it after choosing a preset. This is a functionality regression in a PR described as panel layout only.
Please retain numeric entry with the former ranges alongside the preset lists, or use an editable combo. Add tests setting at least one non-listed value and one formerly allowed value outside each new preset range, confirming the resulting style survives the normal edit/save flow.
Code quality: consolidating added-text and existing-paragraph formatting into one module is a useful reduction in duplication, and the FontChoice result keeps file/font actions outside the widget renderer. Disabled Planned controls are explicit rather than pretending to implement features. However, replacing continuous controls with small preset tables silently narrows the UI contract, and the current alignment/Bold/disabled-control test does not catch it.
I read the full functional diff, the previous extras_panel and current formatting implementation, tests, metadata/discussion and the engine's arbitrary-value handling. This is a source-derived usability regression, not a locally reproduced failure. No new local tests or native UI session were run; hosted CI success alone does not test the missing numeric-entry cases. No merge or code change was performed.
Review of storytold#2249: the panel's preset lists narrowed what could be set to 50-200 % and -1..5 pt, where the old fields took 10-400 % and -5..50 pt. Each now has a number field over the old range again, with the list of common values beside it. Test: the_format_panel_takes_any_scale_and_character_spacing types values between the listed ones and beyond them, saves and reopens, and compares the paragraph with the engine's own rewrite. It fails without the field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FredApps
left a comment
There was a problem hiding this comment.
Re-reviewed current head ff0c630, including the exact incremental diff from 0062fe3 and the complete updated formatting module and regression.
The blocking regression in review 5553612441 is addressed. Horizontal scale and character spacing now have typed numeric fields alongside their presets, preserving the previous ranges (10–400% and -5–50 pt). The new UI test enters 80%, 250%, 0.25 pt and -2 pt through the labelled controls, then saves/reopens and compares the resulting text width with the engine's own formatting. No remaining blocking finding in this follow-up; approving.
Code quality: The shared number-with-presets helper keeps the range, step and suffix explicit without duplicating the two controls. Accessibility labels are associated with the spin buttons. The regression covers both off-preset values and persisted output, rather than only testing the list contents. The previously reviewed module extraction remains appropriately scoped.
Validation verified from hosted logs: CI run 38968903326 succeeded. The named test the_format_panel_takes_any_scale_and_character_spacing passed on actual Ubuntu 24.04.5 LTS and Microsoft Windows Server 2025 jobs (117202131353 and 117202131457). Both checked out synthetic merge 596b37c with parents 9e5b70d and this exact PR head. The run also passed its Clippy and wasm checks. I did not independently rerun local UI tests for this revision.
The cited CI validates only its recorded base/head pair. A separate live-main observation at approximately 22:10 UTC returned 924c4da, which was not the CI-tested base. This review does not authorize a merge or assert integration with whichever main commit is current at merge time.
…resh attribution Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Raised in storytold#3037: the size list sat lower than the colour swatch and the alignment button beside it. egui lays a list out from the default control height and grows it downward, so a row's shorter controls ended up off its middle. Each row is now one control height and centres what it holds, at a list's own height. The spacing rows' columns are measured from what they hold, so the second list no longer runs past the panel; the number fields' lists open from a small arrow, which leaves the field the room its value needs. Test: format_text_rows_line_up_and_fit_the_panel checks the size list is centred with the swatch and alignment button and as tall as the font list, that each spacing row shares one line, and that nothing passes the panel. It fails without the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@p6laris thanks for raising #3037. It's fixed in ce4be82: every row of Format text now centres its controls, so the size list is the same height as the font list and lines up with the colour and alignment buttons beside it. The spacing rows also fit inside the panel now. |
FredApps
left a comment
There was a problem hiding this comment.
Re-reviewed current head ce4be82. Its parent is the previously approved 308f537; I read the complete three-file incremental change, the full updated format_text_ui.rs module and current discussion. No remaining blocking finding in this follow-up; approving.
The row helper centres the size list, swatch and alignment control, and the spacing columns account for icons, gaps and the preset arrows. The earlier numeric-entry fix remains intact: horizontal scale still accepts 10–400% and character spacing -5–50 pt, with presets as an alternative rather than a replacement. The new regression checks actual control geometry and panel bounds instead of only asserting the controls exist.
Verified hosted CI run 39036459012 is successful. Both format_text_rows_line_up_and_fit_the_panel and the existing the_format_panel_takes_any_scale_and_character_spacing pass in the actual Windows job 117407224063 and Ubuntu job 117407224085. Their logs show synthetic merge 272b23c combining this exact head with base 924c4da. Clippy, WASM and dependency checks also pass; fuzz is skipped. I did not rerun local tests or a native visual session.
This remains the panel-layout change described here, not implementation of the disabled Planned controls or approval of the separate broader work in #2155. No merge or workflow approval performed.
FredApps
left a comment
There was a problem hiding this comment.
Re-reviewed head dde60e2 (previously approved ce4be82). The only change since is a merge of main (bc7a12b); the format panel module, its tests and its i18n keys are untouched by the merge. Requesting changes because the merge left the asset manifest inconsistent.
[P1] ATTRIBUTION.toml no longer matches the translation files. At this head the SHA-256 of every crates/ui-egui/src/i18n/*.tsv (all 22 files, ar through zh-hant) is absent from ATTRIBUTION.toml, whereas at ce4be82 and at current main all match. I computed this with git show of each file and the manifest at this exact SHA, no build needed. The hosted CI run 39059127407 shows the same failure on both Ubuntu and Windows: xtask assets::tests::the_real_manifest_passes fails with "SHA-256 does not match ATTRIBUTION.toml" for el, zh-hans, zh-hant, cs, pt-br, es, fr, de, te and others, so cargo xtask assets (part of xtask ci) fails and both test jobs are red.
Fix: after resolving the merge, run cargo xtask assets --write, check cargo xtask assets passes, and push; ATTRIBUTION.md is regenerated by the same command. Nothing else in the PR needs to change.
Also red in that run: dependency licences and advisories (RUSTSEC-2026-0370, -0371, -0372). That looks like a main-wide advisory problem rather than something this PR causes, and I did not investigate it further. The wasm32 check passed.
I did not run local cargo tests for this head, because the finding is already settled by the manifest hashes and the hosted logs. My earlier approval conclusions about the panel itself stand once the manifest is fixed. No merge performed.
What this changes
Format text was a row of separate combos, a Load font button, an Installed fonts combo, a swatch grid and labelled drag values. It now follows Acrobat's Format text arrangement (new
format_text_ui.rs):content_ui::format_panelandedit_text_ui::extras_panel.ATTRIBUTION.tomlentries.Tests:
content.rs::format_text_panel_sets_alignment_and_lists_planned_controls_disabled, plus a unit test for the number fields. The existing formatting tests use the Bold label instead of "B".How you verified it
On Windows 11, on this branch alone (rebased on
mainat 9931329):cargo fmt -p pdfcraft-ui-egui -- --checkcargo clippy -p pdfcraft-ui-egui --all-targets -- -D warningscargo test -p pdfcraft-ui-egui: everything passes exceptreload::unsaved_changes_are_kept_until_reload_is_chosen, which fails onmaintoo on a non-English system (fixed separately in fix(test): reload tests pin the English interface like every other harness #2239)cargo xtask assetsandcargo xtask parity*_covers_ui_literalstests), andATTRIBUTION.tomlcarries their new hashesNever-crash
unwrap/expect/panic!/unreachable!/todo!outside testsunsafeProvenance
ATTRIBUTION.tomlentry;cargo xtask assetspassesSecurity
Parity and reach
edit.format-panelnotes list what the panel applies and what is shown disabled until it exists (edit.listsandedit.text-super-subscriptstayplanned)Anything a reviewer should look at first
#2155 also reworks text formatting (as part of a larger change). This PR is the panel layout only: no engine change, and nothing it shows enabled is new behaviour.
🤖 Generated with Claude Code