Created attachment 132730 [details] [review]
Added methods to get and set the font size of text fields

Created attachment 132756 [details] [review]
Added methods to get and set the font size of text fields

I realized that I also had to change the variable defaultAppearance and call
updateChildrenAppearance() afterwards.

Created attachment 133195 [details] [review]
Added methods to get and set the font size of text fields

New patch adapted to changes from the merged better_object branch.

AnnotFreeText::parseAppearanceString is doing very similar parsing to what you're doing.

Can you can try to generalize/reuse it? Having two sets of code that do the same in in slightly different ways is not a good idea.

Created attachment 133569 [details] [review]
Added methods to get and set the font size of text fields

I changed AnnotFreeText::parseAppearanceString() to use a new static method
FormFieldText::tokenizeDA() from Form.h.

Created attachment 133570 [details] [review]
Added methods to get and set the font size of text fields

Fixes a bug in the last patch.

int FormFieldText::tokenizeDA(GooString* da, GooList* daToks)

returning the position of "Tf" is weird API.

I understand you don't want to search in daToks again since you just searched into it.

To make it less bad i guess you could add a const char * parameter saying "and in addition i want to look for this", is the same but makes the API a bit more saner.

What do you think?

Created attachment 133832 [details] [review]
Added methods to get and set the font size of text fields

I changed the weird API according your suggestion.

Any reason you're using isspaces instead of Lexer::isSpace?

In FormFieldText::setTextFontSize if idx is -1 we should give a warning and skip doing all the work for nothing, no? Should we even return false in that case?

Also i'd prefer if we return -1 in  FormFieldText::getTextFontSize if the font size is not found, seems error return like for "it failed" than 0, that while a weird size i can see some cases in which it is "correct-ish"

(In reply to Albert Astals Cid from comment #9)
> Any reason you're using isspaces instead of Lexer::isSpace?

No special reason, I just used isspace() from libc because I'm familiar with it.
If Lexer::isSpace() is equivalent and the preferred way to do it in poppler,
I'll replace isspace() with it.

> 
> In FormFieldText::setTextFontSize if idx is -1 we should give a warning and
> skip doing all the work for nothing, no? Should we even return false in that
> case?

Ok, I'll do that.

> 
> Also i'd prefer if we return -1 in  FormFieldText::getTextFontSize if the
> font size is not found, seems error return like for "it failed" than 0, that
> while a weird size i can see some cases in which it is "correct-ish"

Ok, that reasonable.

(In reply to Hans-Ulrich Jüttner from comment #10)
> (In reply to Albert Astals Cid from comment #9)
> > Any reason you're using isspaces instead of Lexer::isSpace?
> 
> No special reason, I just used isspace() from libc because I'm familiar with
> it.
> If Lexer::isSpace() is equivalent and the preferred way to do it in poppler,
> I'll replace isspace() with it.

Well, it's what AnnotFreeText::parseAppearanceString was using so yes, let's continue using it unless you have a strong reason not to :)

Created attachment 133987 [details] [review]
Added methods to get and set the font size of text fields

New patch with the changes discussed above.

Do you have any app you plan to use this?

(In reply to Albert Astals Cid from comment #13)
> Do you have any app you plan to use this?

Yes, I'm planning to use this in an application of my company filling
PDF forms from the german health system. The rules for these forms say
that the font size on some multi line text fields has to be chosen
depending on the length of the text filled into that fields.

Ok, pushed, i'd be happier if i could try this on an existing application.

Please note that poppler is GPL and that carries some obligations for the application you develop, i'm sure you already know but just a kind reminder :)

(In reply to Albert Astals Cid from comment #15)
> Ok, pushed, i'd be happier if i could try this on an existing application.
> 
> Please note that poppler is GPL and that carries some obligations for the
> application you develop, i'm sure you already know but just a kind reminder
> :)

Yes, of course, as we only use this laboratory information system ourselves,
we do comply with the GPL of poppler and other libraries.