jgraph / drawio-desktop

Official electron build of draw.io
https://www.diagrams.net
Apache License 2.0
50.95k stars 5.03k forks source link

Multiple close confirmation handlers #1894

Open FarSeeing opened 1 month ago

FarSeeing commented 1 month ago

Preflight Checklist

You must agree to search and the code of conduct. You must fill in this entire template. If you delete part/all or miss parts out your issue will be closed.

Describe the bug When closing a main window several times the confirmation modal is shown multiple times.

To Reproduce Steps to reproduce the behavior:

  1. Open the app
  2. Open or create a new diagram
  3. Modify that diagram
  4. Close the main window by clicking the close button several times
  5. If you're the fastest hand on the Wild West fast enough - the confirmation modal would be shown multiple times

Expected behavior Confirmation modal is shown once

Screenshots If applicable, add screenshots to help explain your problem.

draw.io version (In the Help->About menu of the draw.io editor):

Desktop (please complete the following information):

Additional context There is a check to display modal once by adding a unique id to each close handler but it doesn't work. The lifecycle is close event - isModified-result - another close - another isModified-result so this unique id passes the check.

A proposal solution ```diff index 76fa3e8..a59fa7f 100644 --- a/src/main/electron.js +++ b/src/main/electron.js @@ -269,11 +269,13 @@ function createWindow (opt = {}) mainWindow.webContents.send('resize') }); - let uniqueIsModifiedId; + let isModifiedInProgress; ipcMain.on('isModified-result', async (e, data) => { - if (!validateSender(e.senderFrame) || uniqueIsModifiedId != data.uniqueId) return null; + if (!validateSender(e.senderFrame)) return null; + + isModifiedInProgress = true; if (data.isModified) { @@ -316,23 +318,29 @@ function createWindow (opt = {}) { mainWindow.destroy(); } + + isModifiedInProgress = false; }); mainWindow.on('close', (event) => { - uniqueIsModifiedId = Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); - if (__DEV__) { const index = windowsRegistry.indexOf(mainWindow) - console.log('Window on close', index, uniqueIsModifiedId) + console.log('Window on close', index, isModifiedInProgress) + } + + if (isModifiedInProgress) + { + event.preventDefault(); + return; } const contents = mainWindow.webContents if (contents != null) { - contents.send('isModified', uniqueI - sModifiedId); + contents.send('isModified'); event.preventDefault(); } ```

And a sidenote that the close even handler should probably handle a case when window contents have not loaded the JS (because of a CSP error maybe) and thus cannot reply with a isModified-result event.