svrooij / node-sonos-ts

:speaker: Sonos control library, use this library in your own appliction.
https://sonos-ts.svrooij.io/
MIT License
81 stars 18 forks source link

Feature: Notification queue #89

Closed svrooij closed 3 years ago

svrooij commented 3 years ago

Currently playing a notification while an other notification is already playing leads to a situation that the playback cannot be restored.

Maybe we can solve this with a NotificationQueue, my idea is to do the following (very open to suggestions):

  1. Add items to NotificationQueue
  2. If PlayingNotification (new) === true just return
  3. Capture original values to revert back to
  4. Set PlayingNotification (new) to true
  5. play all items from queue (check queue length if 0 return, take first item, play item, wait for stop, remove item from queue, call itself)
  6. Revert to original
  7. set PlayingNotification to false.

Want to pick this one up @theimo1221?

theimo1221 commented 3 years ago

Within 79bfb91 I implemented the feature. The behaviour is as following:

Additionally I extended the debug output and added some variable states to it. Additionally I fixed an issue where reattaching Event Listeners for a device wouldn't work due to set isSubscribed flag. For that I had to reset flag in the removeListener Event.

theimo1221 commented 3 years ago

Remaining issue

Option Timeout is kinda problematic, as when should it be used?

svrooij commented 3 years ago

@theimo1221 I've spend some time to separate the notification queue from the other changes that I made in that branch (shouldn't have done that). As you can see in the above commit, I haven't changed the functional tests that test the notification feature.

That should mean it works as before when playing only one notification. The Timeout (which is better documented based on your feedback) still works per notification. Existing users can start using the new feature without code change, with the only exception that if they want to know when a specific notification has played they need to use the notificationFired callback.

By checking the dependency graph I see that at the moment not much (8, I manage 3) apps depend on this library.

theimo1221 commented 3 years ago

As I said before, you are the one to make the final decision as this crossroad.

Personally I see more downsides in the callback variant, as with the other variant you can just use the promise and rely on it to fire once this notification is finished (queued or not). While with the other variant users have to think wether to use callback or Promise. We can't remove the promise completly as we have to stay backwards compatible, otherwise the your solution + removing the promise completly would result in the same as my solution.

svrooij commented 3 years ago

I also see that there is a difference between FinishedWithThisNotification and AddedToQueue but without changing the signature of PlayNotification you cannot change it to return FinishedPlayingNotification or AddedToQueue.

I'm just more on the ShootAndForget side, because I personally don't care about if it played. I also see some (very thin) use of knowing if a certain notification has played, that is way you can now also use the notificationFired callback.

If you upload a file to somewhere with node, you can just wait for it to finish or get the upload progress in a callback.

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.4.0-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.4.0-beta.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: