Commit d4b4be23 authored by Tobias Deiminger's avatar Tobias Deiminger Committed by Albert Astals Cid

More review fixes

Text color is indicated by nonstroking color in graphics state
	Assumption: Text rendering mode is 'fill'.

Increase color precision for lossless roundtrip of 16 bit integers
	Our API takes QColor from user. We want to support a lossless roundtrip
	of QColor (16 bit per channel internally) through document save and
	load, and empirically found .5f is best match.

Check only .5f case of color channel roundtrip. Include 65535.
	We check if precision == 5 is sufficient, and fail if not. We know that
	precision < 5 will never work, because target set contains less numbers
	than uint16 range.

Use smart pointer in textFont and textColor

Add test for CMYK QColor roundtrip

Support QColor::Cmyk to AnnotColor::colorCMYK conversion

Add simple test for font size. Fix actual/expected args.

Model font name as class Object, type objName
	Take into account that ISO 32000 says Tf operand is always an object of
	PDF type "name". Further benefit: class Object introduces ownership
	semantcis.

Use more std::unique_ptr and fix some coding syle
	Some places assumed ownership implicitely. Make it more explicit.

Move parse/constructAppearanceString into DefaultAppearance
	We gain cohesion and automatic memory management.

Fix minor styling issues

Use std::make_unique from C++14
parent b67e7ab7
This diff is collapsed.
......@@ -40,6 +40,7 @@
#pragma interface
#endif
#include <memory>
#include "Object.h"
class XRef;
......@@ -354,18 +355,24 @@ private:
class DefaultAppearance {
public:
DefaultAppearance(const GooString &fontTag, int fontPtSize, AnnotColor *fontColor = nullptr);
const GooString &getFontTag() const { return *fontTag; }
int getFontPtSize() const { return fontPtSize; }
const AnnotColor *getFontColor() const { return fontColor; }
~DefaultAppearance();
DefaultAppearance(Object &&fontName, double fontPtSize, std::unique_ptr<AnnotColor> fontColor);
DefaultAppearance(GooString *da);
void setFontName(Object &&fontNameA);
const Object &getFontName() const { return fontName; }
void setFontPtSize(double fontPtSizeA);
double getFontPtSize() const { return fontPtSize; }
void setFontColor(std::unique_ptr<AnnotColor> fontColorA);
const AnnotColor *getFontColor() const { return fontColor.get(); }
GooString *toAppearanceString() const;
DefaultAppearance(DefaultAppearance &) = delete;
DefaultAppearance& operator=(const DefaultAppearance&) = delete;
private:
GooString *fontTag;
int fontPtSize;
AnnotColor *fontColor;
Object fontName;
double fontPtSize;
std::unique_ptr<AnnotColor> fontColor;
};
//------------------------------------------------------------------------
......@@ -532,7 +539,7 @@ public:
void setDrawColor(const AnnotColor *color, GBool fill);
void setLineStyleForBorder(const AnnotBorder *border);
void setTextFont(const GooString &fontTag, double fontSize);
void setTextFont(const Object &fontName, double fontSize);
void drawCircle(double cx, double cy, double r, GBool fill);
void drawCircleTopLeft(double cx, double cy, double r);
void drawCircleBottomRight(double cx, double cy, double r);
......@@ -1006,7 +1013,7 @@ public:
void setIntent(AnnotFreeTextIntent new_intent);
// getters
DefaultAppearance *getDefaultAppearance() const;
std::unique_ptr<DefaultAppearance> getDefaultAppearance() const;
AnnotFreeTextQuadding getQuadding() const { return quadding; }
// return rc
const GooString *getStyleString() const { return styleString; }
......@@ -1019,8 +1026,6 @@ public:
protected:
void initialize(PDFDoc *docA, Dict *dict);
static GooString *constructAppearanceString(const GooString &fontTag, double fontSize, const AnnotColor *fontColor);
static void parseAppearanceString(GooString *da, double &fontSize, AnnotColor* &fontColor, GooString **fontTag);
void generateFreeTextAppearance();
// required
......
......@@ -1817,7 +1817,7 @@ class TextAnnotationPrivate : public AnnotationPrivate
Annotation * makeAlias() override;
Annot* createNativeAnnot(::Page *destPage, DocumentData *doc) override;
void setDefaultAppearanceToNative();
DefaultAppearance *getDefaultAppearanceFromNative() const;
std::unique_ptr<DefaultAppearance> getDefaultAppearanceFromNative() const;
// data fields
TextAnnotation::TextType textType;
......@@ -1852,11 +1852,14 @@ Annot* TextAnnotationPrivate::createNativeAnnot(::Page *destPage, DocumentData *
// Set pdfAnnot
PDFRectangle rect = boundaryToPdfRectangle(boundary, flags);
if (textType == TextAnnotation::Linked) {
pdfAnnot = new AnnotText(destPage->getDoc(), &rect);
} else {
DefaultAppearance da(GooString("Invalid_font"), textFont.pointSize(), convertQColor(textColor));
pdfAnnot = new AnnotFreeText(destPage->getDoc(), &rect, da);
if (textType == TextAnnotation::Linked)
{
pdfAnnot = new AnnotText{ destPage->getDoc(), &rect };
}
else
{
DefaultAppearance da{ { objName, "Invalid_font" }, static_cast<double>( textFont.pointSize() ), std::unique_ptr<AnnotColor>{ convertQColor( textColor ) } };
pdfAnnot = new AnnotFreeText{ destPage->getDoc(), &rect, da };
}
// Set properties
......@@ -1875,21 +1878,24 @@ Annot* TextAnnotationPrivate::createNativeAnnot(::Page *destPage, DocumentData *
void TextAnnotationPrivate::setDefaultAppearanceToNative()
{
if (pdfAnnot && pdfAnnot->getType() == Annot::typeFreeText) {
AnnotFreeText * ftextann = static_cast<AnnotFreeText*>(pdfAnnot);
AnnotColor *color = convertQColor(textColor);
DefaultAppearance da(GooString("Invalid_font"), textFont.pointSize(), color);
ftextann->setDefaultAppearance(da);
if ( pdfAnnot && pdfAnnot->getType() == Annot::typeFreeText )
{
AnnotFreeText * ftextann = static_cast<AnnotFreeText*>( pdfAnnot );
DefaultAppearance da{ { objName, "Invalid_font" }, static_cast<double>( textFont.pointSize() ), std::unique_ptr<AnnotColor>{ convertQColor( textColor ) } };
ftextann->setDefaultAppearance( da );
}
}
DefaultAppearance *TextAnnotationPrivate::getDefaultAppearanceFromNative() const
std::unique_ptr<DefaultAppearance> TextAnnotationPrivate::getDefaultAppearanceFromNative() const
{
if (pdfAnnot && pdfAnnot->getType() == Annot::typeFreeText) {
AnnotFreeText * ftextann = static_cast<AnnotFreeText*>(pdfAnnot);
if ( pdfAnnot && pdfAnnot->getType() == Annot::typeFreeText )
{
AnnotFreeText * ftextann = static_cast<AnnotFreeText*>( pdfAnnot );
return ftextann->getDefaultAppearance();
} else {
return nullptr;
}
else
{
return {};
}
}
......@@ -2082,17 +2088,16 @@ QFont TextAnnotation::textFont() const
{
Q_D( const TextAnnotation );
if (!d->pdfAnnot)
if ( !d->pdfAnnot )
return d->textFont;
QFont font;
if (d->pdfAnnot->getType() == Annot::typeFreeText)
if ( d->pdfAnnot->getType() == Annot::typeFreeText )
{
DefaultAppearance *da = d->getDefaultAppearanceFromNative();
if (da)
if ( std::unique_ptr<DefaultAppearance> da{ d->getDefaultAppearanceFromNative() } )
{
font.setPointSize( da->getFontPtSize() );
delete da;
}
}
return font;
......@@ -2111,15 +2116,15 @@ QColor TextAnnotation::textColor() const
{
Q_D( const TextAnnotation );
if (!d->pdfAnnot)
if ( !d->pdfAnnot )
return d->textColor;
QColor color;
DefaultAppearance *da = d->getDefaultAppearanceFromNative();
if (da)
color = convertAnnotColor(da->getFontColor());
delete da;
return color;
if ( std::unique_ptr<DefaultAppearance> da{ d->getDefaultAppearanceFromNative() } )
{
return convertAnnotColor( da->getFontColor() );
}
return {};
}
void TextAnnotation::setTextColor( const QColor &color )
......@@ -5103,10 +5108,26 @@ QColor convertAnnotColor( const AnnotColor *color )
AnnotColor* convertQColor( const QColor &c )
{
if (!c.isValid() || c.alpha() == 0)
if ( c.alpha() == 0 )
return new AnnotColor(); // Transparent
else
return new AnnotColor(c.redF(), c.greenF(), c.blueF());
AnnotColor *newcolor;
switch ( c.spec() )
{
case QColor::Rgb:
case QColor::Hsl:
case QColor::Hsv:
newcolor = new AnnotColor( c.redF(), c.greenF(), c.blueF() );
break;
case QColor::Cmyk:
newcolor = new AnnotColor( c.cyanF(), c.magentaF(), c.yellowF(), c.blackF() );
break;
case QColor::Invalid:
default:
newcolor = new AnnotColor();
break;
}
return newcolor;
}
//END utility annotation functions
......
#include <cmath>
#include <memory>
#include <sstream>
#include <QtTest/QtTest>
#include <QTemporaryFile>
#include <poppler-qt5.h>
#include "goo/GooString.h"
#include "goo/gstrtod.h"
class TestAnnotations : public QObject
{
Q_OBJECT
private slots:
void checkFontColor();
void checkQColorPrecision();
void checkFontSizeAndColor();
};
void TestAnnotations::checkFontColor()
/* Is .5f sufficient for 16 bit color channel roundtrip trough save and load on all architectures? */
void TestAnnotations::checkQColorPrecision() {
bool precisionOk = true;
for (int i = std::numeric_limits<uint16_t>::min(); i <= std::numeric_limits<uint16_t>::max(); i++) {
double normalized = static_cast<uint16_t>(i) / static_cast<double>(std::numeric_limits<uint16_t>::max());
GooString* serialized = GooString::format("{0:.5f}", normalized);
double deserialized = gatof( serialized->getCString() );
uint16_t denormalized = std::round(deserialized * std::numeric_limits<uint16_t>::max());
if (static_cast<uint16_t>(i) != denormalized) {
precisionOk = false;
break;
}
}
QVERIFY(precisionOk);
}
void TestAnnotations::checkFontSizeAndColor()
{
const QString contents{"foobar"};
const QColor textColor{0xAB, 0xCD, 0xEF};
const std::vector<QColor> testColors{QColor::fromRgb(0xAB, 0xCD, 0xEF),
QColor::fromCmyk(0xAB, 0xBC, 0xCD, 0xDE)};
const QFont testFont("Helvetica", 20);
QTemporaryFile tempFile;
QVERIFY(tempFile.open());
......@@ -32,15 +56,14 @@ void TestAnnotations::checkFontColor()
};
QVERIFY(page);
std::unique_ptr<Poppler::TextAnnotation> annot{
new Poppler::TextAnnotation{Poppler::TextAnnotation::InPlace}
};
annot->setBoundary(QRectF(0.0, 0.0, 1.0, 1.0));
annot->setContents(contents);
annot->setTextColor(textColor);
page->addAnnotation(annot.get());
for (const auto& color : testColors) {
auto annot = std::make_unique<Poppler::TextAnnotation>(Poppler::TextAnnotation::InPlace);
annot->setBoundary(QRectF(0.0, 0.0, 1.0, 1.0));
annot->setContents(contents);
annot->setTextFont(testFont);
annot->setTextColor(color);
page->addAnnotation(annot.get());
}
std::unique_ptr<Poppler::PDFConverter> conv(doc->pdfConverter());
QVERIFY(conv);
......@@ -61,12 +84,19 @@ void TestAnnotations::checkFontColor()
QVERIFY(page);
auto annots = page->annotations();
QCOMPARE(1, annots.size());
QCOMPARE(Poppler::Annotation::AText, annots.constFirst()->subType());
auto annot = static_cast<Poppler::TextAnnotation*>(annots.constFirst());
QCOMPARE(contents, annot->contents());
QCOMPARE(textColor, annot->textColor());
QCOMPARE(annots.size(), static_cast<int>(testColors.size()));
auto &&annot = annots.constBegin();
for (const auto& color : testColors) {
QCOMPARE((*annot)->subType(), Poppler::Annotation::AText);
auto textAnnot = static_cast<Poppler::TextAnnotation*>(*annot);
QCOMPARE(textAnnot->contents(), contents);
QCOMPARE(textAnnot->textFont().pointSize(), testFont.pointSize());
QCOMPARE(static_cast<int>(textAnnot->textColor().spec()), static_cast<int>(color.spec()));
QCOMPARE(textAnnot->textColor(), color);
if (annot != annots.end())
++annot;
}
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment