Chore: Refactor WebViewController (#13133)

packages/app-desktop/integration-tests/models/MainScreen.ts

@@ -72,4 +72,10 @@ export default class MainScreen {
 		await setFilePickerResponse(electronApp, [path]);
 		await activateMainMenuItem(electronApp, 'HTML - HTML document (Directory)', 'Import');
 	}
+
+	public async pluginPanelLocator(pluginId: string) {
+		return this.page.locator(
+			`iframe[id^=${JSON.stringify(`plugin-view-${pluginId}`)}]`,
+		);
+	}
 }

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

@@ -45,6 +45,41 @@ test.describe('pluginApi', () => {
 		}));
 	});
 
+	test('should report the correct visibility state for dialogs', async ({ startAppWithPlugins }) => {
+		const { app, mainWindow } = await startAppWithPlugins(['resources/test-plugins/dialogs.js']);
+		const mainScreen = await new MainScreen(mainWindow).setup();
+		await mainScreen.createNewNote('Dialog test note');
+
+		const editor = mainScreen.noteEditor;
+		const expectVisible = async (visible: boolean) => {
+			// Check UI visibility
+			if (visible) {
+				await expect(mainScreen.dialog).toBeVisible();
+			} else {
+				await expect(mainScreen.dialog).not.toBeVisible();
+			}
+
+			// Check visibility reported through the plugin API
+			await expect.poll(async () => {
+				await mainScreen.goToAnything.runCommand(app, 'getTestDialogVisibility');
+
+				const editorContent = await editor.contentLocator();
+				return editorContent.textContent();
+			}).toBe(JSON.stringify({
+				visible: visible,
+				active: visible,
+			}));
+		};
+		await expectVisible(false);
+
+		await mainScreen.goToAnything.runCommand(app, 'showTestDialog');
+		await expectVisible(true);
+
+		// Submitting the dialog should include form data in the output
+		await mainScreen.dialog.getByRole('button', { name: 'Okay' }).click();
+		await expectVisible(false);
+	});
+
 	test('should be possible to create multiple toasts with the same text from a plugin', async ({ startAppWithPlugins }) => {
 		const { app, mainWindow } = await startAppWithPlugins(['resources/test-plugins/showToast.js']);
 		const mainScreen = await new MainScreen(mainWindow).setup();
@@ -122,5 +157,30 @@ test.describe('pluginApi', () => {
 		await msleep(Second);
 		await expect(noteEditor.codeMirrorEditor).toHaveText(expectedUpdatedText);
 	});
+
+	test('should support hiding and showing panels', async ({ startAppWithPlugins }) => {
+		const { mainWindow, app } = await startAppWithPlugins(['resources/test-plugins/panels.js']);
+		const mainScreen = await new MainScreen(mainWindow).setup();
+		await mainScreen.createNewNote('Test note (panels)');
+
+		const panelLocator = await mainScreen.pluginPanelLocator('org.joplinapp.plugins.example.panels');
+
+		const noteEditor = mainScreen.noteEditor;
+		await mainScreen.goToAnything.runCommand(app, 'testShowPanel');
+		await expect(noteEditor.codeMirrorEditor).toHaveText('visible');
+
+		// Panel should be visible
+		await expect(panelLocator).toBeVisible();
+		// The panel should have the expected content
+		const panelContent = panelLocator.contentFrame();
+		await expect(
+			panelContent.getByRole('heading', { name: 'Panel content' }),
+		).toBeAttached();
+
+		await mainScreen.goToAnything.runCommand(app, 'testHidePanel');
+		await expect(noteEditor.codeMirrorEditor).toHaveText('hidden');
+
+		await expect(panelLocator).not.toBeVisible();
+	});
 });
 

packages/app-desktop/integration-tests/resources/test-plugins/dialogs.js

@@ -47,5 +47,22 @@ joplin.plugins.register({
 				}));
 			},
 		});
+
+		await joplin.commands.register({
+			name: 'getTestDialogVisibility',
+			label: 'Returns the dialog visibility state',
+			execute: async () => {
+				// panels.visible should also work for dialogs.
+				const visible = await joplin.views.panels.visible(dialogHandle);
+				// For dialogs, isActive should return the visibility.
+				// (Prefer panels.visible for dialogs).
+				const active = await joplin.views.panels.isActive(dialogHandle);
+
+				await joplin.commands.execute('editor.setText', JSON.stringify({
+					visible,
+					active,
+				}));
+			},
+		});
 	},
 });

packages/app-desktop/integration-tests/resources/test-plugins/panels.js

@@ -0,0 +1,71 @@
+// Allows referencing the Joplin global:
+/* eslint-disable no-undef */
+
+// Allows the `joplin-manifest` block comment:
+/* eslint-disable multiline-comment-style */
+
+/* joplin-manifest:
+{
+	"id": "org.joplinapp.plugins.example.panels",
+	"manifest_version": 1,
+	"app_min_version": "3.1",
+	"name": "JS Bundle test",
+	"description": "JS Bundle Test plugin",
+	"version": "1.0.0",
+	"author": "",
+	"homepage_url": "https://joplinapp.org"
+}
+*/
+
+const waitFor = async (condition) => {
+	const wait = () => {
+		return new Promise(resolve => {
+			setTimeout(() => resolve(), 100);
+		});
+	};
+	for (let i = 0; i < 100; i++) {
+		if (await condition()) {
+			return;
+		}
+
+		// Pause for a brief delay
+		await wait();
+	}
+
+	throw new Error('Condition was never true');
+};
+
+joplin.plugins.register({
+	onStart: async function() {
+		const panels = joplin.views.panels;
+		const view = await panels.create('panelTestView');
+		await panels.setHtml(view, '<h1>Panel content</h1><p>Test</p>');
+		await panels.hide(view);
+
+
+		await joplin.commands.register({
+			name: 'testShowPanel',
+			label: 'Test panel visibility',
+			execute: async () => {
+				await panels.show(view);
+				await waitFor(async () => {
+					return await panels.visible(view);
+				});
+				await joplin.commands.execute('editor.setText', 'visible');
+			},
+		});
+
+		await joplin.commands.register({
+			name: 'testHidePanel',
+			label: 'Test: Hide the panel',
+			execute: async () => {
+				await panels.hide(view);
+				await waitFor(async () => {
+					return !await panels.visible(view);
+				});
+
+				await joplin.commands.execute('editor.setText', 'hidden');
+			},
+		});
+	},
+});

packages/lib/commands/showEditorPlugin.ts

@@ -44,7 +44,7 @@ export const runtime = (): CommandRuntime => {
 				throw new Error(`No controller registered for editor view ${editorView.id}`);
 			}
 
-			const previousVisible = editorView.parentWindowId === windowId && controller.isVisible();
+			const previousVisible = editorView.parentWindowId === windowId && controller.visible;
 
 			if (show && previousVisible) {
 				logger.info(`Editor is already visible: ${editorViewId}`);
@@ -68,7 +68,7 @@ export const runtime = (): CommandRuntime => {
 			};
 			Setting.setValue('plugins.shownEditorViewIds', getUpdatedShownViewIds());
 
-			controller.setOpened(show);
+			await controller.setOpen(show);
 		},
 	};
 };

packages/lib/services/plugins/WebviewController.ts

@@ -179,8 +179,8 @@ export default class WebviewController extends ViewController {
 	public emitUpdate(event: EditorUpdateEvent) {
 		if (!this.updateListener_) return;
 
-		if (this.containerType_ === ContainerType.Editor && (!this.isActive() || !this.isVisible())) {
+		if (this.containerType_ === ContainerType.Editor && (!this.active || !this.visible)) {
-			logger.info('emitMessage: Not emitting update because editor is disabled or hidden:', this.pluginId, this.handle, this.isActive(), this.isVisible());
+			logger.info('emitMessage: Not emitting update because editor is disabled or hidden:', this.pluginId, this.handle, this.active, this.visible);
 			return;
 		}
 
@@ -196,34 +196,10 @@ export default class WebviewController extends ViewController {
 		this.updateListener_ = callback;
 	}
 
-	// ---------------------------------------------
-	// Specific to panels
-	// ---------------------------------------------
-
-	private showWithAppLayout() {
-		return this.containerType === ContainerType.Panel && !!this.store.getState().mainLayout;
-	}
-
-	public async show(show = true): Promise<void> {
-		if (this.showWithAppLayout()) {
-			this.store.dispatch({
-				type: 'MAIN_LAYOUT_SET_ITEM_PROP',
-				itemKey: this.handle,
-				propName: 'visible',
-				propValue: show,
-			});
-		} else {
-			this.setStoreProp('opened', show);
-		}
-	}
-
-	public async hide(): Promise<void> {
-		return this.show(false);
-	}
-
 	public get visible(): boolean {
 		const appState = this.store.getState();
 
+		if (this.containerType_ === ContainerType.Panel) {
 			// Mobile: There is no appState.mainLayout
 			if (!this.showWithAppLayout()) {
 				return this.storeView.opened;
@@ -232,44 +208,91 @@ export default class WebviewController extends ViewController {
 			const mainLayout = appState.mainLayout;
 			const item = findItemByKey(mainLayout, this.handle);
 			return item ? item.visible : false;
+		} else if (this.containerType_ === ContainerType.Editor) {
+			const state = this.storeView as PluginEditorViewState;
+			return state.active && state.opened;
+		} else if (this.containerType_ === ContainerType.Dialog) {
+			return this.storeView.opened;
 		}
 
-	// ---------------------------------------------
+		const exhaustivenessCheck: never = this.containerType_;
-	// Specific to dialogs
+		throw new Error(`Unknown container type: ${exhaustivenessCheck}`);
-	// ---------------------------------------------
+	}
 
-	public async open(): Promise<DialogResult> {
+	public setOpen(show = true): null|Promise<DialogResult|null> {
+		this.setStoreProp('opened', show);
+
+		if (this.containerType_ === ContainerType.Panel) {
+			if (this.showWithAppLayout()) {
+				this.store.dispatch({
+					type: 'MAIN_LAYOUT_SET_ITEM_PROP',
+					itemKey: this.handle,
+					propName: 'visible',
+					propValue: show,
+				});
+			}
+			return null;
+		} else if (this.containerType_ === ContainerType.Dialog) {
 			if (this.closeResponse_) {
 				this.closeResponse_.resolve(null);
 				this.closeResponse_ = null;
 			}
 
+			if (show) {
 				this.store.dispatch({
 					type: 'VISIBLE_DIALOGS_ADD',
 					name: this.handle,
 				});
 
-		this.setStoreProp('opened', true);
+				return new Promise<DialogResult>((resolve, reject) => {
-
-		// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
-		return new Promise((resolve: Function, reject: Function) => {
 					this.closeResponse_ = { resolve, reject };
 				});
-	}
+			} else {
-
-	public close() {
 				this.store.dispatch({
 					type: 'VISIBLE_DIALOGS_REMOVE',
 					name: this.handle,
 				});
 
-		this.setStoreProp('opened', false);
+				return null;
+			}
+		} else if (this.containerType_ === ContainerType.Editor) {
+			return null;
+		}
+
+		const exhaustivenessCheck: never = this.containerType_;
+		throw new Error(`Unknown container type: ${exhaustivenessCheck}`);
+	}
+
+	public async hide(): Promise<void> {
+		await this.setOpen(false);
+	}
+
+	// ---------------------------------------------
+	// Specific to panels
+	// ---------------------------------------------
+
+	private showWithAppLayout() {
+		return this.containerType === ContainerType.Panel && !!this.store.getState().mainLayout;
+	}
+
+
+	// ---------------------------------------------
+	// Specific to dialogs
+	// ---------------------------------------------
+
+	public close() {
+		void this.setOpen(false);
 	}
 
 	public closeWithResponse(result: DialogResult) {
-		this.close();
+		const responseCallback = this.closeResponse_;
-		this.closeResponse_.resolve(result);
+		// Clear the closeResponse_ to prevent the default behavior
+		// (which sends a response of null).
 		this.closeResponse_ = null;
+
+		this.close();
+
+		responseCallback?.resolve(result);
 	}
 
 	public get buttons(): ButtonSpec[] {
@@ -300,20 +323,16 @@ export default class WebviewController extends ViewController {
 		this.setStoreProp('active', active);
 	}
 
-	public isActive(): boolean {
+	public get active(): boolean {
+		// For compatibility with older versions of Joplin
+		if (this.containerType_ !== ContainerType.Editor) {
+			return this.visible;
+		}
+
 		const state = this.storeView as PluginEditorViewState;
 		return state.active;
 	}
 
-	public setOpened(visible: boolean) {
-		this.setStoreProp('opened', visible);
-	}
-
-	public isVisible(): boolean {
-		const state = this.storeView as PluginEditorViewState;
-		return state.active && state.opened;
-	}
-
 	public async requestSaveNote(event: SaveNoteEvent) {
 		if (!this.saveNoteListener_) {
 			logger.warn('Note save requested, but no save handler was registered. View ID: ', this.storeView?.id);

packages/lib/services/plugins/api/JoplinViewsDialogs.ts

@@ -125,7 +125,7 @@ export default class JoplinViewsDialogs {
 	 * On desktop, this closes any copies of the dialog open in different windows.
 	 */
 	public async open(handle: ViewHandle): Promise<DialogResult> {
-		return this.controller(handle).open();
+		return this.controller(handle).setOpen(true);
 	}
 
 	/**

packages/lib/services/plugins/api/JoplinViewsEditor.ts

@@ -90,7 +90,7 @@ export default class JoplinViewsEditors {
 			controller.setEditorTypeId(editorTypeId);
 			this.plugin.addViewController(controller);
 			// Restore the last open/closed state for the editor
-			controller.setOpened(Setting.value('plugins.shownEditorViewIds').includes(editorTypeId));
+			void controller.setOpen(Setting.value('plugins.shownEditorViewIds').includes(editorTypeId));
 
 			return () => {
 				this.plugin.removeViewController(controller);
@@ -271,7 +271,7 @@ export default class JoplinViewsEditors {
 	 * Tells whether the editor is active or not.
 	 */
 	public async isActive(handle: ViewHandle): Promise<boolean> {
-		return this.controller(handle).isActive();
+		return this.controller(handle).active;
 	}
 
 	/**
@@ -280,7 +280,7 @@ export default class JoplinViewsEditors {
 	 * `true`. Otherwise it will return `false`.
 	 */
 	public async isVisible(handle: ViewHandle): Promise<boolean> {
-		return this.controller(handle).isVisible();
+		return this.controller(handle).visible;
 	}
 
 }

packages/lib/services/plugins/api/JoplinViewsPanels.ts

@@ -114,7 +114,7 @@ export default class JoplinViewsPanels {
 	 * Shows the panel
 	 */
 	public async show(handle: ViewHandle, show = true): Promise<void> {
-		await this.controller(handle).show(show);
+		await this.controller(handle).setOpen(show);
 	}
 
 	/**
@@ -136,7 +136,7 @@ export default class JoplinViewsPanels {
 	 * whether the editor plugin view supports editing the current note.
 	 */
 	public async isActive(handle: ViewHandle): Promise<boolean> {
-		return this.controller(handle).isActive();
+		return this.controller(handle).active;
 	}
 
 }