[Poppler-bugs] [Bug 107151] Add font color in Poppler

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=107151

--- Comment #15 from Tobias Deiminger <haxtibal at posteo.de> ---
Comment on attachment 140860
  --> https://bugs.freedesktop.org/attachment.cgi?id=140860
This patch creates and parses the DA string in AnnotFreeText and supports RGB,
CMYK and Gray color models that may be passed to poppler in setTextColor
function.

Review of attachment 140860:
-----------------------------------------------------------------

::: poppler/Annot.cc
@@ +800,5 @@
> +  DefaultAppearance::fontTag = fontTag.copy();
> +}
> +
> +DefaultAppearance::~DefaultAppearance() {
> +  delete fontTag;

If fontColor gets an raw pointer (as suggested in review Annot.h, struct
DefaultAppearance), then we need to delete fontColor here.
fontColor is "owned by" DefaultAppearance, thus DefaultAppearance is
responsible for delete.

@@ +2840,5 @@
>  
> +GooString *AnnotFreeText::constructAppearanceString(const GooString &fontTag, double fontSize, const AnnotColor &fontColor) {
> +  const double *colorData = fontColor.getValues();
> +  AnnotColor::AnnotColorSpace colorSpace = fontColor.getSpace();
> +  if (colorSpace == AnnotColor::AnnotColorSpace::colorRGB)

In case of AnnotColorSpace::colorTransparent, I just wouldn't insert any color
operator into the GooString.

AnnotColor was originally intended for the C entry of Annotation Dictionaries,
where colorTransparent means "No colour; transparent". Now we reuse class
AnnotColor for DA, but here text color = "No colour; transparent" doesn't make
much sense; so turn it into kind of nop. Or should we get a new class
VariableTextColor instead?

::: poppler/Annot.h
@@ +355,5 @@
> +  DefaultAppearance(const GooString &fontTag, int fontPtSize, AnnotColor fontColor);
> +  ~DefaultAppearance();
> +  GooString *fontTag;
> +  int fontPtSize;
> +  AnnotColor fontColor;

Seems color in DA is optional*. Therefore fontColor should become a raw
pointer, so that we can leave it undefined with fontColor = nullptr if it is
not part of the DA. Color can be defaulted to black, where necessary (like
already done in AnnotFreeText::generateFreeTextAppearance).

*see PDF standard, Variable Text: "At a minimum, the string shall include a Tf
(text font) operator".

::: qt5/src/poppler-annotation.cc
@@ +1883,5 @@
> +    return ftextann->getAppearanceString();
> +}
> +
> +void TextAnnotationPrivate::setDefaultAppearance()
> +{

Alternatively we could (or even should) reuse the existing utility function
AnnotColor* convertQColor( const QColor &c ) here. But convertQColor looks a
bit simplified, we might want to enhance it.

@@ +1897,5 @@
> +	DefaultAppearance da(GooString("Invalid_font"), textFont.pointSize(),
> +	                     AnnotColor(textColor.cyanF(), textColor.magentaF(), textColor.yellowF(), textColor.blackF()));
> +    AnnotFreeText * ftextann = static_cast<AnnotFreeText*>(pdfAnnot);
> +    ftextann->setAppearanceString(da);
> +  }

My bad, it's not enum AnnotColorSpace but enum QColor::Spec that we handle
here. So there's no colorTransparent and colorGray. But for example Hsl and
Invalid. For any QColor::Spec that can't be mapped to AnnotColorSpace, I'd
construct a DefaultAppearance(..., fontColor = nullptr).

http://doc.qt.io/qt-5/qcolor.html#Spec-enum

@@ +2133,5 @@
> +        const double *colorData = da.fontColor.getValues();
> +        if (da.fontColor.getSpace() == AnnotColor::AnnotColorSpace::colorRGB)
> +          color.setRgbF(colorData[0], colorData[1], colorData[2]);
> +        else if (da.fontColor.getSpace() == AnnotColor::AnnotColorSpace::colorCMYK)
> +          color.setCmykF(colorData[0], colorData[1], colorData[2], colorData[3]);

We can instead reuse the existing utility function QColor convertAnnotColor(
AnnotColor *color ) here. It conveniently handles all AnnotColorSpace cases.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20180729/4d4b40ab/attachment.html>