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

Improve Sonos notification timeout handling #3808

Merged
merged 1 commit into from Jul 6, 2017

Conversation

Projects
None yet
4 participants
@ivivanov-bg
Copy link
Contributor

commented Jul 6, 2017

  1. Extract the notification timeout as a thing configuration parameter
  2. Increase the default notification timeout to 40 sec. (as 20 s is too short time)

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:sonos_notification_duration_param branch from dac9a37 to 210a885 Jul 6, 2017

<default>60</default>
</parameter>
</config-description>
</config-description:config-descriptions>

This comment has been minimized.

Copy link
@hakan42

hakan42 Jul 6, 2017

Contributor

Please add a linefeed at the end of this file.

This comment has been minimized.

Copy link
@ivivanov-bg

ivivanov-bg Jul 6, 2017

Author Contributor

done

updateConfiguration(c);
}

this.notificationTimeout = getConfigAs(ZonePlayerConfiguration.class).notificationTimeout;

This comment has been minimized.

Copy link
@hakan42

hakan42 Jul 6, 2017

Contributor

Please initiliaze the notification Timeout to the default here and don't use the NPE-safety check in the getter?

This comment has been minimized.

Copy link
@ivivanov-bg

ivivanov-bg Jul 6, 2017

Author Contributor

done

@@ -2801,4 +2815,8 @@ private long sleepStrTimeToSeconds(String sleepTime) {
int seconds = Integer.parseInt(units[2]);
return 3600 * hours + 60 * minutes + seconds;
}

protected Integer getNotificationTimeout() {
return this.notificationTimeout != null ? this.notificationTimeout : DEFAULT_NOTIFICATION_TIMEOUT;

This comment has been minimized.

Copy link
@hakan42

hakan42 Jul 6, 2017

Contributor

This safety check is not necessary (and the getter a standard getter method) if you initialize the default in the initialize() method (see my previous comment)

1. Extract the notification timeout as a thing configuration parameter
2. Increase the default notification timeout to 40 sec. (as 20 s is too short time)

Signed-off-by: Ivaylo Ivanov <ivivanov.bg@gmail.com>

@ivivanov-bg ivivanov-bg force-pushed the ivivanov-bg:sonos_notification_duration_param branch from 210a885 to 6d74040 Jul 6, 2017

@sjka sjka merged commit db27358 into eclipse:master Jul 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
@sjka

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2017

Thank you both!

lolodomo added a commit to lolodomo/smarthome that referenced this pull request Jul 24, 2017

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

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

lolodomo added a commit to lolodomo/smarthome that referenced this pull request Jul 24, 2017

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

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

lolodomo added a commit to lolodomo/smarthome that referenced this pull request Jul 24, 2017

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

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

lolodomo added a commit to lolodomo/smarthome that referenced this pull request Jul 24, 2017

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

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

lolodomo added a commit to lolodomo/smarthome that referenced this pull request Jul 25, 2017

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

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

maggu2810 added a commit that referenced this pull request Jul 25, 2017

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

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

@ivivanov-bg ivivanov-bg deleted the ivivanov-bg:sonos_notification_duration_param branch Aug 18, 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.