Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sonos binding: fix bug with notification timeout handling #3883

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
4 participants
@lolodomo
Copy link
Contributor

commented Jul 24, 2017

Fix bug introduced by PR #3808

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo lolodomo force-pushed the lolodomo:sonos_notif branch from d87ebe4 to cedd88f Jul 24, 2017

@maggu2810
Copy link
Contributor

left a comment

In general I agree with the change, but added some comments.

@@ -11,9 +11,8 @@
</parameter>
<parameter name="notificationTimeout" type="integer">
<label>Notification Timeout</label>
<description>Specifies the amount of time for which the notification sound will be played</description>
<description>Specifies the amount of time in seconds for which the notification sound will be played</description>

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

Why not using the unit attribute and the unitLabel element?

*/
private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 40000;

private static final Integer DEFAULT_NOTIFICATION_TIMEOUT = 20;

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

The config-description XML is using a default of 40 seconds, you change it here to 20 seconds.
I don't know what is better 20 or 40.

This comment has been minimized.

Copy link
@lolodomo

lolodomo Jul 24, 2017

Author Contributor

I changed the XML value after your comment.

@@ -2388,7 +2388,7 @@ private void waitForFinishedNotification() {
// check Sonos state events to determine the end of the notification sound
String notificationTitle = stateMap.get("CurrentTitle");
long playstart = System.currentTimeMillis();
while (System.currentTimeMillis() - playstart < this.notificationTimeout.longValue()) {
while (System.currentTimeMillis() - playstart < (this.notificationTimeout.longValue() * 1000)) {

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

Why adding unneeded braces?

@@ -2407,7 +2407,7 @@ private void waitForTransportState(String state) {
while (!stateMap.get("TransportState").equals(state)) {
try {
Thread.sleep(50);
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) {
if (System.currentTimeMillis() - start > (this.notificationTimeout.longValue() * 1000)) {

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

Why adding unneeded braces?

@@ -2423,7 +2423,7 @@ private void waitForNotTransportState(String state) {
while (stateMap.get("TransportState").equals(state)) {
try {
Thread.sleep(50);
if (System.currentTimeMillis() - start > this.notificationTimeout.longValue()) {
if (System.currentTimeMillis() - start > (this.notificationTimeout.longValue() * 1000)) {

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

Why adding unneeded braces?

<description>Specifies the amount of time for which the notification sound will be played</description>
<default>40</default>
<required>true</required>
<description>Specifies the amount of time in seconds for which the notification sound will be played</description>

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Jul 24, 2017

Contributor

Why not using the unit atrribute and the unitLabel element?

This comment has been minimized.

Copy link
@lolodomo

lolodomo Jul 24, 2017

Author Contributor

Because I don't know this ability ;) If it is described somewhere, I will try to change that.

This comment has been minimized.

Copy link
@cweitkamp

cweitkamp Jul 27, 2017

Contributor

Nice to know. I found PR #889 but no directly hint in the docs.)

EDIT: I am wrong. One can find it in the https://github.com/eclipse/smarthome/blob/091a79696626da2298968cf4ffbe82bdadab6a66/docs/documentation/development/bindings/xml-reference.md section.

@lolodomo lolodomo force-pushed the lolodomo:sonos_notif branch 2 times, most recently from 0406b52 to c113646 Jul 24, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

@maggu2810 : I changed the config setting to include unit and unitLabel but I have not tested again after this change.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

Please wait, there is another thing weird with PR 3808, that is the thing update. If the thing setting is not set, the binding should just use its default value without the need to update the thing setting.

Sonos binding: fix bug with notification timeout handling
Fix bug introduced by PR #3808

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

@lolodomo lolodomo force-pushed the lolodomo:sonos_notif branch from c113646 to e3aea88 Jul 25, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

Last change done.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

Regarding the timeout value, 20s looks like a reasonable duration for notifications. Users needing very long notification will oncrease the setting.

@maggu2810 maggu2810 merged commit 1f70312 into eclipse:master Jul 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
<default>40</default>
<required>true</required>
<description>Specifies the amount of time in seconds for which the notification sound will be played</description>
<unitLabel>second</unitLabel>

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jul 25, 2017

Member

Isn't "s" the normal label? "5 s" looks better to me than "5 second"...

This comment has been minimized.

Copy link
@lolodomo

lolodomo Jul 25, 2017

Author Contributor

Be free to change it if you think it is better.

This comment has been minimized.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jul 27, 2017

Member

Good catch, so this line should be removed completely!

@lolodomo lolodomo deleted the lolodomo:sonos_notif branch Jul 25, 2017

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

@kaikreuzer kaikreuzer added the bug label Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.