<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <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 <a class="email" href="mailto:haxtibal@posteo.de" title="Tobias Deiminger <haxtibal@posteo.de>"> Tobias Deiminger</a> <pre>Comment on <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> <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 <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> <a href='page.cgi?id=splinter.html&bug=107151&attachment=140860'>[review]</a>: ----------------------------------------------------------------- ::: 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). <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 @@ > + 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>