davicorreiajr / spotify-now-playing

Spotify now playing information and control popup for macOS menu bar
MIT License
216 stars 11 forks source link

Notifications are set to "Alerts" by default #26

Closed bressan3 closed 5 years ago

bressan3 commented 5 years ago

What are you reporting?

In which context are you running the app?

OS: MacOS 10.14.2, App version: 0.6.0

Description

When running App and turning on the Activate Notifications option, notifications will show up as Alerts instead of Banners like they should. The App is automatically setting Spotify notifications as Alerts when it runs for the first time (even if they were set as Banners before). The only way to make the App display notifications like it should is to do it manually by going to System Settings -> Notifications -> Spotify and set Spotify alert style as Banners.

Notifications look like this by default...:

screen shot 2019-01-11 at 12 03 05 am

...But instead, they should look like this:

screen shot 2019-01-11 at 12 02 45 am

I've read some issue openings from both Terminal-Notifier and Node-Notifier's Github repos. Node-Notifier is used in this App and uses Terminal-Notifier do display notifications on MacOS. By doing so, I found a post saying that changing a certain parameter in a .plist file may solve this issue.

In this App's case, I found out this can be done by opening the file called Info.plist located under spotify-now-playing-0.6.0/node_modules/node-notifier/vendor/terminal-notifier.app/Contents and changing the <string>alert</string> to <string>banner</string>.

Since this file and the folders containing it are all generated after executing yarn install, I can't just make a pull request changing this <string> value to the proper one. Do you know if there's a way to force the desired setting through Electron so when yarn install is executed (maybe by adding something to package.json)?

davicorreiajr commented 5 years ago

Maybe I didn't understand correctly, but if I got it right, you want to make in a way that the notification is always a banner, right?

I don't know if I like this, because we'd be overwriting the user's preference. For example, for me, since the first time you opened that pull request, it worked like a banner (I had to go to the preferences and changed it to see what you were talking about.

Anyway, what do you think? In summary, I don't like the idea of overwrite some system pref.

bressan3 commented 5 years ago

Not necessarily. What I wanted to do was change the default notification/alert style setting from Alerts to Banners since Alerts can be kind of annoying since they have to be dismissed manually by the user and some people may not be aware that they can change the notification style under System Preferences. I don't want to make it so the notifications are always displayed as banners. It should be up to the user to chose what kind of notification/alert style they want.

And yea... I don't know if overwriting the Node-Notifer module settings would be a good idea since you'd probably have to do it manually every time you'd run yarn install.

Another alternative to this issue would be adding a timeout to the notification so even when the alert style is set to "Alerts", the notifications would go away automatically after a certain time period (let's say 5 seconds).

davicorreiajr commented 5 years ago

hey, first of all, sorry for taking so long to answer.

What I found weird is that I never had to change from Alerts to Banners, it's been like Banners for me since the first time you implemented this. I agree with you that the Alerts would be quite annoying, but I didn't understand yet why the default for me was a Banner and for you an Alert. I uninstalled and installed the app again, and it keeps to work as a Banner.

About the code change, there're ways to solve the problem (when you need to change a package), but I'm not sure if it's worth it. The timeout seems easier. Do you know if the timeout works for both Alert and Banner cases?

bressan3 commented 5 years ago

I completely agree with you and don't think it's worth it to change the package. And yes, the timeout is supposed to work with both Alerts and Banners.

davicorreiajr commented 5 years ago

Fixed with #27