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

Update notifications have an unrecognizable icon and "SnoreToast" as the title #367

Closed
jnm2 opened this issue May 10, 2023 · 11 comments
Closed
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jnm2
Copy link
Contributor

jnm2 commented May 10, 2023

Describe the bug

There's nothing in these toasts that tells me that Pomatez is the app that is prompting them. My reaction every time is "What is SnoreToast? Did I get unwanted software on my machine?"

image

To Reproduce

  1. Have a version of Pomatez installed that is not the latest
  2. Make sure it is configured to start at Windows startup
  3. Sign out and back into your Windows user account to trigger the update check
  4. See two Windows notification toasts appear a little while after Pomatez starts, similar to the screenshot above.

Expected behavior
The icon in the toasts should be the Pomatez icon and the title in the toast should contain the name 'Pomatez.'

Screenshots
See description.

Desktop (please complete the following information):

  • OS: Windows 11
  • App Version: 1.2.2
  • Installer Used: Windows installer
@roldanjr roldanjr added the bug Something isn't working label May 10, 2023
@roldanjr roldanjr moved this to Bugs in Pomatez Roadmap May 10, 2023
@roldanjr roldanjr self-assigned this May 11, 2023
@roldanjr
Copy link
Member

Hi @jnm2, thanks for letting us know about this behavior. Unfortunately, I can't verify this behavior in Windows 11 at this moment, maybe @huangyinhaow and @sekwah41 can help us fix this issue if they have some time to look at this, otherwise, we'll just keep this bug ticket open until I am able to test it out in actual Windows 11 machine.

I tried to replicate it in Linux but it seems fine here. I can't test it also in MacOS since my current device is broken.

@jnm2
Copy link
Contributor Author

jnm2 commented May 14, 2023

Pomatez ships %localappdata%\Programs\pomatez\resources\app.asar.unpacked\node_modules\node-notifier\vendor\snoreToast\snoretoast-x64.exe which comes from a dependency called node-notifier which is brought in here.

I found instructions at https://github.com/mikaelbr/node-notifier#usage-windowstoaster referencing the text SnoreToast:

The default behaviour is to have the underlying toaster applicaton as appID. This works as expected, but shows SnoreToast as text in the notification.

With the Fall Creators Update, Notifications on Windows 10 will only work as expected if a valid appID is specified. Your appID must be exactly the same value that was registered during the installation of your app.

@sekwah41
Copy link
Member

sekwah41 commented May 15, 2023

Ive seen that it shows as snoretoast on my windows machine too now I remember. Kind of reminds me of the old why is my keyboard a toaster thread 😅 https://superuser.com/questions/792607/why-does-windows-think-that-my-wireless-keyboard-is-a-toaster

image

@sekwah41
Copy link
Member

Luckily it works fine but it does look oddly worrying to users. Ive got some other tasks to sort first but I believe it should be an easy fix or switching out of the library.

@sekwah41 sekwah41 added the good first issue Good for newcomers label May 15, 2023
@sekwah41 sekwah41 self-assigned this May 20, 2023
@sekwah41
Copy link
Member

sekwah41 commented May 20, 2023

So the issue is, the library returns a different object depending on the running OS, we need to either construct our own notifier if Windows is detected, or see if we can construct it with a valid appid via the library.

If we were using JS it would be as easy as just shoving the value on there as it would ignore it if its not valid, though it causes type issues because it HAS to have one of the types and cant decide between them.

@jnm2
Copy link
Contributor Author

jnm2 commented May 20, 2023

Wouldn't be surprised if the type definitions could be corrected to match whatever the JS usage pattern is. TS's type system is so powerful because that's it's whole goal.

@sekwah41
Copy link
Member

@jnm2 in the pr ive just merged the two types, it is possible

@sekwah41
Copy link
Member

image

@sekwah41
Copy link
Member

I still need to check things e.g. the portable mode one, and make sure the update logic works for the interaction. But its most of the way there

@sekwah41
Copy link
Member

I'm switching back to windows as my main OS for desktop at least due to other apps I'm developing so I'll pick this up later this week or next week to validate and fully fix.

@sekwah41
Copy link
Member

I believe this is fixed, and we also have a tauri version too now :)

@github-project-automation github-project-automation bot moved this from Bugs to Closed/Done in Pomatez Roadmap Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
Archived in project
Development

No branches or pull requests

3 participants