#89 Removal of events and compliance to tests

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 485092610

123 of 140 (87.86%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 76.908%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/sonos-device.ts	116	133	87.22%
<!--	Total:	123	140	87.86%	-->

Totals
Change from base Build 449145378:	0.4%
Covered Lines:	1244
Relevant Lines:	1629

💛 - Coveralls

theimo1221 commented 3 years ago

Following points are still open on my list and I'll try to finish them tomorow morning

[ ] Use PlayNotificationOption.timeout
[x] Add Test for playing multiple notifications
[ ] Add Test for Notification Timeout to trigger properly

svrooij commented 3 years ago

It seems we are at a stalemate.

We can discuss the code over and over, but your code adds a lot of complexity over my proposed solution.

Differences in implementation

your version disconnects the thread
needs changes to tests
it tries to solve an issue that might not even be an issue
I think applications don't care if the notification actually played, you really need to convince me about this. As in an actual use case where this is useful. And if you find an use case, please explain why an optional callback isn't the correct solution.

The need to change the tests before they succeed, means in my opinion that (probably) the existing functionality also changed.

I'm also the one supporting this library, and will probably do that for the next few years.

I'm not going to accept your PR because:

you changed the test functionality before the test would succeed.
I don't get the code (to complex), that means I cannot support it.

So I propose I release a new beta with my version. Then you can actually test my solution. Then if you still see the need for your version, you can add it as PlayNotificationTwo (or PlayNotificationTh) with a remark that it's a separate implementation that isn't supported by the library author, with a link to a new issue discussing the new notification queue.

That way I'm still fully capable to support the library like I've been doing for years and you get your "improved" PlayNotification option.

theimo1221 commented 3 years ago

EDIT: There were some missunderstandings which could be resolved rereading your comment and additionally I got previous tests to work without changes as written below

In my opinion your solution lacks the following components:

PlayNotification Promise get's resolved with false, if it's added to the queue
PlayNotificationOptions.timeout is only considered while waiting on the STOPPED event of this queue item
If there is a call to PlayNotification while it is currently reverting, issue with "reverting back to normal" still exists

These components are properly handled in my solution.

I'm open to introduce you better to my code and additionally I can give you my mobile number in case you need fast support regarding future maintenance.

theimo1221 commented 3 years ago

@svrooij I restructured my changes in the sonos-device.test.ts file and reverted some of the changes to the test file. Besides some minor-fixes in the testfile (e.g. missing done, ), the only remaining difference is my solution calls AVTransportService.GetPositionInfo() even in the TTS 'return false when not playing', but I'll resolve that as well soon.

theimo1221 commented 3 years ago

@svrooij Above described difference is now fixed as well, resulting in overall no changes to the testfiles except:

Added Tests
Fixing of copyPaste Error within `PlayNotification(...) executes right requests, as previous it checked twice for result to be true, instead 1 time result, 1 time nockResult
Adding missing done callback and fireing in 'throws error when incorrect delay is specified'

Thus resolving your first concern regarding this:

you changed the test functionality before the test would succeed.

Regarding the second concern (I don't get the code (to complex), that means I cannot support it.) feel free to ask me any question you may have.

svrooij commented 3 years ago

I see a lot of progress in the commits since my last comment.

The code is getting closer to a good solution for the notification queue, but the thread is still detached (with setTimeout) and that just doesn't feel right. I cannot oversee the issues that might cause.

Your code still adds complexity to the already complicated PlayNotification code. This was already the most complicated part of this entire library. And that was the first thing I mentioned to you when you said you wanted to improve it.

I currently don't have the time to test this or to figure out how the new code works. Day job fills my time. My proposition to allow this to move forwards, by adding your code as a new method instead of replacing the original (for now, not forever), users of this library can switch between the two implementations and maybe vote for the best one.

That would also allow you to use your implementation in the mean time, and allows me to let users choose (and test) in sonos2mqtt, which is currently the biggest application using this library anyway. It has 70k downloads on dockerhub so there will for sure be some users that want to test it out.

theimo1221 commented 3 years ago

Thanks mate, we can proceed with that! 👍

I was quite busy with work recent days as well, but I now have some vacation (starting on monday) so I would have some time if needed.

Detached Thread Topic

As long as we want to resolve the individual promises of the different PlayNotificationCalls -depending on the options- immediatly after each played we have to decoupel the Queue Playing Task from the Thread in which the first PlayNotification call happened. Even in your solution the second and further calls are in a different thread than the playing queue, while additionally (at least the last version I have seen) the other promises where resolved with false, if queue was already playing.

theimo1221 commented 3 years ago

As today my Sonos System got stuck in a queue (had no Timeout defined) I added the option to give an item a specific timeout.

Example: Once I enter my office first time in the morning there is an playback routine:

(5s) Information if I need to put out trash
(180s) Last downloaded complete Radio News of a regional station
(20s) Weather Report from Weather API spoken by TTS

But the weather Report can be triggered separately as well, so I can't just say the weather Report gets 225s Timeout. Especially as I have all Weather Report items split in seperate messages to increse likelyhood of beeing able to use a cached variant of an already generated TTS mp3 file.

With the new option I can give each item it's mp3 Duration (plus some threshhold) as a specific timeout. So in case any event getting lost or beeing prevented by some other play action it can resolve properly.

svrooij commented 3 years ago

Merged by https://github.com/svrooij/node-sonos-ts/commit/94d2d5e2d1adb7a5415d677fb78d2c6786b6f9d6

svrooij / node-sonos-ts