feat: allow playing default notification sound #50
Conversation
|
I've been testing this but I don't know how to change the default sound on macOS lol Note that with this change if you provide an unknown sound, the default one will be played. |
I think for testing purposes, it is enough to know that a sound is played.
It is kinda considered a breaking change but I think for the better, no? I mean if you provide a sound, you want a sound to be played even if it doesn't exist, otherwise don't pass a sound at all to make it silent, plus if this is the default macOS sound, I think we should follow it. |
|
We might still want to allow explicitly specifying “no sound” when using the sound option. So maybe bring back “_mute” (or similar)? Otherwise looks p good to me |
|
If you want to specify a "no sound" option, then just pass an empty string |
|
oh I missed that, sorry was checking this rather quickly |
There was a problem hiding this comment.
I came across this project because I am currently looking at notifications for https://github.com/squidowl/halloy. :)
objc/notify.m
Outdated
| { | ||
| userNotification.soundName = options[@"sound"]; | ||
| if (![options[@"sound"] isEqualToString:@"NSUserNotificationDefaultSoundName"]) |
There was a problem hiding this comment.
Not sure i find NSUserNotificationDefaultSoundName to be best suited for a default sound option.
I propose a enum instead:
enum Sound {
Default,
Custom(String)
}
There was a problem hiding this comment.
this will probably require a breaking change so I will let it up to the maintainers whether they want this or not
There was a problem hiding this comment.
I think this breaking change is acceptable in this case, there are going to be a few more of these before the next release anyway, so please go break things :D
| @@ -242,8 +241,8 @@ impl<'a> Notification<'a> { | |||
| _ => "no", | |||
| }), | |||
| NSString::from_str(match self.sound { | |||
| Some(sound) if check_sound(sound) => sound, | |||
There was a problem hiding this comment.
Is the check_sound not needed anymore?
There was a problem hiding this comment.
I think it is better to let the OS decided whether this is a valid sound or not.
There was a problem hiding this comment.
Yeah it's hard to determine if a sound is valid, the OS pretty much checks the entire file system 😂
There was a problem hiding this comment.
what is the verdict here? leave it in?
objc/notify.m
Outdated
| { | ||
| userNotification.soundName = options[@"sound"]; | ||
| if (![options[@"sound"] isEqualToString:@"NSUserNotificationDefaultSoundName"]) |
There was a problem hiding this comment.
I think this breaking change is acceptable in this case, there are going to be a few more of these before the next release anyway, so please go break things :D
| @@ -242,8 +241,8 @@ impl<'a> Notification<'a> { | |||
| _ => "no", | |||
| }), | |||
| NSString::from_str(match self.sound { | |||
| Some(sound) if check_sound(sound) => sound, | |||
There was a problem hiding this comment.
what is the verdict here? leave it in?
|
thanks for the contribution @amrbashir, sorry for the late response, if you could go ahead and apply these quick changes I'd go ahead and merge this. |
hoodie
force-pushed
the
feat/default-sound
branch
3 times, most recently
from
ee19343
to
c0ffee2
Compare
Co-Authored-By: Hendrik Sollich <[email protected]>
hoodie
force-pushed
the
feat/default-sound
branch
from
c0ffee2
to
c0ffeec
Compare
|
I found a way to add the |
|
Great choice to go with the enum approach. Well done on this! |
| /// | ||
| /// ```no_run | ||
| /// # use mac_notification_sys::*; | ||
| /// let _ = Notification::new().sound("Blow"); |
There was a problem hiding this comment.
example is wrong here but ok :D
ref: tauri-apps/tauri#7269