<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#c14">Comment # 14</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 @@ +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 <a class="bz_bug_link bz_status_NEW " title="NEW - FreeText annotation ignores font" href="show_bug.cgi?id=81748">bug 81748</a>). @@ +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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>