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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #31 from Tobias Deiminger <haxtibal at posteo.de> ---
Comment on attachment 141142
  --> https://bugs.freedesktop.org/attachment.cgi?id=141142
new version of the patch

Review of attachment 141142:
-----------------------------------------------------------------

::: poppler/Annot.cc
@@ +2820,5 @@
> +DefaultAppearance *AnnotFreeText::getDefaultAppearance() const {
> +  double fontSize;
> +  AnnotColor *fontColor;
> +  GooString *fontTag;
> +  parseAppearanceString(appearanceString, fontSize, fontColor, fontTag);

parseAppearanceString(..., GooString** fontTag) would be nice, IMO. Then we can
call it as
parseAppearanceString(appearanceString, fontSize, fontColor, nullptr);
and avoid the superfluous new/delete of fontTag

@@ +2844,5 @@
>    return GfxFont::makeFont(xref, "AnnotDrawFont", dummyRef, fontDict);
>  }
>  
> +GooString *AnnotFreeText::constructAppearanceString(const GooString &fontTag, double fontSize, const AnnotColor *fontColor) {
> +  const double *colorData = fontColor->getValues();

As Albert noted, fontColor may be null here (see call from
AnnotFreeText::AnnotFreeText). That's still a potential segfault. We can either
check for fontColor == nullptr and make writing color optional. Or enforce a
fontColor value by changing the signature to const AnnotColor &fontColor, and
adapt all calling locations.

@@ +2861,5 @@
> +    case AnnotColor::AnnotColorSpace::colorCMYK: //=4
> +      cstr = GooString::format("{0:.2f} {1:.2f} {2:.2f} {3:.2f} k ", colorData[0], colorData[1], colorData[2], colorData[3]);
> +      break;
> +  }
> +  const GooString * str = GooString::format("/{0:s} {1:.2f} Tf", &fontTag, fontSize);

That's where I meant we could use AnnotAppearanceBuilder instead of raw
GooString::format.

::: poppler/Annot.h
@@ +358,5 @@
> +  const GooString &getFontTag() const { return *fontTag; }
> +  int getFontPtSize() const { return fontPtSize; }
> +  const AnnotColor *getFontColor() const { return fontColor; }
> +  ~DefaultAppearance();
> +  DefaultAppearance(DefaultAppearance &ger) = delete;

As Albert suggested, also delete the copy assignment operator here:
DefaultAppearance& operator=(const DefaultAppearance&) = delete;

The "ger" in copy Ctor is superfluous, type alone is enough. But that's
probably a matter of taste.

@@ +997,4 @@
>    Object getAppearanceResDict() override;
>    void setContents(GooString *new_content) override;
>  
> +  void setAppearanceString(const DefaultAppearance &da);

Rename to setDefaultAppearance (as we seem to agree about name now)

-- 
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/20180817/9215bcec/attachment-0001.html>