Comment on attachment 140496 [details] [review]
This patch adds font color in poppler-qt5 frontend by introducing 'rg' operator

Review of attachment 140496 [details] [review]:
-----------------------------------------------------------------

::: qt5/src/poppler-annotation.h
@@ +483,5 @@
>      void setTextIcon( const QString &icon );
>  
>      QFont textFont() const;
> +    QColor textColor() const;
> +    void setTextFont( const QFont &font, const QColor &color );

Can we split setTextFont into two?
void setTextFont( const QFont &font );
void setTextColor( const QColor &color );

There are two reasons why: Make it symmetrical to the textFont/textColor get methods. And more important, changing setTextFont signature would break API and ABI compatibility of the public poppler-annotations.h interface. Adding a new non-virtual method however keeps both API and ABI compatible. I don't know the exact poppler API policy, but that should be considerable anyway.

Created attachment 140512 [details] [review]
This updated patch includes setTextFont and setTextColor functions

Updating the patch to include setTextFont and setTextColor functions. The default font color is set to black.

I'm not very happy with the appearance creation being in the qt frontend, but well it kind of was there already so i guess we can let it pass, but i really don't like the new method that parses the color, can't we use AnnotFreeText::parseAppearanceString ?

(In reply to Albert Astals Cid from comment #3)
> I'm not very happy with the appearance creation being in the qt frontend,
> but well it kind of was there already so i guess we can let it pass, but i
> really don't like the new method that parses the color, can't we use
> AnnotFreeText::parseAppearanceString ?

I definitely agree that creating and parsing DA should be the concern of poppler/Annot.cc, probably class AnnotFreeText.

But Dileep just carried on what was already there, not only regarding creation but also regarding parsing: The long existing TextAnnotation::textFont also parses DA in Qt5 frontend.

To be consistent, we should then move all the DA creation and parsing to poppler/Annot.cc. Both existing and new functionality. Albert, would that be good for you? Dileep, do you think it's possible for you?

(In reply to Tobias Deiminger from comment #4)
> (In reply to Albert Astals Cid from comment #3)
> > I'm not very happy with the appearance creation being in the qt frontend,
> > but well it kind of was there already so i guess we can let it pass, but i
> > really don't like the new method that parses the color, can't we use
> > AnnotFreeText::parseAppearanceString ?
> 
> I definitely agree that creating and parsing DA should be the concern of
> poppler/Annot.cc, probably class AnnotFreeText.
> 
> But Dileep just carried on what was already there, not only regarding
> creation but also regarding parsing: The long existing
> TextAnnotation::textFont also parses DA in Qt5 frontend.
> 
> To be consistent, we should then move all the DA creation and parsing to
> poppler/Annot.cc. Both existing and new functionality. Albert, would that be
> good for you? Dileep, do you think it's possible for you?

As basically I'm working on the project Okular, that uses the Qt5 frontend of the poppler, I think it should be added and extended to where it has initially existed. As I passed the second evaluation, I guess it should be marked as "done" as in the remaining 16 days, I have to accomplish the remaining tasks.

The fact that you are working on a Google Summer of Code is no excuse to commit sub-par code anyway nor here nor to okular.

"passing second evaluation" was a broad decission. It doesn't mean a particular patch is ready to land. Albert is the poppler maintainer, he has to be confident the patch is good for the project before he can merge it. That's independent from GSoC.

If the patch is sub-par, but the patch was mainly blueprinted from existing code, then the existing code is sub-par too (see TextAnnotationPrivate::toAppearanceString, TextAnnotation::textFont). Let's focus on how to fix both and get this patch ready. I'll come up with some ideas soon, don't hesitate to be faster.

Ok, here's my take. Problems with solutions (draft).

Problem 1: Qt5 shouldn't care about low-level DA creation

GooString * TextAnnotationPrivate::toAppearanceString(const QFont &font)
...
  // Qt5 shouldn't need to know how to create Tf operator
  GooString * s = GooString::format("{0:f} {1:f} {2:f} rg /Invalid_font {3:d} Tf", r, g, b, font.pointSize());

used like

GooString * da = toAppearanceString(textFont, textColor);
pdfAnnot = new AnnotFreeText(destPage->getDoc(), &rect, da);
  ... or ...
ftextann->setAppearanceString(da);


Problem 2: Qt5 shouldn't care about low level DA parsing

QFont TextAnnotation::textFont() const    // existing code
...
  QString style = QString::fromLatin1( da->getCString() );
  // Qt5 shouldn't need to know how to parse Tf operator
  QRegExp rx(QStringLiteral("(\\d+)(\\.\\d*)? Tf")); 

and

QColor TextAnnotation::textColor() const  // new code
...
  QString style = QString::fromLatin1( da->getCString() );
  // Qt5 shouldn't need to know how to parse rg operator
  QRegExp rx(QStringLiteral("(\\d\\.\\d*) (\\d\\.\\d*) (\\d\\.\\d*) rg"));


Solution 1

Don't consume raw DA GooString in class AnnotFreeText, but take higher level data

Instead of
  AnnotFreeText(PDFDoc *docA, PDFRectangle *rect, GooString *da);
  AnnotFreeText::setAppearanceString(GooString *new_string)

make it like

  struct AppearanceStringData {
    GooString *fontTag;
    int fontPtSize;
    AnnotColor* fontColor;
  };

  AnnotFreeText(PDFDoc *docA, PDFRectangle *rect, const AppearanceStringData& apStr);
  AnnotFreeText::setAppearanceString(const AppearanceStringData& apStr);

and do all the low level stuff like Tf construction and invalidating appearance in the changed AnnotFreeText methods.


Solution 2

Add a new method to class AnnotFreeText
  AnnotFreeText::getAppearanceString(AppearanceStringData& apStr) const;

Fill apStr with data from parseAppearanceString (protected function)
  static void parseAppearanceString(GooString *da, double &fontsize, AnnotColor* &fontcolor);

How about it? Abert?

I don't really have an opinion of using solution 1 or solution 2 they seem similar enough, probably solution 2 is less invasive since we can keep the old functions around which is "good" for less code changes but also probably encourages people to use the "wrong" functions, so I guess I'll leave it to the choosing of whoever does the code see which of the 2 seems to end up with better code.

(In reply to Albert Astals Cid from comment #9)
> I don't really have an opinion of using solution 1 or solution 2 they seem
> similar enough, probably solution 2 is less invasive since we can keep the
> old functions around which is "good" for less code changes but also probably
> encourages people to use the "wrong" functions, so I guess I'll leave it to
> the choosing of whoever does the code see which of the 2 seems to end up
> with better code.

1 solves "creation", 2 solves "parsing". They could be done both at the same time. From what I understand you care a bit more about 2.

Dileep, any chance you spend few GSoC hours on finishing this?

I have created a draft patch here: https://cgit.kde.org/scratch/dileepsankhla/okular-gsoc2018-typewriter.git/tree/font_color_poppler_core/font_color.patch

Tobias, I have the following issues with this patch:

1) Whenever I create a typewriter annotation in the modified okular, I get "SIGABRT Error". May you please help me in identifying what and where it is happening?

2) I have hardcoded textElement.setAttribute( QStringLiteral("font"), QString("Invalid_font") ); in Poppler::TextAnnotation::store. How can I parse textFont string from the DA dict in poppler core instead of poppler-qt5?

3) What do you say about the patch? Is it neat and promising?

Please answer my queries so that I can move ahead.

(In reply to Dileep Sankhla from comment #11)
> I have created a draft patch here:
> https://cgit.kde.org/scratch/dileepsankhla/okular-gsoc2018-typewriter.git/
> tree/font_color_poppler_core/font_color.patch
I changed your patch to make it work, see https://cgit.kde.org/scratch/dileepsankhla/okular-gsoc2018-typewriter.git/plain/font_color_poppler_core/0001-Create-and-parse-DA-string-in-AnnotFreeText.patch

> 1) Whenever I create a typewriter annotation in the modified okular,
> I get "SIGABRT Error"
The crash was because pointers inside apStr were not yet initalized when createNativeAnnot was called.

> 2) I have hardcoded textElement.setAttribute( QStringLiteral("font")
Ah, noted this too late. With my patch textFont() reads from core. So you can revert your line to 
textElement.setAttribute( QStringLiteral("font"), textFont().toString() )
However, because font family is not yet saved in the PDF (only "Invalid_font" is saved) and just a default QFont is constructed, QFont::family() is probably lost on document reload. This is independent from this patch and probably happens also with released poppler versions. Did not check it.

> 3) What do you say about the patch? Is it neat and promising
There were some problems, like globals should be avoided almost always in C++, and you should use automatic memory management where possible (here we can do it by putting variables on stack), and the getters for text and color had been removed but we still need them. There's still something to do, grep for "TODO" in my patch.

Created attachment 140860 [details] [review]
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.

Thanks Tobias!
Based on this patch, the further font family implementation will be carried out.

@Tobias @Albert @Oliver If you have any issue with this patch, please help me in figuring it out as soon as possible so that I can begin to work on the font family based on the current patch in the last GSoC phase.

Comment on attachment 140860 [details] [review]
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 attachment 140860 [details] [review]:
-----------------------------------------------------------------

::: 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 bug 81748).

@@ +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.

Comment on attachment 140860 [details] [review]
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 attachment 140860 [details] [review]:
-----------------------------------------------------------------

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

http://doc.qt.io/qt-5/qcolor.html#Spec-enum

@@ +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.

Created attachment 140935 [details] [review]
This patch includes Annot.cc to create and parse DA string

Thanks @Tobias and @Albert for the review comments. I hope this patch is what you expected at least for the font color with "invalid_font" Tf.
The font family patch will be patched from this current patch for including font families in poppler.

(In reply to Dileep Sankhla from comment #16)
> Created attachment 140935 [details] [review] [review]
> This patch includes Annot.cc to create and parse DA string

You mailed me yesterday, regarding review and fixing a segfault in your yet unpublished patch. Seems I was too slow, you fixed it yourself :) I have a local patch pending anyway now...

> Thanks @Tobias and @Albert for the review comments. I hope this patch is
> what you expected at least for the font color with "invalid_font" Tf.
> The font family patch will be patched from this current patch for including
> font families in poppler.

I think it's good, there are some minor issues only, like adding a bit more const and performing a nullptr check on delete of DefaultAppearance::fontColor. It would be in my local patch, is it ok for you if we merge?

Finally, we should run available automated tests. Albert, I know how to execute poppler/qt5/tests/*. What about poppler/regtest/poppler-regtest, are there examples how to use it?

(In reply to Tobias Deiminger from comment #17)
> (In reply to Dileep Sankhla from comment #16)
> > Created attachment 140935 [details] [review] [review] [review]
> > This patch includes Annot.cc to create and parse DA string
> 
> You mailed me yesterday, regarding review and fixing a segfault in your yet
> unpublished patch. Seems I was too slow, you fixed it yourself :) I have a
> local patch pending anyway now...
> 
> > Thanks @Tobias and @Albert for the review comments. I hope this patch is
> > what you expected at least for the font color with "invalid_font" Tf.
> > The font family patch will be patched from this current patch for including
> > font families in poppler.
> 
> I think it's good, there are some minor issues only, like adding a bit more
> const and performing a nullptr check on delete of
> DefaultAppearance::fontColor. It would be in my local patch, is it ok for
> you if we merge?
Yes I think now you should produce the final patch that's able to be merged finally and please correct all the minor mistakes and submit the attachment here. I would love to watch what I missed and in this end moment, I'm working on my local fontfamily branch. Waiting for your patch :)

> 
> Finally, we should run available automated tests. Albert, I know how to
> execute poppler/qt5/tests/*. What about poppler/regtest/poppler-regtest, are
> there examples how to use it?
Now do I need to write the tests too?? What's this?

> > Finally, we should run available automated tests. Albert, I know how to
> > execute poppler/qt5/tests/*. What about poppler/regtest/poppler-regtest, are
> > there examples how to use it?
> Now do I need to write the tests too?? What's this?

Sorry, my bad. I misinterpreted it.

(In reply to Tobias Deiminger from comment #17)
> Finally, we should run available automated tests. Albert, I know how to
> execute poppler/qt5/tests/*. What about poppler/regtest/poppler-regtest, are
> there examples how to use it?

You have not changed how rendering works (or have you, i have not looked at the patch), right?

If you don't that won't help you since poppler-regtest only is about rendering stuff and making sure results don't change

Created attachment 140963 [details] [review]
Add FreeText font color and DA string creation and parsing in the Poppler core. Add new methods TextAnnotation::textColor and TextAnnotation::setTextColor to adjust the text color of FreeText annot.

Thank you Tobias Deiminger for co-authoring the patch.

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?

(In reply to Albert Astals Cid from comment #22)
>  * 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?
We model the /DA entry here. It's written "default appearance string" in PDF 1.7, P. 396. And because we do some abstraction and our model is no more a string, I thought we strip "string", and what remains is "DefaultAppearance".

I think we should not mix up /DA with /AP, even if they're similar. AP is the annotations appearance, while DA establishes context if AP is missing and we need to create a AP replacement in-memory. DA has limited set of allowed operators, while AP can do almost everything.

Does this make sense?

Btw., some other PDF libraries expose DA actually as string. See [0]. I don't think exposing as string is a good idea. Just wanted to mention it, as it might be the last time we think about that API for a while.

[0] https://pdfbox.apache.org/docs/2.0.1/javadocs/org/apache/pdfbox/pdmodel/fdf/FDFAnnotationFreeText.html#setDefaultAppearance(java.lang.String)
[1] https://github.com/gdelugre/origami/blob/98ea557af7aa9e926aac564bf89e6e0ead4a1a5e/lib/origami/annotations.rb#L309

One more possible improvement from my side
* In constructAppearanceString, we do some raw cstr = GooString::format(...) to construct an appearance string. How about using (and extending) AnnotAppearanceBuilder instead? E.g. use existing AnnotAppearanceBuilder::setDrawColor, add new AnnotAppearanceBuilder::setTextFont, ...

@Dileep: Will you implement Alberts suggestions? And/or shall I pick up some items from there?

(In reply to Tobias Deiminger from comment #23)
> Btw., some other PDF libraries expose DA actually as string.
(In reply to Tobias Deiminger from comment #24)
> How about using (and extending) AnnotAppearanceBuilder instead?

Yet another idea. We could even let Qt5 frontend use AnnotAppearanceBuilder directly (todo: extend it by some get* methods), keep the AnnotFreeText::setAppearanceString(GooString*) API, and finally pass AnnotAppearanceBuilder string to setAppearanceString. Vice versa for get.

That would preserve the flexibility of a string interface, while it keeps away the burden of dealing with low level operator parsing from the frontends.

The bad thing is that it means some more rework of the patch again.

(In reply to Albert Astals Cid from comment #22)
> * GooString *fontTag;
>   parseAppearanceString(appearanceString, fontsize, fontcolor, fontTag);
> Is this a memory leak?

If you mean fontTag:
parseAppearanceString allocates fontTag, DefaultAppearance Ctor makes a copy, and the very next line to DefaultAppearance Ctor is "delete fontTag".

If you mean fontcolor:
parseAppearanceString allocates fontcolor, DefaultAppearance takes ownership and DefaultAppearance::~DefaultAppearance will eventually care for deletion.

The latter is leaky in conjunction with the copy-Ctor issue you mentioned, we have to fix that. Besides that I don't see a leak, am I missing something?

(In reply to Tobias Deiminger from comment #26)
> (In reply to Albert Astals Cid from comment #22)
> > * GooString *fontTag;
> >   parseAppearanceString(appearanceString, fontsize, fontcolor, fontTag);
> > Is this a memory leak?
> 
> If you mean fontTag:
> parseAppearanceString allocates fontTag, DefaultAppearance Ctor makes a
> copy, and the very next line to DefaultAppearance Ctor is "delete fontTag".

I mean fontTag, but that is on line 

@@ -2872,7 +2924,8 @@ void AnnotFreeText::generateFreeTextAppearance()


fontTag is never used not passed anywhere.

(In reply to Tobias Deiminger from comment #23)
> (In reply to Albert Astals Cid from comment #22)
> >  * 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?
> We model the /DA entry here. It's written "default appearance string" in PDF
> 1.7, P. 396. And because we do some abstraction and our model is no more a
> string, I thought we strip "string", and what remains is "DefaultAppearance".
> 
> I think we should not mix up /DA with /AP, even if they're similar. AP is
> the annotations appearance, while DA establishes context if AP is missing
> and we need to create a AP replacement in-memory. DA has limited set of
> allowed operators, while AP can do almost everything.
> 
> Does this make sense?
> 
> Btw., some other PDF libraries expose DA actually as string. See [0]. I
> don't think exposing as string is a good idea. Just wanted to mention it, as
> it might be the last time we think about that API for a while.
> 
> [0]
> https://pdfbox.apache.org/docs/2.0.1/javadocs/org/apache/pdfbox/pdmodel/fdf/
> FDFAnnotationFreeText.html#setDefaultAppearance(java.lang.String)
> [1]
> https://github.com/gdelugre/origami/blob/
> 98ea557af7aa9e926aac564bf89e6e0ead4a1a5e/lib/origami/annotations.rb#L309

Ok, fair enough

(In reply to Albert Astals Cid from comment #27)
> @@ -2872,7 +2924,8 @@ void AnnotFreeText::generateFreeTextAppearance()
> fontTag is never used not passed anywhere.
Alright. Leak, definitely. Wasn't aware of that place.

How about changing parseAppearanceString signature to
void AnnotFreeText::parseAppearanceString(..., GooString** fontTag)?

Then we can pass nullptr to indicate we don't need the fontTag.
Else we had a useless new-delete cycle in generateFreeTextAppearance.

Created attachment 141142 [details] [review]
new version of the patch

New version done together with Albert at Akademy

Comment on attachment 141142 [details] [review]
new version of the patch

Review of attachment 141142 [details] [review]:
-----------------------------------------------------------------

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

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/50.