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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #14 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
@@ +2816,5 @@
>  
> +DefaultAppearance AnnotFreeText::getAppearanceString() const {
> +  double fontSize;
> +  AnnotColor *fontColor;
> +  parseAppearanceString(appearanceString, fontSize, fontColor);

We should parse fontTag in parseAppearanceString too, and not return
"Invalid_font" (you need this latest for font family. Oliver drafted it
already, see his patch in bug 81748).

@@ +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)

I'd use switch here to handle the enum.

@@ +2845,5 @@
> +    return GooString::format("{0:.2f} {1:.2f} {2:.2f} rg /{3:s} {4:.2f} Tf", colorData[0], colorData[1], colorData[2], &fontTag, fontSize);
> +  else if (colorSpace == AnnotColor::AnnotColorSpace::colorCMYK)
> +    return GooString::format("{0:.2f} {1:.2f} {2:.2f} {3:.2f} k /{4:s} {5:.2f} Tf", colorData[0], colorData[1], colorData[2], colorData[3], &fontTag, fontSize);
> +  else
> +    return GooString::format("{0:.2f} g /{1:s} {2:.2f} Tf", colorData[0], &fontTag, fontSize);

The tag and size part is unconditional. Better don't repeat it, you can use
GooString::append to make only color conditional.

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

Was unsure myself, but think it's better to turn struct into a class, and make
the members private. Provide const accessors like
  const GooString &getFontTag() const { return *fontTag; }
  int &getFontPtSize() { return fontPtSize; }
  const AnnotColor &getFontColor() const { return fontColor; }
so that users don't get tempted to do a delete da->fontTag, and to make the
structure immutable.

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

Don't repeat yourself. Make only construction of AnnotColor conditional, the
remaining code is identical and can be done unconditionally.

@@ +1884,5 @@
> +}
> +
> +void TextAnnotationPrivate::setDefaultAppearance()
> +{
> +  if (textColor.spec() == QColor::Rgb)

I prefer using switch to handle enums, so that one gets reminded to provide a
default path for unmentioned values.

@@ +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);
> +  }

There's no path for colorTransparent and colorGray, nor are there default
paths.

-- 
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/20180728/9f925627/attachment-0001.html>