Skip to content
This repository has been archived by the owner. 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

@lolodomo lolodomo commented Jul 24, 2017

Fix bug introduced by PR #3808

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

Copy link
Contributor

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

@maggu2810 maggu2810 Jul 24, 2017

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

@maggu2810 maggu2810 Jul 24, 2017

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

@lolodomo lolodomo Jul 24, 2017

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

@maggu2810 maggu2810 Jul 24, 2017

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

@maggu2810 maggu2810 Jul 24, 2017

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

@maggu2810 maggu2810 Jul 24, 2017

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

@maggu2810 maggu2810 Jul 24, 2017

Why not using the unit atrribute and the unitLabel element?

Copy link
Contributor Author

@lolodomo lolodomo Jul 24, 2017

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

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

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

@lolodomo lolodomo 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.

Fix bug introduced by PR #3808

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

@lolodomo lolodomo commented Jul 25, 2017

Last change done.

@lolodomo
Copy link
Contributor Author

@lolodomo lolodomo 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-archived:master Jul 25, 2017
2 checks passed
<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

@kaikreuzer kaikreuzer Jul 25, 2017

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

Copy link
Contributor Author

@lolodomo lolodomo Jul 25, 2017

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

Copy link
Contributor

@cweitkamp cweitkamp Jul 27, 2017

Copy link
Contributor

@kaikreuzer kaikreuzer Jul 27, 2017

Good catch, so this line should be removed completely!

@lolodomo lolodomo deleted the 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants