Don't emit unused Helvetica

[ssssota]

Helvetica font is referenced when I don't use it.
Like this:

• sample code

(async () => {
  const url = 'https://pdf-lib.js.org/assets/ubuntu/Ubuntu-R.ttf'
  const fontBytes = await require('node-fetch')(url).then(res => res.arrayBuffer())
  const pdfDoc = await require('pdf-lib').PDFDocument.create()
  pdfDoc.registerFontkit(require('@pdf-lib/fontkit'))
  pdfDoc.addPage().drawText('This is text in an embedded font!', {
    x: 40, y: 450, size: 35,
    font: await pdfDoc.embedFont(fontBytes),
  })
  require('fs').writeFileSync('sample.pdf', await pdfDoc.save())
})()

• sample.pdf font properties(with Acrobat Reader DC)
  

[Hopding]

Hello @ssssota! Thanks so much for working on this! Apologies for my late response. I've been very busy these past few months taking care of some personal stuff and starting my new job at GitHub 🙂

I'm hoping to get around to reviewing this PR in the next few weeks now that I've got some more free time.

Please note that you'll need to either update this PR (via a force push) or open a new PR with your changes. The tl;dr is that I recently had to do a fairly extensive git history rewrite that caused a bunch of conflicts. Please read https://github.com/Hopding/pdf-lib#git-history-rewrite for details. Apologies for the hassle!

[ssssota]

Hi @Hopding!
I tried update via a force push. Please check it!

[Hopding]

You're right that embedding the helvetica font when a custom one is supplied isn't ideal, so I like the idea of fixing that. However, this implementation pushes a lot of confusing conditional logic and non-null assertions into drawPage. What do you think of this approach? It should solve the problem but keep things separated a bit better:

diff --git a/src/api/PDFPage.ts b/src/api/PDFPage.ts
index 51215c89..ce2116b1 100644
--- a/src/api/PDFPage.ts
+++ b/src/api/PDFPage.ts
@@ -893,14 +893,12 @@ export default class PDFPage {
     assertOrUndefined(options.wordBreaks, 'options.wordBreaks', [Array]);
     assertIsOneOfOrUndefined(options.blendMode, 'options.blendMode', BlendMode);
 
-    const [originalFont] = this.getFont();
-    if (options.font) this.setFont(options.font);
-    const [font, fontKey] = this.getFont();
+    const { oldFont, newFont, newFontKey } = this.setOrEmbedFont(options.font);
 
     const fontSize = options.size || this.fontSize;
 
     const wordBreaks = options.wordBreaks || this.doc.defaultWordBreaks;
-    const textWidth = (t: string) => font.widthOfTextAtSize(t, fontSize);
+    const textWidth = (t: string) => newFont.widthOfTextAtSize(t, fontSize);
     const lines =
       options.maxWidth === undefined
         ? lineSplit(cleanText(text))
@@ -908,7 +906,7 @@ export default class PDFPage {
 
     const encodedLines = new Array(lines.length) as PDFHexString[];
     for (let idx = 0, len = lines.length; idx < len; idx++) {
-      encodedLines[idx] = font.encodeText(lines[idx]);
+      encodedLines[idx] = newFont.encodeText(lines[idx]);
     }
 
     const graphicsStateKey = this.maybeEmbedGraphicsState({
@@ -920,7 +918,7 @@ export default class PDFPage {
     contentStream.push(
       ...drawLinesOfText(encodedLines, {
         color: options.color ?? this.fontColor,
-        font: fontKey,
+        font: newFontKey,
         size: fontSize,
         rotate: options.rotate ?? degrees(0),
         xSkew: options.xSkew ?? degrees(0),
@@ -932,7 +930,10 @@ export default class PDFPage {
       }),
     );
 
-    if (options.font) this.setFont(originalFont);
+    if (options.font) {
+      if (oldFont) this.setFont(oldFont);
+      else this.resetFont();
+    }
   }
 
   /**
@@ -1450,6 +1451,19 @@ export default class PDFPage {
     this.drawEllipse({ ...options, xScale: size, yScale: size });
   }
 
+  private setOrEmbedFont(font?: PDFFont) {
+    const oldFont = this.font;
+    const oldFontKey = this.fontKey;
+
+    if (font) this.setFont(font);
+    else this.getFont();
+
+    const newFont = this.font!;
+    const newFontKey = this.fontKey!;
+
+    return { oldFont, oldFontKey, newFont, newFontKey };
+  }
+
   private getFont(): [PDFFont, string] {
     if (!this.font || !this.fontKey) {
       const font = this.doc.embedStandardFont(StandardFonts.Helvetica);
@@ -1458,6 +1472,11 @@ export default class PDFPage {
     return [this.font!, this.fontKey!];
   }
 
+  private resetFont(): void {
+    this.font = undefined;
+    this.fontKey = undefined;
+  }
+
   private getContentStream(useExisting = true): PDFContentStream {
     if (useExisting && this.contentStream) return this.contentStream;
     this.contentStream = this.createContentStream();

[github-actions]

This PR is stale because it has been open 3 weeks with no activity. It will be be closed in 2 days unless there is new activity. See MAINTAINERSHIP.md#pull-requests for details.

[ssssota]

@Hopding
I'm sorry for the late response.
And thank you for the suggestion! I think it's very good!

I applied your suggestion, please check it!

[Hopding]

Looks great. Thanks @ssssota!