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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #22 from Albert Astals Cid <aacid at kde.org> ---
There's lots of things that need improving in this patch:
 * DefaultAppearance::fontTag = new GooString();
Don't do this, do just fontTag, if in the other constructor you feel like
variable names collide, append an A to the input vars like done elsewhere

 * DefaultAppearance name
Are we sure this is the best name for the class? Wouldn't AnnotAppearance make
more sense? What's "Default" about this class?

 * DefaultAppearance use
This class is wrongly constructed/used, it has a GooString pointer and no copy
constructor/asignment operator, which means that the default copy constructor
will just assign the same pointer value to the copy, and then you have two
instances with the same pointer, which means that you'll get a double delete in
the destructor. You need to either implement the copy constructor/asignment
operator or mark them as deleted and then make sure you just pass pointers
around. I'd suggest the second but if you want the first, that's also fine

 * constructAppearanceString
Maybe it would make more sense to pass the DefaultAppearance class here as
parameter?

 * AnnotFreeText::setAppearanceString(const DefaultAppearance &da)
This needs a rename, i'm not setting an string here

 * DefaultAppearance AnnotFreeText::getAppearanceString() const {
This needs a rename, i'm not getting an string here

 * GooString *AnnotFreeText::constructAppearanceString(const GooString
&fontTag, double fontSize, const AnnotColor *fontColor) {
This assumes fontColor won't be null, but the default DefaultAppearance
constructor has a null pointer for fontColor, so that looks a bit fishy.

* GooString *fontTag;
  parseAppearanceString(appearanceString, fontsize, fontcolor, fontTag);
Is this a memory leak?

-- 
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/20180816/bd50633e/attachment.html>