Add method to change notification sound #33
Conversation
There was a problem hiding this comment.
You'll need to bring the changes from NativePHP/electron#257 into this repository now, as they've been combined
|
I have moved the changes made in NativePHP/electron#257 to the electron folder in this repository, i have also tested my changes in a demo app and they seem to work for me. |
|
@gwleuverink what do you think we should do with this |
| if (usingLocalFile && typeof sound === 'string') { | ||
| playSound(sound).catch((err) => { | ||
| notifyLaravel('events', { | ||
| event: '\\Native\\Desktop\\Events\\Notifications\\NotificationSoundFailed', |
There was a problem hiding this comment.
I don't see this event anywhere in this PR. Is this even needed at this stage? If the sound fails, can't it just silently fail? (pun intended)
There was a problem hiding this comment.
I agree. If it errors somehow we should see it in the browser log. These events should be used for things devs can use on the Laravel side, not for errors
There was a problem hiding this comment.
Sorry i had not realised this event did not exist, i was also busy making a different pull request to something unrelated and must have gotten things messed up, i have removed this in my latest commits and am now letting it fail silently.
| const createNotification = (opts: any) => { | ||
| try { | ||
| if (typeof (Notification as any) === 'function') { | ||
| return new (Notification as any)(opts); | ||
| } | ||
| } catch (e) { | ||
|
|
||
| } | ||
|
|
||
| return { | ||
| show: () => {}, | ||
| on: (_: string, __: Function) => {}, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I don't understand the value of this factory... when would the Notification module not be available?
There was a problem hiding this comment.
I removed this factory in the newest commits, i had initially created it due to some errors i was getting but since they are resolved this factory can also be removed.
| let player: any; | ||
| try { | ||
| player = require('play-sound')(); | ||
| } catch (e) { | ||
| player = null; | ||
| } |
There was a problem hiding this comment.
I have changes this to simply import, here i was also getting errors with just importing but i have also resolved these issues (i caused them myself due to wrong setup of the repo 🥲 )
| const playSound = async (sound: string) => { | ||
| const filePath = normalizePath(sound); | ||
| try { | ||
| await fs.promises.access(filePath, fs.constants.R_OK); | ||
| } catch (err) { | ||
| return Promise.reject(new Error(`sound file not accessible: ${filePath}`)); | ||
| } | ||
|
|
||
| return new Promise<void>((resolve, reject) => { | ||
| if (player) { | ||
| player.play(filePath, (err: any) => { | ||
| if (err) return reject(err); | ||
| resolve(); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const { exec } = require('child_process'); | ||
| exec(`afplay ${JSON.stringify(filePath)}`, (err: any) => { | ||
| if (err) return reject(err); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
I feel like this is more complicated than it needs to be because we're trying to spot if playing the sound failed later on... I just don't know that it's that valuable
There was a problem hiding this comment.
I agree, I have completely removed this function and moved sound playing straight into the playSoundLib().play function. I have also tested this and it seems to work on my machine.
Can we merge main back to this branch & run |
There was a problem hiding this comment.
This is a great addition 👍🏻 Thanks @Jianbe-03! Could clarify a few questions in the comments?
| } catch (e) { | ||
| const { exec } = require('child_process'); | ||
| exec(`afplay "${sound}"`, () => {}); | ||
| } |
There was a problem hiding this comment.
Can you elaborate this catch clause? afplay is a macOS utility. It won't work cross-platform.
In what cases would the sound not play using the play-sound lib? Could we perhaps validate the path earlier?
| toastXml | ||
| }); | ||
|
|
||
| if (usingLocalFile && typeof sound === 'string') { |
There was a problem hiding this comment.
I think you can drop && typeof sound === 'string' as it's already covered in the isLocalFile function
| toastXml | ||
| }); | ||
|
|
||
| if (usingLocalFile && typeof sound === 'string') { |
There was a problem hiding this comment.
Would the new silent method work if you also provide a custom sound?
For example this would play a sound even if the silent() method was chained (if i'm reading this correctly):
$notification = Notification::title('Hello from NativePHP')
->sound('./path/to/file.mp3');
if($someCondition) {
$notification->silent();
}
$notification->show();If this is the case you might need to add that to the if clause
There was a problem hiding this comment.
Ah great catch, yes the audio currently still plays if the silent() method is chained, ill add that to the if clause so its actually silent.
…orm specific afplay command since playSoundLib already does this
3 participants
This pull request adds support for sound notifications and silent mode to the
Notificationclass. These changes allow notifications to specify a custom sound and to be marked as silent, providing more flexibility in how notifications are delivered and displayed.New notification features:
soundandsilentproperties to theNotificationclass, allowing notifications to specify a sound and whether they should be silent.sound()andsilent()methods in theNotificationclass to set these new properties.show()method to includesoundandsilentin the notification payload.This addition does require an update to the electron repository, i made the changes for this pull request in here: NativePHP/electron#257