<html>
    <head>
      <base href="https://bugs.freedesktop.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add font color in Poppler"
   href="https://bugs.freedesktop.org/show_bug.cgi?id=107151#c15">Comment # 15</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add font color in Poppler"
   href="https://bugs.freedesktop.org/show_bug.cgi?id=107151">bug 107151</a>
              from <span class="vcard"><a class="email" href="mailto:haxtibal@posteo.de" title="Tobias Deiminger <haxtibal@posteo.de>"> <span class="fn">Tobias Deiminger</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=140860" name="attach_140860" title="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.">attachment 140860</a> <a href="attachment.cgi?id=140860&action=edit" title="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.">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=107151&attachment=140860'>[review]</a>
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 <span class=""><a href="attachment.cgi?id=140860" name="attach_140860" title="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.">attachment 140860</a> <a href="attachment.cgi?id=140860&action=edit" title="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.">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=107151&attachment=140860'>[review]</a>:
-----------------------------------------------------------------

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

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 @@
<span class="quote">>  
> +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)</span >

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 @@
<span class="quote">> +  DefaultAppearance(const GooString &fontTag, int fontPtSize, AnnotColor fontColor);
> +  ~DefaultAppearance();
> +  GooString *fontTag;
> +  int fontPtSize;
> +  AnnotColor fontColor;</span >

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 @@
<span class="quote">> +    return ftextann->getAppearanceString();
> +}
> +
> +void TextAnnotationPrivate::setDefaultAppearance()
> +{</span >

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 @@
<span class="quote">> +      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);
> +  }</span >

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).

<a href="http://doc.qt.io/qt-5/qcolor.html#Spec-enum">http://doc.qt.io/qt-5/qcolor.html#Spec-enum</a>

@@ +2133,5 @@
<span class="quote">> +        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]);</span >

We can instead reuse the existing utility function QColor convertAnnotColor(
AnnotColor *color ) here. It conveniently handles all AnnotColorSpace cases.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>