automerge / pushpin

A collaborative corkboard app
BSD 3-Clause "New" or "Revised" License
625 stars 53 forks source link

Issue with some pdfs #369

Open Gozala opened 4 years ago

Gozala commented 4 years ago

For whatever reason pushpin seems to have issue with a following pdf https://tools.ietf.org/pdf/rfc2557.pdf

pvh commented 4 years ago

I've seen this before. I see a ton of errors in the console like this:

Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_).".
C:\Users\pvh\Dev\pushpin\node_modules\pdfjs-dist\build\pdf.js:549 Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_".".
C:\Users\pvh\Dev\pushpin\node_modules\pdfjs-dist\build\pdf.js:549 Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_z.".
C:\Users\pvh\Dev\pushpin\node_modules\pdfjs-dist\build\pdf.js:549 Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_[.".
C:\Users\pvh\Dev\pushpin\node_modules\pdfjs-dist\build\pdf.js:549 Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_F.".
C:\Users\pvh\Dev\pushpin\node_modules\pdfjs-dist\build\pdf.js:549 Warning: getPathGenerator - ignoring character: "Error: Requesting object that isn't resolved yet Courier_path_8.".

I think this is a bug in how I've set up PDF.js and remember seeing something about it in the FAQ. Hopefully any easy fix with easy confirmation.

Gozala commented 4 years ago

I tried hypothesis on that pdf as I recall them using pdf.js and it seems to work fine in their embedding https://via.hypothes.is/https://tools.ietf.org/pdf/rfc2557.pdf#annotations:zUk5UDM2Eeq1FYMkzSVmuw

Gozala commented 4 years ago

I suppose that is their embedding https://github.com/hypothesis/pdf.js-hypothes.is

Gozala commented 4 years ago

I made some changes to try and follow instructions for cmaps here https://www.npmjs.com/package/react-pdf

diff --git a/webpack.config.ts b/webpack.config.ts
index ab3abd69..bb1c18ff 100644
--- a/webpack.config.ts
+++ b/webpack.config.ts
@@ -239,6 +239,12 @@ export default [
         chunks: ['background'],
         filename: 'background.html',
       }),
+      new CopyPlugin([
+        {
+          from: 'node_modules/pdfjs-dist/cmaps/',
+          to: 'assets/pdfjs-cmaps/',
+        },
+      ]),
       ...(isDev
         ? [
             new HardSourcePlugin({
diff --git a/src/renderer/components/content-types/files/PdfContent.tsx b/src/renderer/components/content-types/files/PdfContent.tsx
index bcf633ce..ac0d1907 100644
--- a/src/renderer/components/content-types/files/PdfContent.tsx
+++ b/src/renderer/components/content-types/files/PdfContent.tsx
@@ -118,7 +118,14 @@ export default function PdfContent(props: ContentProps) {
     <>
       {header}
       {buffer ? (
-        <Document file={{ data: buffer }} onLoadSuccess={onDocumentLoadSuccess}>
+        <Document
+          file={{ data: buffer }}
+          onLoadSuccess={onDocumentLoadSuccess}
+          options={{
+            cMapUrl: '/assets/pdfjs-cmaps/',
+            cMapPacked: true,
+          }}
+        >
           <Page
             loading=""
             pageNumber={pageNum}

But I see no requests for cmaps and issues still seem to be present.

Gozala commented 4 years ago

This seems relevant https://github.com/wojtekmaj/react-pdf/issues/275

Gozala commented 4 years ago

Tried following to see if the problem was with electron:

document.documentElement.innerHTML = "<iframe style='width: 100%;height:100%;top:0;left:0;position:fixed;'src='https://via.hypothes.is/https://tools.ietf.org/pdf/rfc2557.pdf#annotations:zUk5UDM2Eeq1FYMkzSVmuw'>"

Seems to work as expected and there seems to be no requests to cmap files

Gozala commented 4 years ago

I am now suspecting that hypothesis might be doing this https://github.com/mozilla/pdf.js/issues/4244#issuecomment-463865708

Gozala commented 4 years ago

Surprisingly setting a following setting disableFontFace: false makes it work. I don't see a way to switch pages but that might be unrelated.

Gozala commented 4 years ago

This does not seem ideal as per docs https://mozilla.github.io/pdf.js/api/draft/module-pdfjsLib.html

By default fonts are converted to OpenType fonts and loaded via font face rules. If disabled, fonts will be rendered using a built-in font renderer that constructs the glyphs with primitive path commands. The default value is false.

Gozala commented 4 years ago

Some poking around debugger also showed that font that causes error is 'Courier' with 'monospace' fallback. However there seems to be no compiled glimpse and that is why those errors are logged.