From 9065606dfc5806f037d38c9194e8e94df723f68e Mon Sep 17 00:00:00 2001 From: mberrys <212453881+mberrys@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:16:01 -0700 Subject: [PATCH 1/6] fix(core): refuse outline, metadata and attachment shapes the writers cannot carry Adversarial review of the #272 train (docs/reviews/2026-10-11-dev-review-272.md), findings 1-6. A named or unparsed outline destination no longer resolves to page 0, so a toggle cannot delete a hyperref bookmark and export refuses it typed; the outline rewrite guard requires a valid GoTo destination the object factory can write; the XMP merge pre-scans element and attribute occurrences, substitutes attribute-form properties, declares the namespace on replaced and appended properties and refuses an ambiguous duplicate as xmp-unreadable; attachment add/remove refuses an inline specification with the new attachment/specification-not-addressable code instead of unlisting it; form values propagate to same-named fields as PDFFormManager::setFormFieldValue does, preferring a widget-bearing match; fromString validates the hash field (the empty run stays valid: that is what an in-memory document serializes) so a corrupted hash cannot decode. --- LoopLibCore/sources/pdfauthoringedit.cpp | 25 +- LoopLibCore/sources/pdfdirectedit.cpp | 287 +++++++++++-- LoopLibCore/sources/pdfdirectedit.h | 5 +- LoopLibCore/sources/pdfdocumentcontext.cpp | 33 +- UnitTests/tst_authoringedittest.cpp | 130 ++++++ UnitTests/tst_directedittest.cpp | 454 ++++++++++++++++++++- 6 files changed, 901 insertions(+), 33 deletions(-) diff --git a/LoopLibCore/sources/pdfauthoringedit.cpp b/LoopLibCore/sources/pdfauthoringedit.cpp index b63f561f..9124bb24 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 529d4b85..045c7213 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 de3bd2ea..4e4764a2 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 769d0dcc..d282d9a8 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/UnitTests/tst_authoringedittest.cpp b/UnitTests/tst_authoringedittest.cpp index afc53089..e78dfb88 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 806db1fd..7a374a3c 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) From ae87c50aeed97bd7270b11fda81a5af11a8b6463 Mon Sep 17 00:00:00 2001 From: mberrys <212453881+mberrys@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:16:02 -0700 Subject: [PATCH 2/6] fix(interaction): correct capture units, copy permissions, the text-layer cache and the signature release Findings 7-14. composeCanvasImage draws each tile at its placed rect: the composed image's own device pixel ratio already maps viewport units to device pixels, so the previous *ratio double-counted HiDPI; extractPage refuses a copy-restricted document with PdfTool's own sentence; the cached empty text layout can no longer turn a no-text-layer refusal into an available empty selection; a refused signature-check completion releases the one-check flag when it is still the current check (a superseded completion cannot release a newer one); the annotation cap leaves 3500 of 4000 frame slots so the steering primitives always fit; the unreachable supersede branch is gone; the 18-id annotation command table is pinned by a literal list. --- LoopLibInteraction/sources/canvassnapshot.cpp | 11 +- .../sources/documentfacadeedits.cpp | 4 - .../sources/documentfacadestate.cpp | 18 +++ LoopLibInteraction/sources/overlaybuilder.h | 7 +- .../sources/pdfdocumentimageextractor.cpp | 8 + .../pdfdocumenttextselectionsource.cpp | 7 + UnitTests/tst_annotationauthoringtest.cpp | 39 +++++ UnitTests/tst_documentfacadetest.cpp | 77 +++++++++ UnitTests/tst_interactioncontrollertest.cpp | 153 ++++++++++++++++++ UnitTests/tst_overlayframetest.cpp | 55 +++++++ UnitTests/tst_quickcanvastest.cpp | 32 ++++ 11 files changed, 400 insertions(+), 11 deletions(-) diff --git a/LoopLibInteraction/sources/canvassnapshot.cpp b/LoopLibInteraction/sources/canvassnapshot.cpp index 8caf1062..5e34e65c 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 ad9c9aa8..145273e0 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 cff43657..e9dc82f3 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 e37e5d7a..1f6e1f52 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 b884dff0..48b969a0 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 ab9b3c2e..5e9e0b9d 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/tst_annotationauthoringtest.cpp b/UnitTests/tst_annotationauthoringtest.cpp index dedf44e4..65a8f4a5 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_documentfacadetest.cpp b/UnitTests/tst_documentfacadetest.cpp index d8a646d0..76820355 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_interactioncontrollertest.cpp b/UnitTests/tst_interactioncontrollertest.cpp index 869d13c8..f39713d9 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 f9f59955..8a1599ba 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 8f07f1bf..38021096 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. // From dd74e28fa416c1c8e27834b394622f2f7f846ceb Mon Sep 17 00:00:00 2001 From: mberrys <212453881+mberrys@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:16:03 -0700 Subject: [PATCH 3/6] fix(quick): wire the missing shell receivers and fence the ink-coverage readout Findings 9, 16-19, 21. The Quick shell answers documentMailRequested with a composed mail-client hand-off and bookmarkImportRequested/bookmarkExportRequested with the file dialogs that answer the host invokables, so no command reports a hand-off nobody performs; the soft-proof state label binds softProofSummary as the property it is; 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 actionExportDocument already used, and copySelectedText refuses with the CLI sentence; the annotations tab button gets its siblings' wrapping content item in both QML copies. Tests: the bookmark round trip asserts the custom record and reads the invocation result; screenshot-overwrite refusal and the stale-readout fence are pinned. --- LoopEditor/editorhost.cpp | 50 +++++-- LoopEditor/qml/DocumentPane.qml | 9 ++ LoopEditor/qml/Main.qml | 50 +++++++ LoopEditor/qml/MenuModel.qml | 1 + LoopEditor/qml/ProductionPreviewPane.qml | 2 +- UnitTests/tst_editorhosttest.cpp | 134 +++++++++++++++++- .../qml/DocumentPane.qml | 9 ++ .../qml/Main.qml | 50 +++++++ .../qml/MenuModel.qml | 1 + .../qml/ProductionPreviewPane.qml | 2 +- 10 files changed, 289 insertions(+), 19 deletions(-) diff --git a/LoopEditor/editorhost.cpp b/LoopEditor/editorhost.cpp index e28f9ef1..fae3d003 100644 --- a/LoopEditor/editorhost.cpp +++ b/LoopEditor/editorhost.cpp @@ -666,6 +666,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 +2902,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 +4907,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) { @@ -5676,17 +5700,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/qml/DocumentPane.qml b/LoopEditor/qml/DocumentPane.qml index 4fe4d260..7a9fdf8e 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 52c52122..83076fc3 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,27 @@ 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 onDocumentMailRequested(path, revision) { + // The host announced the hand-off before this handler ran; opening + // the mail client is the shell's job. The published path and the + // revision it was published as travel in the message so the + // receiving side can tell which revision the file is. + const name = path.replace(/^.*[\\/]/, "") + const subject = qsTr("Published document: %1").arg(name) + const body = qsTr("The published document %1 is at revision %2.").arg(path).arg(revision) + Qt.openUrlExternally("mailto:?subject=" + encodeURIComponent(subject) + + "&body=" + encodeURIComponent(body)) + } 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 3ed992db..7db6ef35 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 9c9af017..b2d51c29 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/UnitTests/tst_editorhosttest.cpp b/UnitTests/tst_editorhosttest.cpp index a6912a54..ea99c81d 100644 --- a/UnitTests/tst_editorhosttest.cpp +++ b/UnitTests/tst_editorhosttest.cpp @@ -607,6 +607,7 @@ private slots: void softProofWithoutAUsableOutputIntentIsAVisibleState(); void platesListSeparationsAndCarryVisibilityWithoutTouchingTheDocument(); void inkCoverageReadoutAtAPointIsMeasuredOrExplicitlyUnavailable(); + void inkCoverageRefusesSupersededJobsAndDropsTheReadoutOnRevisionChange(); // Shell settings, render options and diagnostics (#239). void settingsRoundTripThroughTheOneStore(); @@ -617,6 +618,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 +3838,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 +4187,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 +4358,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 +4378,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 +4650,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 +4837,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); diff --git a/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml b/tools/ProductQuickAccessibilitySmoke/qml/DocumentPane.qml index 4fe4d260..7a9fdf8e 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 52c52122..83076fc3 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,27 @@ 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 onDocumentMailRequested(path, revision) { + // The host announced the hand-off before this handler ran; opening + // the mail client is the shell's job. The published path and the + // revision it was published as travel in the message so the + // receiving side can tell which revision the file is. + const name = path.replace(/^.*[\\/]/, "") + const subject = qsTr("Published document: %1").arg(name) + const body = qsTr("The published document %1 is at revision %2.").arg(path).arg(revision) + Qt.openUrlExternally("mailto:?subject=" + encodeURIComponent(subject) + + "&body=" + encodeURIComponent(body)) + } 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 3ed992db..7db6ef35 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 9c9af017..b2d51c29 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 } From 9652d5760fcd1cfa5e89c80f3a4aa0492c898f1e Mon Sep 17 00:00:00 2001 From: mberrys <212453881+mberrys@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:16:03 -0700 Subject: [PATCH 4/6] fix(ci): gate the #126 checker and close the ratchet, identity and vocabulary gaps Findings 23-28. check_quick_localization.py and the MenuModel label agree again (actionExportDocument gets the ordinary switch arm) and the checker, its unit tests and the help-strings staleness check run whole-tree in ci.yml and check-change.py; the handler-coverage ratchet refuses to grow unless --accept-new-gaps records a deliberate gap with its reason; the identity checker scans .in/.ui/.qrc and the Wix installer template names the real LOOP artifacts; disposition and target are validated against the schema vocabulary in verify-command-catalog.py and the Linux twin; UnitTestsLoopStateVisual loads the compiled catalogs via its ctest environment; ADR-014 pins the shipped counts. --- .github/workflows/ci.yml | 5 ++ UnitTests/CMakeLists.txt | 7 ++ WixInstaller/createInstaller.ps1.in | 4 +- docs/LOOP_SHELL_CONTRACT.md | 7 +- ...adr-014-direct-revision-authoring-edits.md | 17 ++--- scripts/agent/check-change.py | 10 +++ scripts/ci/check_loop_identity.py | 3 + scripts/ci/check_quick_localization.py | 12 +++- scripts/ci/test_check_loop_identity.py | 18 ++++++ scripts/ci/test_check_quick_localization.py | 32 +++++++++- scripts/ci/test_command_catalog.py | 56 +++++++++++++++- scripts/verify-command-catalog.py | 64 ++++++++++++++++++- scripts/verify-loop-shell-contract.py | 9 +++ 13 files changed, 224 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4132559b..23702536 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/UnitTests/CMakeLists.txt b/UnitTests/CMakeLists.txt index 68154b11..bff594d7 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/WixInstaller/createInstaller.ps1.in b/WixInstaller/createInstaller.ps1.in index e446cfd0..85d00da6 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/docs/LOOP_SHELL_CONTRACT.md b/docs/LOOP_SHELL_CONTRACT.md index 13208907..bf73f5da 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 5151ed05..1c819a97 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/scripts/agent/check-change.py b/scripts/agent/check-change.py index ddf6a46a..5dccc5ec 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 37e0e65b..3e146d2e 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 be4d19c4..a235bc77 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 1f9f45a1..f0ff31a3 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 9cd29fe4..2ade01ed 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 13c6fcf6..aadc1dcd 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 d05b7110..bf075b40 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 88b712c0..44b71bb2 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 From 6d5eebd86ba05428cf8d18a47a0358618bd98ad5 Mon Sep 17 00:00:00 2001 From: mberrys <212453881+mberrys@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:16:04 -0700 Subject: [PATCH 5/6] docs(review): record the adversarial review of the #272 train The finding set with per-item dispositions, the dismissed items (signature badge; the relative include, dismissed with build evidence), the deferred items (history re-verification on the interactive thread; the self-referential accessibility test), the verified-clean list and the change fragment with its evidence manifest. --- changes/fix-dev-review-272.evidence.yaml | 37 +++++++ changes/fix-dev-review-272.md | 4 + docs/reviews/2026-10-11-dev-review-272.md | 119 ++++++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 changes/fix-dev-review-272.evidence.yaml create mode 100644 changes/fix-dev-review-272.md create mode 100644 docs/reviews/2026-10-11-dev-review-272.md diff --git a/changes/fix-dev-review-272.evidence.yaml b/changes/fix-dev-review-272.evidence.yaml new file mode 100644 index 00000000..3cbe8663 --- /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 00000000..8fceb403 --- /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 and Bookmark Import/Export get their missing Quick shell receivers (file dialogs, mail client) 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/reviews/2026-10-11-dev-review-272.md b/docs/reviews/2026-10-11-dev-review-272.md new file mode 100644 index 00000000..943c1d1f --- /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. From 8b4f975f6d50fd11e9aa789736cb2c6ddb770022 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 16:47:21 +0000 Subject: [PATCH 6/6] fix(quick): open the mail hand-off from the host, not the Quick shell The Quick shell policy forbids Qt.openUrlExternally in QML. The Send by E-Mail receiver added in this train called it, which failed the quick shell policy gate in CI. The host now builds the mailto URL (subject and body carry the file name and revision) and opens it through QDesktopServices. A client that does not take the message is refused with mail/client-unavailable instead of reported as handed off. The QML receivers are removed from both shells. The unit test captures the mailto URL with a URL handler, so no mail client is launched. Claude-Session: https://claude.ai/code/session_01WWpvxcKrZtceyLLgMhoZ3k --- LoopEditor/editorhost.cpp | 26 +++++++++++++++-- LoopEditor/editorhost.h | 10 ++++--- LoopEditor/qml/Main.qml | 11 ------- UnitTests/tst_editorhosttest.cpp | 29 +++++++++++++++++++ changes/fix-dev-review-272.md | 2 +- .../qml/Main.qml | 11 ------- 6 files changed, 59 insertions(+), 30 deletions(-) diff --git a/LoopEditor/editorhost.cpp b/LoopEditor/editorhost.cpp index fae3d003..0762e33e 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 @@ -5602,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(); @@ -5627,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; } diff --git a/LoopEditor/editorhost.h b/LoopEditor/editorhost.h index fb7c98bf..d2cb4f0a 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/Main.qml b/LoopEditor/qml/Main.qml index 83076fc3..b7542090 100644 --- a/LoopEditor/qml/Main.qml +++ b/LoopEditor/qml/Main.qml @@ -453,17 +453,6 @@ ApplicationWindow { bookmarkExportDialog.revision = host.outputRevision() bookmarkExportDialog.open() } - function onDocumentMailRequested(path, revision) { - // The host announced the hand-off before this handler ran; opening - // the mail client is the shell's job. The published path and the - // revision it was published as travel in the message so the - // receiving side can tell which revision the file is. - const name = path.replace(/^.*[\\/]/, "") - const subject = qsTr("Published document: %1").arg(name) - const body = qsTr("The published document %1 is at revision %2.").arg(path).arg(revision) - Qt.openUrlExternally("mailto:?subject=" + encodeURIComponent(subject) + - "&body=" + encodeURIComponent(body)) - } function onSettingsPanelRequested() { if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) settingsDialog.open() diff --git a/UnitTests/tst_editorhosttest.cpp b/UnitTests/tst_editorhosttest.cpp index ea99c81d..7dd13a6c 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 @@ -4898,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(); @@ -4921,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. @@ -4936,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/changes/fix-dev-review-272.md b/changes/fix-dev-review-272.md index 8fceb403..8757dd5d 100644 --- a/changes/fix-dev-review-272.md +++ b/changes/fix-dev-review-272.md @@ -1,4 +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 and Bookmark Import/Export get their missing Quick shell receivers (file dialogs, mail client) 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. +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/tools/ProductQuickAccessibilitySmoke/qml/Main.qml b/tools/ProductQuickAccessibilitySmoke/qml/Main.qml index 83076fc3..b7542090 100644 --- a/tools/ProductQuickAccessibilitySmoke/qml/Main.qml +++ b/tools/ProductQuickAccessibilitySmoke/qml/Main.qml @@ -453,17 +453,6 @@ ApplicationWindow { bookmarkExportDialog.revision = host.outputRevision() bookmarkExportDialog.open() } - function onDocumentMailRequested(path, revision) { - // The host announced the hand-off before this handler ran; opening - // the mail client is the shell's job. The published path and the - // revision it was published as travel in the message so the - // receiving side can tell which revision the file is. - const name = path.replace(/^.*[\\/]/, "") - const subject = qsTr("Published document: %1").arg(name) - const body = qsTr("The published document %1 is at revision %2.").arg(path).arg(revision) - Qt.openUrlExternally("mailto:?subject=" + encodeURIComponent(subject) + - "&body=" + encodeURIComponent(body)) - } function onSettingsPanelRequested() { if (host && host.focusRestoration) host.focusRestoration.remember(window.activeFocusItem) settingsDialog.open()