mustang-im / mustang

Mustang - New full-featured desktop email, chat and video conference client
https://mustang.im
Other
11 stars 3 forks source link

Msg view: Add context menu #192

Closed benbucksch closed 2 months ago

benbucksch commented 3 months ago

Reproduction:

Actual result:

Expected result:

benbucksch commented 3 months ago

I've tried to use electron-context-menu, but it doesn't build.

jermy-c commented 3 months ago

I tried to use electron-context-menu also for #110 and got the same error.

jermy-c commented 3 months ago

electron-vite@2.3.0 with electron@32.0.1 supports ESM output and removes the error but gives the error:

App threw an error during load
file:///Users/jeremyc/Documents/GitHub/mustang/e2/out/main/index.js:35
import { autoUpdater } from "electron-updater";
         ^^^^^^^^^^^
SyntaxError: Named export 'autoUpdater' not found. The requested module 'electron-updater' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'electron-updater';
const { autoUpdater } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadApplicationPackage (file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:129:9)
    at async file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:241:9

After doing that I get the error:

App threw an error during load
ReferenceError: __dirname is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/Users/jeremyc/Documents/GitHub/mustang/e2/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///Users/jeremyc/Documents/GitHub/mustang/e2/out/main/index.js:126492:21
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadApplicationPackage (file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:129:9)
    at async file:///Users/jeremyc/Documents/GitHub/mustang/e2/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js:241:9

It's converting to ESM but the filepaths are being converted from ESM to commonJS e.g.const icon = join$2(__dirname, "../../resources/icon.png");.

Using electron-context-menu@3.6.1 works because that's the last commonJS version. And any version after that requires Node.js 18 and Electron 30 because only version later than those support ESM.

The context menu applies to all windows, we would need to do some communications between the main and renderer processes to let it know which window to apply it.

benbucksch commented 3 months ago

That's great! Can you please make a PR that updates to electron-vite 2.3.0 and electron 32.0.1, and fixes all issues that are caused by it? You'll need to find a different way to get the current directory, instead of __dirname.

jermy-c commented 2 months ago

193 Updates the packages to output ESM and fixes any errors cause by it.

jermy-c commented 2 months ago

contextMenu() does not add context menus to <webview> tags for some reason but only the main window has the context menu.

When not specified, the context menu will be added to all existing and new windows.

benbucksch commented 2 months ago

Yes, I noticed the same. I tried to pass a webview DOM element, but that didn't work. I tried to re-use the electron-context-menu code, by using a lower level function, but it doesn't export any of the lower level functions.

However, I did manage to attach a webviewE.addEventListener("context-menu", ...) event listener, and the event.params contains the interesting info as documented. We can work off that. Note: This is in the frontend code, in our <WebView> svelte component.

So, I think it makes sense to build the context menu ourselves, with our own menu. The menu can then be built using different UI components. The trigger would be the context-menu event above. So, essentially, we only need the parts of the function that take context info, and determine the menu items to show.

It appears the best approach is to fork the module, make the code more flexible. Only a function that accepts the event.params data, and a config, and then returns a list of menu items and functions to call. Ideally, the functions (what happens when you click on a menu item) would be not be inline functions, but separate exported functions that I can override, call, or augment. By default, the menu items should call these functions, but I should be able to change that.

benbucksch commented 2 months ago

DUP of #110