New Embedded Pdf Viewer #6681

asrient · 2022-07-20T08:07:25Z

Major changes made

Set up a new package pdf-viewer.
Update desktop app build process to build and copy the pdf-viewer assets to vender directory.
Update renderMedia to produce an iframe pointing to the new pdf viewer asset files.
Built the embedded viewer using react and pdf.js and using webpack for build.
Unit tests.

Embedded viewer features implemented in this PR

Lazy scroll: render only those pages that is visible to the user at a time.
Page linking support (anchor): always show a particular page first using resource-id#page-no format.
Dynamic page dimension: adopt the page size in accordance with the iframe and window size on resize.
Prevent accidental scrolls: By default at first the viewer's scroll is disabled, click on anywhere on the viewer to enable it, click outside of the viewer to disable it again.

Things not there yet

Support for dynamic themes, currently there are only 2 modes: light/dark (changes according to the theme type).
Context menu or options like setting a page as the anchor from ui.
State persistence in between note-viewer re-renders on note update.
Setting to toggle between custom/electron viewer.

Demo

Lazy loading of pages

Theming

Window resize - dynamic page dimension

Page Anchoring (linking)

Reference links

https://discourse.joplinapp.org/t/coding-phase-week-5-report/26531
https://discourse.joplinapp.org/t/coding-phase-week-4-report/26446
https://discourse.joplinapp.org/t/coding-phase-week-2-and-3-report/26339
https://discourse.joplinapp.org/t/coding-period-week-1-report/26146
https://discourse.joplinapp.org/t/improve-pdf-previewer/25822

…ewer

roman-r-m · 2022-07-20T10:03:39Z

packages/renderer/MdToHtml/renderMedia.ts

@@ -39,6 +41,16 @@ export default function(link: Link, options: Options) {
 	}

 	if (options.pdfViewerEnabled && resource.mime === 'application/pdf') {
+		let anchorPageNo = null;


this and the next few lines could probably go inside the if (options.useCustomPdfViewer) { condition as they aren't used outside of it

laurent22 · 2022-07-21T10:57:01Z

That seems like a lot of hassle to have a separate package for this. Couldn't you just put it under app-desktop/gui? That way we don't need all that build setup.

asrient · 2022-07-21T11:23:27Z

Couldn't you just put it under app-desktop/gui? That way we don't need all that build setup.

I tried doing that at first but figured out since we will be using an iframe, we need to produce a single js bundle for the viewer and app-desktop does not currently use any such bundler like webpack.

Because of the way note is rendered, the note body is converted to html first and then rendered inside an iframe, thus there is no access to react or other node modules. Putting it in a separate packages also makes the viewer more modular, ability to use other modules such as react and lastly we will be able to reuse majority of the code for the fullscreen viewer.

laurent22 · 2022-07-21T12:06:58Z

Thanks for clarifying, it makes sense. We need to make sure it's all setup correctly without having extra manual steps - if you start from a clean repository, then run yarn install from the root, and yarn start from packages/app-desktop, does it work?

Also I believe you should set "private": true in package.json because we don't need to publish this package separately.

And check setupNewRelease.ts - you need to ensure that the package version is automatically updated, and include it in app-desktop using version ~2.9

Finally include build instructions in the README file of the package if you haven't already done so.

asrient · 2022-07-21T13:31:08Z

Alright I will make the changes.

roman-r-m · 2022-07-21T19:49:45Z

Not sure if I did something wrong but after checking out the PR and running yarn install from root followed by yarn start from packages/app-desktop I still seem to have the old viewer.

asrient · 2022-07-22T04:45:18Z

Not sure if I did something wrong but after checking out the PR and running yarn install from root followed by yarn start from packages/app-desktop I still seem to have the old viewer.

That's weird, I just tried doing a fresh install for this branch, I didn't face this issue. If you could take a look if there were any errors or recheck if it has the latest code.

You can also check if:

packages/pdf-viewer exists.
packages/pdf-viewer/dist contains few generated js bundle files.

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/CodeMirror.tsx

laurent22 · 2022-07-22T10:19:06Z

packages/app-desktop/package.json

@@ -138,6 +138,7 @@
    "@fortawesome/fontawesome-free": "^5.13.0",
    "@joeattardi/emoji-button": "^4.6.0",
    "@joplin/lib": "~2.9",
+    "@joplin/pdf-viewer": "~1.0",


Remember it should be ~2.9. Having all the packages at the same version makes it easier to make deployments.

laurent22 · 2022-07-22T10:22:34Z

packages/app-desktop/tools/compileScripts.js

+	const result = spawnSync('yarn', ['run', 'build'], { 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);
+	}
+}


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

laurent22 · 2022-07-22T10:23:19Z

packages/pdf-viewer/Page.tsx

+	const [page, setPage] = useState(null);
+	const [scale, setScale] = useState(null);
+	const [timestamp, setTimestamp] = useState(null);
+	const canvasEl = useRef<HTMLCanvasElement>(null);


laurent22 · 2022-07-22T10:23:31Z

packages/pdf-viewer/Page.tsx

+	const [scale, setScale] = useState(null);
+	const [timestamp, setTimestamp] = useState(null);
+	const canvasEl = useRef<HTMLCanvasElement>(null);
+	const wrapperEl = useRef<HTMLDivElement>(null);


wrapperRef (for consistency)

packages/pdf-viewer/pdfSource.test.ts

packages/pdf-viewer/pdfSource.ts

packages/renderer/MdToHtml.ts

roman-r-m · 2022-07-22T19:56:09Z

After running yarn clean and yarn install again, the viewer is now working.
Looks good to me, however it's not using the full page width. I think it'd be nice to do it so that it's more readable.

laurent22 · 2022-07-28T15:58:40Z

What's the status of this PR?

asrient · 2022-07-28T16:17:22Z

What's the status of this PR?

Hi, sorry for the delay, was facing a few issues with my local install. Will be pushing the updates soon.

…ewer

laurent22 · 2022-07-30T12:17:03Z

I think it's fine now. Please resolve the conflict so that we can merge

…ewer

roman-r-m · 2022-07-30T18:22:25Z

I think it's fine now. Please resolve the conflict so that we can merge

I think this pr makes the new editor enabled by default and there's no option to switch it off. Are you sure you want to merge like this?

asrient · 2022-07-30T18:26:24Z

I think this pr makes the new editor enabled by default and there's no option to switch it off. Are you sure you want to merge like this?

I think having a setting for this and having it disabled by default would be a safer option for now. If you think the same, please give me some more time to add the feature too.

laurent22 · 2022-07-30T19:09:06Z

Hmm, I think it's ok to enable it by default because it's not too critical and it will be in the pre-release first. Or are there any particular issues you're concerned about? Or any known bug?

The flag to switch it off is just for us in case there's some major issue we cannot solve quickly. In that case we can switch it off and release a new version, which gives us time to work on a fix.

roman-r-m · 2022-07-30T19:19:14Z

Nothing in particular.

One thing that has just came to mind - how will printing work with the new viewer? I've not tried it and can't test until tomorrow afternoon

roman-r-m · 2022-08-02T20:16:02Z

Laurent. this looks good to me btw but I'll let you to merge this when you see fit.

laurent22 · 2022-08-04T09:12:13Z

Ok thanks, let's merge then.

laurent22 · 2022-08-11T17:26:39Z

@asrient, some feedback on Discord:

I don't like the new pdf viewer, I found it having reduced resolution, no zoom and no thumbnails...

https://discord.com/channels/610762468960239627/610762468960239629/1007309877305159802

This is actually a good point - at a minimum we need feature parity with the previous default PDF viewer, otherwise it's a regression.

Do you think this is possible? In any case, I would suggest you make a list of features of the default viewer, and then a comparison with the new viewer, so that we have some visibility on what's missing. Maybe some features aren't too much an issue, but others might be. Please let me know if you're ok with this.

asrient · 2022-08-11T17:36:04Z

Hi, since this pr was focused to bring the basic new custom embedded viewer, it admittedly lacks some features that are required to call it complete.

I have already added a few features like zoom and print and will be creating pr soon for them subsequently. I will list the formal comparison here in some time.

But one thing, I don't plan on adding a thumbnail view that the previous version had in the embedded viewer, it will be part of the full screen version.

laurent22 · 2022-08-11T17:39:37Z

Thanks for the update. When the list is ready could you put it in your forum category please?

asrient · 2022-08-11T17:45:09Z

Thanks for the update. When the list is ready could you put it in your forum category please?

Sure, will do that.

laurent22 · 2022-08-18T20:33:04Z

packages/app-desktop/tools/compileScripts.js

+function build(path) {
+	chdir(path);
+
+	const result = spawnSync('yarn', ['run', 'build'], { shell: true });


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.

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

asrient added 12 commits June 22, 2022 01:05

Setup pdf-viewer package and update build system

e0fc7ef

basic embedded viewer with lazy loading scroll

8dbc16d

vertical scroll, add page link support

b348134

update file structure, make page width dynamic

d559ae3

added pdf-viewer tests

f6da847

added ref types

1d073f3

added a new test

dce508d

Update CodeMirror.tsx

aad6ed5

Merge branch 'dev' of https://github.com/laurent22/joplin into pdf-vi…

daa7e34

…ewer

Update package.json

fdb6904

updated pdf.js imports

299aa04

Update .eslintignore

ba3b878

roman-r-m reviewed Jul 20, 2022

View reviewed changes

laurent22 reviewed Jul 22, 2022

View reviewed changes

asrient added 3 commits July 29, 2022 17:49

updates based on comments

deba4ec

Merge branch 'dev' of https://github.com/laurent22/joplin into pdf-vi…

aca4a2c

…ewer

update tests

4be0bec

Merge branch 'dev' of https://github.com/laurent22/joplin into pdf-vi…

fc2094d

…ewer

laurent22 merged commit 6ea40c9 into laurent22:dev Aug 4, 2022

laurent22 reviewed Aug 18, 2022

View reviewed changes

asrient mentioned this pull request Aug 18, 2022

update pdf-viewer build #6762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Embedded Pdf Viewer #6681

New Embedded Pdf Viewer #6681

asrient commented Jul 20, 2022 •

edited

Loading

roman-r-m Jul 20, 2022

laurent22 commented Jul 21, 2022

asrient commented Jul 21, 2022 •

edited

Loading

laurent22 commented Jul 21, 2022 •

edited

Loading

asrient commented Jul 21, 2022 •

edited

Loading

roman-r-m commented Jul 21, 2022

asrient commented Jul 22, 2022

laurent22 Jul 22, 2022

laurent22 Jul 22, 2022

laurent22 Jul 22, 2022

laurent22 Jul 22, 2022

roman-r-m commented Jul 22, 2022 •

edited

Loading

laurent22 commented Jul 28, 2022

asrient commented Jul 28, 2022 •

edited

Loading

laurent22 commented Jul 30, 2022

roman-r-m commented Jul 30, 2022

asrient commented Jul 30, 2022 •

edited

Loading

laurent22 commented Jul 30, 2022 •

edited

Loading

roman-r-m commented Jul 30, 2022

roman-r-m commented Aug 2, 2022

laurent22 commented Aug 4, 2022

laurent22 commented Aug 11, 2022

asrient commented Aug 11, 2022

laurent22 commented Aug 11, 2022

asrient commented Aug 11, 2022

laurent22 Aug 18, 2022

asrient Aug 18, 2022

New Embedded Pdf Viewer #6681

New Embedded Pdf Viewer #6681

Conversation

asrient commented Jul 20, 2022 • edited Loading

Major changes made

Embedded viewer features implemented in this PR

Things not there yet

Demo

Lazy loading of pages

Theming

Window resize - dynamic page dimension

Page Anchoring (linking)

Reference links

roman-r-m Jul 20, 2022

Choose a reason for hiding this comment

laurent22 commented Jul 21, 2022

asrient commented Jul 21, 2022 • edited Loading

laurent22 commented Jul 21, 2022 • edited Loading

asrient commented Jul 21, 2022 • edited Loading

roman-r-m commented Jul 21, 2022

asrient commented Jul 22, 2022

laurent22 Jul 22, 2022

Choose a reason for hiding this comment

laurent22 Jul 22, 2022

Choose a reason for hiding this comment

laurent22 Jul 22, 2022

Choose a reason for hiding this comment

laurent22 Jul 22, 2022

Choose a reason for hiding this comment

roman-r-m commented Jul 22, 2022 • edited Loading

laurent22 commented Jul 28, 2022

asrient commented Jul 28, 2022 • edited Loading

laurent22 commented Jul 30, 2022

roman-r-m commented Jul 30, 2022

asrient commented Jul 30, 2022 • edited Loading

laurent22 commented Jul 30, 2022 • edited Loading

roman-r-m commented Jul 30, 2022

roman-r-m commented Aug 2, 2022

laurent22 commented Aug 4, 2022

laurent22 commented Aug 11, 2022

asrient commented Aug 11, 2022

laurent22 commented Aug 11, 2022

asrient commented Aug 11, 2022

laurent22 Aug 18, 2022

Choose a reason for hiding this comment

asrient Aug 18, 2022

Choose a reason for hiding this comment

asrient commented Jul 20, 2022 •

edited

Loading

asrient commented Jul 21, 2022 •

edited

Loading

laurent22 commented Jul 21, 2022 •

edited

Loading

asrient commented Jul 21, 2022 •

edited

Loading

roman-r-m commented Jul 22, 2022 •

edited

Loading

asrient commented Jul 28, 2022 •

edited

Loading

asrient commented Jul 30, 2022 •

edited

Loading

laurent22 commented Jul 30, 2022 •

edited

Loading