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

Add appID to fix Windows notification management #2483

Merged
merged 3 commits into from
Nov 21, 2020

Conversation

codebykat
Copy link
Member

@codebykat codebykat commented Nov 19, 2020

Fix

Looks like we missed a step in setting up notifications for Windows. Right now Simplenote doesn't show up in the notifications manager on Windows, and if you opt-out you can't re-enable them.

Fixes #2482

On Windows 10, a shortcut to your app with an Application User Model ID must be installed to the Start Menu. This can be overkill during development, so adding node_modules\electron\dist\electron.exe to your Start Menu also does the trick. Navigate to the file in Explorer, right-click and 'Pin to Start Menu'. You will then need to add the line app.setAppUserModelId(process.execPath) to your main process to see notifications.

https://www.electronjs.org/docs/tutorial/notifications#windows

Related: electron/electron#10864 and electron/electron#24330

Test

  1. After receiving at least one notification from Simplenote
  2. On Windows, go to Settings>System>Notifications & actions
  3. Verify that Simplenote appears under "Get notifications from these senders"
  4. Toggle off sounds and verify that only the banner appears, with no sound

Release

Fixed a bug causing the app to be missing from Windows 10 notification settings.

@codebykat
Copy link
Member Author

Oh hey, my second try actually worked!

Screen Shot 2020-11-19 at 3 09 43 PM

Screen Shot 2020-11-19 at 3 09 55 PM

@codebykat codebykat requested a review from a team November 19, 2020 23:12
@codebykat codebykat self-assigned this Nov 19, 2020
@codebykat codebykat marked this pull request as ready for review November 19, 2020 23:12
@codebykat codebykat added this to the 2.3.0 milestone Nov 19, 2020
desktop/app.js Outdated
@@ -28,6 +28,8 @@ module.exports = function main() {
let mainWindow = null;
let isAuthenticated;

app.on('ready', () => app.setAppUserModelId('com.automattic.simplenote'));
Copy link
Member

Choose a reason for hiding this comment

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

looks great but would it benefit us to put it down at the end with the other app.on('ready', …) hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

HAHA okay

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept it as two function calls for symmetry's sake 31ff235

@codebykat codebykat merged commit 96dc858 into develop Nov 21, 2020
@codebykat codebykat deleted the try/fix-windows-notifications branch November 21, 2020 02:24
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.

Windows 10: Can't adjust notification settings
2 participants