update-electron-app icon indicating copy to clipboard operation
update-electron-app copied to clipboard

feat: add custom dialog option

Open ArthurLobopro opened this issue 2 years ago • 10 comments

Hello, how you merged the PR #105 I rewrite the PR #97 to avoid merge conflicts. I am unable to test the feature on windows or macos so if someone can test it I will be grateful :)

ArthurLobopro avatar Oct 23 '23 01:10 ArthurLobopro

I think I added all the missing semicolons, did I forget someone?

ArthurLobopro avatar Nov 07 '23 22:11 ArthurLobopro

@erickzhao I think the most people that want to change something on the dialog only wants to change the update message to something different (Like the Issue #116 ) or in another language (this is my case).

Somehow, a way to do my own custom dialog can be interesting I think. How would you implements that?

ArthurLobopro avatar Nov 08 '23 01:11 ArthurLobopro

I think the possibility to localize the text is a reasonable feature, and should probably be supported without requiring users to re-implement the dialog all together.

For customizing the dialog more deeply, though, we could look at providing a hook, say IUpdateElectronAppOptions .onNotifyUser which takes a function. Then we could export a makeUserNotifier(options: IDialogMessages), which has this code moved to it and returns a function.

https://github.com/electron/update-electron-app/blob/06373904910b96aebd72f31434f81d0184693906/src/index.ts#L186-L202

Then to do localization the user would do:

updateElectronApp({
  ...,
  onNotifyUser: makeUserNotifier({ 
    restartButtonText: 'Restart!!'
  })
})

And if they wanted to more deeply customize the notify they have a hook to do whatever they'd like:

updateElectronApp({
  ...,
  onNotifyUser: (event, releaseNotes, releaseName, releaseDate, updateURL) => {
    if (releaseName === 'AwesomeRelease') {
      // Do fireworks
    }
  }
})

dsanders11 avatar Nov 08 '23 02:11 dsanders11

@dsanders11 great idea! @erickzhao what do you think about that?

ArthurLobopro avatar Nov 08 '23 02:11 ArthurLobopro

I realize if we provide the hook we will lose the logger context, because the logger is declared inside the updateElectronApp function.

https://github.com/electron/update-electron-app/blob/06373904910b96aebd72f31434f81d0184693906/src/index.ts#L154-L156

To solve it I think we can add a listener just to log it:

 autoUpdater.on(
      'update-downloaded',
      (event, releaseNotes, releaseName, releaseDate, updateURL) => {
        log('update-downloaded', [event, releaseNotes, releaseName, releaseDate, updateURL]);
      },
    );
    
 autoUpdater.on(
  'update-downloaded',
  (event, releaseNotes, releaseName, releaseDate, updateURL) => { 
      const { title, restartButtonText, laterButtonText, detail } = opts.dialog;

      const dialogOpts = {
        type: 'info',
        buttons: [restartButtonText, laterButtonText],
        title,
        message: process.platform === 'win32' ? releaseNotes : releaseName,
        detail,
     };

    dialog.showMessageBox(dialogOpts).then(({ response }) => {
        if (response === 0) autoUpdater.quitAndInstall();
    });
  },);

ArthurLobopro avatar Nov 08 '23 14:11 ArthurLobopro

I found another thing.

When we create the dialog, we pick the dialog API from electron in the options

https://github.com/electron/update-electron-app/blob/06373904910b96aebd72f31434f81d0184693906/src/index.ts#L124

https://github.com/electron/update-electron-app/blob/06373904910b96aebd72f31434f81d0184693906/src/index.ts#L132

But to create a separated function to create the notifier, maybe we will need to import directly from electron.

ArthurLobopro avatar Nov 08 '23 14:11 ArthurLobopro

When we create the dialog, we pick the dialog API from electron in the options

It looks like this is just an undocumented option for testing purposes (I'm not familiar with this code base, but that's my take from a quick read), so it could similarly be an undocumented option on IDialogMessages, or we could tweak the signature of the proposed onNotifyUser to receive an object and it could be an undocumented value there:

updateElectronApp({
  ...,
  onNotifyUser: (info) => {
    const { event, releaseNotes, releaseName, releaseDate, updateURL } = info;
    // info.electron is an undocumented value for tests
    if (releaseName === 'AwesomeRelease') {
      // Do fireworks
    }
  }
})

dsanders11 avatar Nov 08 '23 18:11 dsanders11

I don't know why this error happens. The Electron namespace is already referenced here:

https://github.com/electron/update-electron-app/blob/1c9e970c0785f25b9d6528eae0dcd35204c8c08d/src/index.ts#L207

ArthurLobopro avatar Nov 09 '23 01:11 ArthurLobopro

Can someone test it on Windows or Macos? I am using Linux on my PC.

ArthurLobopro avatar Nov 09 '23 01:11 ArthurLobopro

Do we had any progress here?

ArthurLobopro avatar Aug 21 '24 13:08 ArthurLobopro