Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add custom dialog option #126

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ArthurLobopro
Copy link

@ArthurLobopro ArthurLobopro commented Oct 23, 2023

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 :)

Closes #116

@ArthurLobopro ArthurLobopro requested a review from a team as a code owner October 23, 2023 01:54
@erikian erikian mentioned this pull request Oct 23, 2023
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs semicolons to match coding style.

@ArthurLobopro
Copy link
Author

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

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general feedback on the approach:

@ArthurLobopro have you considered instead exposing an API that allows users to hook in their own custom dialog code?

I wonder if a more generalized approach would be better than adding individual API options for the individual buttons.

@ArthurLobopro
Copy link
Author

@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?

@dsanders11 dsanders11 self-requested a review November 8, 2023 01:45
@dsanders11
Copy link
Member

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.

(event, releaseNotes, releaseName, releaseDate, updateURL) => {
log('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();
});
},

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
    }
  }
})

@ArthurLobopro
Copy link
Author

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

@ArthurLobopro
Copy link
Author

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

function log(...args: any[]) {
logger.log(...args);
}

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
Copy link
Author

I found another thing.

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

const { updateSource, updateInterval, logger, electron } = opts;

const { app, autoUpdater, dialog } = electron;

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

@erickzhao erickzhao self-assigned this Nov 8, 2023
@dsanders11
Copy link
Member

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
    }
  }
})

@ArthurLobopro
Copy link
Author

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

const electron: typeof Electron.Main = (opts as any).electron || require('electron');

@ArthurLobopro
Copy link
Author

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

@ArthurLobopro
Copy link
Author

Do we had any progress here?

@doctorXWrites
Copy link

Any update here? I'm looking for some way to be able to show release notes in update dialog.

@ArthurLobopro
Copy link
Author

@doctorXWrites Just waiting for approval, but maybe the reviewer forgot this PR because Github stops showing PRs with no activity after 2 weeks.

I will mark he here to confirm this

@dsanders11

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few things to change that I'm planning on pushing up. Sorry for the wait @ArthurLobopro!

@ArthurLobopro
Copy link
Author

@erickzhao No problem, I'm available to help if necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How do I change the update message?
5 participants