diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4132559b7..237025368 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,6 +130,11 @@ jobs: run: | python3 scripts/ci/test_command_catalog.py python3 scripts/verify-command-catalog.py + - name: Verify Quick localization and operator help strings (#126) + run: | + python3 -m unittest scripts.ci.test_check_quick_localization -q + python3 scripts/update-quick-help-strings.py --check + python3 scripts/ci/check_quick_localization.py - name: Verify preflight check catalog and coverage backlog (#165) run: python3 scripts/ci/test_preflight_check_catalog.py - name: Verify preflight corpus coverage map (#165) diff --git a/LoopEditor/editorhost.cpp b/LoopEditor/editorhost.cpp index e28f9ef1c..0762e33e3 100644 --- a/LoopEditor/editorhost.cpp +++ b/LoopEditor/editorhost.cpp @@ -73,6 +73,7 @@ #include #include #include +#include #include #include #include @@ -86,6 +87,7 @@ #include #include #include +#include #include #include @@ -666,6 +668,15 @@ bool EditorHost::copySelectedText() return false; } + // The document's own copy permission decides before the controller is + // asked, and the refusal carries the sentence the CLI refuses with, so a + // copy-restricted document never reaches the clipboard through this seam. + if (!m_documentModel.canCopy()) + { + announceDocumentState(tr("Copy refused: %1").arg(tr("Document doesn't allow to copy content."))); + return false; + } + // The refusal is typed and named, and the clipboard is left untouched: an // empty string is never copied as success. const pdfinteraction::TextCopyResult result = interaction->selectedText(); @@ -2893,16 +2904,27 @@ void EditorHost::finishInkCoverageJob(const pdf::PDFJobSnapshot& snapshot) return; } - if (!outcome->ok) + const bool measured = snapshot.status == pdf::PDFJobStatus::Succeeded; + if (!measured || !outcome->ok) { - // A readout that could not be produced is a visible state, never a zero: - // "0% ink" and "not measured" are different facts. + // A readout that could not be produced, or one the scheduler has since + // superseded, cancelled or failed, is a visible state, never a zero: + // "0% ink" and "not measured" are different facts, and ink numbers + // measured against another revision are not this revision's numbers. + QString reason = outcome->reason; + if (!measured) + { + reason = QStringLiteral("ink-coverage/stale-or-cancelled"); + } + else if (reason.isEmpty()) + { + reason = QStringLiteral("ink-coverage/not-measured"); + } + QVariantMap readout; readout.insert(QStringLiteral("available"), false); readout.insert(QStringLiteral("pageIndex"), outcome->pageIndex); - readout.insert(QStringLiteral("reason"), outcome->reason.isEmpty() - ? QStringLiteral("ink-coverage/not-measured") - : outcome->reason); + readout.insert(QStringLiteral("reason"), reason); readout.insert(QStringLiteral("channels"), QVariantList()); m_inkCoverageReadout = readout; } @@ -4887,6 +4909,10 @@ void EditorHost::connectFacade() [this](const pdf::PDFRevisionIdentity&, const pdf::PDFRevisionIdentity&) { cancelPreflight(); + // The ink-coverage readout measured the revision that just left; + // dropping it is the honest state, and a fresh measurement + // repopulates it. + m_inkCoverageReadout = QVariantMap(); syncRevisionModels(); if (m_documentBound) { @@ -5578,6 +5604,17 @@ pdfinteraction::DirectEditOutcome EditorHost::submitDocumentDirectEdit(pdfintera return m_session->facade().applyDirectEdit(request); } +QUrl EditorHost::mailHandoffUrl(const QString& path, const QString& revision) +{ + const QString name = QFileInfo(path).fileName(); + QUrlQuery query; + query.addQueryItem(QStringLiteral("subject"), tr("Published document: %1").arg(name)); + query.addQueryItem(QStringLiteral("body"), tr("The published document %1 is at revision %2.").arg(path, revision)); + QUrl url(QStringLiteral("mailto:")); + url.setQuery(query); + return url; +} + bool EditorHost::handOffPublishedRevisionToMail(QString* typedError) { typedError->clear(); @@ -5603,9 +5640,16 @@ bool EditorHost::handOffPublishedRevisionToMail(QString* typedError) return false; } - // The revision travels with the hand-off, so the receiving side can refuse a - // file that is not the revision the shell was showing. - Q_EMIT documentMailRequested(source.path, facade.currentRevision().toString()); + // The revision travels in the message body, so the recipient can tell which + // revision of the file they hold. A client that does not take the message is + // a refusal, not a hand-off. + const QString revision = facade.currentRevision().toString(); + if (!QDesktopServices::openUrl(mailHandoffUrl(source.path, revision))) + { + *typedError = QStringLiteral("mail/client-unavailable"); + return false; + } + Q_EMIT documentMailRequested(source.path, revision); announceDocumentState(tr("The published document was handed to the mail client.")); return true; } @@ -5676,17 +5720,19 @@ void EditorHost::refreshFeatureAvailability() availability.insert(MoveSelectionCommandId, ready && m_session->facade().permissions().allowsCorrection()); // Select, copy and capture (issue #232). Every one of these is enabled with - // a document: Copy deliberately stays enabled without a selection so its - // typed refusal is reachable and visible, rather than the command silently - // disappearing from the menu. + // a document, and copy and extraction additionally follow the document's + // copy permission: Copy deliberately stays enabled without a selection so + // its typed refusal is reachable and visible, rather than the command + // silently disappearing from the menu. for (const QString& id : { QStringLiteral("actionSelectText"), QStringLiteral("actionSelectTextAll"), QStringLiteral("actionDeselectText"), QStringLiteral("actionSelectTable"), - QStringLiteral("actionCopyText"), QStringLiteral("actionGoToNextLine"), - QStringLiteral("actionGoToPreviousLine"), QStringLiteral("actionScreenshot"), - QStringLiteral("actionExtractImage") }) + QStringLiteral("actionGoToNextLine"), QStringLiteral("actionGoToPreviousLine"), + QStringLiteral("actionScreenshot") }) { availability.insert(id, ready); } + availability.insert(QStringLiteral("actionCopyText"), ready && m_documentModel.canCopy()); + availability.insert(QStringLiteral("actionExtractImage"), ready && m_documentModel.canCopy()); // The developer-diagnostics shell surfaces (#239): settings, render options // and the diagnostics bundle. They follow the same flag the menu gate diff --git a/LoopEditor/editorhost.h b/LoopEditor/editorhost.h index fb7c98bf3..d2cb4f0ab 100644 --- a/LoopEditor/editorhost.h +++ b/LoopEditor/editorhost.h @@ -724,10 +724,10 @@ class EditorHost final : public QObject /// asks for one and answers with extractCurrentPageImagesFileUrl(). void imageExtractionRequested(); - /// Send by e-mail hands the current published revision to the shell's mail + /// Send by e-mail hands the current published revision to the desktop mail /// client. `path` is the published artifact and `revision` is the identity it - /// was published as, so the receiving side can refuse a mismatch. Emitted - /// only for an open document with no unsaved changes: an unsaved or stale + /// was published as. Emitted only after the mail client accepted the message, + /// and only for an open document with no unsaved changes: an unsaved or stale /// revision is refused instead, never mailed as if it were the file. void documentMailRequested(const QString& path, const QString& revision); @@ -763,8 +763,10 @@ class EditorHost final : public QObject /// Hands the current published revision to the shell's mail client. Refuses /// when there is no open document, when its source is not a file, or when it /// has unsaved changes: the bytes on disk would then be a different revision - /// than the one on screen. Never writes and never opens anything itself. + /// than the one on screen. Never writes; the mail client is opened only once + /// every refusal above has passed. bool handOffPublishedRevisionToMail(QString* typedError); + static QUrl mailHandoffUrl(const QString& path, const QString& revision); bool requestMoveSelection(const QVariantMap& parameters); // ---- Bookmarks and hyperlinks (issue #233) ------------------------------------- diff --git a/LoopEditor/qml/DocumentPane.qml b/LoopEditor/qml/DocumentPane.qml index 4fe4d2603..7a9fdf8ed 100644 --- a/LoopEditor/qml/DocumentPane.qml +++ b/LoopEditor/qml/DocumentPane.qml @@ -84,6 +84,15 @@ Item { TabButton { objectName: "annotationsTabButton" text: qsTr("Annotations") + contentItem: Text { + text: parent.text + font: parent.font + color: parent.palette.buttonText + wrapMode: Text.WordWrap + horizontalAlignment: Text.AlignHCenter + verticalAlignment: Text.AlignVCenter + Accessible.ignored: true + } Accessible.name: qsTr("Annotations tab") } } diff --git a/LoopEditor/qml/Main.qml b/LoopEditor/qml/Main.qml index 52c521220..b75420903 100644 --- a/LoopEditor/qml/Main.qml +++ b/LoopEditor/qml/Main.qml @@ -338,6 +338,35 @@ ApplicationWindow { onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() } + FileDialog { + id: bookmarkImportDialog + title: qsTr("Import Bookmarks") + nameFilters: [qsTr("Bookmark files (*.json)")] + property string revision: "" + onAccepted: { + if (host) { + host.importBookmarksFileUrl(selectedFile, revision) + } + if (host && host.focusRestoration) host.focusRestoration.restore() + } + onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() + } + + FileDialog { + id: bookmarkExportDialog + title: qsTr("Export Bookmarks") + fileMode: FileDialog.SaveFile + nameFilters: [qsTr("Bookmark files (*.json)")] + property string revision: "" + onAccepted: { + if (host) { + host.exportBookmarksFileUrl(selectedFile, revision) + } + if (host && host.focusRestoration) host.focusRestoration.restore() + } + onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() + } + FileDialog { id: preflightReportDialog title: qsTr("Export Preflight Report") @@ -414,6 +443,16 @@ ApplicationWindow { function onImageExtractionRequested() { extractImagesDialog.open() } + function onBookmarkImportRequested() { + if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) + bookmarkImportDialog.revision = host.outputRevision() + bookmarkImportDialog.open() + } + function onBookmarkExportRequested() { + if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) + bookmarkExportDialog.revision = host.outputRevision() + bookmarkExportDialog.open() + } function onSettingsPanelRequested() { if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) settingsDialog.open() diff --git a/LoopEditor/qml/MenuModel.qml b/LoopEditor/qml/MenuModel.qml index 3ed992db4..7db6ef359 100644 --- a/LoopEditor/qml/MenuModel.qml +++ b/LoopEditor/qml/MenuModel.qml @@ -127,6 +127,7 @@ QtObject { case "actionDeselectText": return qsTr("Deselect Text") case "actionEditDocumentMetadata": return qsTr("Edit Document Metadata") case "actionEncryption": return qsTr("Encryption") + case "actionExportDocument": return qsTr("Export document…") case "actionExtractImage": return qsTr("Extract Image") case "actionFind": return qsTr("Find") case "actionFindNext": return qsTr("Find Next") diff --git a/LoopEditor/qml/ProductionPreviewPane.qml b/LoopEditor/qml/ProductionPreviewPane.qml index 9c9af0178..b2d51c291 100644 --- a/LoopEditor/qml/ProductionPreviewPane.qml +++ b/LoopEditor/qml/ProductionPreviewPane.qml @@ -148,7 +148,7 @@ Pane { ? "" : qsTr("Soft proof %1: %2") .arg(root.host.softProofStateName) - .arg(root.host.softProofSummary()) + .arg(root.host.softProofSummary) Accessible.name: qsTr("Soft proof state") Accessible.description: text } diff --git a/LoopLibCore/sources/pdfauthoringedit.cpp b/LoopLibCore/sources/pdfauthoringedit.cpp index b63f561f7..9124bb241 100644 --- a/LoopLibCore/sources/pdfauthoringedit.cpp +++ b/LoopLibCore/sources/pdfauthoringedit.cpp @@ -73,9 +73,16 @@ PDFObjectReference pageReference(const PDFCatalog* catalog, int pageIndex) } /// Resolves a destination's page. A destination may name the page by reference -/// or by index; a reference that resolves to no page is not a page target. +/// or by index; a named or unparsed destination has no page target here +/// (callers refuse it with their own typed code), and a reference that +/// resolves to no page is not a page target. bool resolveDestinationPage(const PDFCatalog* catalog, const PDFDestination& destination, int* pageIndex) { + if (!destination.isValid() || destination.isNamedDestination()) + { + return false; + } + if (destination.getPageReference().isValid()) { const size_t resolved = catalog->getPageIndexFromPageReference(destination.getPageReference()); @@ -157,8 +164,9 @@ bool removePageBookmarks(const PDFCatalog* catalog, PDFOutlineItem* item, int pa } /// True when the builder's outline writer can write this item back unchanged. -/// It writes Title, a GoTo Dest, a GoToDp action, colour and flags; a -/// structure element or any other action would be silently dropped. +/// It writes Title, a GoTo Dest, a GoToDp action, colour and flags; a structure +/// element, a GoTo whose destination did not parse (the writer would write PDF +/// null there) or any other action would be silently dropped. bool isRewritableItem(const PDFOutlineItem* item) { if (item->getStructureElement().isValid()) @@ -173,8 +181,15 @@ bool isRewritableItem(const PDFOutlineItem* item) return true; } - return dynamic_cast(action) != nullptr || - dynamic_cast(action) != nullptr; + const auto* goTo = dynamic_cast(action); + if (goTo) + { + // A named destination is valid and round-trips (the object factory + // writes the name); only a destination that failed to parse is lossy. + return goTo->getDestination().isValid(); + } + + return dynamic_cast(action) != nullptr; } bool canRewriteOutline(const PDFOutlineItem* item) diff --git a/LoopLibCore/sources/pdfdirectedit.cpp b/LoopLibCore/sources/pdfdirectedit.cpp index 529d4b858..045c72130 100644 --- a/LoopLibCore/sources/pdfdirectedit.cpp +++ b/LoopLibCore/sources/pdfdirectedit.cpp @@ -67,6 +67,8 @@ struct XmpProperty XmpValueShape shape = XmpValueShape::Text; std::optional value; bool written = false; + int elementOccurrences = 0; ///< Matched as an element inside rdf:Description. + int attributeOccurrences = 0; ///< Matched as an attribute on rdf:Description. }; bool isRdfDescription(const QXmlStreamReader& reader) @@ -94,15 +96,114 @@ bool matchesProperty(const QXmlStreamReader& reader, const XmpProperty& property return reader.name() == QString::fromLatin1(localName); } -void writeXmpProperty(QXmlStreamWriter& writer, const XmpProperty& property) +/// The attribute form of `matchesProperty`: a property carried as an attribute +/// on rdf:Description (the common attribute shorthand) matches by qualified +/// name first and by namespace URI plus local name second. +bool matchesPropertyAttribute(const QXmlStreamAttribute& attribute, const XmpProperty& property) { - // The writer only takes qualified names here: every prefix this module - // writes (dc, pdf, xmp, rdf) is already declared by the packet being merged - // into, or is declared by the fresh packet's own elements. + if (attribute.qualifiedName() == QLatin1String(property.qualifiedName)) + { + return true; + } + + const QString uri = attribute.namespaceUri().toString(); + if (uri.isEmpty() || uri != QLatin1String(property.namespaceUri)) + { + return false; + } + const QByteArray localName = property.qualifiedName.mid(property.qualifiedName.indexOf(':') + 1); + return attribute.name() == QString::fromLatin1(localName); +} + +/// Records how each property occurs in the packet: as an element inside an +/// rdf:Description and as an attribute on one. The carried value is whether the +/// packet has at least one rdf:Description; `std::nullopt` means it did not +/// parse. +std::optional recordXmpOccurrences(const QByteArray& existing, std::vector& properties) +{ + QXmlStreamReader reader(existing); + reader.setNamespaceProcessing(true); + + int depth = 0; + int descriptionDepth = -1; + bool descriptionSeen = false; + + while (!reader.atEnd()) + { + reader.readNext(); + if (reader.hasError()) + { + return std::nullopt; + } + + switch (reader.tokenType()) + { + case QXmlStreamReader::StartElement: + { + ++depth; + if (isRdfDescription(reader)) + { + descriptionSeen = true; + descriptionDepth = depth; + for (const QXmlStreamAttribute& attribute : reader.attributes()) + { + for (XmpProperty& property : properties) + { + if (matchesPropertyAttribute(attribute, property)) + { + ++property.attributeOccurrences; + } + } + } + } + else if (descriptionDepth >= 0) + { + for (XmpProperty& property : properties) + { + if (matchesProperty(reader, property)) + { + ++property.elementOccurrences; + } + } + } + break; + } + + case QXmlStreamReader::EndElement: + --depth; + if (depth < descriptionDepth) + { + descriptionDepth = -1; + } + break; + + default: + break; + } + } + + if (reader.hasError()) + { + return std::nullopt; + } + return descriptionSeen; +} + +void writeXmpProperty(QXmlStreamWriter& writer, const XmpProperty& property, bool declareNamespace = false) +{ + // The qualified-name write path bypasses the writer's namespace handling: + // neither the packet being merged into nor the fresh packet necessarily + // declares the prefix at the insertion point, so a merged property declares + // the namespace on the element itself. const QByteArray qualifiedName = property.qualifiedName; const QString value = property.value.value_or(QString()); writer.writeStartElement(QString::fromLatin1(qualifiedName)); + if (declareNamespace) + { + const QByteArray prefix = qualifiedName.left(qualifiedName.indexOf(':')); + writer.writeNamespace(QString::fromLatin1(property.namespaceUri), QString::fromLatin1(prefix)); + } switch (property.shape) { case XmpValueShape::LocalizedAlt: @@ -180,13 +281,103 @@ QByteArray freshXmpPacket(const DocumentMetadataEdit& edit) return output; } +/// Writes an rdf:Description start element token by token when it carries an +/// attribute this edit merges into: the substituted value for a matching +/// property, the original attribute otherwise. Returns false when the element +/// carries no such attribute, in which case the caller copies the token +/// instead. +bool writeStartElementWithMergedAttributes(QXmlStreamWriter& writer, + const QXmlStreamReader& reader, + std::vector& properties) +{ + bool mergesAttribute = false; + for (const XmpProperty& property : properties) + { + if (property.value && !property.written && property.attributeOccurrences == 1) + { + for (const QXmlStreamAttribute& attribute : reader.attributes()) + { + mergesAttribute = mergesAttribute || matchesPropertyAttribute(attribute, property); + } + } + } + if (!mergesAttribute) + { + return false; + } + + writer.writeStartElement(reader.qualifiedName().toString()); + for (const QXmlStreamNamespaceDeclaration& declaration : reader.namespaceDeclarations()) + { + if (declaration.prefix().isEmpty()) + { + // writeNamespace() would invent a prefix for a default declaration. + writer.writeDefaultNamespace(declaration.namespaceUri().toString()); + } + else + { + writer.writeNamespace(declaration.namespaceUri().toString(), declaration.prefix().toString()); + } + } + + for (const QXmlStreamAttribute& attribute : reader.attributes()) + { + XmpProperty* merged = nullptr; + for (XmpProperty& property : properties) + { + if (property.value && !property.written && property.attributeOccurrences == 1 && + matchesPropertyAttribute(attribute, property)) + { + merged = &property; + break; + } + } + + if (merged) + { + merged->written = true; + writer.writeAttribute(attribute.qualifiedName(), *merged->value); + } + else + { + writer.writeAttribute(attribute.qualifiedName(), attribute.value()); + } + } + return true; +} + /// Merges the edit's properties into an existing packet, copying every token it -/// does not own. `std::nullopt` means the packet did not parse, so the caller -/// can refuse rather than overwrite a packet it could not read. +/// does not own. `std::nullopt` means the packet did not parse, or carries a +/// property this edit writes more than once and so cannot be merged +/// unambiguously, and the caller refuses rather than overwriting a packet it +/// could not read. std::optional mergeXmpPacket(const QByteArray& existing, const DocumentMetadataEdit& edit) { std::vector properties = propertiesFor(edit); + // A property carried in a later sibling rdf:Description, or as an + // attribute on one, is still the property; it has to be replaced where it + // is, because appending a second copy beside it would leave the packet + // carrying an old and a new value. + const std::optional descriptionSeen = recordXmpOccurrences(existing, properties); + if (!descriptionSeen) + { + return std::nullopt; + } + for (const XmpProperty& property : properties) + { + if (property.value && property.elementOccurrences + property.attributeOccurrences > 1) + { + return std::nullopt; + } + } + if (!*descriptionSeen) + { + // A packet with no rdf:Description carries no XMP properties to + // preserve, so a standard one is written instead. + return freshXmpPacket(edit); + } + QXmlStreamReader reader(existing); reader.setNamespaceProcessing(true); QByteArray output; @@ -194,7 +385,6 @@ std::optional mergeXmpPacket(const QByteArray& existing, const Docum int depth = 0; int descriptionDepth = -1; - bool descriptionSeen = false; bool insertedMissing = false; while (!reader.atEnd()) @@ -217,8 +407,11 @@ std::optional mergeXmpPacket(const QByteArray& existing, const Docum ++depth; if (isRdfDescription(reader)) { - descriptionSeen = true; descriptionDepth = depth; + if (writeStartElementWithMergedAttributes(writer, reader, properties)) + { + break; + } } else if (descriptionDepth >= 0) { @@ -228,7 +421,10 @@ std::optional mergeXmpPacket(const QByteArray& existing, const Docum if (property.value && !property.written && matchesProperty(reader, property)) { property.written = true; - writeXmpProperty(writer, property); + // The replaced element may be the only place its + // prefix was declared (a child-declared binding), so + // the replacement declares the namespace as well. + writeXmpProperty(writer, property, true); replaced = true; break; } @@ -254,10 +450,11 @@ std::optional mergeXmpPacket(const QByteArray& existing, const Docum insertedMissing = true; for (XmpProperty& property : properties) { - if (property.value && !property.written) + if (property.value && !property.written && property.elementOccurrences == 0 && + property.attributeOccurrences == 0) { property.written = true; - writeXmpProperty(writer, property); + writeXmpProperty(writer, property, true); } } } @@ -280,12 +477,6 @@ std::optional mergeXmpPacket(const QByteArray& existing, const Docum { return std::nullopt; } - if (!descriptionSeen) - { - // A packet with no rdf:Description carries no XMP properties to - // preserve, so a standard one is written instead. - return freshXmpPacket(edit); - } return output; } @@ -423,6 +614,8 @@ const char* getDirectEditRefusalCode(DirectEditRefusal refusal) return "metadata/unchanged"; case DirectEditRefusal::WriteRefused: return "attachment/output-write-refused"; + case DirectEditRefusal::AttachmentSpecificationNotAddressable: + return "attachment/specification-not-addressable"; } return "direct-edit/invalid-request"; @@ -465,13 +658,30 @@ DirectEditOutcome PDFFormValueWriter::setFieldValue(const PDFDocument* document, } PDFFormField* field = nullptr; - formManager.modify([&field, &edit](PDFFormField* candidate) + PDFFormField* firstMatch = nullptr; + formManager.modify([&field, &firstMatch, &edit](PDFFormField* candidate) { - if (!field && candidate->getName(PDFFormField::NameType::FullyQualified) == edit.field) + if (candidate->getName(PDFFormField::NameType::FullyQualified) != edit.field) + { + return; + } + + // Same-named fields are one field per PDF semantics. + // A widget-less parent may match before the editable + // terminal field, so the match carrying widgets wins. + if (!firstMatch) + { + firstMatch = candidate; + } + if (!field && !candidate->getWidgets().empty()) { field = candidate; } }); if (!field) + { + field = firstMatch; + } + if (!field) { outcome.refusal = DirectEditRefusal::FieldNotFound; return outcome; @@ -564,6 +774,25 @@ DirectEditOutcome PDFFormValueWriter::setFieldValue(const PDFDocument* document, outcome.refusal = DirectEditRefusal::FieldNotEditable; return outcome; } + + // PDF semantics treat same-named terminal fields as one field, so the value + // is carried to every other field with the same fully qualified name, as + // the interactive edit path does. The name cannot be empty here: the entry + // guard refused an empty request, and the match above compared against it. + const QString qualifiedName = field->getName(PDFFormField::NameType::FullyQualified); + parameters.scope = PDFFormField::SetValueParameters::Scope::Internal; + formManager.modify([¶meters, &qualifiedName](PDFFormField* candidate) + { + if (parameters.invokingFormField == candidate) + { + return; + } + + if (candidate->getName(PDFFormField::NameType::FullyQualified) == qualifiedName) + { + candidate->setValue(parameters); + } }); + if (!modifier.finalize()) { outcome.refusal = DirectEditRefusal::ValueUnchanged; @@ -651,10 +880,14 @@ DirectEditOutcome PDFAttachmentWriter::add(const PDFDocument* document, const At std::map entries; for (const auto& item : document->getCatalog()->getEmbeddedFiles()) { - if (item.second.getSelfReference().isValid()) + if (!item.second.getSelfReference().isValid()) { - entries[item.first] = item.second.getSelfReference(); + // A specification stored inline in the names array has no reference + // to relist; rebuilding around it would silently unlist it. + outcome.refusal = DirectEditRefusal::AttachmentSpecificationNotAddressable; + return outcome; } + entries[item.first] = item.second.getSelfReference(); } entries[edit.name.toUtf8()] = specification; writeEmbeddedFileNamesTree(builder, entries); @@ -694,10 +927,18 @@ DirectEditOutcome PDFAttachmentWriter::remove(const PDFDocument* document, const std::map entries; for (const auto& item : document->getCatalog()->getEmbeddedFiles()) { - if (item.first != *key && item.second.getSelfReference().isValid()) + if (item.first == *key) + { + continue; + } + if (!item.second.getSelfReference().isValid()) { - entries[item.first] = item.second.getSelfReference(); + // Any other inline specification would have to be relisted, and it + // has no reference to relist; the removal would silently drop it. + outcome.refusal = DirectEditRefusal::AttachmentSpecificationNotAddressable; + return outcome; } + entries[item.first] = item.second.getSelfReference(); } writeEmbeddedFileNamesTree(modifier.getBuilder(), entries); diff --git a/LoopLibCore/sources/pdfdirectedit.h b/LoopLibCore/sources/pdfdirectedit.h index de3bd2eae..4e4764a2d 100644 --- a/LoopLibCore/sources/pdfdirectedit.h +++ b/LoopLibCore/sources/pdfdirectedit.h @@ -55,9 +55,10 @@ enum class DirectEditRefusal AttachmentNotFound, ///< No embedded file carries the requested name. AttachmentNameTaken, ///< An embedded file with that name already exists. AttachmentNameInvalid, ///< The name is empty or would need sanitizing. - XmpUnreadable, ///< The existing XMP packet could not be parsed; nothing was replaced. + XmpUnreadable, ///< The existing XMP packet could not be parsed, or cannot be merged unambiguously; nothing was replaced. MetadataUnchanged, ///< The metadata edit would not change the document. - WriteRefused ///< The atomic output writer refused the target. + WriteRefused, ///< The atomic output writer refused the target. + AttachmentSpecificationNotAddressable ///< An embedded file's specification is stored inline, with no reference to relist. }; /// The stable refusal code, e.g. `form/xfa-unsupported`. `None` maps to an empty diff --git a/LoopLibCore/sources/pdfdocumentcontext.cpp b/LoopLibCore/sources/pdfdocumentcontext.cpp index 769d0dccf..d282d9a8f 100644 --- a/LoopLibCore/sources/pdfdocumentcontext.cpp +++ b/LoopLibCore/sources/pdfdocumentcontext.cpp @@ -30,6 +30,37 @@ namespace pdf { +namespace +{ + +/// True when the value is an even-length run of hex digits, the shape +/// QByteArray::toHex() produces. The empty run is that shape too: a document +/// with no source bytes (an in-memory document after an edit) serializes with +/// an empty hash field, and the round trip must preserve it. Any other value +/// must not be decoded, because QByteArray::fromHex() silently drops non-hex +/// characters and an odd trailing nibble. +bool isHexHash(const QString& value) +{ + if (value.size() % 2 != 0) + { + return false; + } + + for (const QChar character : value) + { + const bool isDigit = character >= QLatin1Char('0') && character <= QLatin1Char('9'); + const bool isLowerHex = character >= QLatin1Char('a') && character <= QLatin1Char('f'); + const bool isUpperHex = character >= QLatin1Char('A') && character <= QLatin1Char('F'); + if (!isDigit && !isLowerHex && !isUpperHex) + { + return false; + } + } + return true; +} + +} // namespace + PDFDocumentIdentity PDFDocumentIdentity::fromDocument(const PDFDocument* document) { if (!document) @@ -67,7 +98,7 @@ PDFRevisionIdentity PDFRevisionIdentity::fromString(const QString& value) // colons, so the first four separators are the structure and everything // after the fourth belongs to the profile identity. QStringList parts = value.split(QLatin1Char(':')); - if (parts.size() < 5) + if (parts.size() < 5 || !isHexHash(parts[0])) { return identity; } diff --git a/LoopLibInteraction/sources/canvassnapshot.cpp b/LoopLibInteraction/sources/canvassnapshot.cpp index 8caf10626..5e34e65c6 100644 --- a/LoopLibInteraction/sources/canvassnapshot.cpp +++ b/LoopLibInteraction/sources/canvassnapshot.cpp @@ -70,11 +70,12 @@ QImage composeCanvasImage(const CanvasSnapshot& snapshot, QRect viewportRect, qr continue; } - // The tile image is drawn into its placed rectangle in device pixels. - // A tile rendered for another resolution is scaled here exactly as the - // scene graph scales it, so the capture shows what the canvas shows. - const QRectF target(tile.placedRect.topLeft() * ratio, tile.placedRect.size() * ratio); - painter.drawImage(target, tile.pixels->image); + // The painter's coordinate space is viewport units: the composed + // image's device pixel ratio maps them to device pixels, so the placed + // rectangle is drawn unchanged. A tile rendered for another resolution + // is scaled here exactly as the scene graph scales it, so the capture + // shows what the canvas shows. + painter.drawImage(QRectF(tile.placedRect), tile.pixels->image); anyTile = true; } painter.end(); diff --git a/LoopLibInteraction/sources/documentfacadeedits.cpp b/LoopLibInteraction/sources/documentfacadeedits.cpp index ad9c9aa82..145273e07 100644 --- a/LoopLibInteraction/sources/documentfacadeedits.cpp +++ b/LoopLibInteraction/sources/documentfacadeedits.cpp @@ -499,10 +499,6 @@ DirectEditOutcome DocumentFacade::applyDocumentEdit(const DirectEditRequest& req // Accept the edit: the new document replaces the current one, which is what // advances the revision exactly once. markModified() is deliberately not // used here -- it would advance the revision a second time for one edit. - if (m_outputState == DocumentOutputState::Pending) - { - supersedePending(CommandTerminalState::Cancelled, QStringLiteral("document/superseded")); - } documentContext->setDocument(coreOutcome.document); if (!m_publishedKey.isEmpty()) { diff --git a/LoopLibInteraction/sources/documentfacadestate.cpp b/LoopLibInteraction/sources/documentfacadestate.cpp index cff43657d..e9dc82f31 100644 --- a/LoopLibInteraction/sources/documentfacadestate.cpp +++ b/LoopLibInteraction/sources/documentfacadestate.cpp @@ -461,6 +461,16 @@ bool DocumentFacade::completeSignatureCheck(const DocumentSignatureCheckToken& t { if (!admitSignatureCheckTransition(token, error)) { + // The fence refuses results that belong to another revision, but the + // refused check must not hold the one-check-at-a-time flag for the rest + // of the session. Release it when this completion is still the current + // check's and only the revision moved on; a superseded completion must + // not release a newer check. + if (token == m_signatureCheckToken && token.generation == m_generation && + token.revision != currentRevision()) + { + m_signatureCheckRunning = false; + } return false; } @@ -476,6 +486,14 @@ bool DocumentFacade::failSignatureCheck(const DocumentSignatureCheckToken& token { if (!admitSignatureCheckTransition(token, error)) { + // Same release as completeSignatureCheck: a refused failure belongs to + // the document that moved on, so the next check may begin; a + // superseded one must not release a newer check. + if (token == m_signatureCheckToken && token.generation == m_generation && + token.revision != currentRevision()) + { + m_signatureCheckRunning = false; + } return false; } if (detail.isEmpty()) diff --git a/LoopLibInteraction/sources/overlaybuilder.h b/LoopLibInteraction/sources/overlaybuilder.h index e37e5d7a8..1f6e1f529 100644 --- a/LoopLibInteraction/sources/overlaybuilder.h +++ b/LoopLibInteraction/sources/overlaybuilder.h @@ -51,8 +51,11 @@ struct OverlayBounds /// Of those, how many may be read-only annotation review marks. Bounded for /// the same reason as findings: a heavily annotated document must not crowd - /// the steering primitives out of the frame. - int maxAnnotationPrimitives = 1000; + /// the steering primitives out of the frame. The cap leaves explicit + /// headroom for them: findings plus annotations can never consume more than + /// 3500 of the 4000 slots, so hover, selection, handles and the drag + /// preview always fit. + int maxAnnotationPrimitives = 500; static OverlayBounds conservativeDefaults() { return OverlayBounds(); } }; diff --git a/LoopLibInteraction/sources/pdfdocumentimageextractor.cpp b/LoopLibInteraction/sources/pdfdocumentimageextractor.cpp index b884dff04..48b969a0e 100644 --- a/LoopLibInteraction/sources/pdfdocumentimageextractor.cpp +++ b/LoopLibInteraction/sources/pdfdocumentimageextractor.cpp @@ -131,6 +131,14 @@ QList PDFDocumentImageExtractor::extractPage(int pageIndex, QString* return refuse(tr("No document is open.")); } + // PdfTool's fetch-images refuses the same operation with + // `pdf.copy-not-permitted`; extraction and the shell's write-to-disk path + // must agree about what the document's permissions allow. + if (!document->getStorage().getSecurityHandler()->isAllowed(pdf::PDFSecurityHandler::Permission::CopyContent)) + { + return refuse(tr("Document doesn't allow to copy content.")); + } + const pdf::PDFCatalog* catalog = document->getCatalog(); const bool pageInRange = catalog && pageIndex >= 0 && static_cast(pageIndex) < catalog->getPageCount(); diff --git a/LoopLibInteraction/sources/pdfdocumenttextselectionsource.cpp b/LoopLibInteraction/sources/pdfdocumenttextselectionsource.cpp index ab9b3c2e1..5e9e0b9d5 100644 --- a/LoopLibInteraction/sources/pdfdocumenttextselectionsource.cpp +++ b/LoopLibInteraction/sources/pdfdocumenttextselectionsource.cpp @@ -138,6 +138,13 @@ pdf::PDFTextLayout* PDFDocumentTextSelectionSource::mutableTextLayout(int pageIn const auto cached = m_layouts.find(pageIndex); if (cached != m_layouts.end()) { + // A page with no text layer caches an empty layout, and the refusal + // must be the same on every call, not only the one that generated it. + // Re-check rather than return the cached empty layout as available. + if (cached.value().getTextBlocks().empty()) + { + return unavailable(tr("This page has no text layer.")); + } return &cached.value(); } diff --git a/UnitTests/CMakeLists.txt b/UnitTests/CMakeLists.txt index 68154b118..bff594d7a 100644 --- a/UnitTests/CMakeLists.txt +++ b/UnitTests/CMakeLists.txt @@ -796,6 +796,13 @@ if(NOT LOOP_BUILD_ONLY_CORE_LIBRARY AND LOOP_BUILD_QUICK_CANVAS) RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LOOP_INSTALL_BIN_DIR} ) add_test(UnitTestsLoopStateVisual "${CMAKE_BINARY_DIR}/${LOOP_INSTALL_BIN_DIR}/UnitTestsLoopStateVisual") + # #126: the shipping-catalog test loads the compiled LOOP_.qm files + # from the translations output dir instead of skipping. The top-level + # LOOP_TRANSLATION_QM_OUTPUT_DIRECTORY names the same path but is set after + # add_subdirectory(UnitTests), so compose the path from LOOP_INSTALL_TRANS_DIR, + # which this directory already sees. + set_tests_properties(UnitTestsLoopStateVisual PROPERTIES + ENVIRONMENT "LOOP_TRANSLATION_TEST_DIRECTORY=${CMAKE_BINARY_DIR}/${LOOP_INSTALL_TRANS_DIR}") endif() add_executable(UnitTestsPageMasterExport diff --git a/UnitTests/tst_annotationauthoringtest.cpp b/UnitTests/tst_annotationauthoringtest.cpp index dedf44e42..65a8f4a58 100644 --- a/UnitTests/tst_annotationauthoringtest.cpp +++ b/UnitTests/tst_annotationauthoringtest.cpp @@ -324,6 +324,7 @@ private slots: void catalogBindsTheWholeAnnotationFamily_data(); void catalogBindsTheWholeAnnotationFamily(); + void catalogFamilyMatchesTheLiteralCommandList(); void parametersMapToTypedRequestsAndRefuseMalformedOnes(); void subtypesRoundTripThroughTheGateway_data(); @@ -381,6 +382,44 @@ void AnnotationAuthoringTest::catalogBindsTheWholeAnnotationFamily() QVERIFY(catalog.setHandler(commandId, handler)); } +void AnnotationAuthoringTest::catalogFamilyMatchesTheLiteralCommandList() +{ + // The family table is the single declaration EditorHost registers handlers + // from, and the data functions above iterate the table itself. Pin it + // against a literal list, so a dropped row fails here instead of running + // one row fewer and passing while the contract still advertises the id. + using Entry = QPair; + const QList expected = { + { QStringLiteral("actionCreateTextHighlight"), pdf::AnnotationAuthoringSubtype::TextHighlight }, + { QStringLiteral("actionCreateTextUnderline"), pdf::AnnotationAuthoringSubtype::TextUnderline }, + { QStringLiteral("actionCreateTextStrikeout"), pdf::AnnotationAuthoringSubtype::TextStrikeout }, + { QStringLiteral("actionCreateTextSquiggly"), pdf::AnnotationAuthoringSubtype::TextSquiggly }, + { QStringLiteral("actionCreateRectangle"), pdf::AnnotationAuthoringSubtype::Rectangle }, + { QStringLiteral("actionCreateEllipse"), pdf::AnnotationAuthoringSubtype::Ellipse }, + { QStringLiteral("actionCreateStraightLine"), pdf::AnnotationAuthoringSubtype::StraightLine }, + { QStringLiteral("actionCreatePolyline"), pdf::AnnotationAuthoringSubtype::Polyline }, + { QStringLiteral("actionCreatePolygon"), pdf::AnnotationAuthoringSubtype::Polygon }, + { QStringLiteral("actionCreateFreehandCurve"), pdf::AnnotationAuthoringSubtype::FreehandCurve }, + { QStringLiteral("actionInlineText"), pdf::AnnotationAuthoringSubtype::InlineText }, + { QStringLiteral("actionStickyNoteComment"), pdf::AnnotationAuthoringSubtype::StickyNoteComment }, + { QStringLiteral("actionStickyNoteHelp"), pdf::AnnotationAuthoringSubtype::StickyNoteHelp }, + { QStringLiteral("actionStickyNoteInsert"), pdf::AnnotationAuthoringSubtype::StickyNoteInsert }, + { QStringLiteral("actionStickyNoteKey"), pdf::AnnotationAuthoringSubtype::StickyNoteKey }, + { QStringLiteral("actionStickyNoteNewParagraph"), pdf::AnnotationAuthoringSubtype::StickyNoteNewParagraph }, + { QStringLiteral("actionStickyNoteNote"), pdf::AnnotationAuthoringSubtype::StickyNoteNote }, + { QStringLiteral("actionStickyNoteParagraph"), pdf::AnnotationAuthoringSubtype::StickyNoteParagraph }, + }; + + const QList& table = pdfinteraction::annotationAuthoringCommandIds(); + QCOMPARE(table.size(), qsizetype(18)); + QCOMPARE(table.size(), expected.size()); + for (qsizetype index = 0; index < expected.size(); ++index) + { + QCOMPARE(table.at(index).first, expected.at(index).first); + QCOMPARE(table.at(index).second, expected.at(index).second); + } +} + void AnnotationAuthoringTest::parametersMapToTypedRequestsAndRefuseMalformedOnes() { using pdf::AnnotationAuthoringSubtype; diff --git a/UnitTests/tst_authoringedittest.cpp b/UnitTests/tst_authoringedittest.cpp index afc530892..e78dfb88d 100644 --- a/UnitTests/tst_authoringedittest.cpp +++ b/UnitTests/tst_authoringedittest.cpp @@ -169,6 +169,73 @@ pdf::PDFDocument buildDocumentWithStructureOutline() return *modifier.getDocument(); } +/// A document whose outline carries one item built by the caller, which the +/// outline writer may not be able to write back. +pdf::PDFDocument buildDocumentWithOutlineItem(pdf::PDFObject item) +{ + const pdf::PDFDocument document = buildDocument(1); + pdf::PDFDocumentModifier modifier(&document); + pdf::PDFDocumentBuilder* builder = modifier.getBuilder(); + + const pdf::PDFObjectReference itemReference = builder->addObject(item); + + pdf::PDFObjectFactory rootFactory; + rootFactory.beginDictionary(); + rootFactory.beginDictionaryItem("First"); + rootFactory << itemReference; + rootFactory.endDictionaryItem(); + rootFactory.beginDictionaryItem("Last"); + rootFactory << itemReference; + rootFactory.endDictionaryItem(); + rootFactory.beginDictionaryItem("Count"); + rootFactory << 1; + rootFactory.endDictionaryItem(); + rootFactory.endDictionary(); + + builder->setOutline(builder->addObject(rootFactory.takeObject())); + + if (!modifier.finalize()) + { + return pdf::PDFDocument(); + } + return *modifier.getDocument(); +} + +/// One outline item whose /Dest is a name: a valid destination that addresses +/// the catalog's name tree rather than a page. +pdf::PDFObject namedDestinationOutlineItem() +{ + pdf::PDFObjectFactory itemFactory; + itemFactory.beginDictionary(); + itemFactory.beginDictionaryItem("Title"); + itemFactory << QStringLiteral("Named"); + itemFactory.endDictionaryItem(); + itemFactory.beginDictionaryItem("Dest"); + itemFactory << pdf::WrapName(QByteArrayLiteral("chapter-one")); + itemFactory.endDictionaryItem(); + itemFactory.endDictionary(); + return itemFactory.takeObject(); +} + +/// One outline item whose /Dest is an array with an unknown view name, which +/// parses to an invalid destination. +pdf::PDFObject unparsedDestinationOutlineItem() +{ + pdf::PDFObjectFactory itemFactory; + itemFactory.beginDictionary(); + itemFactory.beginDictionaryItem("Title"); + itemFactory << QStringLiteral("Broken target"); + itemFactory.endDictionaryItem(); + itemFactory.beginDictionaryItem("Dest"); + itemFactory.beginArray(); + itemFactory << 0; + itemFactory << pdf::WrapName(QByteArrayLiteral("Bogus")); + itemFactory.endArray(); + itemFactory.endDictionaryItem(); + itemFactory.endDictionary(); + return itemFactory.takeObject(); +} + } // namespace class AuthoringEditTest : public QObject @@ -188,6 +255,8 @@ private slots: void invalidEditsAreRefusedWithTypedReasons_data(); void invalidEditsAreRefusedWithTypedReasons(); void structureOutlineRefusesAnOutlineRewrite(); + void namedDestinationOutlineIsNeverAPageTarget(); + void unparsedDestinationOutlineRefusesAnOutlineRewrite(); }; void AuthoringEditTest::bookmarkFileRoundTripsCanonically() @@ -586,6 +655,67 @@ void AuthoringEditTest::structureOutlineRefusesAnOutlineRewrite() QCOMPARE(records->at(0).title, QStringLiteral("Fresh")); } +void AuthoringEditTest::namedDestinationOutlineIsNeverAPageTarget() +{ + const pdf::PDFDocument document = buildDocumentWithOutlineItem(namedDestinationOutlineItem()); + QVERIFY(document.getCatalog()->getOutlineRootPtr()); + QCOMPARE(document.getCatalog()->getOutlineRootPtr()->getChildCount(), size_t(1)); + + // A named destination has no page here: the toggle must add a bookmark on + // page 0 instead of removing the item it cannot address. + pdf::PDFAuthoringEdit edit = bookmarkEdit(pdf::PDFAuthoringEdit::Kind::ToggleBookmark); + edit.pageIndex = 0; + edit.text = QStringLiteral("First page"); + const pdf::PDFAuthoringEditResult added = pdf::PDFAuthoringEditWriter::apply(&document, edit); + QVERIFY2(added.applied(), qPrintable(added.typedError)); + QCOMPARE(added.kind, pdf::PDFAuthoringEditResult::Kind::BookmarkAdded); + + // The file carries both items afterwards, and the named one kept its name. + const std::optional parsed = writeAndRead(*added.document); + QVERIFY(parsed.has_value()); + const QSharedPointer root = parsed->getCatalog()->getOutlineRootPtr(); + QVERIFY(root); + QCOMPARE(root->getChildCount(), size_t(2)); + QCOMPARE(root->getChild(0)->getTitle(), QStringLiteral("Named")); + QCOMPARE(root->getChild(1)->getTitle(), QStringLiteral("First page")); + const auto* namedGoTo = dynamic_cast(root->getChild(0)->getAction()); + QVERIFY(namedGoTo); + QVERIFY(namedGoTo->getDestination().isNamedDestination()); + QCOMPARE(namedGoTo->getDestination().getName(), QByteArrayLiteral("chapter-one")); + QCOMPARE(root->getChild(1)->getAction()->getType(), pdf::ActionType::GoTo); + + // Exporting the outline refuses the named item with the typed code rather + // than reporting page 0 for it. + QString typedError; + QVERIFY(!pdf::PDFAuthoringEditWriter::readOutline(&document, typedError).has_value()); + QCOMPARE(typedError, QStringLiteral("bookmark/unsupported-outline-item")); +} + +void AuthoringEditTest::unparsedDestinationOutlineRefusesAnOutlineRewrite() +{ + const pdf::PDFDocument document = buildDocumentWithOutlineItem(unparsedDestinationOutlineItem()); + QVERIFY(document.getCatalog()->getOutlineRootPtr()); + QCOMPARE(document.getCatalog()->getOutlineRootPtr()->getChildCount(), size_t(1)); + + // The outline writer would write the unparsed destination as PDF null, so + // the item is not rewritable and the toggle refuses before writing. + pdf::PDFAuthoringEdit edit = bookmarkEdit(pdf::PDFAuthoringEdit::Kind::ToggleBookmark); + edit.pageIndex = 0; + edit.text = QStringLiteral("First page"); + const pdf::PDFAuthoringEditResult refused = pdf::PDFAuthoringEditWriter::apply(&document, edit); + QVERIFY(!refused.applied()); + QCOMPARE(refused.typedError, QStringLiteral("bookmark/unsupported-outline-item")); + QVERIFY(refused.document.isNull()); + + // The input document's outline is unchanged, destination included. + const QSharedPointer root = document.getCatalog()->getOutlineRootPtr(); + QCOMPARE(root->getChildCount(), size_t(1)); + QCOMPARE(root->getChild(0)->getTitle(), QStringLiteral("Broken target")); + const auto* goTo = dynamic_cast(root->getChild(0)->getAction()); + QVERIFY(goTo); + QVERIFY(!goTo->getDestination().isValid()); +} + QTEST_MAIN(AuthoringEditTest) #include "tst_authoringedittest.moc" diff --git a/UnitTests/tst_directedittest.cpp b/UnitTests/tst_directedittest.cpp index 806db1fd4..7a374a3cb 100644 --- a/UnitTests/tst_directedittest.cpp +++ b/UnitTests/tst_directedittest.cpp @@ -30,7 +30,9 @@ #include #include +#include #include +#include #include "pdfcatalog.h" #include "pdfdirectedit.h" @@ -254,6 +256,208 @@ QString formValue(const pdf::PDFDocument& document, const QString& name, bool* i return value; } +/// Every value carried by a field with the given fully qualified name, in the +/// order the form manager walks the fields. +QList fieldValues(const pdf::PDFDocument& document, const QString& name) +{ + pdf::PDFFormManager manager(nullptr); + manager.setDocument(pdf::PDFModifiedDocument(const_cast(&document), nullptr, pdf::PDFModifiedDocument::Reset)); + QList values; + manager.apply([&values, &name](const pdf::PDFFormField* field) + { + if (field->getName(pdf::PDFFormField::NameType::FullyQualified) != name) + { + return; + } + + const pdf::PDFObject& object = field->getValue(); + if (object.isName()) + { + values.append(QString::fromLatin1(object.getString())); + } + else if (object.isString()) + { + values.append(pdf::PDFEncoding::convertTextString(object.getString())); + } + else + { + values.append(QString()); + } }); + return values; +} + +/// Parses \p packet with a namespace-aware reader and counts the elements whose +/// raw qualified name is \p qualifiedName. \p wellFormed reports whether every +/// prefix resolved; a false \p wellFormed means the count is meaningless. +int countXmlElements(const QByteArray& packet, const QString& qualifiedName, bool* wellFormed) +{ + QXmlStreamReader reader(packet); + reader.setNamespaceProcessing(true); + int count = 0; + while (!reader.atEnd()) + { + reader.readNext(); + if (reader.tokenType() == QXmlStreamReader::StartElement && reader.qualifiedName() == qualifiedName) + { + ++count; + } + } + *wellFormed = !reader.hasError(); + return count; +} + +/// Counts the character tokens equal to \p value: how many places in the packet +/// carry it, without substring false positives. +int countXmlText(const QByteArray& packet, const QString& value) +{ + QXmlStreamReader reader(packet); + reader.setNamespaceProcessing(true); + int count = 0; + while (!reader.atEnd()) + { + reader.readNext(); + if (reader.tokenType() == QXmlStreamReader::Characters && reader.text() == value) + { + ++count; + } + } + return count; +} + +/// A form with two terminal text fields sharing one fully qualified name, each +/// with its own widget: PDF semantics treat them as one field. +pdf::PDFDocument buildDocumentWithSameNamedTextFields() +{ + pdf::PDFDocumentBuilder builder; + const pdf::PDFObjectReference page = builder.appendPage(QRectF(0, 0, 200, 200)); + + const auto addSharedTextField = [&builder, page](const QRectF& rect) + { + return addMergedField(builder, page, "Tx", QStringLiteral("SharedName"), rect, + [&builder, rect](pdf::PDFDictionary& field) + { + field.addEntry(pdf::PDFInplaceOrMemoryString("V"), + pdf::PDFObjectFactory::createTextString(QStringLiteral("original"))); + field.addEntry(pdf::PDFInplaceOrMemoryString("DA"), + pdf::PDFObject::createString(QByteArrayLiteral("/Helv 0 Tf 0 g"))); + field.addEntry(pdf::PDFInplaceOrMemoryString("AP"), + pdf::PDFObject::createDictionary(std::make_shared( + [&builder, rect]() + { + pdf::PDFDictionary appearance; + appearance.addEntry(pdf::PDFInplaceOrMemoryString("N"), + pdf::PDFObject::createReference(addFormXObject(builder, rect))); + return appearance; + }()))); + }); + }; + + std::vector fields; + fields.push_back(addSharedTextField(QRectF(10, 10, 90, 30))); + fields.push_back(addSharedTextField(QRectF(10, 50, 90, 70))); + builder.createAcroForm(fields); + return builder.build(); +} + +/// A form whose first button field has no widget and shares its fully qualified +/// name with a checkbox that carries a widget and an on-state appearance. +pdf::PDFDocument buildDocumentWithWidgetlessFirstMatch() +{ + pdf::PDFDocumentBuilder builder; + const pdf::PDFObjectReference page = builder.appendPage(QRectF(0, 0, 200, 200)); + + pdf::PDFDictionary widgetless; + widgetless.addEntry(pdf::PDFInplaceOrMemoryString("FT"), pdf::PDFObject::createName("Btn")); + widgetless.addEntry(pdf::PDFInplaceOrMemoryString("T"), pdf::PDFObjectFactory::createTextString(QStringLiteral("SharedBox"))); + widgetless.addEntry(pdf::PDFInplaceOrMemoryString("V"), pdf::PDFObject::createName("Off")); + + std::vector fields; + fields.push_back(builder.addObject(pdf::PDFObject::createDictionary(std::make_shared(std::move(widgetless))))); + fields.push_back(addMergedField(builder, page, "Btn", QStringLiteral("SharedBox"), QRectF(10, 50, 30, 30), + [&builder](pdf::PDFDictionary& field) + { + pdf::PDFDictionary normal; + normal.addEntry(pdf::PDFInplaceOrMemoryString("Yes"), + pdf::PDFObject::createReference(addFormXObject(builder, QRectF(10, 50, 30, 30)))); + normal.addEntry(pdf::PDFInplaceOrMemoryString("Off"), + pdf::PDFObject::createReference(addFormXObject(builder, QRectF(10, 50, 30, 30)))); + pdf::PDFDictionary appearance; + appearance.addEntry(pdf::PDFInplaceOrMemoryString("N"), + pdf::PDFObject::createDictionary(std::make_shared(std::move(normal)))); + field.addEntry(pdf::PDFInplaceOrMemoryString("AP"), + pdf::PDFObject::createDictionary(std::make_shared(std::move(appearance)))); + field.addEntry(pdf::PDFInplaceOrMemoryString("AS"), pdf::PDFObject::createName("Off")); + field.addEntry(pdf::PDFInplaceOrMemoryString("V"), pdf::PDFObject::createName("Off")); + })); + + builder.createAcroForm(fields); + return builder.build(); +} + +/// A document whose EmbeddedFiles name tree carries one attachment with an +/// indirect specification and one whose specification is a direct dictionary +/// value in the names array, which has no reference to relist. +pdf::PDFDocument buildDocumentWithInlineAttachment() +{ + pdf::PDFDocumentBuilder builder; + builder.appendPage(QRectF(0, 0, 200, 200)); + + const auto addEmbeddedFileStream = [&builder](const QByteArray& contents) + { + pdf::PDFDictionary stream; + stream.addEntry(pdf::PDFInplaceOrMemoryString("Type"), pdf::PDFObject::createName("EmbeddedFile")); + stream.addEntry(pdf::PDFInplaceOrMemoryString("Length"), pdf::PDFObject::createInteger(contents.size())); + return builder.addObject(pdf::PDFObject::createStream( + std::make_shared(std::move(stream), QByteArray(contents)))); + }; + + const auto makeFileSpecification = [](const QString& name, pdf::PDFObjectReference file) + { + pdf::PDFDictionary embeddedFiles; + embeddedFiles.addEntry(pdf::PDFInplaceOrMemoryString("F"), pdf::PDFObject::createReference(file)); + embeddedFiles.addEntry(pdf::PDFInplaceOrMemoryString("UF"), pdf::PDFObject::createReference(file)); + + pdf::PDFDictionary specification; + specification.addEntry(pdf::PDFInplaceOrMemoryString("Type"), pdf::PDFObject::createName("Filespec")); + specification.addEntry(pdf::PDFInplaceOrMemoryString("F"), pdf::PDFObject::createString(name.toUtf8())); + specification.addEntry(pdf::PDFInplaceOrMemoryString("UF"), pdf::PDFObjectFactory::createTextString(name)); + specification.addEntry(pdf::PDFInplaceOrMemoryString("EF"), + pdf::PDFObject::createDictionary(std::make_shared(std::move(embeddedFiles)))); + return pdf::PDFObject::createDictionary(std::make_shared(std::move(specification))); + }; + + const pdf::PDFObjectReference normalFile = addEmbeddedFileStream(QByteArrayLiteral("normal")); + const pdf::PDFObjectReference normalSpecification = + builder.addObject(makeFileSpecification(QStringLiteral("normal.txt"), normalFile)); + const pdf::PDFObjectReference inlineFile = addEmbeddedFileStream(QByteArrayLiteral("inline")); + + pdf::PDFObjectFactory treeFactory; + treeFactory.beginDictionary(); + treeFactory.beginDictionaryItem("Names"); + treeFactory.beginArray(); + treeFactory << pdf::PDFObject::createString(QByteArrayLiteral("normal.txt")); + treeFactory << normalSpecification; + treeFactory << pdf::PDFObject::createString(QByteArrayLiteral("inline.txt")); + treeFactory << makeFileSpecification(QStringLiteral("inline.txt"), inlineFile); + treeFactory.endArray(); + treeFactory.endDictionaryItem(); + treeFactory.endDictionary(); + + pdf::PDFObjectFactory catalogFactory; + catalogFactory.beginDictionary(); + catalogFactory.beginDictionaryItem("Names"); + catalogFactory.beginDictionary(); + catalogFactory.beginDictionaryItem("EmbeddedFiles"); + catalogFactory << builder.addObject(treeFactory.takeObject()); + catalogFactory.endDictionaryItem(); + catalogFactory.endDictionary(); + catalogFactory.endDictionaryItem(); + catalogFactory.endDictionary(); + builder.mergeTo(builder.getCatalogReference(), catalogFactory.takeObject()); + + return builder.build(); +} + } // namespace class DirectEditTest : public QObject @@ -270,7 +474,14 @@ private slots: void extractionUsesTheAtomicWriterAndRefusesExecutableAutoOpen(); void metadataRoundTripsInfoAndXmp(); void metadataMergesIntoAnExistingPacketAndKeepsForeignProperties(); + void metadataReplacesAPropertyCarriedInASecondDescription(); + void metadataDeclaresAppendedPropertyNamespaces(); + void metadataSubstitutesAttributeFormProperties(); + void metadataRefusesAmbiguousPropertyOccurrences(); void unreadableXmpPacketIsRefusedInsteadOfReplaced(); + void attachmentInlineSpecificationsAreRefusedNotDropped(); + void sameNamedFormFieldsAreUpdatedTogether(); + void widgetlessFormFieldMatchFallsBackToTheWritableTwin(); void revisionIdentityRoundTripsThroughItsStringForm(); }; @@ -385,6 +596,44 @@ void DirectEditTest::unknownFieldAndForeignChoiceValueAreRefused() } } +void DirectEditTest::sameNamedFormFieldsAreUpdatedTogether() +{ + const pdf::PDFDocument source = buildDocumentWithSameNamedTextFields(); + QCOMPARE(fieldValues(source, QStringLiteral("SharedName")).size(), 2); + + pdf::FormFieldValueEdit edit; + edit.field = QStringLiteral("SharedName"); + edit.text = QStringLiteral("Filled together"); + const pdf::DirectEditOutcome outcome = pdf::PDFFormValueWriter::setFieldValue(&source, edit); + QVERIFY2(outcome.ok(), pdf::getDirectEditRefusalCode(outcome.refusal)); + + // Same-named terminal fields are one field per PDF semantics: both field + // objects carry the new value, not just the one that was addressed. + const QList values = fieldValues(*outcome.document, QStringLiteral("SharedName")); + QCOMPARE(values.size(), 2); + QCOMPARE(values.at(0), QStringLiteral("Filled together")); + QCOMPARE(values.at(1), QStringLiteral("Filled together")); +} + +void DirectEditTest::widgetlessFormFieldMatchFallsBackToTheWritableTwin() +{ + const pdf::PDFDocument source = buildDocumentWithWidgetlessFirstMatch(); + QCOMPARE(fieldValues(source, QStringLiteral("SharedBox")).size(), 2); + + // The first field object with that name has no widget; the editable match + // is its widget-bearing twin, and the value reaches both. + pdf::FormFieldValueEdit edit; + edit.field = QStringLiteral("SharedBox"); + edit.checked = true; + const pdf::DirectEditOutcome outcome = pdf::PDFFormValueWriter::setFieldValue(&source, edit); + QVERIFY2(outcome.ok(), pdf::getDirectEditRefusalCode(outcome.refusal)); + + const QList values = fieldValues(*outcome.document, QStringLiteral("SharedBox")); + QCOMPARE(values.size(), 2); + QCOMPARE(values.at(0), QStringLiteral("Yes")); + QCOMPARE(values.at(1), QStringLiteral("Yes")); +} + void DirectEditTest::attachmentRoundTripsThroughTheNameTree() { const pdf::PDFDocument source = buildFormDocument(false); @@ -465,6 +714,53 @@ void DirectEditTest::attachmentRemovalUnlistsOnlyTheNamedFile() QVERIFY(emptied.document->getCatalog()->getEmbeddedFiles().empty()); } +void DirectEditTest::attachmentInlineSpecificationsAreRefusedNotDropped() +{ + const pdf::PDFDocument source = buildDocumentWithInlineAttachment(); + + // The fixture parses: an indirect specification and one inline in the names + // array, which carries no reference to relist. + const auto& files = source.getCatalog()->getEmbeddedFiles(); + QCOMPARE(files.size(), size_t(2)); + const auto inlineItem = files.find(QByteArrayLiteral("inline.txt")); + QVERIFY(inlineItem != files.cend()); + QVERIFY(!inlineItem->second.getSelfReference().isValid()); + QCOMPARE(inlineItem->second.getPlatformFileName(), QStringLiteral("inline.txt")); + + // Adding another attachment would rebuild the tree carrying only the + // addressable entries, so it refuses instead of unlisting the inline one. + pdf::AttachmentEdit edit; + edit.name = QStringLiteral("extra.txt"); + edit.contents = QByteArrayLiteral("extra"); + const pdf::DirectEditOutcome refused = pdf::PDFAttachmentWriter::add(&source, edit); + QVERIFY(!refused.ok()); + QCOMPARE(refused.refusal, pdf::DirectEditRefusal::AttachmentSpecificationNotAddressable); + QCOMPARE(QString::fromLatin1(pdf::getDirectEditRefusalCode(refused.refusal)), + QStringLiteral("attachment/specification-not-addressable")); + QVERIFY(!refused.document); + + // Removing any other attachment would have to relist the inline one, so it + // refuses the same way... + const pdf::DirectEditOutcome refusedRemoval = pdf::PDFAttachmentWriter::remove(&source, QStringLiteral("normal.txt")); + QVERIFY(!refusedRemoval.ok()); + QCOMPARE(refusedRemoval.refusal, pdf::DirectEditRefusal::AttachmentSpecificationNotAddressable); + QVERIFY(!refusedRemoval.document); + + // ...while removing the inline attachment itself drops it from the tree + // without relisting it, so it proceeds and only unlists that one. + const pdf::DirectEditOutcome removedInline = pdf::PDFAttachmentWriter::remove(&source, QStringLiteral("inline.txt")); + QVERIFY2(removedInline.ok(), pdf::getDirectEditRefusalCode(removedInline.refusal)); + const auto& remaining = removedInline.document->getCatalog()->getEmbeddedFiles(); + QCOMPARE(remaining.size(), size_t(1)); + QVERIFY(remaining.find(QByteArrayLiteral("inline.txt")) == remaining.cend()); + QVERIFY(remaining.find(QByteArrayLiteral("normal.txt")) != remaining.cend()); + + // The refused edits wrote nothing: the source document still carries both. + QCOMPARE(source.getCatalog()->getEmbeddedFiles().size(), size_t(2)); + QVERIFY(source.getCatalog()->getEmbeddedFiles().find(QByteArrayLiteral("inline.txt")) != + source.getCatalog()->getEmbeddedFiles().cend()); +} + void DirectEditTest::extractionUsesTheAtomicWriterAndRefusesExecutableAutoOpen() { const pdf::PDFDocument source = buildFormDocument(false); @@ -595,17 +891,153 @@ void DirectEditTest::metadataMergesIntoAnExistingPacketAndKeepsForeignProperties const QByteArray packet = pdf::PDFMetadataWriter::readXmpPacket(outcome.document.data()); // The property this edit owns is replaced, exactly once, and the packet is // still a packet rather than an XML document with a declaration bolted on. - QVERIFY(packet.contains("")); + // The packet declared dc only on the replaced element, so the replacement + // has to carry the binding itself for the result to parse. + bool wellFormed = false; + QCOMPARE(countXmlElements(packet, QStringLiteral("dc:title"), &wellFormed), 1); + QVERIFY(wellFormed); QVERIFY(packet.contains("New title")); QVERIFY(!packet.contains("Old title")); QVERIFY(!packet.startsWith(""), 1); // Everything else the packet carried survives, including the PDF/A claim. QVERIFY(packet.contains("pdfaid:part")); QVERIFY(packet.contains("pdfaid:conformance")); QVERIFY(packet.contains("xpacket")); } +void DirectEditTest::metadataReplacesAPropertyCarriedInASecondDescription() +{ + pdf::PDFDocumentBuilder builder; + builder.appendPage(QRectF(0, 0, 200, 200)); + // The subject lives in the second rdf:Description: the merge must replace + // it where it is instead of appending a second copy beside the first one. + builder.setCatalogMetadata(QByteArrayLiteral( + "\n" + "\n" + " \n" + " \n" + " Old title\n" + " \n" + " \n" + " Old subject\n" + " \n" + " \n" + "\n" + "")); + const pdf::PDFDocument source = builder.build(); + + pdf::DocumentMetadataEdit edit; + edit.subject = QStringLiteral("New subject"); + const pdf::DirectEditOutcome outcome = pdf::PDFMetadataWriter::update(&source, edit); + QVERIFY2(outcome.ok(), pdf::getDirectEditRefusalCode(outcome.refusal)); + + const QByteArray packet = pdf::PDFMetadataWriter::readXmpPacket(outcome.document.data()); + bool wellFormed = false; + QCOMPARE(countXmlElements(packet, QStringLiteral("dc:description"), &wellFormed), 1); + QVERIFY(wellFormed); + QCOMPARE(countXmlText(packet, QStringLiteral("New subject")), 1); + QVERIFY(!packet.contains("Old subject")); + // The first Description is untouched. + QCOMPARE(countXmlText(packet, QStringLiteral("Old title")), 1); +} + +void DirectEditTest::metadataDeclaresAppendedPropertyNamespaces() +{ + pdf::PDFDocumentBuilder builder; + builder.appendPage(QRectF(0, 0, 200, 200)); + // The packet declares pdfaid on the Description and dc on a child element, + // so the appended pdf:Keywords has to declare pdf itself or the merged + // packet is not namespace-well-formed. + builder.setCatalogMetadata(QByteArrayLiteral( + "\n" + "\n" + " \n" + " \n" + " Old title\n" + " \n" + " \n" + "\n" + "")); + const pdf::PDFDocument source = builder.build(); + + pdf::DocumentMetadataEdit edit; + edit.keywords = QStringLiteral("Loop keywords"); + const pdf::DirectEditOutcome outcome = pdf::PDFMetadataWriter::update(&source, edit); + QVERIFY2(outcome.ok(), pdf::getDirectEditRefusalCode(outcome.refusal)); + + const QByteArray packet = pdf::PDFMetadataWriter::readXmpPacket(outcome.document.data()); + bool wellFormed = false; + QCOMPARE(countXmlElements(packet, QStringLiteral("pdf:Keywords"), &wellFormed), 1); + QVERIFY(wellFormed); + QCOMPARE(countXmlText(packet, QStringLiteral("Loop keywords")), 1); + QCOMPARE(countXmlText(packet, QStringLiteral("Old title")), 1); + QVERIFY(packet.contains("pdfaid:conformance")); +} + +void DirectEditTest::metadataSubstitutesAttributeFormProperties() +{ + pdf::PDFDocumentBuilder builder; + builder.appendPage(QRectF(0, 0, 200, 200)); + // The attribute shorthand on rdf:Description carries the keyword value. + builder.setCatalogMetadata(QByteArrayLiteral( + "\n" + "\n" + " \n" + " \n" + " \n" + " \n" + "\n" + "")); + const pdf::PDFDocument source = builder.build(); + + pdf::DocumentMetadataEdit edit; + edit.keywords = QStringLiteral("New keywords"); + const pdf::DirectEditOutcome outcome = pdf::PDFMetadataWriter::update(&source, edit); + QVERIFY2(outcome.ok(), pdf::getDirectEditRefusalCode(outcome.refusal)); + + const QByteArray packet = pdf::PDFMetadataWriter::readXmpPacket(outcome.document.data()); + bool wellFormed = false; + // The attribute carries the new value and no element form is added beside + // it, so the packet still carries the property exactly once. + QCOMPARE(packet.count("pdf:Keywords=\"New keywords\""), 1); + QCOMPARE(countXmlElements(packet, QStringLiteral("pdf:Keywords"), &wellFormed), 0); + QVERIFY(wellFormed); + QVERIFY(!packet.contains("old keywords")); +} + +void DirectEditTest::metadataRefusesAmbiguousPropertyOccurrences() +{ + pdf::PDFDocumentBuilder builder; + builder.appendPage(QRectF(0, 0, 200, 200)); + // Two Descriptions carry xmp:CreatorTool: replacing one of them would leave + // the packet with an old and a new value, so the merge refuses. + const QByteArray original = QByteArrayLiteral( + "\n" + "\n" + " \n" + " \n" + " Old tool\n" + " \n" + " \n" + " Old tool\n" + " \n" + " \n" + "\n" + ""); + builder.setCatalogMetadata(original); + const pdf::PDFDocument source = builder.build(); + + pdf::DocumentMetadataEdit edit; + edit.creator = QStringLiteral("New tool"); + const pdf::DirectEditOutcome outcome = pdf::PDFMetadataWriter::update(&source, edit); + QVERIFY(!outcome.ok()); + QCOMPARE(outcome.refusal, pdf::DirectEditRefusal::XmpUnreadable); + QCOMPARE(QString::fromLatin1(pdf::getDirectEditRefusalCode(outcome.refusal)), QStringLiteral("metadata/xmp-unreadable")); + QVERIFY(!outcome.document); + // The packet is untouched. + QCOMPARE(pdf::PDFMetadataWriter::readXmpPacket(&source), original); +} + void DirectEditTest::unreadableXmpPacketIsRefusedInsteadOfReplaced() { pdf::PDFDocumentBuilder builder; @@ -638,10 +1070,28 @@ void DirectEditTest::revisionIdentityRoundTripsThroughItsStringForm() QVERIFY(parsed.isValid()); QVERIFY(parsed == identity); + // A document with no source bytes (an in-memory document after an edit) + // serializes with an empty hash field: that is a round-trippable identity, + // not a malformed one. + pdf::PDFRevisionIdentity inMemory; + inMemory.document.documentId = QStringLiteral("1a2b"); + inMemory.documentRevision = 3; + inMemory.cacheGeneration = 1; + inMemory.effectiveProfileIdentity = QStringLiteral("loop-default"); + const pdf::PDFRevisionIdentity inMemoryParsed = pdf::PDFRevisionIdentity::fromString(inMemory.toString()); + QVERIFY(inMemoryParsed.isValid()); + QVERIFY(inMemoryParsed == inMemory); + // A malformed value yields an invalid identity rather than a guessed one. QVERIFY(!pdf::PDFRevisionIdentity::fromString(QString()).isValid()); QVERIFY(!pdf::PDFRevisionIdentity::fromString(QStringLiteral("not-a-revision")).isValid()); QVERIFY(!pdf::PDFRevisionIdentity::fromString(QStringLiteral("00ff:1a2b:notanumber:9:")).isValid()); + + // A hash with non-hex characters or an odd trailing nibble is corrupted: + // it must not come back as a valid identity with a silently dropped hash. + QVERIFY(!pdf::PDFRevisionIdentity::fromString(QStringLiteral("zz:1a2b:7:9:")).isValid()); + QVERIFY(!pdf::PDFRevisionIdentity::fromString(QStringLiteral("00f:1a2b:7:9:")).isValid()); + QVERIFY(!pdf::PDFRevisionIdentity::fromString(QStringLiteral("00ffzz:1a2b:7:9:")).isValid()); } QTEST_MAIN(DirectEditTest) diff --git a/UnitTests/tst_documentfacadetest.cpp b/UnitTests/tst_documentfacadetest.cpp index d8a646d06..76820355a 100644 --- a/UnitTests/tst_documentfacadetest.cpp +++ b/UnitTests/tst_documentfacadetest.cpp @@ -391,6 +391,8 @@ private slots: void signatureCheckReducesVerificationResultsToTheOperatorVocabulary(); void signatureCheckAdmitsOnlyTheCurrentRevisionAndClearsWithTheDocument(); + void refusedSignatureCheckCompletionReleasesTheRunningCheck(); + void supersededSignatureCheckCompletionDoesNotReleaseTheNewerCheck(); // The typed direct-edit gateway (ADR-014, issue #233). void authoringEditAdvancesTheRevisionAndWritesTheOutline(); @@ -1426,6 +1428,81 @@ void DocumentFacadeTest::signatureCheckAdmitsOnlyTheCurrentRevisionAndClearsWith QCOMPARE(harness.facade->signatureFacts().signatureCount, 0); } +void DocumentFacadeTest::refusedSignatureCheckCompletionReleasesTheRunningCheck() +{ + Harness harness; + harness.facade->open(QStringLiteral("/corpus/fixture.pdf")); + QTRY_COMPARE(harness.facade->state(), pdfinteraction::DocumentState::Ready); + + QString error; + const auto token = harness.facade->beginSignatureCheck(error); + QVERIFY2(token.has_value(), qPrintable(error)); + + // A revision change while Core verifies: the completion no longer belongs + // to the current revision and is refused, but it must release the + // one-check-at-a-time flag instead of wedging every later check. + harness.context.markModified(); + + pdf::PDFSignatureVerificationResult validResult; + validResult.setFlag(pdf::PDFSignatureVerificationResult::Certificate_OK, true); + validResult.setFlag(pdf::PDFSignatureVerificationResult::Signature_OK, true); + validResult.validate(); + + QVERIFY(!harness.facade->completeSignatureCheck(*token, { validResult }, error)); + QCOMPARE(error, QStringLiteral("signature/not-current-or-terminal")); + + const auto resubmitted = harness.facade->beginSignatureCheck(error); + QVERIFY2(resubmitted.has_value(), qPrintable(error)); + QVERIFY2(harness.facade->completeSignatureCheck(*resubmitted, { validResult }, error), qPrintable(error)); + QCOMPARE(harness.facade->signatureFacts().state, pdfinteraction::DocumentSignatureState::Valid); + + // The failed-check path releases the same way. + const auto failing = harness.facade->beginSignatureCheck(error); + QVERIFY(failing.has_value()); + harness.context.markModified(); + QVERIFY(!harness.facade->failSignatureCheck(*failing, + QStringLiteral("The file changed since the document was loaded."), + error)); + QCOMPARE(error, QStringLiteral("signature/not-current-or-terminal")); + QVERIFY2(harness.facade->beginSignatureCheck(error).has_value(), qPrintable(error)); +} + +void DocumentFacadeTest::supersededSignatureCheckCompletionDoesNotReleaseTheNewerCheck() +{ + Harness harness; + harness.facade->open(QStringLiteral("/corpus/fixture.pdf")); + QTRY_COMPARE(harness.facade->state(), pdfinteraction::DocumentState::Ready); + + QString error; + const auto oldToken = harness.facade->beginSignatureCheck(error); + QVERIFY(oldToken.has_value()); + harness.context.markModified(); + QVERIFY(!harness.facade->completeSignatureCheck(*oldToken, {}, error)); + + const auto current = harness.facade->beginSignatureCheck(error); + QVERIFY2(current.has_value(), qPrintable(error)); + + // The old completion is superseded now: it is refused, and it must not + // clear the newer check's running flag. + QVERIFY(!harness.facade->completeSignatureCheck(*oldToken, {}, error)); + QCOMPARE(error, QStringLiteral("signature/not-current-or-terminal")); + QVERIFY(!harness.facade->beginSignatureCheck(error)); + QCOMPARE(error, QStringLiteral("signature/check-already-running")); + + QVERIFY(!harness.facade->failSignatureCheck(*oldToken, QStringLiteral("stale"), error)); + QCOMPARE(error, QStringLiteral("signature/not-current-or-terminal")); + QVERIFY(!harness.facade->beginSignatureCheck(error)); + QCOMPARE(error, QStringLiteral("signature/check-already-running")); + + // The newer check is the one that completes. + pdf::PDFSignatureVerificationResult validResult; + validResult.setFlag(pdf::PDFSignatureVerificationResult::Certificate_OK, true); + validResult.setFlag(pdf::PDFSignatureVerificationResult::Signature_OK, true); + validResult.validate(); + QVERIFY2(harness.facade->completeSignatureCheck(*current, { validResult }, error), qPrintable(error)); + QCOMPARE(harness.facade->signatureFacts().state, pdfinteraction::DocumentSignatureState::Valid); +} + void DocumentFacadeTest::realDocumentRoundTripsThroughCoreReaderAndWriter() { QTemporaryDir directory; diff --git a/UnitTests/tst_editorhosttest.cpp b/UnitTests/tst_editorhosttest.cpp index a6912a545..7dd13a6cd 100644 --- a/UnitTests/tst_editorhosttest.cpp +++ b/UnitTests/tst_editorhosttest.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -35,12 +36,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -540,6 +543,22 @@ QString readFormFieldValue(const pdf::PDFDocument* document, const QString& name } // namespace +// Takes the mailto URL that the desktop would open, so the hand-off is observed +// without launching a mail client. +class MailUrlSink : public QObject +{ + Q_OBJECT + +public: + QList received; + +public Q_SLOTS: + void receive(const QUrl& url) + { + received.append(url); + } +}; + class EditorHostTest : public QObject { Q_OBJECT @@ -607,6 +626,7 @@ private slots: void softProofWithoutAUsableOutputIntentIsAVisibleState(); void platesListSeparationsAndCarryVisibilityWithoutTouchingTheDocument(); void inkCoverageReadoutAtAPointIsMeasuredOrExplicitlyUnavailable(); + void inkCoverageRefusesSupersededJobsAndDropsTheReadoutOnRevisionChange(); // Shell settings, render options and diagnostics (#239). void settingsRoundTripThroughTheOneStore(); @@ -617,6 +637,8 @@ private slots: /// Issue #232: extraction refuses to overwrite an existing file up front. void imageExtractionRefusesToOverwriteAnExistingFile(); + /// Issue #232: a screenshot capture refuses to overwrite an existing file. + void screenshotCaptureRefusesToOverwriteAnExistingFile(); // Bookmarks and hyperlinks (#233, ADR-014). void bookmarkCommandsWriteTheOutlineThroughTheCatalog(); @@ -3835,6 +3857,63 @@ void EditorHostTest::inkCoverageReadoutAtAPointIsMeasuredOrExplicitlyUnavailable QVERIFY(!empty.requestInkCoverageAtPoint(10.0, 10.0)); } +/// The ink-coverage readout is a measurement of one revision. A job the +/// scheduler marks stale publishes the not-measured state, never ink numbers +/// measured against a superseded revision, and a revision change drops the +/// readout instead of letting it outlive its revision. +void EditorHostTest::inkCoverageRefusesSupersededJobsAndDropsTheReadoutOnRevisionChange() +{ + const QString path = preflightFixturesDir() + QStringLiteral("/color-cmyk.pdf"); + QVERIFY2(QFileInfo::exists(path), qPrintable(path)); + + EditorHost host; + host.openFileUrl(QUrl::fromLocalFile(path)); + QTRY_VERIFY_WITH_TIMEOUT(host.hasDocument(), 30000); + host.setViewportGeometry(96.0 / 25.4, 1.0, 1024, 768); + QTRY_COMPARE_WITH_TIMEOUT(host.currentPage(), 0, 30000); + + // A real measurement first: it is the readout the revision change below + // must drop. + QVERIFY(host.requestInkCoverageAtPoint(150.0, 150.0)); + QTRY_VERIFY_WITH_TIMEOUT(host.inkCoverageReadout().value(QStringLiteral("available")).toBool(), 30000); + + // An edit moves the document to a new revision. The readout measured the + // revision that just left, so it is dropped rather than kept on. + const QString beforeEdit = host.sessionForTest()->facade().currentRevision().toString(); + QSignalSpy finished(&host.sessionForTest()->catalog(), &pdfinteraction::CommandCatalog::invocationFinished); + const quint64 bookmark = host.invokeCommand(QStringLiteral("actionBookmarkPage"), + { { QStringLiteral("title"), QStringLiteral("Chapter one") } }); + QVERIFY(bookmark != 0); + const std::optional bookmarkResult = terminalResultFor(finished, bookmark); + QVERIFY(bookmarkResult.has_value()); + QVERIFY2(bookmarkResult->state == pdfinteraction::CommandTerminalState::Completed, + qPrintable(bookmarkResult->typedError)); + QVERIFY(host.sessionForTest()->facade().currentRevision().toString() != beforeEdit); + QVERIFY(host.inkCoverageReadout().isEmpty()); + + // A stale job still delivers its completion, but the completion is not a + // measurement. The revision fence is moved ahead of the request so the + // scheduler marks the job stale the way a superseded run is marked, then a + // live request under it publishes the not-measured shape. + pdf::PDFJobScheduler& scheduler = host.sessionForTest()->scheduler(); + const QString documentKey = host.sessionForTest()->revisionSource()->documentKey(); + QVERIFY(!documentKey.isEmpty()); + const QString currentRevision = host.sessionForTest()->facade().currentRevision().toString(); + scheduler.setCurrentRevision(documentKey, currentRevision + QStringLiteral("-superseded")); + QVERIFY(host.requestInkCoverageAtPoint(150.0, 150.0)); + QTRY_COMPARE_WITH_TIMEOUT(host.inkCoverageReadout().value(QStringLiteral("reason")).toString(), + QStringLiteral("ink-coverage/stale-or-cancelled"), 30000); + QVERIFY(!host.inkCoverageReadout().value(QStringLiteral("available")).toBool()); + QCOMPARE(host.inkCoverageReadout().value(QStringLiteral("pageIndex")).toInt(), 0); + QVERIFY(!host.inkCoverageReadout().contains(QStringLiteral("totalInkCoverage"))); + + // A fresh measurement on the current revision repopulates the readout. + scheduler.setCurrentRevision(documentKey, currentRevision); + QVERIFY(host.requestInkCoverageAtPoint(150.0, 150.0)); + QTRY_VERIFY_WITH_TIMEOUT(host.inkCoverageReadout().value(QStringLiteral("available")).toBool(), 30000); + QVERIFY(host.inkCoverageReadout().value(QStringLiteral("totalInkCoverage")).toDouble() > 0.0); +} + void EditorHostTest::settingsRoundTripThroughTheOneStore() { @@ -4127,6 +4206,47 @@ void EditorHostTest::imageExtractionRefusesToOverwriteAnExistingFile() QCOMPARE(untouched.readAll(), QByteArray("keep-me")); } +/// Issue #232. A screenshot capture refuses to overwrite an existing file and +/// leaves its bytes untouched, so a repeated capture cannot silently replace a +/// saved one. +void EditorHostTest::screenshotCaptureRefusesToOverwriteAnExistingFile() +{ + QTemporaryDir directory; + QVERIFY(directory.isValid()); + const QString path = writePagesFixture(directory.filePath(QStringLiteral("capture.pdf")), 1); + QVERIFY(!path.isEmpty()); + + EditorHost host; + host.openFileUrl(QUrl::fromLocalFile(path)); + QTRY_VERIFY_WITH_TIMEOUT(host.hasDocument(), 15000); + host.setViewportGeometry(96.0 / 25.4, 1.0, 1024, 768); + host.goToPage(0); + QTRY_COMPARE_WITH_TIMEOUT(host.currentPage(), 0, 15000); + waitForSurfacesToSettle(host); + + // The capture refuses because of the existing file only once there is a + // composed image: wait for an admitted canvas tile, then prove composition + // with a capture into a free path, so the refusal below is the overwrite + // policy and not an empty canvas. + pdfinteraction::PageSurfaceCoordinator* surfaces = host.sessionForTest()->surfaces(); + QTRY_VERIFY_WITH_TIMEOUT(!surfaces->snapshot().isEmpty(), 15000); + const QString fresh = directory.filePath(QStringLiteral("fresh.png")); + QVERIFY(host.captureCanvasFileUrl(QUrl::fromLocalFile(fresh))); + QVERIFY(QFileInfo::exists(fresh)); + + const QString existing = directory.filePath(QStringLiteral("existing.png")); + QFile planted(existing); + QVERIFY(planted.open(QIODevice::WriteOnly)); + planted.write("keep-me"); + planted.close(); + + QVERIFY(!host.captureCanvasFileUrl(QUrl::fromLocalFile(existing))); + + QFile captured(existing); + QVERIFY(captured.open(QIODevice::ReadOnly)); + QCOMPARE(captured.readAll(), QByteArray("keep-me")); +} + void EditorHostTest::bookmarkCommandsWriteTheOutlineThroughTheCatalog() { QTemporaryDir directory; @@ -4257,9 +4377,19 @@ void EditorHostTest::bookmarkImportExportRoundTripsAndRefusesAStaleRevision() waitForSurfacesToSettle(source); QSignalSpy finished(&source.sessionForTest()->catalog(), &pdfinteraction::CommandCatalog::invocationFinished); - QVERIFY(source.invokeCommand(QStringLiteral("actionBookmarkPage"), - { { QStringLiteral("title"), QStringLiteral("Chapter one") } }) != 0); - QVERIFY(source.invokeCommand(QStringLiteral("actionBookmarkAutoGenerate")) != 0); + const quint64 custom = source.invokeCommand(QStringLiteral("actionBookmarkPage"), + { { QStringLiteral("title"), QStringLiteral("Chapter one") } }); + QVERIFY(custom != 0); + const std::optional customResult = terminalResultFor(finished, custom); + QVERIFY(customResult.has_value()); + QVERIFY2(customResult->state == pdfinteraction::CommandTerminalState::Completed, + qPrintable(customResult->typedError)); + const quint64 generated = source.invokeCommand(QStringLiteral("actionBookmarkAutoGenerate")); + QVERIFY(generated != 0); + const std::optional generatedResult = terminalResultFor(finished, generated); + QVERIFY(generatedResult.has_value()); + QVERIFY2(generatedResult->state == pdfinteraction::CommandTerminalState::Completed, + qPrintable(generatedResult->typedError)); const pdf::PDFDocumentPointer sourceDocument = source.sessionForTest()->context().getDocumentPointer(); QString typedError; @@ -4267,6 +4397,13 @@ void EditorHostTest::bookmarkImportExportRoundTripsAndRefusesAStaleRevision() pdf::PDFAuthoringEditWriter::readOutline(sourceDocument.data(), typedError); QVERIFY2(sourceRecords.has_value(), qPrintable(typedError)); QCOMPARE(sourceRecords->size(), 3); + // The fixture has no outline of its own, so an auto-generate regression + // that replaced the outline would still produce three records. The + // operator's bookmark must be one of them. + pdf::PDFBookmarkRecord customRecord; + customRecord.title = QStringLiteral("Chapter one"); + customRecord.pageIndex = 0; + QVERIFY2(sourceRecords->contains(customRecord), "the custom bookmark at page 0 survives auto-generation"); // Export through the catalog writes the interchange file. const QString bookmarkPath = directory.filePath(QStringLiteral("bookmarks.json")); @@ -4532,7 +4669,10 @@ void EditorHostTest::formValueEditRoutesThroughTheCatalogIntoCoreAndProducesANew parameters.insert(QStringLiteral("revision"), afterText.toString()); QVERIFY(host.invokeCommand(QStringLiteral("actionSetFormFieldValue"), parameters) != 0); QCOMPARE(finished.count(), 2); - QCOMPARE(finished.last().at(0).value().state, pdfinteraction::CommandTerminalState::Completed); + { + const pdfinteraction::CommandResult checkboxResult = finished.last().at(0).value(); + QVERIFY2(checkboxResult.state == pdfinteraction::CommandTerminalState::Completed, qPrintable(checkboxResult.typedError)); + } bool isName = false; QCOMPARE(readFormFieldValue(session->context().getDocument(), QStringLiteral("LoopCheck"), &isName), QStringLiteral("Yes")); QVERIFY(isName); @@ -4716,7 +4856,10 @@ void EditorHostTest::attachmentEditsAndExtractionRouteThroughTheShell() parameters.insert(QStringLiteral("revision"), session->facade().currentRevision().toString()); QVERIFY(host.invokeCommand(QStringLiteral("actionAddAttachment"), parameters) != 0); QCOMPARE(finished.count(), 2); - QCOMPARE(finished.last().at(0).value().state, pdfinteraction::CommandTerminalState::Completed); + { + const pdfinteraction::CommandResult executableResult = finished.last().at(0).value(); + QVERIFY2(executableResult.state == pdfinteraction::CommandTerminalState::Completed, qPrintable(executableResult.typedError)); + } QVERIFY(host.extractAttachmentFileUrl(QUrl::fromLocalFile(extractionDirectory.path()), QStringLiteral("setup.exe"))); QCOMPARE(host.attachmentExtraction().value(QStringLiteral("status")).toString(), QStringLiteral("written")); QCOMPARE(host.attachmentExtraction().value(QStringLiteral("autoOpenRefused")).toBool(), true); @@ -4774,6 +4917,10 @@ void EditorHostTest::sendByEMailHandsOverOnlyThePublishedRevision() QVERIFY(host.isCommandEnabled(QStringLiteral("actionSend_by_E_Mail"))); QSignalSpy mailSpy(&host, &EditorHost::documentMailRequested); + MailUrlSink mailUrls; + QDesktopServices::setUrlHandler(QStringLiteral("mailto"), &mailUrls, "receive"); + auto clearMailHandler = qScopeGuard([] + { QDesktopServices::unsetUrlHandler(QStringLiteral("mailto")); }); QSignalSpy finished(&session->catalog(), &pdfinteraction::CommandCatalog::invocationFinished); const pdf::PDFRevisionIdentity published = session->facade().currentRevision(); @@ -4797,6 +4944,11 @@ void EditorHostTest::sendByEMailHandsOverOnlyThePublishedRevision() QCOMPARE(mailSpy.count(), 1); QCOMPARE(mailSpy.last().at(0).toString(), path); QCOMPARE(mailSpy.last().at(1).toString(), published.toString()); + QCOMPARE(mailUrls.received.count(), 1); + QCOMPARE(mailUrls.received.last().scheme(), QStringLiteral("mailto")); + const QUrlQuery sent(mailUrls.received.last()); + QCOMPARE(sent.queryItemValue(QStringLiteral("subject")), QStringLiteral("Published document: mail.pdf")); + QVERIFY(sent.queryItemValue(QStringLiteral("body")).contains(published.toString())); // An unsaved edit makes the file on disk a different revision than the one // on screen, so the hand-off is refused rather than mailed as current. @@ -4812,6 +4964,7 @@ void EditorHostTest::sendByEMailHandsOverOnlyThePublishedRevision() QCOMPARE(refused.state, pdfinteraction::CommandTerminalState::Failed); QCOMPARE(refused.typedError, QStringLiteral("mail/unsaved-changes")); QCOMPARE(mailSpy.count(), 1); + QCOMPARE(mailUrls.received.count(), 1); // Saving publishes the revision the shell was showing, and the hand-off // works again with the saved file and that same revision. diff --git a/UnitTests/tst_interactioncontrollertest.cpp b/UnitTests/tst_interactioncontrollertest.cpp index 869d13c86..f39713d99 100644 --- a/UnitTests/tst_interactioncontrollertest.cpp +++ b/UnitTests/tst_interactioncontrollertest.cpp @@ -35,10 +35,12 @@ #include #include +#include #include #include +#include "documentloader.h" #include "hittestsource.h" #include "inputintent.h" #include "interactioncontroller.h" @@ -50,8 +52,14 @@ #include "pagesurfacerenderer.h" #include "viewportcontroller.h" +#include "pdfdocumentbuilder.h" #include "pdfdocumentcontext.h" +#include "pdfdocumentimageextractor.h" +#include "pdfdocumenttextselectionsource.h" +#include "pdfdocumentwriter.h" #include "pdfevidencegraph.h" +#include "pdfjobscheduler.h" +#include "pdfsecurityhandler.h" namespace { @@ -81,6 +89,56 @@ pdf::PDFRevisionIdentity makeRevision(const QString& documentId, quint64 documen return revision; } +/// One 2x2 RGB image XObject painted on a page with no text at all: the +/// fixture for "a page with no text layer" and for image extraction. Mirrors +/// the DocumentViewSession suite's fixture so both suites extract the same +/// content. +void appendImageOnlyPage(pdf::PDFDocumentBuilder& builder) +{ + const pdf::PDFObjectReference page = builder.appendPage(QRectF(0, 0, 200, 200)); + + QByteArray imageData; + for (int pixel = 0; pixel < 4; ++pixel) + { + imageData.append('\xff'); + imageData.append('\0'); + imageData.append('\0'); + } + + pdf::PDFDictionary imageDictionary; + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("Type"), pdf::PDFObject::createName("XObject")); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("Subtype"), pdf::PDFObject::createName("Image")); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("Width"), pdf::PDFObject::createInteger(2)); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("Height"), pdf::PDFObject::createInteger(2)); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("ColorSpace"), pdf::PDFObject::createName("DeviceRGB")); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString("BitsPerComponent"), pdf::PDFObject::createInteger(8)); + imageDictionary.addEntry(pdf::PDFInplaceOrMemoryString(pdf::PDF_STREAM_DICT_LENGTH), pdf::PDFObject::createInteger(imageData.size())); + pdf::PDFStream imageStream(std::move(imageDictionary), std::move(imageData)); + const pdf::PDFObjectReference imageReference = + builder.addObject(pdf::PDFObject::createStream(std::make_shared(imageStream))); + + const QByteArray content = "q 40 0 0 40 80 80 cm /Im0 Do Q"; + + pdf::PDFDictionary contentDictionary; + contentDictionary.addEntry(pdf::PDFInplaceOrMemoryString(pdf::PDF_STREAM_DICT_LENGTH), + pdf::PDFObject::createInteger(content.size())); + pdf::PDFStream contentStream(std::move(contentDictionary), QByteArray(content)); + const pdf::PDFObjectReference contentReference = + builder.addObject(pdf::PDFObject::createStream(std::make_shared(contentStream))); + + pdf::PDFDictionary xObjects; + xObjects.addEntry(pdf::PDFInplaceOrMemoryString("Im0"), pdf::PDFObject::createReference(imageReference)); + pdf::PDFDictionary resources; + resources.addEntry(pdf::PDFInplaceOrMemoryString("XObject"), + pdf::PDFObject::createDictionary(std::make_shared(std::move(xObjects)))); + + pdf::PDFDictionary pageUpdate; + pageUpdate.addEntry(pdf::PDFInplaceOrMemoryString("Resources"), + pdf::PDFObject::createDictionary(std::make_shared(std::move(resources)))); + pageUpdate.addEntry(pdf::PDFInplaceOrMemoryString("Contents"), pdf::PDFObject::createReference(contentReference)); + builder.mergeTo(page, pdf::PDFObject::createDictionary(std::make_shared(std::move(pageUpdate)))); +} + /// Fixed-size pages and a transform simple enough to invert by hand, the same /// fake tst_viewportcontrollertest.cpp uses. A layout test does not need a PDF. class FakeGeometrySource final : public pdfinteraction::IPageGeometrySource @@ -366,6 +424,12 @@ private Q_SLOTS: void steppingByLineWalksTheTextLayer(); void tableSelectionUsesTheBlockUnderThePointer(); + // The Core-backed interaction sources, against real documents: extraction + // permission parity with PdfTool's fetch-images, and the selection source's + // layout cache. + void imageExtractionRefusesWhenCopyContentIsDenied(); + void selectionSourceRefusesAPageWithNoTextLayerOnEveryCall(); + private: void setViewport(); pdfinteraction::InteractionTarget objectTarget() const; @@ -1662,6 +1726,95 @@ void InteractionControllerTest::tableSelectionUsesTheBlockUnderThePointer() QVERIFY(qAbs(m_textSource->lastBlockPoint.y() - 30.0) <= 1.0); } +void InteractionControllerTest::imageExtractionRefusesWhenCopyContentIsDenied() +{ + // Only a reader-created handler reports the permissions the file declares, + // so the fixture is a real encrypted file read through the real loader. + QTemporaryDir directory; + QVERIFY(directory.isValid()); + const QString path = directory.filePath(QStringLiteral("copy-restricted.pdf")); + + { + pdf::PDFDocumentBuilder builder; + appendImageOnlyPage(builder); + + pdf::PDFSecurityHandlerFactory::SecuritySettings settings; + settings.algorithm = pdf::PDFSecurityHandlerFactory::AES_256; + settings.permissions = 0; + settings.userPassword = QStringLiteral("user"); + settings.ownerPassword = QStringLiteral("owner"); + settings.id = builder.build().getIdPart(0); + builder.setSecurityHandler(pdf::PDFSecurityHandlerFactory::createSecurityHandler(settings)); + + const pdf::PDFDocument document = builder.build(); + pdf::PDFDocumentWriter writer(nullptr); + QVERIFY(bool(writer.write(path, &document, false))); + } + + const auto loadDocument = [&path](const QString& password) -> pdf::PDFDocumentPointer + { + pdfinteraction::PDFReaderDocumentLoader loader( + [password](bool* ok, const pdf::PDFOperationControl*) + { + *ok = true; + return password; + }); + pdf::PDFJobContext jobContext(std::make_shared(), + pdf::PDFProcessingLimits::conservativeDefaults(), {}); + const pdfinteraction::DocumentLoadResult result = + loader.load(pdfinteraction::DocumentSource{ path }, jobContext); + return result.document; + }; + + // User authorization without CopyContent: the extraction is refused with + // the same sentence PdfTool's fetch-images refuses with. + const pdf::PDFDocumentPointer restricted = loadDocument(QStringLiteral("user")); + QVERIFY(restricted); + QVERIFY(!restricted->getStorage().getSecurityHandler()->isAllowed(pdf::PDFSecurityHandler::Permission::CopyContent)); + pdf::PDFDocumentContext restrictedContext(restricted); + pdfinteraction::PDFDocumentImageExtractor restrictedExtractor(&restrictedContext); + QString typedError; + const QList refused = restrictedExtractor.extractPage(0, &typedError); + QVERIFY(refused.isEmpty()); + QCOMPARE(typedError, QStringLiteral("Document doesn't allow to copy content.")); + + // Owner authorization satisfies the same permission: the page carries one + // image, so the refusal above is the permission and not a missing fixture. + const pdf::PDFDocumentPointer owner = loadDocument(QStringLiteral("owner")); + QVERIFY(owner); + pdf::PDFDocumentContext ownerContext(owner); + pdfinteraction::PDFDocumentImageExtractor ownerExtractor(&ownerContext); + typedError.clear(); + const QList extracted = ownerExtractor.extractPage(0, &typedError); + QCOMPARE(extracted.size(), 1); + QVERIFY(typedError.isEmpty()); +} + +void InteractionControllerTest::selectionSourceRefusesAPageWithNoTextLayerOnEveryCall() +{ + // The source against a real revision: the first call generates an empty + // layout for the page and caches it, and the second must report the same + // refusal instead of handing the cached empty layout back as available. + pdf::PDFDocumentBuilder builder; + appendImageOnlyPage(builder); + const pdf::PDFDocumentPointer document(new pdf::PDFDocument(builder.build())); + pdf::PDFDocumentContext context(document); + pdfinteraction::PDFDocumentTextSelectionSource source(&context); + + const QPointF start(10.0, 10.0); + const QPointF end(150.0, 150.0); + + const pdfinteraction::TextSelectionResult first = source.selectRange(0, start, end); + QVERIFY(!first.available); + QCOMPARE(first.unavailableReason, QStringLiteral("This page has no text layer.")); + QVERIFY(!source.hasTextLayer(0)); + + const pdfinteraction::TextSelectionResult second = source.selectRange(0, start, end); + QVERIFY(!second.available); + QCOMPARE(second.unavailableReason, QStringLiteral("This page has no text layer.")); + QVERIFY(!source.hasTextLayer(0)); +} + QTEST_GUILESS_MAIN(InteractionControllerTest) #include "tst_interactioncontrollertest.moc" diff --git a/UnitTests/tst_overlayframetest.cpp b/UnitTests/tst_overlayframetest.cpp index f9f599552..8a1599ba4 100644 --- a/UnitTests/tst_overlayframetest.cpp +++ b/UnitTests/tst_overlayframetest.cpp @@ -145,6 +145,7 @@ private Q_SLOTS: void textSelectionPaintsInTheSelectionBandWithoutATarget(); void annotationMarksPaintUnderFindingsWithTheirOwnSelection(); void annotationMarksAreBounded(); + void findingAndAnnotationCapsLeaveHeadroomForSteeringPrimitives(); private: std::unique_ptr m_geometry; @@ -261,6 +262,60 @@ void OverlayFrameTest::annotationMarksAreBounded() QCOMPARE(frame.primitives.at(1).layer, pdfinteraction::OverlayLayer::Findings); } +void OverlayFrameTest::findingAndAnnotationCapsLeaveHeadroomForSteeringPrimitives() +{ + // The defaults face the corpus-scale frame: findings and annotation marks + // beyond their caps, plus the hover and selection the user is steering. The + // two marker caps must leave headroom for the steering primitives, because + // annotations are emitted before them and the total bound is the last + // resort, not the intended one. + const pdfinteraction::OverlayBounds bounds = pdfinteraction::OverlayBounds::conservativeDefaults(); + + QList findings; + for (int index = 0; index < bounds.maxFindingPrimitives + 100; ++index) + { + findings.push_back(makeTarget(pdfinteraction::InteractionTargetKind::Finding, + QStringLiteral("finding-%1").arg(index), + QRectF(qreal(index % 90), qreal((index / 90) % 90), 1.0, 1.0))); + } + + QList annotations; + for (int index = 0; index < bounds.maxAnnotationPrimitives + 300; ++index) + { + annotations.push_back(makeTarget(pdfinteraction::InteractionTargetKind::None, + QStringLiteral("annotation-%1").arg(index), + QRectF(qreal(index % 90), qreal((index / 90) % 90), 1.0, 1.0))); + } + + m_overlays->setFindings(findings); + m_overlays->setAnnotations(annotations); + m_state.setHovered(makeTarget(pdfinteraction::InteractionTargetKind::Finding, + QStringLiteral("hovered"), QRectF(20.0, 20.0, 10.0, 10.0))); + m_state.setSelected(makeTarget(pdfinteraction::InteractionTargetKind::Finding, + QStringLiteral("selected"), QRectF(30.0, 30.0, 10.0, 10.0))); + + const pdfinteraction::OverlayFrame frame = m_overlays->build(m_state, makeToken()); + + QVERIFY(frame.primitives.size() <= bounds.maxPrimitives); + + // The steering primitives survive the saturated marker bands. + QVERIFY(findPrimitive(frame, QStringLiteral("hovered#hover"))); + QVERIFY(findPrimitive(frame, QStringLiteral("selected#selection"))); + + // The annotation band is capped at the constant, and the shortfall is + // reported rather than implied. + int annotationPrimitives = 0; + for (const pdfinteraction::OverlayPrimitive& primitive : frame.primitives) + { + if (primitive.layer == pdfinteraction::OverlayLayer::Annotations) + { + ++annotationPrimitives; + } + } + QCOMPARE(annotationPrimitives, bounds.maxAnnotationPrimitives); + QCOMPARE(frame.droppedPrimitives, 400); +} + void OverlayFrameTest::tiesBreakByRegistrationSequence() { m_overlays->setFindings({ makeTarget(pdfinteraction::InteractionTargetKind::Finding, QStringLiteral("second"), QRectF(20.0, 20.0, 20.0, 20.0)), diff --git a/UnitTests/tst_quickcanvastest.cpp b/UnitTests/tst_quickcanvastest.cpp index 8f07f1bfd..38021096f 100644 --- a/UnitTests/tst_quickcanvastest.cpp +++ b/UnitTests/tst_quickcanvastest.cpp @@ -281,6 +281,7 @@ private Q_SLOTS: void measureToolDragThroughTheCanvasReportsProductionUnits(); void textSelectionOnAPageWithNoTextLayerReportsThatState(); void canvasCompositionPlacesTilesInDevicePixels(); + void canvasCompositionDrawsPlacedTilesInViewportUnitsOnHiDpi(); void admittedTilesReachTheSceneGraph(); void revisionReplacementLeavesNoStaleTile(); @@ -900,6 +901,37 @@ void QuickCanvasTest::canvasCompositionPlacesTilesInDevicePixels() QVERIFY(pdfinteraction::composeCanvasImage(snapshot, QRect(100, 100, 40, 20), 1.0).isNull()); } +void QuickCanvasTest::canvasCompositionDrawsPlacedTilesInViewportUnitsOnHiDpi() +{ + // At a device pixel ratio other than 1 the painter's coordinate space is + // still viewport units: the placed rectangle is drawn unchanged and the + // composed image's own ratio maps it to device pixels. Multiplying the + // placed rect by the ratio again would move and grow the tile. + pdfinteraction::CanvasSnapshot snapshot; + snapshot.token = pdfinteraction::RevisionFencedToken(); + + QImage red(10, 10, QImage::Format_ARGB32_Premultiplied); + red.fill(Qt::red); + + pdfinteraction::CanvasTile tile; + tile.key.pageIndex = 0; + tile.pixels = pdfinteraction::makeSurfaceBuffer(red); + tile.placedRect = QRect(10, 0, 10, 10); + snapshot.tiles.push_back(tile); + + const QImage composed = pdfinteraction::composeCanvasImage(snapshot, QRect(0, 0, 40, 20), 2.0); + QVERIFY(!composed.isNull()); + QCOMPARE(composed.size(), QSize(80, 40)); + + // Device pixels inside placedRect * 2 carry the tile colour. + QCOMPARE(composed.pixelColor(30, 10), QColor(Qt::red)); + + // Outside it the canvas white shows: at the placed rect's viewport + // coordinates when read as device pixels, and beyond its device rect. + QCOMPARE(composed.pixelColor(10, 10), QColor(Qt::white)); + QCOMPARE(composed.pixelColor(50, 10), QColor(Qt::white)); +} + // --------------------------------------------------------------------------- // P4-S6: scene-graph lifecycle and canvas parity. // diff --git a/WixInstaller/createInstaller.ps1.in b/WixInstaller/createInstaller.ps1.in index e446cfd0f..85d00da62 100644 --- a/WixInstaller/createInstaller.ps1.in +++ b/WixInstaller/createInstaller.ps1.in @@ -1,2 +1,2 @@ -candle -d"DevEnvDir=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\\" -d"SolutionDir=${LOOP_WIX_DIR}" -d"SolutionExt=.sln" -d"SolutionFileName=LOUPE.sln" -d"SolutionName=LOUPE" -d"SolutionPath=LOUPE.sln" -d"Configuration=Release" -d"OutDir=bin\Release\" -d"Platform=x64" -d"ProjectDir=${LOOP_WIX_DIR}" -d"ProjectExt=.wixproj" -d"ProjectFileName=LOUPE.wixproj" -d"ProjectName=LOUPE" -d"ProjectPath=LOUPE.wixproj" -d"TargetDir=bin\Release\" -d"TargetExt=.msi" -d"TargetFileName=LOUPE.msi" -d"TargetName=LOUPE" -d"TargetPath=bin\Release\LOUPE.msi" -out obj\Release\ -arch x64 -ext "C:\Program Files (x86)\WiX Toolset v3.11\bin\\WixUIExtension.dll" Product.wxs -Light -out .\bin\Release\LOUPE.msi -pdbout .\bin\Release\LOUPE.wixpdb -cultures:null -ext "C:\Program Files (x86)\WiX Toolset v3.11\bin\\WixUIExtension.dll" -contentsfile obj\Release\LOUPE.wixproj.BindContentsFileListnull.txt -outputsfile obj\Release\LOUPE.wixproj.BindOutputsFileListnull.txt -builtoutputsfile obj\Release\LOUPE.wixproj.BindBuiltOutputsFileListnull.txt -wixprojectfile .\LOUPE.wixproj obj\Release\Product.wixobj +candle -d"DevEnvDir=C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\\" -d"SolutionDir=${LOOP_WIX_DIR}" -d"SolutionExt=.sln" -d"SolutionFileName=LOOP.sln" -d"SolutionName=LOOP" -d"SolutionPath=LOOP.sln" -d"Configuration=Release" -d"OutDir=bin\Release\" -d"Platform=x64" -d"ProjectDir=${LOOP_WIX_DIR}" -d"ProjectExt=.wixproj" -d"ProjectFileName=LOOP.wixproj" -d"ProjectName=LOOP" -d"ProjectPath=LOOP.wixproj" -d"TargetDir=bin\Release\" -d"TargetExt=.msi" -d"TargetFileName=LOOP.msi" -d"TargetName=LOOP" -d"TargetPath=bin\Release\LOOP.msi" -out obj\Release\ -arch x64 -ext "C:\Program Files (x86)\WiX Toolset v3.11\bin\\WixUIExtension.dll" Product.wxs +Light -out .\bin\Release\LOOP.msi -pdbout .\bin\Release\LOOP.wixpdb -cultures:null -ext "C:\Program Files (x86)\WiX Toolset v3.11\bin\\WixUIExtension.dll" -contentsfile obj\Release\LOOP.wixproj.BindContentsFileListnull.txt -outputsfile obj\Release\LOOP.wixproj.BindOutputsFileListnull.txt -builtoutputsfile obj\Release\LOOP.wixproj.BindBuiltOutputsFileListnull.txt -wixprojectfile .\LOOP.wixproj obj\Release\Product.wixobj diff --git a/changes/fix-dev-review-272.evidence.yaml b/changes/fix-dev-review-272.evidence.yaml new file mode 100644 index 000000000..3cbe8663d --- /dev/null +++ b/changes/fix-dev-review-272.evidence.yaml @@ -0,0 +1,37 @@ +format_version: 1 +kind: evidence +claims: + - id: core-mutation-and-authoring-fixes + evidence: + - unit:agent-policy:core + - architecture:docs/generated/architecture-catalog.json + - security:scripts/ci/check_source_integrity.py + - differential:UnitTestsConversionOracle + - id: interaction-gateway-fixes + evidence: + - unit:agent-policy:interaction + - id: quick-shell-fixes + evidence: + - unit:agent-policy:quick + - id: build-policy-ctest-wiring + evidence: + - unit:agent-policy:build_policy + - id: gates-and-generated-registers + evidence: + - architecture:scripts/generate-architecture-catalogs.py + - unit:scripts/ci/test_check_governance_registers.py + - unit:scripts/ci/test_check_loop_identity.py + - unit:scripts/github/test_sync_milestones.py + - architecture:scripts/ci/check_governance_registers.py + - architecture:scripts/ci/check_loop_identity.py + - unit:scripts/agent/test_architecture_contracts.py + - architecture:scripts/agent/check-architecture.py + - architecture:scripts/agent/generate-adapters.py + - id: packaging-lanes + evidence: + - packaging:linux-build + - packaging:windows-build + - security:codeql +unresolved: + - core:scripts/ci/check_independent_validation_gate.py + - Linux and Windows packaging lanes (linux-build, windows-build) and codeql run in CI on the pull request; this worktree proves the focused suites, the whole-tree checkers and the change gate locally. diff --git a/changes/fix-dev-review-272.md b/changes/fix-dev-review-272.md new file mode 100644 index 000000000..8757dd5d8 --- /dev/null +++ b/changes/fix-dev-review-272.md @@ -0,0 +1,4 @@ +Category: fixed +Audience: operators and developers of the Quick shell; release owners +Breaking-Change: no +Summary: Adversarial review of the dev train promoted by PR #272 (five subsystem reviewers, findings verified against the source) fixed in this PR with regression tests. Core: named-destination outline items no longer resolve to page 0 (toggle no longer deletes them, export refuses them with its typed code, auto-generate no longer skips page 1); the XMP merge now declares the namespaces it writes, replaces a property where the packet actually carries it (including the attribute form) and refuses an unmergeable packet instead of duplicating it; attachment add/remove refuses an entry stored as a direct object with the new additive `attachment/specification-not-addressable` code instead of silently unlisting it; form value edits propagate to same-named fields like the canonical path and prefer a widget-bearing field; the outline rewrite guard refuses a GoTo whose destination the writer would null; `PDFRevisionIdentity::fromString` rejects a corrupted hash. Interaction: the HiDPI screenshot composes tiles at their placed rect (no double device-pixel-ratio), with a ratio-2 test; image extraction and copy text refuse a copy-restricted document like `PdfTool` does, at the library and at the host; the no-text-layer refusal holds on every call, not just the first; a refused signature-check completion releases the running state instead of wedging verification; the overlay bounds reserve documented headroom for the steering primitives; a dead supersede branch is removed; the 18-id annotation command table is pinned literally by its tests. Editor: Send by E-Mail opens the desktop mail client from the host (refused, not reported as handed off, when the client does not take the message) and Bookmark Import/Export get their missing Quick shell receivers (file dialogs) instead of reporting a hand-off nobody performs; the soft-proof state label binds the property it meant; the ink-coverage readout publishes only for a succeeded job and does not outlive its revision; Copy and image extraction join the copy-permission gate the export command already used; the annotations tab button matches its siblings' wrapping; the reported relative-include nit is dismissed with build evidence (the plain include breaks `UnitTestsLoopStateVisual`, which compiles the file directly), recorded in the review report. Gates: `check_quick_localization.py` and the MenuModel label agree, and the checker and the help-strings staleness check are wired into CI and the whole-tree guards (the pseudo-locale generator has no check mode, so it stays a manual build-time tool); the handler-coverage ratchet records new gaps only through an explicit opt-in; the identity checker scans packaging templates and the leftover pre-rebrand packaging template is corrected; the action disposition vocabulary is validated on the Linux lane too; ADR-014's counts match the registers. Deferred with the review report: history re-verification on the interactive thread (needs the worker pattern) and a self-referential accessibility test. See `docs/reviews/2026-10-11-dev-review-272.md` for the labelled finding set, the dismissed items and the verified-clean list. diff --git a/docs/LOOP_SHELL_CONTRACT.md b/docs/LOOP_SHELL_CONTRACT.md index 13208907b..bf73f5dab 100644 --- a/docs/LOOP_SHELL_CONTRACT.md +++ b/docs/LOOP_SHELL_CONTRACT.md @@ -249,9 +249,10 @@ python scripts/verify-command-catalog.py ``` It also enforces the handler-coverage ratchet: every KEEP or ABSORB action is -`implemented` or listed in `docs/shell-handler-coverage.json`, the recorded gaps -have not moved in either direction, and the first run of the check records the -current gap set (`--record-ratchet` rewrites the baseline). +`implemented` or listed in `docs/shell-handler-coverage.json`, and the recorded +gaps have not moved in either direction. Recording rewrites the baseline +(`--record-ratchet`) but refuses to add ids unless `--accept-new-gaps ` +records a deliberate new gap. `UnitTestsDocumentFacade` covers the runtime side — catalog loading, invocation and cancellation, and the document lifecycle — with no `QWidget` and no QML diff --git a/docs/adr/adr-014-direct-revision-authoring-edits.md b/docs/adr/adr-014-direct-revision-authoring-edits.md index 5151ed056..1c819a978 100644 --- a/docs/adr/adr-014-direct-revision-authoring-edits.md +++ b/docs/adr/adr-014-direct-revision-authoring-edits.md @@ -38,14 +38,15 @@ round trip per edit would make the Fix workspace the authoring surface. What exists to build on: -- **Command surface.** `docs/loop-shell-actions.json` declares 109 actions - (`expected_action_count: 109`). 34 of the commands these three issues name are - already declared but unbound — 16 bookmark/link ids (#233), 18 annotation ids - (#234), and `actionSend_by_E_Mail` (#242) — and they are recorded in the - handler-coverage ratchet (`docs/shell-handler-coverage.json`) that PR 1 - landed. The form, attachment, and metadata families have **no command ids at - all**; only `actionProperties` and `actionSend_by_E_Mail` exist in those - neighbourhoods. #242's command surface therefore has to be created, not just +- **Command surface.** `docs/loop-shell-actions.json` declares 113 actions + (`expected_action_count: 113`). 35 of the commands these three issues name + were already declared but unbound — 16 bookmark/link ids (#233), 18 annotation + ids (#234), and `actionSend_by_E_Mail` (#242) — and the handler-coverage + ratchet (`docs/shell-handler-coverage.json`) that PR 1 landed recorded them + until this PR bound them; the register now lists the still-unbound KEEP or + ABSORB ids. The form, attachment, and metadata families had **no command ids + at all**; only `actionProperties` and `actionSend_by_E_Mail` existed in those + neighbourhoods. #242's command surface therefore had to be created, not just bound. - **Core writers.** `LoopLibCore/sources/pdfannotation.{h,cpp}` carries the annotation type model (`AnnotationType`, `PDFAnnotationBorder`, diff --git a/docs/reviews/2026-10-11-dev-review-272.md b/docs/reviews/2026-10-11-dev-review-272.md new file mode 100644 index 000000000..943c1d1f2 --- /dev/null +++ b/docs/reviews/2026-10-11-dev-review-272.md @@ -0,0 +1,119 @@ +# Adversarial review of the dev train promoted by PR #272 + +Date: 2026-10-11 +Scope: `origin/unstable...origin/dev` (15fed584), 60 commits / 43 non-merge, 194 files +Method: five independent adversarial reviewers, one per subsystem slice (Core PDF mutation, +Interaction gateway, Editor/UI, gates and generated registers, tests), same rubric; findings +verified against the source before classification; the fixes land in the accompanying PR against +`dev`. + +Model diversity was not available (all reviewers ran on the session model); the signal comes from +the five disjoint entry points, so agreements below mean the same defect was reachable from +different slices. + +## Status of this report + +Findings are labelled with the disposition of the accompanying fix PR: + +- **fixed**: corrected in the review-fix PR, with a regression test where one is constructible. +- **deferred**: real, not fixed here; listed as follow-up work. +- **dismissed**: reclassified or rejected on evidence, with the reason. + +## Findings + +### Core (LoopLibCore) + +| # | Sev | Finding | Disposition | +|---|-----|---------|-------------| +| 1 | critical | Named-destination outline items (hyperref/LaTeX shape) resolve to page 0, so toggle deletes them, export rewrites them and auto-generate skips page 1 | fixed: `resolveDestinationPage` refuses named or unparsed destinations; callers refuse with their typed code | +| 2 | critical | XMP merge can emit undeclared namespace prefixes, duplicate a property carried in another `rdf:Description`, and never replace attribute-form properties, while reporting success | fixed: presence pre-scan, attribute substitution, namespace declaration on replaced and appended properties, refusal on ambiguous duplicates | +| 3 | warning | Attachment add/remove silently unlists file specifications stored as direct objects | fixed: refuses with `attachment/specification-not-addressable` | +| 4 | warning | Form value edit updates only the first same-named field; the canonical path propagates to all | fixed: propagation to same-named fields, widget-bearing field preferred | +| 5 | warning | Outline rewrite guard accepts GoTo items the writer nulls (`/Dest null`, state loss) | fixed: guard requires a valid destination | +| 6 | nit | `PDFRevisionIdentity::fromString` accepts a corrupted hash as a valid identity | fixed: hash part validated | + +### Interaction (LoopLibInteraction) + +| # | Sev | Finding | Disposition | +|---|-----|---------|-------------| +| 7 | critical | HiDPI screenshot composes every tile at ratio-times its placed rect (double-applied device pixel ratio) | fixed: placed rect drawn unchanged; ratio-2 composition test added | +| 8 | critical | Image extraction ignores the document's CopyContent permission while PdfTool refuses the same operation | fixed: library refusal mirroring `pdf-tool` plus the host availability gate | +| 9 | warning | Copy text ignores the CopyContent permission | fixed: host refusal plus availability gate | +| 10 | warning | The no-text-layer refusal is one-shot: the empty layout is cached before the check, later calls report an available empty selection | fixed: cached path re-checks | +| 11 | warning | A refused signature-check completion wedges `m_signatureCheckRunning`, leaving the facts "not-signed" for a signed document and every later check refused | fixed: the refused current check releases the running state | +| 12 | warning | Annotation cap plus finding cap equal the frame budget, so annotations can starve the steering primitives | fixed: documented headroom reserve | +| 13 | nit | Unreachable supersede branch in the document-edit family | fixed: dead branch removed | +| 14 | warning | Annotation-family tests are driven by the table they should pin; a dropped table row ships a dead "implemented" command invisibly | fixed: the 18-id set is pinned literally | + +### Editor and Quick shell + +| # | Sev | Finding | Disposition | +|---|-----|---------|-------------| +| 15 | warning | Signature verdict is not invalidated when an in-place edit advances the revision | dismissed: the facts describe the source bytes by documented design; the operator-facing defect was the wedge (finding 11), which is fixed | +| 16 | warning | Send by E-Mail announces a mail hand-off that no receiver performs | fixed: the shell receiver opens the mail client with the published file and revision | +| 17 | warning | Bookmark import/export finish Completed while doing nothing (their file-dialog half has no receiver) | fixed: shell receivers answer the host invokables | +| 18 | warning | Soft-proof state label bound to a property as a function call, so it never renders | fixed: property binding | +| 19 | warning | Ink-coverage readout is not fenced to the job outcome or the revision | fixed: only `Succeeded` publishes; the readout does not survive a revision change | +| 20 | warning | Revision-change path re-verifies the whole history and runs N+1 queries on the interactive thread | deferred: needs the worker pattern, tracked below | +| 21 | nit | Annotations tab button lacks the wrapping content item of its three siblings | fixed | +| 22 | nit | LoopLibQuick reaches into LoopLibInteraction with a `../../` include | dismissed: the relative include is load-bearing. `UnitTestsLoopStateVisual` compiles `loopstatevisual.cpp` directly with `LoopLibQuick/sources` on its include path but not `LoopLibInteraction/sources`, so the plain include fails to compile (reproduced by the build); LoopLibInteraction's `INTERFACE` include dirs do not reach a target that only compiles the file | + +### Gates, scripts, generated registers + +| # | Sev | Finding | Disposition | +|---|-----|---------|-------------| +| 23 | critical | `check_quick_localization.py` is red on its own dev tip (`actionExportDocument` label form) | fixed: the label and the checker agree; the branch has coverage | +| 24 | warning | The `#126` checker and the help-strings staleness check run in no gate | fixed: wired into `ci.yml` and the whole-tree guards | +| 25 | warning | The handler-coverage ratchet can be grown by its own tool; "may only shrink" is enforced nowhere | fixed: recording new gaps is an explicit opt-in; tests cover both directions | +| 26 | warning | The identity checker cannot see packaging templates; a template still naming the product by its pre-rebrand name sat in the tree | fixed: templates scanned, tokens corrected | +| 27 | warning | The action disposition vocabulary is validated only by the PowerShell lane | fixed: validated in the Python checker and its twin | +| 28 | nit | ADR-014 pins counts that disagree with the shipped registers | fixed: corrected against the registers | + +### Tests + +| # | Sev | Finding | Disposition | +|---|-----|---------|-------------| +| 29 | warning | Bookmark import/export test cannot fail if the bookmark command no-ops | fixed: the custom record is asserted and the invocation's result is read | +| 30 | warning | The claimed screenshot overwrite refusal has no test | fixed if constructible, else recorded in the PR | +| 31 | warning | The only test of the ten shipping catalogs is always skipped; nothing wires the env var it needs | fixed: the directory is wired via ctest, assertion strengthened only where the catalogs carry it | +| 32 | nit | Accessible-readout test is self-referential | deferred: needs the host composition extracted or driven through a host; tracked below | + +## Verified clean (checked and held up) + +Counts and digests agree across their pins (113 descriptors / 94 implemented / 19 declared / 13 +unbound ratchet in the tests, the catalog JSON, the checker and the register). The QML product and +smoke trees are byte-identical at review time. The soft-proof and display-colour filters stay +presentation-only (no surface key, no render request) and colour identity joins the surface key. +The governed publish path's plan-digest acknowledgment gate is sound and is checked worker-side +against the source document. Every direct-edit writer sets a typed refusal instead of half-writing; +the annotation command-id table matches `docs/loop-shell-actions.json`. The `#231` ratchet ids are +pre-existing; the implementation/handler sets match with no orphans. `check_loop_identity.py`, +`verify-command-catalog.py`, `check_governance_registers.py` and the milestone sync pass on this +tree. + +## Deferred and dismissed detail + +- **Deferred: history re-verification on the interactive thread (finding 20).** The revision-change + path calls `PDFOperationHistoryStore::verify()` (a full-chain re-hash), reads the event table + twice and issues one execution query per event, synchronously, on the thread the host registers + as the blocking-thread-guarded interactive thread. Correct fix is to move the read into the + existing scheduler-worker pattern; that is a restructure, not a review fix. +- **Deferred: the accessible-readout test (finding 32).** The test proves the setter, not the host + composition. Covering the composition needs the summary built through the host (or extracted + into a pure function); noted rather than widened here. +- **Dismissed: signature badge staleness (finding 15).** `documentfacade.h` documents that the + facts describe the source bytes the document was loaded from and that an in-session revision + change does not retract them; the badge wording matches that contract. The operator-visible + defect was the wedge (finding 11) and the missing re-submission path, which the fix addresses at + the rejection site. +- **Noted, not filed**: a duplicate `setCurrentRevision` call in the host, a late ink-coverage + completion after document close can repopulate the readout (the outcome map is not cancelled on + close), a retained display-filtered tile copy, `setTextSelection`'s missing `isCurrent()` guard + (no current caller can pass a stale-but-valid token), duplicated `usableBounds` helpers in two + files, the outline writer drops a GoTo's `/SD` (structure destination) on any rewrite (the same + class as finding 5, outside its scope), and the translations catalogs predate the #123 + print/export strings, so the new MenuModel switch string extracts on the next `LOOP_lupdate` + refresh. +- **Behaviour note**: `UnitTestsLoopStateVisual` now fails when the compiled catalogs are absent + instead of skipping; the agent-fast lane builds `LOOP_lrelease` (LoopEditor depends on it), so + the catalogs exist before the suite runs there. diff --git a/scripts/agent/check-change.py b/scripts/agent/check-change.py index ddf6a46a9..5dccc5ecd 100644 --- a/scripts/agent/check-change.py +++ b/scripts/agent/check-change.py @@ -423,6 +423,16 @@ def main() -> int: add_result(evidence, "command_catalog_cases", [python, "-m", "unittest", "scripts.ci.test_command_catalog", "-q"], ROOT, args.dry_run) + # The #126 Quick localization guard: the checker, its unit tests and the + # help-strings staleness check audit the whole tree, so they run on every + # change like source_integrity does. + add_result(evidence, "quick_localization", + [python, "scripts/ci/check_quick_localization.py"], ROOT, args.dry_run) + add_result(evidence, "quick_localization_cases", + [python, "-m", "unittest", "scripts.ci.test_check_quick_localization", "-q"], + ROOT, args.dry_run) + add_result(evidence, "quick_help_strings", + [python, "scripts/update-quick-help-strings.py", "--check"], ROOT, args.dry_run) # Whole-tree contract guards: they audit invariants of the tree, not just the # diff, so they run on every change like source_integrity does. add_result( diff --git a/scripts/ci/check_loop_identity.py b/scripts/ci/check_loop_identity.py index 37e0e65b6..3e146d2e8 100644 --- a/scripts/ci/check_loop_identity.py +++ b/scripts/ci/check_loop_identity.py @@ -22,6 +22,7 @@ ".cpp", ".h", ".hpp", + ".in", ".json", ".md", ".ps1", @@ -33,7 +34,9 @@ ".yml", ".yaml", ".qml", + ".qrc", ".desktop", + ".ui", ".xml", } diff --git a/scripts/ci/check_quick_localization.py b/scripts/ci/check_quick_localization.py index be4d19c40..a235bc775 100644 --- a/scripts/ci/check_quick_localization.py +++ b/scripts/ci/check_quick_localization.py @@ -40,6 +40,16 @@ def catalog_errors(root, language, glossary_sources): return errors +def menu_label_ids(menu_text): + """Command ids whose literal label a switch arm makes extractable. + + The switch arm is the accepted form: the guard form used for the two + print/export overrides is not counted, so an id that only appears there + fails the coverage comparison below until it gets a switch arm. + """ + return set(re.findall(r'case "([^"]+)": return qsTr\(', menu_text)) + + def check(root=ROOT, project_json=None): header = (root / "LoopLibInteraction/sources/shellglossary.h").read_text(encoding="utf-8") glossary_sources = set(re.findall(r'QT_TRANSLATE_NOOP\("LoopGlossary", "([^"]+)"\)', header)) @@ -49,7 +59,7 @@ def check(root=ROOT, project_json=None): actions = json.loads((root / "docs/loop-shell-actions.json").read_text(encoding="utf-8"))["actions"] expected = {action["id"] for action in actions if action["disposition"] not in ("HIDE", "STOP-SHIPPING")} menu = (root / "LoopEditor/qml/MenuModel.qml").read_text(encoding="utf-8") - actual = set(re.findall(r'case "([^"]+)": return qsTr\(', menu)) + actual = menu_label_ids(menu) if actual != expected: errors.append("Extractable menu labels do not cover the manifest commands") if project_json is not None: diff --git a/scripts/ci/test_check_loop_identity.py b/scripts/ci/test_check_loop_identity.py index 1f9f45a19..f0ff31a3a 100644 --- a/scripts/ci/test_check_loop_identity.py +++ b/scripts/ci/test_check_loop_identity.py @@ -3,6 +3,7 @@ from __future__ import annotations +import tempfile import unittest import sys from pathlib import Path @@ -50,6 +51,23 @@ def test_unallowlisted_legacy_token_is_reported(self) -> None: ) self.assertEqual(findings, ["active.txt:1: legacy product token"]) + def test_template_suffix_is_scanned(self) -> None: + legacy_token = "lo" + "upe" + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "createInstaller.ps1.in" + path.write_text( + f"-d\"TargetName={legacy_token}\" -out .\\bin\\Release\\{legacy_token}.msi\n", + encoding="utf-8", + ) + content = check_loop_identity.text_for(path) + self.assertIsNotNone(content, "'.in' packaging templates must be scanned") + self.assertEqual( + check_loop_identity.legacy_token_findings( + {"WixInstaller/createInstaller.ps1.in": content} + ), + ["WixInstaller/createInstaller.ps1.in:1: legacy product token"], + ) + if __name__ == "__main__": raise SystemExit(unittest.main()) diff --git a/scripts/ci/test_check_quick_localization.py b/scripts/ci/test_check_quick_localization.py index 9cd29fe4a..2ade01ed1 100644 --- a/scripts/ci/test_check_quick_localization.py +++ b/scripts/ci/test_check_quick_localization.py @@ -1,9 +1,10 @@ +import json import tempfile import unittest from pathlib import Path import xml.etree.ElementTree as ET -from scripts.ci.check_quick_localization import VERDICTS, catalog_errors +from scripts.ci.check_quick_localization import VERDICTS, catalog_errors, menu_label_ids class CatalogLocalizationTests(unittest.TestCase): @@ -53,5 +54,34 @@ def test_same_spelling_and_missing_nonverdict_translation_are_valid(self): self.assertEqual(self.errors(VERDICTS | {"Image resolution"}), []) +class MenuLabelCoverageTests(unittest.TestCase): + def test_switch_arms_are_extracted(self): + menu = ( + ' case "actionOpen": return qsTr("Open")\n' + ' case "actionClose": return qsTr("Close")\n' + ) + self.assertEqual(menu_label_ids(menu), {"actionOpen", "actionClose"}) + + def test_guard_only_label_is_not_coverage(self): + menu = ( + ' if (entry && entry.id === "actionExportDocument")' + ' return qsTr("Export document…")\n' + ) + self.assertEqual(menu_label_ids(menu), set()) + + def test_repository_menu_arms_cover_every_shipped_command(self): + root = Path(__file__).resolve().parents[2] + actions = json.loads( + (root / "docs/loop-shell-actions.json").read_text(encoding="utf-8") + )["actions"] + expected = { + action["id"] + for action in actions + if action["disposition"] not in ("HIDE", "STOP-SHIPPING") + } + menu = (root / "LoopEditor/qml/MenuModel.qml").read_text(encoding="utf-8") + self.assertEqual(menu_label_ids(menu), expected) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/ci/test_command_catalog.py b/scripts/ci/test_command_catalog.py index 13c6fcf62..aadc1dcd8 100644 --- a/scripts/ci/test_command_catalog.py +++ b/scripts/ci/test_command_catalog.py @@ -203,6 +203,27 @@ def test_unknown_capability_is_rejected(self): errors = self.mutate("actionAbout", capability="document.everything") self.assertTrue(any("capability" in error for error in errors), errors) + def test_disposition_typo_is_rejected(self): + policy = copy.deepcopy(POLICY) + policy["actions"][2]["disposition"] = "Absorb" + errors = CatalogFixture(self, policy=policy).run() + self.assertTrue( + any( + "actionBleedFixup" in error and "disposition" in error + for error in errors + ), + errors, + ) + + def test_target_outside_the_vocabulary_is_rejected(self): + policy = copy.deepcopy(POLICY) + policy["actions"][2]["target"] = "Production Preview" + errors = CatalogFixture(self, policy=policy).run() + self.assertTrue( + any("actionBleedFixup" in error and "target" in error for error in errors), + errors, + ) + def test_implemented_command_may_not_stay_unclassified(self): errors = self.mutate("actionOpen", capability="unclassified") self.assertTrue(any("real capability" in error for error in errors), errors) @@ -264,11 +285,42 @@ def test_baseline_entry_for_advanced_command_is_rejected(self): errors, ) - def test_recording_writes_the_current_gap_set(self): + def test_recording_growth_is_refused_by_default(self): fixture = CatalogFixture(self, ratchet_unbound=[]) self.assertTrue(fixture.run(), "an empty baseline must fail while a gap exists") - unbound = verifier.record_ratchet(fixture.policy_path, fixture.ratchet_path) + with self.assertRaises(verifier.ContractError) as caught: + verifier.record_ratchet(fixture.policy_path, fixture.ratchet_path) + self.assertIn("actionBleedFixup", str(caught.exception)) + recorded = json.loads(fixture.ratchet_path.read_text(encoding="utf-8")) + self.assertEqual(recorded["unbound"], [], "a refusal must not half-write the baseline") + + def test_recording_growth_with_the_opt_in_records_the_reason(self): + fixture = CatalogFixture(self, ratchet_unbound=[]) + unbound = verifier.record_ratchet( + fixture.policy_path, fixture.ratchet_path, accept_new_gaps="lands with #999" + ) self.assertEqual(unbound, ["actionBleedFixup"]) + recorded = json.loads(fixture.ratchet_path.read_text(encoding="utf-8")) + self.assertEqual( + recorded["accepted_gaps"], {"actionBleedFixup": "lands with #999"} + ) + self.assertEqual(fixture.run(), []) + + def test_recording_the_shrink_path_drops_bound_ids(self): + fixture = CatalogFixture(self, ratchet_unbound=["actionBleedFixup"]) + verifier.IMPLEMENTED_COMMANDS = frozenset( + {"actionOpen", "actionClose", "actionBleedFixup"} + ) + policy = copy.deepcopy(POLICY) + for action in policy["actions"]: + if action["id"] == "actionBleedFixup": + action["command"]["availability"] = "implemented" + action["command"]["capability"] = "none" + fixture.write(policy) + unbound = verifier.record_ratchet(fixture.policy_path, fixture.ratchet_path) + self.assertEqual(unbound, []) + recorded = json.loads(fixture.ratchet_path.read_text(encoding="utf-8")) + self.assertEqual(recorded["unbound"], []) self.assertEqual(fixture.run(), []) def test_unsorted_or_duplicated_baseline_is_reported(self): diff --git a/scripts/verify-command-catalog.py b/scripts/verify-command-catalog.py index d05b7110f..bf075b408 100644 --- a/scripts/verify-command-catalog.py +++ b/scripts/verify-command-catalog.py @@ -44,6 +44,15 @@ # so none of them owes the Quick shell a handler. RATCHET_DISPOSITIONS = frozenset({"KEEP", "ABSORB"}) +# The disposition/target vocabulary, mirrored from verify-loop-shell-contract.ps1 +# and docs/schemas/loop-shell-actions.schema.json. A typo'd disposition falls out +# of the ratchet filter above, so the dead menu entry it produces would never be +# counted as a gap. +DISPOSITIONS = frozenset({"KEEP", "ADVANCED", "ABSORB", "HIDE", "OPEN", "STOP-SHIPPING"}) +TARGETS = frozenset( + {"Document", "Preflight", "Production", "Inspect", "Fix", "Pages", "Compare", "Advanced"} +) + # Commands that have a handler in LoopLibInteraction (DocumentFacade for # document lifecycle, ViewportCommandBridge for page/zoom/rotate). A command # may only leave this set by being implemented somewhere else, never by being @@ -510,16 +519,45 @@ def check_handler_ratchet(policy: dict, baseline: list[str]) -> list[str]: def record_ratchet( - policy_path: Path = POLICY_PATH, ratchet_path: Path = RATCHET_PATH + policy_path: Path = POLICY_PATH, + ratchet_path: Path = RATCHET_PATH, + accept_new_gaps: str | None = None, ) -> list[str]: - """Rewrites the ratchet baseline from the current contract.""" + """Rewrites the ratchet baseline, refusing to add ids without a reason. + + The baseline is a shrink-only record: recording drops the ids that gained a + handler. Growing it is a deliberate act, so a new id needs the + accept_new_gaps reason, which is stored beside the ids it covers. + """ policy = load_policy(policy_path) unbound = unbound_actions(policy) payload: dict = {"schema_version": 1} if ratchet_path.is_file(): payload = json.loads(ratchet_path.read_text(encoding="utf-8")) + recorded = payload.get("unbound", []) + unbound_ids = set(unbound) + additions = sorted(unbound_ids - set(recorded)) + + accepted = { + action_id: reason + for action_id, reason in dict(payload.get("accepted_gaps", {})).items() + if action_id in unbound_ids + } + if additions: + if accept_new_gaps is None: + raise ContractError( + f"recording would add {', '.join(additions)} to the shrink-only " + f"ratchet {ratchet_path}; pass --accept-new-gaps to " + "record a deliberate new gap" + ) + accepted.update({action_id: accept_new_gaps for action_id in additions}) + payload["unbound"] = unbound + if accepted: + payload["accepted_gaps"] = accepted + else: + payload.pop("accepted_gaps", None) ratchet_path.write_text( json.dumps(payload, indent=2, ensure_ascii=False) + "\n", encoding="utf-8" ) @@ -545,6 +583,14 @@ def validate_catalog( if action_id in seen_ids: errors.append(f"{action_id}: duplicate action id") seen_ids.add(action_id) + disposition = action.get("disposition") + if disposition not in DISPOSITIONS: + errors.append( + f"{action_id}: disposition {disposition!r} is not a known disposition" + ) + target = action.get("target") + if target is not None and target not in TARGETS: + errors.append(f"{action_id}: target {target!r} is not a known target") if "command" not in action: errors.append(f"{action_id}: has no command descriptor") continue @@ -601,10 +647,22 @@ def main(argv: list[str] | None = None) -> int: action="store_true", help="rewrite docs/shell-handler-coverage.json from the current contract", ) + parser.add_argument( + "--accept-new-gaps", + metavar="REASON", + help="with --record-ratchet, record newly unbound ids with this reason", + ) args = parser.parse_args(argv) + if args.accept_new_gaps is not None and not args.record_ratchet: + parser.error("--accept-new-gaps requires --record-ratchet") + if args.record_ratchet: - unbound = record_ratchet() + try: + unbound = record_ratchet(accept_new_gaps=args.accept_new_gaps) + except ContractError as exc: + print(f"{RATCHET_PATH.name}: {exc}", file=sys.stderr) + return 1 print( f"Handler ratchet recorded: {len(unbound)} KEEP/ABSORB commands " f"await a handler in {RATCHET_PATH.name}." diff --git a/scripts/verify-loop-shell-contract.py b/scripts/verify-loop-shell-contract.py index 88b712c09..44b71bb21 100644 --- a/scripts/verify-loop-shell-contract.py +++ b/scripts/verify-loop-shell-contract.py @@ -108,6 +108,15 @@ def main() -> int: raise ContractError( f"Editor action count mismatch: expected_action_count={expected_count}, policy={len(actions)}" ) + for action in actions: + if action.get("disposition") not in VALID_DISPOSITIONS: + raise ContractError( + f"invalid editor action disposition for {action.get('id')}: {action.get('disposition')!r}" + ) + if action.get("target") not in VALID_TARGETS: + raise ContractError( + f"invalid editor action target for {action.get('id')}: {action.get('target')!r}" + ) plugin_surfaces = { surface["artifact"]: surface diff --git a/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml b/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml index 4fe4d2603..7a9fdf8ed 100644 --- a/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml +++ b/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml @@ -84,6 +84,15 @@ Item { TabButton { objectName: "annotationsTabButton" text: qsTr("Annotations") + contentItem: Text { + text: parent.text + font: parent.font + color: parent.palette.buttonText + wrapMode: Text.WordWrap + horizontalAlignment: Text.AlignHCenter + verticalAlignment: Text.AlignVCenter + Accessible.ignored: true + } Accessible.name: qsTr("Annotations tab") } } diff --git a/tools/ProductQuickAccessibilitySmoke/qml/Main.qml b/tools/ProductQuickAccessibilitySmoke/qml/Main.qml index 52c521220..b75420903 100644 --- a/tools/ProductQuickAccessibilitySmoke/qml/Main.qml +++ b/tools/ProductQuickAccessibilitySmoke/qml/Main.qml @@ -338,6 +338,35 @@ ApplicationWindow { onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() } + FileDialog { + id: bookmarkImportDialog + title: qsTr("Import Bookmarks") + nameFilters: [qsTr("Bookmark files (*.json)")] + property string revision: "" + onAccepted: { + if (host) { + host.importBookmarksFileUrl(selectedFile, revision) + } + if (host && host.focusRestoration) host.focusRestoration.restore() + } + onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() + } + + FileDialog { + id: bookmarkExportDialog + title: qsTr("Export Bookmarks") + fileMode: FileDialog.SaveFile + nameFilters: [qsTr("Bookmark files (*.json)")] + property string revision: "" + onAccepted: { + if (host) { + host.exportBookmarksFileUrl(selectedFile, revision) + } + if (host && host.focusRestoration) host.focusRestoration.restore() + } + onRejected: if (host && host.focusRestoration) host.focusRestoration.restore() + } + FileDialog { id: preflightReportDialog title: qsTr("Export Preflight Report") @@ -414,6 +443,16 @@ ApplicationWindow { function onImageExtractionRequested() { extractImagesDialog.open() } + function onBookmarkImportRequested() { + if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) + bookmarkImportDialog.revision = host.outputRevision() + bookmarkImportDialog.open() + } + function onBookmarkExportRequested() { + if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) + bookmarkExportDialog.revision = host.outputRevision() + bookmarkExportDialog.open() + } function onSettingsPanelRequested() { if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) settingsDialog.open() diff --git a/tools/ProductQuickAccessibilitySmoke/qml/MenuModel.qml b/tools/ProductQuickAccessibilitySmoke/qml/MenuModel.qml index 3ed992db4..7db6ef359 100644 --- a/tools/ProductQuickAccessibilitySmoke/qml/MenuModel.qml +++ b/tools/ProductQuickAccessibilitySmoke/qml/MenuModel.qml @@ -127,6 +127,7 @@ QtObject { case "actionDeselectText": return qsTr("Deselect Text") case "actionEditDocumentMetadata": return qsTr("Edit Document Metadata") case "actionEncryption": return qsTr("Encryption") + case "actionExportDocument": return qsTr("Export document…") case "actionExtractImage": return qsTr("Extract Image") case "actionFind": return qsTr("Find") case "actionFindNext": return qsTr("Find Next") diff --git a/tools/ProductQuickAccessibilitySmoke/qml/ProductionPreviewPane.qml b/tools/ProductQuickAccessibilitySmoke/qml/ProductionPreviewPane.qml index 9c9af0178..b2d51c291 100644 --- a/tools/ProductQuickAccessibilitySmoke/qml/ProductionPreviewPane.qml +++ b/tools/ProductQuickAccessibilitySmoke/qml/ProductionPreviewPane.qml @@ -148,7 +148,7 @@ Pane { ? "" : qsTr("Soft proof %1: %2") .arg(root.host.softProofStateName) - .arg(root.host.softProofSummary()) + .arg(root.host.softProofSummary) Accessible.name: qsTr("Soft proof state") Accessible.description: text }