Windows: Fix PDF, video, and audio rendering (#10881)

.eslintignore

@@ -476,6 +476,7 @@ packages/app-desktop/integration-tests/sidebar.spec.js
 packages/app-desktop/integration-tests/simpleBackup.spec.js
 packages/app-desktop/integration-tests/util/activateMainMenuItem.js
 packages/app-desktop/integration-tests/util/createStartupArgs.js
+packages/app-desktop/integration-tests/util/extendedExpect.js
 packages/app-desktop/integration-tests/util/firstNonDevToolsWindow.js
 packages/app-desktop/integration-tests/util/getImageSourceSize.js
 packages/app-desktop/integration-tests/util/setFilePickerResponse.js

.gitignore

@@ -453,6 +453,7 @@ packages/app-desktop/integration-tests/sidebar.spec.js
 packages/app-desktop/integration-tests/simpleBackup.spec.js
 packages/app-desktop/integration-tests/util/activateMainMenuItem.js
 packages/app-desktop/integration-tests/util/createStartupArgs.js
+packages/app-desktop/integration-tests/util/extendedExpect.js
 packages/app-desktop/integration-tests/util/firstNonDevToolsWindow.js
 packages/app-desktop/integration-tests/util/getImageSourceSize.js
 packages/app-desktop/integration-tests/util/setFilePickerResponse.js

packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts

@@ -6,7 +6,8 @@ import shim from '@joplin/lib/shim';
 
 const { themeStyle } = require('@joplin/lib/theme');
 import Note from '@joplin/lib/models/Note';
-import { MarkupToHtmlOptions } from './types';
+import { MarkupToHtmlOptions, ResourceInfos } from './types';
+import { resourceFullPath } from '@joplin/lib/models/utils/resourceUtils';
 
 interface HookDependencies {
 	themeId: number;
@@ -20,12 +21,16 @@ interface HookDependencies {
 export default function useMarkupToHtml(deps: HookDependencies) {
 	const { themeId, customCss, plugins, whiteBackgroundNoteRendering } = deps;
 
+	const resourceBaseUrl = useMemo(() => {
+		return `joplin-content://note-viewer/${Setting.value('resourceDir')}/`;
+	}, []);
+
 	const markupToHtml = useMemo(() => {
 		return markupLanguageUtils.newMarkupToHtml(plugins, {
-			resourceBaseUrl: `joplin-content://note-viewer/${Setting.value('resourceDir')}/`,
+			resourceBaseUrl,
 			customCss: customCss || '',
 		});
-	}, [plugins, customCss]);
+	}, [plugins, customCss, resourceBaseUrl]);
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	return useCallback(async (markupLanguage: number, md: string, options: MarkupToHtmlOptions = null): Promise<any> => {
@@ -39,7 +44,7 @@ export default function useMarkupToHtml(deps: HookDependencies) {
 		md = md || '';
 
 		const theme = themeStyle(themeId);
-		let resources = {};
+		let resources: ResourceInfos = {};
 
 		if (options.replaceResourceInternalToExternalLinks) {
 			md = await Note.replaceResourceInternalToExternalLinks(md, { useAbsolutePaths: true });
@@ -58,9 +63,16 @@ export default function useMarkupToHtml(deps: HookDependencies) {
 			codeHighlightCacheKey: 'useMarkupToHtml',
 			settingValue: deps.settingValue,
 			whiteBackgroundNoteRendering,
+			itemIdToUrl: (id: string, urlParameters = '') => {
+				if (!(id in resources) || !resources[id]) {
+					return null;
+				}
+
+				return resourceFullPath(resources[id].item, resourceBaseUrl) + urlParameters;
+			},
 			...options,
 		});
 
 		return result;
-	}, [themeId, markupToHtml, whiteBackgroundNoteRendering, deps.settingValue]);
+	}, [themeId, markupToHtml, whiteBackgroundNoteRendering, resourceBaseUrl, deps.settingValue]);
 }

packages/app-desktop/integration-tests/markdownEditor.spec.ts

@@ -2,6 +2,7 @@ import { test, expect } from './util/test';
 import MainScreen from './models/MainScreen';
 import { join } from 'path';
 import getImageSourceSize from './util/getImageSourceSize';
+import setFilePickerResponse from './util/setFilePickerResponse';
 
 
 test.describe('markdownEditor', () => {
@@ -27,6 +28,75 @@ test.describe('markdownEditor', () => {
 		await expect(await getImageSourceSize(image)).toMatchObject([117, 30]);
 	});
 
+	test('preview pane should render PDFs', async ({ mainWindow, electronApp }) => {
+		const mainScreen = new MainScreen(mainWindow);
+		await mainScreen.createNewNote('PDF attachments');
+		const editor = mainScreen.noteEditor;
+
+		await editor.focusCodeMirrorEditor();
+
+		await setFilePickerResponse(electronApp, [join(__dirname, 'resources', 'small-pdf.pdf')]);
+		await editor.attachFileButton.click();
+
+		const viewerFrame = mainScreen.noteEditor.getNoteViewerIframe();
+		const pdfLink = viewerFrame.getByText('small-pdf.pdf');
+		await expect(pdfLink).toBeVisible();
+
+		const expectToBeRendered = async () => {
+
+			// PDF preview should render
+			const pdfViewer = viewerFrame.locator('object[data$=".pdf"]');
+			// Should create the PDF viewer. Note: This is not sufficient to determine that the PDF viewer
+			// has rendered.
+			await expect(pdfViewer).toBeAttached();
+
+			// Verify that the PDF viewer has rendered. This relies on how Chrome/Electron loads custom PDFs
+			// in an object.
+			// If this breaks due to an Electron upgrade,
+			// 1. manually verify that the PDF viewer has loaded and
+			// 2. replace this test with a screenshot comparison (https://playwright.dev/docs/test-snapshots)
+			await expect.poll(
+				() => pdfViewer.evaluate((handle) => {
+					const embed = (handle as HTMLObjectElement).contentDocument.querySelector('embed');
+					return !!embed;
+				}),
+			).toBe(true);
+		};
+
+		await expectToBeRendered();
+
+		// Should still render after switching editors
+		await mainScreen.noteEditor.toggleEditorsButton.click();
+		await mainScreen.noteEditor.richTextEditor.waitFor();
+		await mainScreen.noteEditor.toggleEditorsButton.click();
+
+		await expectToBeRendered();
+	});
+
+	test('preview pane should render video attachments', async ({ mainWindow, electronApp }) => {
+		const mainScreen = new MainScreen(mainWindow);
+		await mainScreen.createNewNote('Media attachments');
+		const editor = mainScreen.noteEditor;
+
+		await editor.focusCodeMirrorEditor();
+		await setFilePickerResponse(electronApp, [join(__dirname, 'resources', 'video.mp4')]);
+		await editor.attachFileButton.click();
+
+		const videoLocator = editor.getNoteViewerIframe().locator('video');
+		const expectVideoToRender = async () => {
+			await expect(videoLocator).toBeSeekableMediaElement(6.9, 7);
+		};
+
+		await expectVideoToRender();
+
+		// Should be able to render again if the editor is closed and re-opened.
+		await mainScreen.noteEditor.toggleEditorsButton.click();
+		await mainScreen.noteEditor.richTextEditor.waitFor();
+		await mainScreen.noteEditor.toggleEditorsButton.click();
+
+		await expectVideoToRender();
+	});
+
 	test('arrow keys should navigate the toolbar', async ({ mainWindow }) => {
 		const mainScreen = new MainScreen(mainWindow);
 		await mainScreen.waitFor();

packages/app-desktop/integration-tests/util/extendedExpect.ts

@@ -0,0 +1,59 @@
+import { expect, Locator } from '@playwright/test';
+
+const extendedExpect = expect.extend({
+	async toBeSeekableMediaElement(videoLocator: Locator, seeksTo: number, playsUntil: number) {
+		let pass = true;
+
+		let nextAssertionStep = '';
+		const assertionName = 'toBeSeekableMediaElement';
+		let resultMessage = () => `${assertionName}: Passed`;
+
+		try {
+			await extendedExpect(videoLocator).toBeVisible();
+
+			const getDuration = () => {
+				return videoLocator.evaluate((video) => {
+					if (!(video instanceof HTMLMediaElement)) {
+						throw new Error('Not a media element');
+					}
+
+					return video.duration;
+				});
+			};
+
+			nextAssertionStep = 'Media should be long enough to seek and play';
+			await extendedExpect.poll(getDuration).toBeGreaterThanOrEqual(Math.max(seeksTo, playsUntil));
+
+			nextAssertionStep = 'Should not have a loading error';
+			await extendedExpect(videoLocator).toHaveJSProperty('error', null);
+
+			nextAssertionStep = `Should seek to ${this.utils.printExpected(seeksTo)}`;
+			await videoLocator.evaluate((video: HTMLMediaElement, playsFrom: number) => {
+				video.pause();
+				video.currentTime = playsFrom;
+			}, seeksTo);
+
+			const getCurrentTime = () => {
+				return videoLocator.evaluate((video: HTMLVideoElement) => {
+					return video.currentTime;
+				});
+			};
+			await extendedExpect.poll(getCurrentTime).toBeCloseTo(seeksTo);
+
+			nextAssertionStep = `Should play until ${this.utils.printExpected(playsUntil)}`;
+			await videoLocator.evaluate((video: HTMLMediaElement) => video.play());
+			await extendedExpect.poll(getCurrentTime).toBeGreaterThan(playsUntil);
+		} catch (error) {
+			pass = false;
+			resultMessage = () => error.toString();
+		}
+
+		return {
+			pass,
+			message: () => `${assertionName}: ${nextAssertionStep}:\n  ${resultMessage()}`,
+			name: assertionName,
+		};
+	},
+});
+
+export default extendedExpect;

packages/app-desktop/integration-tests/util/test.ts

@@ -60,5 +60,4 @@ export const test = base.extend<JoplinFixtures>({
 	},
 });
 
-
-export { expect } from '@playwright/test';
+export { default as expect } from './extendedExpect';

packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts

@@ -6,7 +6,6 @@ import resolvePathWithinDir from '@joplin/lib/utils/resolvePathWithinDir';
 import { LoggerWrapper } from '@joplin/utils/Logger';
 import * as fs from 'fs-extra';
 import { createReadStream } from 'fs';
-import { Readable } from 'stream';
 import { fromFilename } from '@joplin/lib/mime-utils';
 
 export interface CustomProtocolHandler {
@@ -14,6 +13,62 @@ export interface CustomProtocolHandler {
 	allowReadAccessToFile(path: string): { remove(): void };
 }
 
+
+// In some cases, the NodeJS built-in adapter (Readable.toWeb) closes its controller twice,
+// leading to an error dialog. See:
+// - https://github.com/nodejs/node/blob/e578c0b1e8d3dd817e692a0c5df1b97580bc7c7f/lib/internal/webstreams/adapters.js#L454
+// - https://github.com/nodejs/node/issues/54205
+// We work around this by creating a more-error-tolerant custom adapter.
+const nodeStreamToWeb = (resultStream: fs.ReadStream) => {
+	resultStream.pause();
+
+	let closed = false;
+
+	return new ReadableStream({
+		start: (controller) => {
+			resultStream.on('data', (chunk) => {
+				if (closed) {
+					return;
+				}
+
+				if (Buffer.isBuffer(chunk)) {
+					controller.enqueue(new Uint8Array(chunk));
+				} else {
+					controller.enqueue(chunk);
+				}
+
+				if (controller.desiredSize <= 0) {
+					resultStream.pause();
+				}
+			});
+
+			resultStream.on('error', (error) => {
+				controller.error(error);
+			});
+
+			resultStream.on('end', () => {
+				if (!closed) {
+					closed = true;
+					controller.close();
+				}
+			});
+		},
+		pull: (_controller) => {
+			if (closed) {
+				return;
+			}
+
+			resultStream.resume();
+		},
+		cancel: () => {
+			if (!closed) {
+				closed = true;
+				resultStream.close();
+			}
+		},
+	}, { highWaterMark: resultStream.readableHighWaterMark });
+};
+
 // Allows seeking videos.
 // See https://github.com/electron/electron/issues/38749 for why this is necessary.
 const handleRangeRequest = async (request: Request, targetPath: string) => {
@@ -42,7 +97,7 @@ const handleRangeRequest = async (request: Request, targetPath: string) => {
 	}
 
 	// Note: end is inclusive.
-	const resultStream = Readable.toWeb(createReadStream(targetPath, { start: startByte, end: endByte }));
+	const resultStream = createReadStream(targetPath, { start: startByte, end: endByte });
 
 	// See the HTTP range requests guide: https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests
 	const headers = new Headers([
@@ -52,10 +107,9 @@ const handleRangeRequest = async (request: Request, targetPath: string) => {
 		['Content-Range', `bytes ${startByte}-${endByte}/${stat.size}`],
 	]);
 
+
 	return new Response(
-		// This cast is necessary -- .toWeb produces a different type
-		// from the global ReadableStream.
-		resultStream as ReadableStream,
+		nodeStreamToWeb(resultStream),
 		{ headers, status: 206 },
 	);
 };

packages/renderer/MdToHtml/renderMedia.ts

@@ -16,7 +16,12 @@ export interface Options {
 }
 
 function resourceUrl(resourceFullPath: string): string {
-	if (resourceFullPath.indexOf('http://') === 0 || resourceFullPath.indexOf('https://')) return resourceFullPath;
+	if (
+		resourceFullPath.indexOf('http://') === 0 || resourceFullPath.indexOf('https://') === 0 || resourceFullPath.indexOf('joplin-content://') === 0 ||
+		resourceFullPath.indexOf('file://') === 0
+	) {
+		return resourceFullPath;
+	}
 	return `file://${toForwardSlashes(resourceFullPath)}`;
 }
 

packages/renderer/types.ts

@@ -1,8 +1,7 @@
 import { MarkupLanguage } from './MarkupToHtml';
 import { Options as NoteStyleOptions } from './noteStyle';
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-export type ItemIdToUrlHandler = (resource: any)=> string;
+export type ItemIdToUrlHandler = (resourceId: string, urlParameters?: string)=> string;
 
 interface ResourceEntity {
 	id: string;

packages/renderer/utils.ts

@@ -171,8 +171,10 @@ export const imageReplacement = function(ResourceModel: OptionsResourceModel, ma
 	if (ResourceModel.isSupportedImageMimeType(mime)) {
 		let newSrc = '';
 
-		if (itemIdToUrl) {
-			newSrc = itemIdToUrl(resource.id);
+		const timestampParameter = `?t=${resource.updated_time}`;
+		const idToUrlResult = itemIdToUrl?.(resource.id, timestampParameter) ?? null;
+		if (idToUrlResult !== null) {
+			newSrc = idToUrlResult;
 		} else {
 			const temp = [];
 
@@ -183,7 +185,7 @@ export const imageReplacement = function(ResourceModel: OptionsResourceModel, ma
 			}
 
 			temp.push(ResourceModel.filename(resource));
-			temp.push(`?t=${resource.updated_time}`);
+			temp.push(timestampParameter);
 
 			newSrc = temp.join('');
 		}