Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Embedded Pdf Viewer #6681

Merged
merged 16 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ packages/tools/node_modules
packages/tools/PortableAppsLauncher
packages/turndown-plugin-gfm/
packages/turndown/
packages/pdf-viewer/dist
plugin_types/
readme/

Expand Down Expand Up @@ -1909,6 +1910,27 @@ packages/lib/uuid.js.map
packages/lib/versionInfo.d.ts
packages/lib/versionInfo.js
packages/lib/versionInfo.js.map
packages/pdf-viewer/Page.d.ts
packages/pdf-viewer/Page.js
packages/pdf-viewer/Page.js.map
packages/pdf-viewer/VerticalPages.d.ts
packages/pdf-viewer/VerticalPages.js
packages/pdf-viewer/VerticalPages.js.map
packages/pdf-viewer/hooks/useIsFocused.d.ts
packages/pdf-viewer/hooks/useIsFocused.js
packages/pdf-viewer/hooks/useIsFocused.js.map
packages/pdf-viewer/hooks/useIsVisible.d.ts
packages/pdf-viewer/hooks/useIsVisible.js
packages/pdf-viewer/hooks/useIsVisible.js.map
packages/pdf-viewer/miniViewer.d.ts
packages/pdf-viewer/miniViewer.js
packages/pdf-viewer/miniViewer.js.map
packages/pdf-viewer/pdfSource.d.ts
packages/pdf-viewer/pdfSource.js
packages/pdf-viewer/pdfSource.js.map
packages/pdf-viewer/pdfSource.test.d.ts
packages/pdf-viewer/pdfSource.test.js
packages/pdf-viewer/pdfSource.test.js.map
packages/plugin-repo-cli/commands/updateRelease.d.ts
packages/plugin-repo-cli/commands/updateRelease.js
packages/plugin-repo-cli/commands/updateRelease.js.map
Expand Down
21 changes: 21 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,27 @@ packages/lib/uuid.js.map
packages/lib/versionInfo.d.ts
packages/lib/versionInfo.js
packages/lib/versionInfo.js.map
packages/pdf-viewer/Page.d.ts
packages/pdf-viewer/Page.js
packages/pdf-viewer/Page.js.map
packages/pdf-viewer/VerticalPages.d.ts
packages/pdf-viewer/VerticalPages.js
packages/pdf-viewer/VerticalPages.js.map
packages/pdf-viewer/hooks/useIsFocused.d.ts
packages/pdf-viewer/hooks/useIsFocused.js
packages/pdf-viewer/hooks/useIsFocused.js.map
packages/pdf-viewer/hooks/useIsVisible.d.ts
packages/pdf-viewer/hooks/useIsVisible.js
packages/pdf-viewer/hooks/useIsVisible.js.map
packages/pdf-viewer/miniViewer.d.ts
packages/pdf-viewer/miniViewer.js
packages/pdf-viewer/miniViewer.js.map
packages/pdf-viewer/pdfSource.d.ts
packages/pdf-viewer/pdfSource.js
packages/pdf-viewer/pdfSource.js.map
packages/pdf-viewer/pdfSource.test.d.ts
packages/pdf-viewer/pdfSource.test.js
packages/pdf-viewer/pdfSource.test.js.map
packages/plugin-repo-cli/commands/updateRelease.d.ts
packages/plugin-repo-cli/commands/updateRelease.js
packages/plugin-repo-cli/commands/updateRelease.js.map
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,8 @@ function CodeMirror(props: NoteBodyEditorProps, ref: any) {
resourceInfos: props.resourceInfos,
contentMaxWidth: props.contentMaxWidth,
mapsToLine: true,
// Always using useCustomPdfViewer for now, we can add a new setting for it in future if we need to.
useCustomPdfViewer: true,
laurent22 marked this conversation as resolved.
Show resolved Hide resolved
}));

if (cancelled) return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export interface MarkupToHtmlOptions {
plugins?: Record<string, any>;
bodyOnly?: boolean;
mapsToLine?: boolean;
useCustomPdfViewer?: boolean;
}

export default function useMarkupToHtml(deps: HookDependencies) {
Expand Down
11 changes: 11 additions & 0 deletions packages/app-desktop/gui/note-viewer/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,17 @@
e.preventDefault();
}));

document.addEventListener('click', webviewLib.logEnabledEventHandler(e => {
document.querySelectorAll('.media-pdf').forEach(element => {
if(!!element.contentWindow){
element.contentWindow.postMessage({
type: 'blur'
}, '*');
}
}
);
}));

let lastClientWidth_ = NaN, lastClientHeight_ = NaN, lastScrollTop_ = NaN;

window.addEventListener('resize', webviewLib.logEnabledEventHandler(() => {
Expand Down
1 change: 1 addition & 0 deletions packages/app-desktop/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@
"@fortawesome/fontawesome-free": "^5.13.0",
"@joeattardi/emoji-button": "^4.6.0",
"@joplin/lib": "~2.9",
"@joplin/pdf-viewer": "~2.9",
"@joplin/renderer": "~2.9",
"async-mutex": "^0.1.3",
"codemirror": "^5.56.0",
Expand Down
83 changes: 51 additions & 32 deletions packages/app-desktop/tools/compileScripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,46 +13,65 @@ function fileIsNewerThan(path1, path2) {
return stat1.mtime > stat2.mtime;
}

function convertJsx(path) {
function convertJsx(paths) {
chdir(`${__dirname}/..`);

fs.readdirSync(path).forEach((filename) => {
const jsxPath = `${path}/${filename}`;
const p = jsxPath.split('.');
if (p.length <= 1) return;
const ext = p[p.length - 1];
if (ext !== 'jsx') return;
p.pop();

const basePath = p.join('.');

const jsPath = `${basePath}.min.js`;

if (fileIsNewerThan(jsxPath, jsPath)) {
console.info(`Compiling ${jsxPath}...`);

// { shell: true } is needed to get it working on Windows:
// https://discourse.joplinapp.org/t/attempting-to-build-on-windows/22559/12
const result = spawnSync('yarn', ['run', 'babel', '--presets', 'react', '--out-file', jsPath, jsxPath], { shell: true });
if (result.status !== 0) {
const msg = [];
if (result.stdout) msg.push(result.stdout.toString());
if (result.stderr) msg.push(result.stderr.toString());
console.error(msg.join('\n'));
if (result.error) console.error(result.error);
process.exit(result.status);
paths.forEach(path => {
fs.readdirSync(path).forEach((filename) => {
const jsxPath = `${path}/${filename}`;
const p = jsxPath.split('.');
if (p.length <= 1) return;
const ext = p[p.length - 1];
if (ext !== 'jsx') return;
p.pop();

const basePath = p.join('.');

const jsPath = `${basePath}.min.js`;

if (fileIsNewerThan(jsxPath, jsPath)) {
console.info(`Compiling ${jsxPath}...`);

// { shell: true } is needed to get it working on Windows:
// https://discourse.joplinapp.org/t/attempting-to-build-on-windows/22559/12
const result = spawnSync('yarn', ['run', 'babel', '--presets', 'react', '--out-file', jsPath, jsxPath], { shell: true });
if (result.status !== 0) {
const msg = [];
if (result.stdout) msg.push(result.stdout.toString());
if (result.stderr) msg.push(result.stderr.toString());
console.error(msg.join('\n'));
if (result.error) console.error(result.error);
process.exit(result.status);
}
}
}
});
});
}

function build(path) {
chdir(path);

const result = spawnSync('yarn', ['run', 'build'], { shell: true });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that you were calling yarn from here... Did you notice that launching the dev desktop app now takes more than 20 seconds, while before it would take less than 2 seconds?

Please fix this as soon as possible - any build step needs to happen when running yarn install from the root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for this, I did notice it before but didn't know it's a wrong approach. I was thinking of optimising the built time for pdf-viewer instead.
I have posted a fix for it now: #6762

if (result.status !== 0) {
const msg = [];
if (result.stdout) msg.push(result.stdout.toString());
if (result.stderr) msg.push(result.stderr.toString());
console.error(msg.join('\n'));
if (result.error) console.error(result.error);
process.exit(result.status);
}
}
Comment on lines +54 to +63
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you refactor convertJsx so as not to have all this duplicated code?


module.exports = function() {
convertJsx(`${__dirname}/../gui`);
convertJsx(`${__dirname}/../gui/MainScreen`);
convertJsx(`${__dirname}/../gui/NoteList`);
convertJsx(`${__dirname}/../plugins`);
convertJsx([
`${__dirname}/../gui`,
`${__dirname}/../gui/MainScreen`,
`${__dirname}/../gui/NoteList`,
`${__dirname}/../plugins`,
]);

build(`${__dirname}/../../pdf-viewer`);

// TODO: should get from node_modules @joplin/lib
const libContent = [
fs.readFileSync(`${basePath}/packages/lib/string-utils-common.js`, 'utf8'),
fs.readFileSync(`${basePath}/packages/lib/markJsUtils.js`, 'utf8'),
Expand Down
8 changes: 8 additions & 0 deletions packages/app-desktop/tools/copyApplicationAssets.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ async function main() {
src: langSourceDir,
dest: `${buildLibDir}/tinymce/langs`,
},
{
src: resolve(__dirname, '../../pdf-viewer/dist'),
dest: `${buildLibDir}/@joplin/pdf-viewer`,
},
];

const files = [
Expand All @@ -87,6 +91,10 @@ async function main() {
src: resolve(__dirname, '../../lib/services/plugins/sandboxProxy.js'),
dest: `${buildLibDir}/@joplin/lib/services/plugins/sandboxProxy.js`,
},
{
src: resolve(__dirname, '../../pdf-viewer/index.html'),
dest: `${buildLibDir}/@joplin/pdf-viewer/index.html`,
},
];

// First we delete all the destination directories, then we copy the files.
Expand Down
1 change: 1 addition & 0 deletions packages/pdf-viewer/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
dist/*
100 changes: 100 additions & 0 deletions packages/pdf-viewer/Page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import React, { useEffect, useRef, useState } from 'react';
import useIsVisible from './hooks/useIsVisible';
import { PdfData, ScaledSize } from './pdfSource';
import useAsyncEffect, { AsyncEffectEvent } from '@joplin/lib/hooks/useAsyncEffect';

require('./pages.css');

export interface PageProps {
pdf: PdfData;
pageNo: number;
focusOnLoad: boolean;
isAnchored: boolean;
scaledSize: ScaledSize;
isDarkTheme: boolean;
container: React.MutableRefObject<HTMLElement>;
}


export default function Page(props: PageProps) {
const [error, setError] = useState(null);
const [page, setPage] = useState(null);
const [scale, setScale] = useState(null);
const [timestamp, setTimestamp] = useState(null);
const canvasRef = useRef<HTMLCanvasElement>(null);
const wrapperRef = useRef<HTMLDivElement>(null);
const isVisible = useIsVisible(canvasRef, props.container);

useEffect(() => {
if (!isVisible || !page || !props.scaledSize || (scale && props.scaledSize.scale === scale)) return;
try {
const viewport = page.getViewport({ scale: props.scaledSize.scale || 1.0 });
const canvas = canvasRef.current;
canvas.width = viewport.width;
canvas.height = viewport.height;
const ctx = canvas.getContext('2d');
const pageTimestamp = new Date().getTime();
setTimestamp(pageTimestamp);
page.render({
canvasContext: ctx,
viewport,
// Used so that the page rendering is throttled to some extent.
// https://stackoverflow.com/questions/18069448/halting-pdf-js-page-rendering
continueCallback: function(cont: any) {
if (timestamp !== pageTimestamp) {
return;
}
cont();
},
});
setScale(props.scaledSize.scale);

} catch (error) {
error.message = `Error rendering page no. ${props.pageNo}: ${error.message}`;
setError(error);
throw error;
}
}, [page, props.scaledSize, isVisible]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always pass the absolute necessary dependencies and nothing more. In this case, I think you only need props.scaledSize.scale. And page.getViewport({ scale: props.scaledSize.scale || 1.0 }) I believe. Doing so prevents unnecessary rerenders.


useAsyncEffect(async (event: AsyncEffectEvent) => {
if (page || !isVisible || !props.pdf) return;
try {
const _page = await props.pdf.getPage(props.pageNo);
if (event.cancelled) return;
setPage(_page);
} catch (error) {
console.error('Page load error', props.pageNo, error);
setError(error);
}
}, [page, props.scaledSize, isVisible]);

useEffect(() => {
if (props.focusOnLoad) {
props.container.current.scrollTop = wrapperRef.current.offsetTop;
// console.warn('setting focus on page', props.pageNo, wrapperRef.current.offsetTop);
}
}, [props.focusOnLoad]);

let style: any = {};
if (props.scaledSize) {
style = {
...style,
width: props.scaledSize.width,
height: props.scaledSize.height,
};
}

return (
<div className="page-wrapper" ref={wrapperRef} style={style}>
<canvas ref={canvasRef} className="page-canvas" style={style}>
<div>
{error ? 'ERROR' : 'Loading..'}
</div>
Page {props.pageNo}
</canvas>
<div className="page-info">
{props.isAnchored ? '📌' : ''} Page {props.pageNo}
</div>
</div>
);
}
17 changes: 17 additions & 0 deletions packages/pdf-viewer/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# PDF VIEWER

//Todo

## Usage

```javascript
import viewer from '@joplin/pdf-viewer';
```

## Notes

//Todo

## License

MIT
Loading