Repository navigation
feat(pdf): add PDF editing tools to the donut - #428
Conversation
|
@KnifeLemon Small admin note: this is a feature PR. I opened it before noticing that the repository label workflow runs only once on |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @Perricheno, and I've added the feature label. This is careful work: the page tools keep text and vectors as PDF objects, the editor renders its previews off the UI thread, and the migration leaves customised donuts alone. CI is green. A few things before it can go in:
- Bookmarks, form fields and metadata. Rotate left/right go through
SplitOrExtract, and Crop throughPdfPageTools.CropAll; both copy the pages into a newPdfDocument. For a whole-document rotate or crop that drops the outline, AcroForm fields, page labels and document info thatCompressAsyncis careful to keep. When every page is kept, please open the file withPdfDocumentOpenMode.Modifyand change/Rotateand the crop box in place. - Split shown as "Extract".
preset.split_pdf.shortmakes the existing Split PDF (one file per page) read "Extract" on the Edit ring. Please keep it as "Split". - Large PDFs.
PdfPageTools.Previewreads the whole file and all page sizes again for every thumbnail, so a 500-page PDF is read 500 times. Please open the document once per editor (or cache the bytes and sizes). The editor's constructor also counts the pages on the UI thread before the window shows. - Tools in groups.
EnsureConsistencyand the group editor now allow every tool in a group, the image Crop tool included. If that's intended,docs/ARCHITECTURE.mdshould say so; its "Crop overlay" section also needs the PDF crop and the new page editor. - docs/ENGINES.md: the PDFsharp row should mention rotating, cropping and page organising.
- ko.json:
pdf_editor.hintends in 합니다체 ("추출할 수 있습니다"); the app uses 해요체 ("추출할 수 있어요").
Settings schema 33 is already taken by earlier PRs. We'll renumber the migration when we merge, so nothing to do there.
|
Addressed all six points in b0f9c92.
Local focused checks:
The broader engine run reached 1,432 passed and 50 skipped; its five local failures are unrelated installed-tool/font/codec differences (LibreOffice, Pandoc priority, Korean CAD font, and FFmpeg libspeex), while all affected PDF tests pass. |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @Perricheno, all six points are in, and done well: whole-document rotate and crop now change /Rotate and the crop box in a copy opened in modify mode, the tests check a real outline, form field, page labels and document info, Split reads "Split" again in every language, the editor opens its PDFs once after the window is up and reuses one snapshot per file, the docs cover groups, PDF crop and the page editor, and the Korean hint is in 해요체. On b0f9c92 merged with today's main: PdfPageToolsTests 9/9, the editor, toolbar and localization tests pass apart from Nepali (see the end), the Core settings tests 94/94, and dotnet format is clean.
The new modify-in-place path brings two problems, and both are easy to hit with real files:
- PDFs with permission restrictions. Many PDFs (bank statements, invoices, e-books) are encrypted with only an owner password: they open without a password but don't allow changes.
ModifyCopyopens the copy withPdfDocumentOpenMode.Modify, which PDFsharp refuses for those, andPdfSharpConverter.Openturns that into "The PDF is password-protected…". I tried a two-page PDF with only an owner password: the Rotate right preset (with and without compression) and PDF Crop both fail now, while the page-copy path in the previous commit handled it.PdfSharpConverter.OpenForChangesalready has the answer for compression: when Modify fails because of the owner password, copy the pages into a new document. Using the same fallback inModifyCopykeeps these files working (their outline and forms are lost, as before). The page editor's whole-document rotate goes through the same helper. - Read-only source files.
File.Copykeeps the read-only attribute, so for a read-only PDF (from a USB stick or CD, or a folder that marks files read-only) saving the copy fails with "Access to the path … is denied". I got that for both PDF Crop and the rotate preset. The no-rotation shortcut (degrees == 0→File.Copy) would also hand the user a read-only output. Please clearFileAttributes.ReadOnlyon the copy, or copy the bytes through a stream instead ofFile.Copy.
A test for each (an owner-password-only PDF and a read-only source), next to the structure tests, would keep them covered.
About #437: both PRs now carry a cached PDF preview class that does the same job, PdfPreviewSource here and PdfPreviewDocument in #437 (that one also renders the whole thumbnail strip in one PDFium load). Since both PRs are yours, could you keep one of them in whichever PR lands first and have the other use it when it's rebased? The two branches also conflict in 8 files where both add a tool (docs/ARCHITECTURE.md, docs/ENGINES.md, PresetAvailability, RadialController, WindowService, BuiltInData, AppSettings, UserDataStore), so the second one needs a rebase anyway.
Nepali (ne.json) was merged into main after your last push, so Every_language_has_exactly_the_english_keys(ne) now fails on the 19 new keys once main is merged in. That's main moving under you: we'll add the Nepali strings ourselves when merging, and renumber the settings schema too, so nothing to do there.
|
Thanks for catching both cases. Fixed in 85d529f:
Validation: PDF page tools and compression tests: 33 passed, 1 skipped because Ghostscript is not installed. Formatting checks for the changed engine and test files are clean. Agreed about the shared preview code. |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @Perricheno, both fixes are right: ModifyCopy goes through OpenForChanges (an owner-password-only PDF falls back to copying its pages, one that needs a password to open still gets the usual message), and CopyWritable clears the read-only attribute on every generated copy (crop, editor rotation, the no-rotation shortcut, both compression outcomes), with nine regression tests. On 85d529f: PdfPageToolsTests and the compression tests 34/34 (Ghostscript included), the editor, crop, group, donut and localization tests 168/168, Core 455/458, and dotnet format is clean.
The next step is the rebase you offered: #437 goes in first (with #429 just before it), then please rebase this onto main, drop PdfPreviewSource and use PdfPreviewDocument, and register the PDF tools next to the sheet studio. Things main changed under this branch that the rebase will meet:
PdfSharpConverter: main now has aWithoutMetadatashortcut for "Remove metadata" that also usesOpenForChanges. Keep both of your catch clauses there, and addRotation: PdfRotation.Noneto that shortcut's pattern so a preset that rotates and removes metadata isn't sent down the metadata-only path.- The PDF donut's back face now has "Remove metadata" (schema 37); with "Edit" first it has 11 slices, which fits.
- If #439 (Cut a clip) is merged before your rebase, tools are told apart by
Preset.Toolthere, andIsCropToolcounts a tool without one as the crop. Keep thetool-pdf-*check ahead of the crop check, inPresetAvailabilityand where the window is opened.
Don't worry about Nepali, Thai or the schema number: we'll add the 19 strings to ne.json and th.json and renumber when merging. I'll approve after the rebase.
To get this into the next release, we'll push the remaining changes to your branch ourselves today (maintainer edits are on). Have a look at the commits when they land, and tell us if anything looks off.
The PDF Edit group's settings migration moves after main's schema 43 to 46 (USM takes 44 and Cut a clip 45, both going into main first). The new tools sit after the spreadsheet print studio in the library, and "Edit" is first on the PDF donut's back face next to main's "Remove metadata". Tool dispatch and availability keep the print studio. Nepali and Thai strings for the PDF page tools. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cut a clip (KnifeLemon#439) is schema 45 and USM 44 on main; the PDF Edit group stays at 46. WindowService.ShowTool opens the page editor for "Reorder / delete pages" and "Add pages" before the crop (their presets have no Tool of their own, which reads as a crop; "Crop PDF" is one), and PresetAvailability judges the PDF tools before the image crop. Turkish strings for the PDF page tools. The palette tests now expect "Edit" among the built-in groups and leave the PDF tools out where a single kind is the point. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What does this change?
Adds one Edit group to the back of the PDF donut. It fans out to six actions: Crop, Rotate left, Rotate right, Pages, Extract, and Add PDF.
Crop changes PDF crop boxes, rotations use the page
/Rotatevalue, and the page organiser copies PDF pages directly, so text and vector content are not rasterised. The organiser can reorder, delete, rotate, add, and extract pages. Existing libraries receive the new group through schema migration 33 without replacing customised donut layouts.How was it tested?
dotnet format --verify-no-changes(Core, Engines, and App projects)dotnet test(the complete UI suite includes unrelated repository-root/platform tests)Screenshots (UI changes)
Editstays inside the PDF donut. Selecting it fans out the six small actions, so direct operations remain one click away and onlyPages/Addopen the organiser.The organiser now renders real thumbnails for every page and a large preview of the selected page while reordering, rotating, deleting, extracting, or combining PDFs.
Both screenshots are produced by headless UI tests at the controls' real sizes.
Checklist
src/Filee.App/Assets/i18n/*.jsonfiles