Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Sonos binding: fix bug with notification timeout handling #3883

Merged
merged 1 commit into from Jul 25, 2017
Merged

Sonos binding: fix bug with notification timeout handling #3883

merged 1 commit into from Jul 25, 2017

Conversation

lolodomo
Copy link
Contributor

Fix bug introduced by PR #3808

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

Copy link
Contributor

@maggu2810 maggu2810 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using the unit atrribute and the unitLabel element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@cweitkamp cweitkamp Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@lolodomo
Copy link
Contributor Author

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.

Fix bug introduced by PR #3808

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

Last change done.

@lolodomo
Copy link
Contributor Author

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-archived:master Jul 25, 2017
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, so this line should be removed completely!

@lolodomo lolodomo deleted the sonos_notif branch July 25, 2017 09:34
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants