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

[WIP] OS Notification on update downloaded and minor refactor #194

Closed
wants to merge 5 commits into from

Conversation

AlphaStyle
Copy link
Contributor

fix 165

Enabling and disabling OS Notification requires restart.
Moved center-on-primary-display.js and notify.js to a shared helpers folder in the root directory.

@hql287 Could you test if this actually works? Anything you would like to improve?

Copy link
Owner

@hql287 hql287 left a comment

Choose a reason for hiding this comment

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

@AlphaStyle Sorry for taking a bit long to get back to you. I just tried your PR, please see my comments below. Thanks! :)

@@ -185,6 +185,7 @@ function setInitialValues() {
language: 'en',
sound: 'default',
muted: false,
notification: true,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is not necessary. The notification should always be on, don't you think?

const appConfig = require('electron-settings');

const Notify = options => {
const permission = appConfig.get('general.notification');
Copy link
Owner

Choose a reason for hiding this comment

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

We also should remove the conditional check here

@@ -69,6 +71,7 @@ autoUpdater.on('download-progress', progressObj => {

// Update Downloaded
autoUpdater.on('update-downloaded', info => {
Notify({ title: 'Manta Update', body: 'Updates has been downloaded' });
Copy link
Owner

Choose a reason for hiding this comment

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

This needs translation. Please refers to PR #187 for implementation detail of i18next

@@ -239,6 +240,7 @@ function migrateData() {
language: appSettings.language,
sound: appSettings.sound,
muted: appSettings.muted,
notification: appSettings.notification,
Copy link
Owner

@hql287 hql287 Feb 2, 2018

Choose a reason for hiding this comment

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

Also, this is actually not needed as notification is a new value.

@hql287
Copy link
Owner

hql287 commented Feb 2, 2018

This should be an easy fix and I figured it would be easier to make a new PR, plus I haven't heard back from you so I'm closing this in favour of #201.

@hql287 hql287 closed this Feb 2, 2018
@AlphaStyle
Copy link
Contributor Author

I am sorry for not responding! I know it is somewhat rude to start a PR and then dont follow up, and I appolegies for this. Most of the time I can only code during the weekends.

@hql287
Copy link
Owner

hql287 commented Feb 4, 2018

@AlphaStyle Don't worry, it's all good. I understand :)

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.

Integrate system Notification with autoUpdater
2 participants